png++: diffs in test bench libpng12 vs. libpng16

Hello,

I'm the author and maintainer of png++ project (a C++ wrapper for libpng):
  http://www.nongnu.org/pngpp/
  https://packages.debian.org/source/stable/png++

I've received a report that when using libpng16, the test suite of png++
produces numerous failures, which I can confirm on my laptop with Ubuntu
16.04 and libpng-1.6.20.

TL;DR: the same code compiled against libpng12 vs. 16 can produce slightly
different output; to which extent is that expected and what is the best way
to fix failing test suite?

The way that test suite is designed is that it runs a number of conversions
using png++ over about a 100 of pre-selected test files from the official
pngsuite and compares the resulting files with the expected "standard"
output files using cmp(1).  This has proven to be very reliable with
libpng12 series and helped me to catch some nasty bugs in my code in the
past.

However, when using libpng16 instead, I'm seeing about 5% failure test rate
(74 failed out of total 1566).  I have translated both output and standard
files to plain text using sng(1) and so far I can see the following classes
of faliure:

1. When using png_set_rgb_to_gray with error_action=1 (silent), in the
output file there is an off-by-one type of error in the grayscale component
probably due to rounding (in `cmp' directory is the expected result, in
`out' -- the actual output):

--- test/cmp/pngsuite/basi2c16.png.GA.16.sng    2016-11-20
06:40:22.755271236 +0100
+++ test/out/pngsuite/basi2c16.png.GA.16.sng    2016-11-20
06:40:22.759271267 +0100
 #SNG: from stdin
 IHDR {
     width: 32; height: 32; bitdepth: 16;
     using grayscale alpha;
     with interlace;        # type adam7 assumed
 }
 gAMA {1.0000}
 IMAGE {
     pixels hex
- ed83ffff *ebc2ffff* ea00ffff e83effff *e67dffff* e4bbffff *e2faffff*
e138ffff *df77ffff* ddb5ffff *dbf4ffff* da32ffff *d871ffff* d6afffff
*d4eeffff* d32cffff d16affff *cfa9ffff* cde7ffff *cc26ffff* ca64ffff
*c8a3ffff* c6e1ffff *c520ffff* c35effff c19cffff *bfdbffff* be19ffff
*bc58ffff* ba96ffff *b8d5ffff* b713ffff
+ ed83ffff *ebc1ffff* ea00ffff e83effff *e67cffff* e4bbffff *e2f9ffff*
e138ffff *df76ffff* ddb5ffff *dbf3ffff* da32ffff *d870ffff* d6afffff
*d4edffff* d32cffff d16affff *cfa8ffff* cde7ffff *cc25ffff* ca64ffff
*c8a2ffff* c6e1ffff *c51fffff* c35effff c19cffff *bfdaffff* be19ffff
*bc57ffff* ba96ffff *b8d4ffff* b713ffff
...
(every line is different, but in every line only about a half of positions
are different)

2. More interestingly, some of the files are not different from the point
of view of sng, but the difference can be seen with od(1), for example:

$ diff -u <(od -t x1z cmp/pngsuite/s01i3p01.png.RGB.16.out) <(od -t x1z
out/pngsuite/s01i3p01.png.RGB.16.out)
...
@@ -1,7 +1,7 @@
 0000000 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52  >.PNG........IHDR<
 0000020 00 00 00 01 00 00 00 01 10 02 00 00 01 b7 e0 bf  >................<
 0000040 0b 00 00 00 04 67 41 4d 41 00 01 86 a0 31 e8 96  >.....gAMA....1..<
-0000060 5f 00 00 00 *0f* 49 44 41 54 08 99 63 60 *60 60 60*
 >_....IDAT..c````<
-0000100 *f8 cf 00 00 02 05 01 00 5f c9 c7 29* 00 00 00 00
 >........_..)....<
-0000120 49 45 4e 44 ae 42 60 82                          >IEND.B`.<
-0000130
+0000060 5f 00 00 00 *0d* 49 44 41 54 08 99 63 60 *00 82 ff*
 >_....IDAT..c`...<
+0000100 *0c 00 02 05 01 00 b0 60 e2 9e *00 00 00 00 49 45
 >.......`......IE<
+0000120 4e 44 ae 42 60 82                                >ND.B`.<
+0000126

There seems to be a diff in the latest byte of the gAMA chunk (why?) and
that is probably the reason why the complete IDAT chunk is completely
different as well.  Still from sng, both files produce identical output:

$ sng <cmp/pngsuite/s01i3p01.png.RGB.16.out
#SNG: from stdin
IHDR {
    width: 1; height: 1; bitdepth: 16;
    using color;
    with interlace;        # type adam7 assumed
}
gAMA {1.0000}
IMAGE {
    pixels hex 00000000ff00 ;
}

3. More interestingly, there is one failure where the differences are only
in the IDAT chunk (but only in first 2 and latest 4 bytes before the final
zero quadruplet):

$ diff -u <(od -t x1z cmp/pngsuite/s09i3p02.png.RGB.8.out) <(od -t x1z
out/pngsuite/s09i3p02.png.RGB.8.out)
...
@@ -1,11 +1,11 @@
 0000000 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52  >.PNG........IHDR<
 0000020 00 00 00 09 00 00 00 09 08 02 00 00 01 18 f4 a1  >................<
 0000040 d1 00 00 00 04 67 41 4d 41 00 01 86 a0 31 e8 96  >.....gAMA....1..<
-0000060 5f 00 00 00 56 49 44 41 54 *08 99* 65 8e 5b 0e c0
 >_...VIDAT..e.[..<
+0000060 5f 00 00 00 56 49 44 41 54 *18 95* 65 8e 5b 0e c0
 >_...VIDAT..e.[..<
 0000100 40 08 02 87 66 ef 6c 8f d0 3d f5 f4 a3 f6 4d 4c  >@...f.l..=....ML<
 0000120 50 23 42 10 60 a1 21 0b 90 63 7b 77 11 1f 13 62  >P#B.`.!..c{w...b<
 0000140 fa 24 e6 52 bf 45 e7 cb 42 14 2d 06 90 3a 8c 18  >.$.R.E..B.-..:..<
 0000160 80 13 80 7a 6b 82 b8 f2 47 b6 2b ea 34 74 31 5b  >...zk...G.+.4t1[<
-0000200 fb 0d f2 c4 68 ae dc 46 d5 bc 03 d2 4b 22 71 *9b*
 >....h..F....K"q.<
-0000220 *7e 6b ee* 00 00 00 00 49 45 4e 44 ae 42 60 82
>~k.....IEND.B`.<
+0000200 fb 0d f2 c4 68 ae dc 46 d5 bc 03 d2 4b 22 71 *f4*
 >....h..F....K"q.<
+0000220 *78 f0 4e* 00 00 00 00 49 45 4e 44 ae 42 60 82
>x.N....IEND.B`.<
 0000237

The sng output for both files is:

#SNG: from stdin
IHDR {
    width: 9; height: 9; bitdepth: 8;
    using color;
    with interlace;        # type adam7 assumed
}
gAMA {1.0000}
IMAGE {
    pixels hex
00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00
00ff00 ff7700 ff7700 ff7700 ff7700 ff7700 ff7700 ff7700 00ff00
00ff00 ff7700 ff00ff ff00ff ff00ff ff00ff ff00ff ff7700 00ff00
00ff00 ff7700 ff00ff 0077ff 0077ff 0077ff ff00ff ff7700 00ff00
00ff00 ff7700 ff00ff 0077ff 00ff00 0077ff ff00ff ff7700 00ff00
00ff00 ff7700 ff00ff 0077ff 0077ff 0077ff ff00ff ff7700 00ff00
00ff00 ff7700 ff00ff ff00ff ff00ff ff00ff ff00ff ff7700 00ff00
00ff00 ff7700 ff7700 ff7700 ff7700 ff7700 ff7700 ff7700 00ff00
00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00 00ff00
}


Now to my questions:

- Is it worth trying to find the root causes for these differences or was
it never supposed to be 100% stable?
- Should I change my approach to testing or am I missing something?
- Is there a way to adapt the png++ code to produce the same output both
with libpng12 and 16?  For example, in order to re-enable strip_16 I've had
to make the following change in my code at two points (patch against trunk
of png++ attached just in case someone wants to try it):

-#ifdef PNG_READ_16_TO_8_SUPPORTED
+#ifdef PNG_READ_STRIP_16_TO_8_SUPPORTED

- Are there more changes between libpng12 and 16 that might need similar
treatment?  I'm currently reading through
http://www.libpng.org/pub/png/libpng-manual.txt to see what might have
changed since I've taken a look last time (to be honest, that was quite a
while).

Cheers!
--
Alex

------------------------------------------------------------------------------

_______________________________________________
png-mng-implement mailing list
png-mng-implement-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

Index: convert_color_space.hpp
===================================================================
--- convert_color_space.hpp	(revision 143)
+++ convert_color_space.hpp	(working copy)
@@ -108,10 +108,10 @@
             {
                 if (io.get_bit_depth() == 16 && traits::get_bit_depth() == 8)
                 {
-#ifdef PNG_READ_16_TO_8_SUPPORTED
+#ifdef PNG_READ_STRIP_16_TO_8_SUPPORTED
                     io.set_strip_16();
 #else
-                    throw error("expected 8-bit data but found 16-bit; recompile with PNG_READ_16_TO_8_SUPPORTED");
+                    throw error("expected 8-bit data but found 16-bit; recompile with PNG_READ_STRIP_16_TO_8_SUPPORTED");
 #endif
                 }
                 if (io.get_bit_depth() != 16 && traits::get_bit_depth() == 16)
Index: io_base.hpp
===================================================================
--- io_base.hpp	(revision 143)
+++ io_base.hpp	(working copy)
@@ -388,7 +388,7 @@
         }
 #endif
 
-#if defined(PNG_READ_16_TO_8_SUPPORTED)
+#if defined(PNG_READ_STRIP_16_TO_8_SUPPORTED)
         void set_strip_16() const
         {
             TRACE_IO_TRANSFORM("png_set_strip_16\n");