Last Comment Bug 682677 - --with-system-png with libpng-1.5 is broken after Bug 600556 fixed
: --with-system-png with libpng-1.5 is broken after Bug 600556 fixed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Glenn Randers-Pehrson
:
Mentors:
Depends on: 770122
Blocks: 600556
  Show dependency treegraph
 
Reported: 2011-08-28 00:37 PDT by ojab
Modified: 2012-07-03 01:00 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use png_get_* instead of direct access to png_info (1.42 KB, patch)
2011-08-29 06:42 PDT, ojab
joe: review+
Details | Diff | Review
Updated patch (1.50 KB, patch)
2011-08-29 19:19 PDT, ojab
joe: review+
Details | Diff | Review
Change return value from PRBool to bool (1.50 KB, patch)
2011-08-31 09:06 PDT, ojab
no flags Details | Diff | Review
Do not define PNG_NO_EASY_ACCESS (2.09 KB, patch)
2011-09-06 05:53 PDT, ojab
no flags Details | Diff | Review
Does not require PNG_EASY_ACCESS_SUPPORTED (2.56 KB, patch)
2011-09-06 13:34 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Review
Supplied missing right parenthesis (2.56 KB, patch)
2011-09-06 17:34 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Review
v04: correct location (image/decoders) (2.35 KB, patch)
2011-10-21 13:55 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Review
v05: Revised checkin message (2.44 KB, patch)
2011-10-21 19:41 PDT, Glenn Randers-Pehrson
joe: review+
Details | Diff | Review
v06: fixed indentation, added braces, joined checkin msg lines (2.58 KB, patch)
2011-11-03 17:35 PDT, Glenn Randers-Pehrson
joe: review+
Details | Diff | Review

Description ojab 2011-08-28 00:37:19 PDT
linux x86_64, gcc-4.6.1, libpng-1.5.4 (w/ apng patch).

In file included from /sources/mozilla-central/modules/libpr0n/src/RasterImage.cpp:61:0:
/sources/mozilla-central/modules/libpr0n/decoders/nsPNGDecoder.h: In member function ‘bool mozilla::imagelib::nsPNGDecoder::HasValidInfo() const’:
/sources/mozilla-central/modules/libpr0n/decoders/nsPNGDecoder.h:78:26: error: invalid use of incomplete type ‘png_info {aka struct png_info_def}’
/usr/include/libpng15/png.h:696:16: error: forward declaration of ‘png_info {aka struct png_info_def}’
/sources/mozilla-central/modules/libpr0n/decoders/nsPNGDecoder.h: In member function ‘PRInt32 mozilla::imagelib::nsPNGDecoder::GetPixelDepth() const’:
/sources/mozilla-central/modules/libpr0n/decoders/nsPNGDecoder.h:87:17: error: invalid use of incomplete type ‘png_info {aka struct png_info_def}’
/usr/include/libpng15/png.h:696:16: error: forward declaration of ‘png_info {aka struct png_info_def}’
make[7]: *** [RasterImage.o] Error 1
make[7]: Leaving directory `/home/ojab/opt/xulrunner/modules/libpr0n/src'
make[6]: *** [src_libs] Error 2
make[6]: Leaving directory `/home/ojab/opt/xulrunner/modules/libpr0n'
Comment 1 ojab 2011-08-29 06:42:15 PDT
Created attachment 556537 [details] [diff] [review]
Use png_get_* instead of direct access to png_info

Don't know who should be reviewer for this.
Comment 2 Joe Drew (not getting mail) 2011-08-29 13:11:39 PDT
Comment on attachment 556537 [details] [diff] [review]
Use png_get_* instead of direct access to png_info

Review of attachment 556537 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/decoders/nsPNGDecoder.h
@@ +72,4 @@
>  
>    void EndImageFrame();
>  
> +  // Check if PNG is valid ICO (32bpp RGBA)

Update this to say something more along the lines of "Check if this PNG can be used in an ICO (is 32 bpp RGBA)" and r=me.
Comment 3 ojab 2011-08-29 19:19:25 PDT
Created attachment 556742 [details] [diff] [review]
Updated patch

Added link to MSDN blogs, just like in http://hg.mozilla.org/mozilla-central/rev/1f86f6af9434#l3.307 (should it be comments in both places?)
Also there is a function returning bool in original code, but PRBool in my patch. Which should be used?
Comment 4 Joe Drew (not getting mail) 2011-08-31 08:19:21 PDT
Comment on attachment 556742 [details] [diff] [review]
Updated patch

