682677 - --with-system-png with libpng-1.5 is broken after Bug 600556 fixed

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[ojab]

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]

Attachment: 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)]

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]

Attachment: 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)]

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]

Attachment: Change return value from PRBool to bool

Comment 6

[Ed Morley [:emorley]]

In my queue :-)

Comment 7

[Ed Morley [:emorley]]

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]]

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]

Attachment: 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]]

(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]

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]

Attachment: Does not require PNG_EASY_ACCESS_SUPPORTED

Someone please run this through Try

Comment 13

[Ed Morley [:emorley]]

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

[(no longer active)]

Joe, does this need review?

Comment 15

[Glenn Randers-Pehrson]

Parse error is because of a missing right parenthesis on the png_get_IHDR() call.

Comment 16

[Glenn Randers-Pehrson]

Attachment: Supplied missing right parenthesis

Comment 17

[Ed Morley [:emorley]]

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]]

I pushed this to Try https://tbpl.mozilla.org/?tree=Try&rev=9e3046874f19

Comment 19

[ojab]

So is it try passed or failed?

Comment 20

[ojab]

ping?

Comment 21

[Glenn Randers-Pehrson]

The try report has a few red and orange entries but I don't know what they mean.

Comment 22

[Daniel Holbert [:dholbert] (PTO until 4/14)]

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]

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]

Attachment: v04: correct location (image/decoders)

This should be run through "try" again.

Comment 25

[Daniel Holbert [:dholbert] (PTO until 4/14)]

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]

Attachment: v05: Revised checkin message

Comment 27

[Glenn Randers-Pehrson]

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)]

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]

Attachment: v06: fixed indentation, added braces, joined checkin msg lines

Comment 30

[Glenn Randers-Pehrson]

@joe, please transfer your review+ from v05 to v06 if it looks OK to you now.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aff1bd412058

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aff1bd412058