Menu
▾
▴
png-mng-implement
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-06 13:36:34
|
libpng-1.5.0 is available from ftp://ftp.simplesystems.org/pub/png/src and from http://libpng.sf.net There are no changes from libpng-1.5.0rc07, except for fixing a couple of tiny typos in the manual. Read the ANNOUNCE file and the section on libpng-1.4 to 1.5 differences in libpng-manual.txt that come with libpng to see the differences from 1.4.5. The major changes include moving the png and png_info structs into their own private header files, and provision of a new, more thorough test program (pngvalid.c), and an awk-based system of maintaining the new pnglibconf.h file that keeps track of how libpng was configured (i.e., what features were supported when libpng was built). Most of this work was done by John Bowler. Except for the accessibility of the png and png_info structs (which we have been deprecating for more than a decade), the API isn't significantly changed. Applications built with libpng14 without compiler warnings about using deprecated features should also build without modification with libpng15. Please reply to the png-mng-implement list. Glenn |
|
From: Alexander S. <ale...@gm...> - 2011-01-08 18:11:43
Attachments:
read_to_gray.c
basi3p01.png
|
On Thu, Jan 6, 2011 at 15:36, Glenn Randers-Pehrson <gl...@gm...> wrote: > > Except for the accessibility of the png and png_info > structs (which we have been deprecating for more than > a decade), the API isn't significantly changed. Applications > built with libpng14 without compiler warnings about using > deprecated features should also build without modification > with libpng15. Well, first let me apologize for not testing until the official release. Anyway, any major release is bound to be followed by a correcting minor one... So, I've built libpng-1.5.0, confirmed that it passes self-tests and then installed to $HOME... to discover that with this version, over the half of png++ tests on pngsuite fail. Some of the tests report difference against standard output, some of them crash. I've checked with locally installed libpng-1.4 and -1.2 and it passed all png++ tests as before. After some hours debugging I was able to put up a minimal example to reproduce the problem (see attached read_to_gray.c file). The program uses input transformations to convert the input image into 8-bit grayscale format (colormap -> rgb, rgb -> gray): png_set_palette_to_rgb(png_ptr); png_set_rgb_to_gray(png_ptr, 1, -1.0, -1.0); To reproduce the problem, run the complied example on basi3p01.png from pngsuite: http://www.schaik.com/pngsuite/basi3p01.png Here's a sample backtrace: Program received signal SIGABRT, Aborted. 0x00007ffff7855ba5 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 64 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. in ../nptl/sysdeps/unix/sysv/linux/raise.c (gdb) bt #0 0x00007ffff7855ba5 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #1 0x00007ffff78596b0 in abort () at abort.c:92 #2 0x00007ffff788f43b in __libc_message (do_abort=<value optimized out>, fmt=<value optimized out>) at ../sysdeps/unix/sysv/linux/libc_fatal.c:189 #3 0x00007ffff7921537 in __fortify_fail ( msg=0x7ffff796b253 "stack smashing detected") at fortify_fail.c:32 #4 0x00007ffff7921500 in __stack_chk_fail () at stack_chk_fail.c:29 #5 0x00007ffff7bc5cad in png_do_read_interlace (png_ptr=0x603250) at pngrutil.c:3115 #6 0x00007ffff7bb3fa1 in png_read_row (png_ptr=0x603250, row=0x607b50 "", dsp_row=0x0) at pngread.c:718 #7 0x0000000000400ddf in main (argc=2, argv=0x7fffffffe2c8) at read_to_gray.c:50 The problem manifests itself through incorrect calculations of row_info struct `channels' and `pixel_depth' fields in png_do_rgb_to_gray: Breakpoint 1, png_do_rgb_to_gray (png_ptr=0x603250, row_info=0x603458, row=0x607ab1 "") at pngrtran.c:2425 2425 png_uint_32 row_width = row_info->width; (gdb) p *row_info $1 = {width = 4, rowbytes = 4, color_type = 3 '\003', bit_depth = 8 '\b', channels = 1 '\001', pixel_depth = 8 '\b'} ... 2661 row_info->channels -= 2; (gdb) 2662 row_info->color_type = (png_byte)(row_info->color_type & (gdb) 2664 row_info->pixel_depth = (png_byte)(row_info->channels * (gdb) 2666 row_info->rowbytes = PNG_ROWBYTES(row_info->pixel_depth, row_width); (gdb) p *row_info $2 = {width = 4, rowbytes = 4, color_type = 1 '\001', bit_depth = 8 '\b', channels = 255 '\377', pixel_depth = 248 '\370'} Some incorrect assumption in this code leads to row_info->channels subtracted by 2 where it's value was 1, which leads to underflow and wrongly calculated pixel_depth value. The pixel_depth field is later used by png_do_read_interlace to set number of bytes per pixel and finally passed to png_memcpy which aborts. The problem also manifests itself on non-interlaced images from pngsuite, which is not unexpected. Unfortunately, I couldn't come up with a fix myself, but I'm willing to test any corrected version. -- Regards, Alex |
|
From: <jb...@ac...> - 2011-01-08 19:28:46
|
From: Alexander Shulgin [mailto:ale...@gm...] >Some incorrect assumption in this code leads to row_info->channels subtracted by 2 where it's value was 1 Yes, I made some changes here, at least to insert default cases to silence compiler warnings. I thought they were all obvious, but I must have got something wrong. I'll have to add a test case to pngvalid.c too - clearly we should be validating the rgb to gray transform at least to the extent of making sure it does the correct thing with originally gray pixels. John Bowler <jb...@ac...> |
|
From: <jb...@ac...> - 2011-01-08 19:58:08
|
From: Alexander Shulgin [mailto:ale...@gm...] >png_do_rgb_to_gray By examination the routine is not robust if called on a row corresponding to a palette image. In that case there is one channel (containing palette indices) and the color_type member says it is color. The routine proceeds to subtract two from the number of channels... Obviously png_do_rgb_to_gray should be robust, but the problem has been introduced elsewhere and I need to find out what happened. Meanwhile the attached one liner should remove the crash, but may still leave palette images as palette images. John Bowler <jb...@ac...> |
|
From: <jb...@ac...> - 2011-01-08 20:16:56
|
An attempt to fix up some hilarious code at line 4234 of 1.4.5 pngrtran.c - a single case 'switch' that must always be entered (see the lines immediately above). Alexander, can you tell me if the problems all go away if you just *delete* the 'else' at the start of line 4219 of the libpng-1.5.0 pngrtran.c - the line that says: else if (row_info->bit_depth == 8) I.e. change it to: if (row_info->bit_depth == 8) 'make check' is fine with that change too (in addition to the other one.) John Bowler <jb...@ac...> |
|
From: Alexander S. <ale...@gm...> - 2011-01-08 20:46:18
|
On Sat, Jan 8, 2011 at 22:16, <jb...@ac...> wrote: > An attempt to fix up some hilarious code at line 4234 of 1.4.5 pngrtran.c - a single case 'switch' that must always be entered (see the lines immediately above). > > Alexander, can you tell me if the problems all go away if you just *delete* the 'else' at the start of line 4219 of the libpng-1.5.0 pngrtran.c - the line that says: > > else if (row_info->bit_depth == 8) > > I.e. change it to: > > if (row_info->bit_depth == 8) > > 'make check' is fine with that change too (in addition to the other one.) Yep, that fixes the crash. Thanks! :) Now I have only "12 OF 790 FAILED" which I'm going to investigate a bit later. -- Alex |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-08 21:00:28
|
Patch is pushed to the GIT repository for libpng-1.5.1beta01 ("devel" branch).
Glenn
On Sat, Jan 8, 2011 at 3:45 PM, Alexander Shulgin
<ale...@gm...> wrote:
> On Sat, Jan 8, 2011 at 22:16, <jb...@ac...> wrote:
>> An attempt to fix up some hilarious code at line 4234 of 1.4.5 pngrtran.c - a single case 'switch' that must always be entered (see the lines immediately above).
>>
>> Alexander, can you tell me if the problems all go away if you just *delete* the 'else' at the start of line 4219 of the libpng-1.5.0 pngrtran.c - the line that says:
>>
>> else if (row_info->bit_depth == 8)
>>
>> I.e. change it to:
>>
>> if (row_info->bit_depth == 8)
>>
>> 'make check' is fine with that change too (in addition to the other one.)
>
> Yep, that fixes the crash. Thanks! :)
>
> Now I have only "12 OF 790 FAILED" which I'm going to investigate a bit later.
>
> --
> Alex
>
|
|
From: Alexander S. <ale...@gm...> - 2011-01-10 08:48:06
Attachments:
libpng-1.5.0-meld-sng-diff.png
|
On Sat, Jan 8, 2011 at 22:45, Alexander Shulgin <ale...@gm...> wrote:
>
> Now I have only "12 OF 790 FAILED" which I'm going to investigate a bit later.
All these failing tests have something to do with input transformation
from 16-bit RGB(A) image to 8-bit grayscale (+Alpha).
The results are slightly different from the standard images obtained
with earlier versions of libpng. Attached is a sample meld window to
illustrate the different pixel values on SNG files obtained from the
failing result image and standard one.
I've briefly looked through the list of changes for 1.5 and the only
relevant one seems to be correcting of overflows in gamma handling.
Could it be the case that the old images are indeed incorrect and new
ones are good? How can I check that?
All the original images from pngsuite on which the problem occurs have
"gAMA {1.0000}" reported by sng. Here's the list of them:
basi2c16.sng
basi6a16.png
basn2c16.png
basn6a16.png
bgan6a16.png
bgyn6a16.png
--
Regards,
Alex
|
|
From: <jb...@ac...> - 2011-01-10 16:36:51
|
From: Alexander Shulgin [mailto:ale...@gm...] >All these failing tests have something to do with input transformation from 16-bit RGB(A) image to 8-bit grayscale (+Alpha). Are you setting a screen gamma? >How can I check that? The easiest way is to bring up all three images (original, old output, new output) and compare the pixel values by hand. The few hex encoded values I looked at in your window were different by 1, and I think it's 1 in the LSBit. If gamma correction is being done then insignificant changes in the 16 bit calculations are expected and these can, and do, cause significant changes when converted to 8 bit because digitization to 8 bits is losses perceptual information (i.e. 8 bit isn't accurate enough.) Can you send me the output images (old and new) from basn2c16.png plus the transforms you set (actually, screen gamma is enough if you set it?) John Bowler <jb...@ac...> |
|
From: Alexander S. <ale...@gm...> - 2011-01-15 08:05:04
|
On Mon, Jan 10, 2011 at 18:23, <jb...@ac...> wrote: > From: Alexander Shulgin [mailto:ale...@gm...] Sorry for the delay in response. >>All these failing tests have something to do with input transformation from 16-bit RGB(A) image to 8-bit grayscale (+Alpha). > > Are you setting a screen gamma? No. >>How can I check that? > > The easiest way is to bring up all three images (original, old output, new output) and compare the pixel values by hand. The few hex encoded values I looked at in your window were different by 1, and I think it's 1 in the LSBit. If gamma correction is being done then insignificant changes in the 16 bit calculations are expected and these can, and do, cause significant changes when converted to 8 bit because digitization to 8 bits is losses perceptual information (i.e. 8 bit isn't accurate enough.) > > Can you send me the output images (old and new) from basn2c16.png plus the transforms you set (actually, screen gamma is enough if you set it?) Please find attached output images along with source code to produce them. out-lp14.png is produced when linking with libpng-1.4.4, -lp15 -- with libpng 1.5.0. -- Alex |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 13:45:05
|
I compared Alexander's images by converting them to .txt format with ImageMagick and then running a diff. There are some off-by-one differences, none larger, about evenly divided between +1 and -1. This, in pnglibconf.h, might be relevant: /*#undef PNG_READ_16_TO_8_ACCURATE_SCALE_SUPPORTED*/ In libpng14 it's enabled. Glenn |
|
From: <jb...@ac...> - 2011-01-15 16:48:23
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >I compared Alexander's images by converting them to .txt format with ImageMagick >and then running a diff. There are some off-by-one differences, none larger, about >evenly divided between +1 and -1. This needs further investigation; these are 16 bit color images being converted to greyscale, specifically: basi2c16.sng basi6a16.png basn2c16.png basn6a16.png bgan6a16.png bgyn6a16.png But, apart from the minor control flow change in png_do_rgb_to_grayscale (I don't think this affects the code at all) there's no change in the arithmetic, not even the order of evaluation. What is more the calculations are integer ones and, so far as I can see, there is no change in either png_do_read_transformations or png_do_chop (the 16 to 8 conversion), and the order is the same (greyscale first, preserving 16 bit, then scale) - obviously changing the order will change the results. The only thing I can see wrong is that png_do_rgb_to_grayscale is truncating the integer calculation, not rounding, but it does that in 1.4 as well - when that's fixed there will be lots of by 1 changes, but it hasn't been fixed yet. John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 16:56:47
|
On Sat, Jan 15, 2011 at 11:48 AM, <jb...@ac...> wrote: > so far as I can see, there is no change in either png_do_read_transformations or png_do_chop (the 16 to 8 conversion), and the order is the same (greyscale first, preserving 16 bit, then scale) - obviously changing the order will change the results. Doesn't the "greyscale first" part involve gamma-to-1 and back, when the input file has a gAMA chunk? Glenn |
|
From: <jb...@ac...> - 2011-01-15 17:57:36
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >Doesn't the "greyscale first" part involve gamma-to-1 and back, when the input file has a gAMA chunk? Yes, but there is no 'back' because the test program doesn't set a screen gamma so it shouldn't be happening. I can reproduce the problem with bas2nc16.png. Most likely I did something which is causing the code to do a no-op gamma transform, but I can't see what yet... If the code was doing the correct gamma transform it would be producing a file with a gamma of about 2 (0.5 in PNG terms), but then the file would be completely different. John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 18:28:31
|
On Sat, Jan 15, 2011 at 12:57 PM, <jb...@ac...> wrote: > From: Glenn Randers-Pehrson [mailto:gl...@gm...] >>Doesn't the "greyscale first" part involve gamma-to-1 and back, when the input file has a gAMA chunk? > > Yes, but there is no 'back' because the test program doesn't set a screen gamma so it shouldn't be happening. When doing rgb_to_gray it uses png_ptr->gamma which is the file gamma if there's no screen gamma (note the comment in png.c: /* Probably doing rgb_to_gray */) Glenn |
|
From: <jb...@ac...> - 2011-01-15 20:20:13
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >When doing rgb_to_gray it uses png_ptr->gamma which is the file gamma if there's >no screen gamma (note the comment in png.c: /* Probably doing rgb_to_gray */) Yes, I just discovered that by stepping through the code (after three hours staring at output and wondering why the result is off by one in *both* 1.4 and 1.5 - just in slightly different places!) So my conclusion is that the change is harmless - the gamma table calculation changed, so the errors move a bit, but both 1.4 and 1.5 produce about the same number of off-by-one errors (see below.) We assume that if the screen gamma is not specified by the application the file matches the screen (after much debate a few years back, I still disagree BTW ;-) Therefore we can do linearization - for either rgb to gray or for alpha composition - if either the file gAMA is given or the screen gamma is given by the app or (best) if both are present. Since the gamma algorithm changed between 1.4 and 1.5 we get different off-by-one errors. In fact I did the exact calculation for basn2c16.png and compared the 1.4 and 1.5 results. I used the integer coefficients (6868 etc) which are slightly wrong because, contrary to the comment, they are rounded: red_int = 6968; /* .212671 * 32768 + .5 */ green_int = 23434; /* .715160 * 32768 + .5 */ red_int should be 6969, green_int is correct and the derived blue_int should be 0.072169 * 32768 + .5 - 2365 - whereas it comes out as 2366. In both cases (1.4 and 1.5) about half the values are off-by-one. This is partly (but not completely) because of PNG_MAX_GAMMA_8 - which limits the 16 bit gamma correction accuracy when the output is only 8 bits (it, in fact, only uses 11 bits of each 16 bit value.) There are quite a lot of errors mounting up in there and there are sufficient to generate off-by-one frequently. This can probably be improved in the future. John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 20:39:18
|
On Sat, Jan 15, 2011 at 3:19 PM, <jb...@ac...> wrote: > > red_int should be 6969, green_int is correct and the derived blue_int should be 0.072169 * 32768 + .5 - 2365 - whereas it comes out as 2366. > > There are quite a lot of errors mounting up in there and there are sufficient to generate off-by-one frequently. This can probably be improved in the future. > What do you recommend for now (1.5.1)? Just leave things as the are for now? Glenn |
|
From: <jb...@ac...> - 2011-01-15 20:58:32
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >What do you recommend for now (1.5.1)? > >Just leave things as they are for now? Yes, the current behavior is not significantly worse (or better) than 1.4 and this can be fixed in 1.5.2 John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 21:08:52
|
On Sat, Jan 15, 2011 at 3:58 PM, <jb...@ac...> wrote: > From: Glenn Randers-Pehrson [mailto:gl...@gm...] >>What do you recommend for now (1.5.1)? >> >>Just leave things as they are for now? > > Yes, the current behavior is not significantly worse (or better) than 1.4 and > this can be fixed in 1.5.2 It is significantly worse in that the numbers aren't exactly the same, without a good reason, for people who check their results by comparing "signature". We need to at a minimum put some kind of explanation in the 1.4 to 1.5 differences section of the manual. Looking at Poynton's documents, it seems that the weights for computing Luma keep changing slightly. The numbers in the gamma FAQ aren't quite the same as those in the color FAQ, and the numbers in the PDF copy are different from those in the HTML. None of them are exactly the same as ours (copied out of a Poynton document at the time), but the numbers we are using are within that ballpark. Glenn |
|
From: <jb...@ac...> - 2011-01-15 21:35:41
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >It is significantly worse in that the numbers aren't exactly the same, without a good reason The numbers are different because the gamma correction algorithm has changed very slightly and this has changed the results in png_rgb_to_gray and in alpha composition (png_set_background for example.) The greyscale calculation hasn't changed in 1.5 - it has the same very small arithmetic errors as 1.4. This applies even if the original image was already linear (gamma == 1.0) and, therefore, it is not necessary to linearize the image. This is because libpng has *not* been changed to optimize that case correctly, yet. John Bowler <jb...@ac...> |
|
From: <jb...@ac...> - 2011-01-15 22:55:59
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >Looking at Poynton's documents, it seems that the weights for computing Luma keep changing slightly. IRC if you want luma in the sense Poynton uses it you need to encode the r,g,b values according to the NTSC formula first. He says here: http://www.poynton.com/PDFs/Transforms_among_luma_coeff.pdf " I assert that no significant performance benefit would be obtained by choosing, for HDTV, luma coefficients that theoretically match the chosen HDTV primaries. The reason that there would be no significant benefit is that the right numbers would still be used in the wrong equation! Color science provides the “correct” coefficients for a calculation performed in tristimulus (linear light) values, but in video, we perform the calculation on nonlinear (gamma corrected) values instead!" libpng, of course, uses the wrong numbers in the right equation; I think we discussed that before and agreed to continue using the wrong numbers (I still disagree here as well.) If we want to do it right then the CIE told us how a long time ago - we compute the L of CIE L*a*b. John Bowler <jb...@ac...> |
|
From: <jb...@ac...> - 2011-01-15 23:23:33
|
From: jb...@ac... [mailto:jb...@ac...] >If we want to do it right then the CIE told us how a long time ago - we compute the L of CIE L*a*b. No, that's wrong - that's an encoding too. Rgb_to_gray should calculate the CIE Y. Calculating L from that gives a number in the range 0..100 that is a perceptually linear measure of the grey level, but that's not what should be projected from a display device because the latter (Y to L) transform happens inside our brains. Anyway, if the app wants that it's simply a matter of setting the output gamma to 3 (ok, that ignores the linear part, but PNG and libpng always do that.) If we did calculate the Y the behavior would be well defined and easy to understand, whereas at present the cHRM chunk is ignored so it is difficult to say what is happening other than that it produces a tristimulus value that is something like a green. John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-01-15 13:48:32
|
My mistake, it's enabled inside /* ... */ in libpng14 (i.e., not enabled!) So that's not the explanation. Glenn On Sat, Jan 15, 2011 at 8:44 AM, Glenn Randers-Pehrson <gl...@gm...> wrote: > I compared Alexander's images by converting > them to .txt format with ImageMagick and then > running a diff. There are some off-by-one > differences, none larger, about evenly divided > between +1 and -1. > > This, in pnglibconf.h, might be relevant: > > /*#undef PNG_READ_16_TO_8_ACCURATE_SCALE_SUPPORTED*/ > > In libpng14 it's enabled. > > Glenn > |
|
From: Alexander S. <ale...@gm...> - 2011-01-15 13:52:23
|
On Sat, Jan 15, 2011 at 15:48, Glenn Randers-Pehrson <gl...@gm...> wrote: > My mistake, it's enabled inside /* ... */ in libpng14 > (i.e., not enabled!) Yes, that's what I see. > So that's not the explanation. I have my standard test result generated with libpng-1.2. Let's see what happens if I enable that macro in 1.4 (which passes all the tests w/o that) and try it. -- Alex |
|
From: <jb...@ac...> - 2011-01-15 15:50:41
Attachments:
scale-test.c
|
From: Alexander Shulgin [mailto:ale...@gm...] > I have my standard test result generated with libpng-1.2. Let's see > what happens if I enable that macro in 1.4 (which passes all the tests > w/o that) and try it. ... >Just tried. With the accurate scaling macro defined it now produces >different images for all 16 => 8 bit cases, not only 16-bit RGB(A) to 8-bit >G(A) as we see in original issue with 1.5. That's expected. The 'inaccurate' default method just drops the low bits, the accurate, and spec defined, arithmetic is: round((value/65535)*255) I.e. round(value/257). The 'inaccurate' method is wrong (off by one) in 16256 of the 65536 cases - see the attached program. Personally I've always preferred the 'inaccurate' method, it performs a slight scaling of the input color space, but so what - the output space is bigger than the input; 8 bit can represent values -.02 to 1.002, 16 bit only represents values in the range -.000008 to 1.000008! John Bowler <jb...@ac...> |
Thanks for helping keep SourceForge clean.
X