Review of attachment 556742 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/decoders/nsPNGDecoder.h
@@ +73,5 @@
>    void EndImageFrame();
>  
> +  // Check if PNG is valid ICO (32bpp RGBA)
> +  // http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
> +  PRBool IsValidICO() const

bool's better.
Comment 5 ojab 2011-08-31 09:06:47 PDT
Created attachment 557204 [details] [diff] [review]
Change return value from PRBool to bool
Comment 6 Ed Morley [:emorley] 2011-08-31 15:04:40 PDT
In my queue :-)
Comment 7 Ed Morley [:emorley] 2011-08-31 15:24:32 PDT
Meant to say - to save time for future patches, could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)
Comment 8 Ed Morley [:emorley] 2011-08-31 16:27:58 PDT
Failed try on multiple platforms:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=1aa55d709d0d
{
In file included from /builds/slave/try-osx-dbg/build/modules/libpr0n/decoders/nsPNGDecoder.cpp:44:
/builds/slave/try-osx-dbg/build/modules/libpr0n/decoders/nsPNGDecoder.h: In member function 'bool mozilla::imagelib::nsPNGDecoder::IsValidICO() const':
/builds/slave/try-osx-dbg/build/modules/libpr0n/decoders/nsPNGDecoder.h:79: error: 'MOZ_PNG_get_color_type' was not declared in this scope
/builds/slave/try-osx-dbg/build/modules/libpr0n/decoders/nsPNGDecoder.h:80: error: 'MOZ_PNG_get_bit_depth' was not declared in this scope
/builds/slave/try-osx-dbg/build/modules/libpr0n/src/Decoder.h: In constructor 'mozilla::imagelib::Decoder::Decoder()':
/builds/slave/try-osx-dbg/build/modules/libpr0n/src/Decoder.h:217: warning: 'mozilla::imagelib::Decoder::mInFrame' will be initialized after
/builds/slave/try-osx-dbg/build/modules/libpr0n/src/Decoder.h:205: warning:   'bool mozilla::imagelib::Decoder::mDecodeDone'
/builds/slave/try-osx-dbg/build/modules/libpr0n/src/Decoder.cpp:47: warning:   when initialized here
make[6]: *** [nsPNGDecoder.o] Error 1
}

After fixing, can you build locally/send to try (or ask me to) to confirm & adjust your hgrc (comment 7) before asking for checkin-needed. Thanks :-)
Comment 9 ojab 2011-09-06 05:53:38 PDT
Created attachment 558454 [details] [diff] [review]
Do not define PNG_NO_EASY_ACCESS

HG MQ just don't want to write my username in patch, `hg export` output with username in the attached file (now tested with both bundled & external libpng).
In this patch I've removed PNG_NO_EASY_ACCESS define from mozpngconf.h, (it was there from Bug 208607), should I request review again?
Comment 10 Ed Morley [:emorley] 2011-09-06 05:56:52 PDT
(In reply to ojab from comment #9)
> should I request review again?

Might be worth doing so, just in case :-)
Comment 11 Glenn Randers-Pehrson 2011-09-06 07:59:36 PDT
Undefining PNG_NO_EASY_ACCESS brings in about 400 lines of code in pngget.c.
It might be more efficient to use png_get_IHDR() to retrieve the bit-depth
and color-type.
Comment 12 Glenn Randers-Pehrson 2011-09-06 13:34:03 PDT
Created attachment 558585 [details] [diff] [review]
Does not require PNG_EASY_ACCESS_SUPPORTED

Someone please run this through Try
Comment 13 Ed Morley [:emorley] 2011-09-06 14:31:43 PDT
Was going to push to try but get parse error on qimport.
Perhaps check out the hgrc suggestions here? 
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-06 15:20:53 PDT
Joe, does this need review?
Comment 15 Glenn Randers-Pehrson 2011-09-06 17:22:37 PDT
Parse error is because of a missing right parenthesis on the png_get_IHDR() call.
Comment 16 Glenn Randers-Pehrson 2011-09-06 17:34:56 PDT
Created attachment 558683 [details] [diff] [review]
Supplied missing right parenthesis
Comment 17 Ed Morley [:emorley] 2011-09-07 00:50:38 PDT
That's not the reason (the patch would have imported, it just would have failed try - so good that's found too :-) , you need to use mercurial diff instead.

Errors while parsing patch:
unknown patch content: "diff -u8p -r -N -x '.mozconfig*' -x configure src-central/modules/libpr0n/decoders/nsICODecoder.cpp src/modules/libpr0n/decoders/nsICODecoder.cpp\n"

ie: Instead of:
diff -u8p -r -N -x '.mozconfig*' -x configure src-central/modules/libpr0n/decoders/nsICODecoder.cpp src/modules/libpr0n/decoders/nsICODecoder.cpp
--- src-central/modules/libpr0n/decoders/nsICODecoder.cpp	2011-09-05 09:00:54.000000000 -0500
+++ src/modules/libpr0n/decoders/nsICODecoder.cpp	2011-09-06 13:08:58.000000000 -0500

the output should look like:
diff --git a/modules/libpr0n/decoders/nsICODecoder.cpp b/modules/libpr0n/decoders/nsICODecoder.cpp
--- a/modules/libpr0n/decoders/nsICODecoder.cpp
+++ b/modules/libpr0n/decoders/nsICODecoder.cpp
Comment 18 Jonathan Watt [:jwatt] 2011-09-25 11:20:10 PDT
I pushed this to Try https://tbpl.mozilla.org/?tree=Try&rev=9e3046874f19
Comment 19 ojab 2011-09-27 09:38:23 PDT
So is it try passed or failed?
Comment 20 ojab 2011-10-18 00:19:43 PDT
ping?
Comment 21 Glenn Randers-Pehrson 2011-10-18 04:28:38 PDT
The try report has a few red and orange entries but I don't know what they mean.
Comment 22 Daniel Holbert [:dholbert] 2011-10-21 10:42:37 PDT
You can ignore those reds/oranges -- edmorley starred them all as known intermittent issues. (so, the patch here wasn't responsible for them)
Comment 23 Glenn Randers-Pehrson 2011-10-21 11:03:43 PDT
Thanks.  I have to redo the patch, though, because it doesn't conform to the
latest (mercurial) patch format and also decoders/nsICODecoder.cpp has been recently
relocated to the image directory.
Comment 24 Glenn Randers-Pehrson 2011-10-21 13:55:03 PDT
Created attachment 568764 [details] [diff] [review]
v04: correct location (image/decoders)

This should be run through "try" again.
Comment 25 Daniel Holbert [:dholbert] 2011-10-21 14:01:43 PDT
Comment on attachment 568764 [details] [diff] [review]
v04: correct location (image/decoders)

Triggered a full Try run for this:
  https://tbpl.mozilla.org/?tree=Try&rev=71a3c4b8649c

Also, RE the commit message:
>Bug 682677 - --with-system-png with libpng-1.5 is broken

Generally, m-c commit messages are supposed to summarize the change, rather than summarizing what was broken.  Could you update that before this lands?
Comment 26 Glenn Randers-Pehrson 2011-10-21 19:41:12 PDT
Created attachment 568838 [details] [diff] [review]
v05: Revised checkin message
Comment 27 Glenn Randers-Pehrson 2011-10-21 19:43:43 PDT
Comment on attachment 568838 [details] [diff] [review]
v05: Revised checkin message

Tryserver again shows a few failures that seem to be unrelated to this patch.
Comment 28 Joe Drew (not getting mail) 2011-11-03 15:39:13 PDT
Comment on attachment 568838 [details] [diff] [review]
v05: Revised checkin message

Review of attachment 568838 [details] [diff] [review]:
-----------------------------------------------------------------

Also make sure your checkin comment is on one line, even if it's way more than 80 characters.

::: image/decoders/nsPNGDecoder.h
@@ +84,5 @@
> +    int png_bit_depth,
> +        png_color_type;
> +
> +    if (png_get_IHDR(mPNG, mInfo, &png_width, &png_height, &png_bit_depth,
> +          &png_color_type, NULL, NULL, NULL))

Please indent this correctly. And, for good measure, can you add braces around the if and else bodies?

@@ +87,5 @@
> +    if (png_get_IHDR(mPNG, mInfo, &png_width, &png_height, &png_bit_depth,
> +          &png_color_type, NULL, NULL, NULL))
> +
> +      return (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA &&
> +            png_bit_depth == 8);

Same here.

@@ +90,5 @@
> +      return (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA &&
> +            png_bit_depth == 8);
> +
> +    else
> +      return PR_FALSE;

should be false now; we finally switched!
Comment 29 Glenn Randers-Pehrson 2011-11-03 17:35:35 PDT
Created attachment 571851 [details] [diff] [review]
v06: fixed indentation, added braces, joined checkin msg lines
Comment 30 Glenn Randers-Pehrson 2011-11-03 17:43:06 PDT
@joe, please transfer your review+ from v05 to v06 if it looks OK to you now.
Comment 32 Ed Morley [:emorley] 2011-11-06 05:39:30 PST
https://hg.mozilla.org/mozilla-central/rev/aff1bd412058

Note You need to log in before you can comment on or make changes to this bug.