The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ojab, Assigned: Glenn Randers-Pehrson)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
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'
(Reporter)

Updated

6 years ago
Blocks: 600556
(Reporter)

Comment 1

6 years ago
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.
Attachment #556537 - Flags: review?
Attachment #556537 - Flags: review? → review?(joe)
Assignee: nobody → ojab
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #556537 - Flags: review?(joe) → review+
(Reporter)

Comment 3

6 years ago
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?
Attachment #556537 - Attachment is obsolete: true
Attachment #556742 - Flags: review?(joe)
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.
Attachment #556742 - Flags: review?(joe) → review+
(Reporter)

Comment 5

6 years ago
Created attachment 557204 [details] [diff] [review]
Change return value from PRBool to bool
Attachment #556742 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
In my queue :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
Version: unspecified → Trunk
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 :-)
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 :-)
(Reporter)

Comment 9

6 years ago
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?
Attachment #557204 - Attachment is obsolete: true
(In reply to ojab from comment #9)
> should I request review again?

Might be worth doing so, just in case :-)
(Reporter)

Updated

6 years ago
Attachment #558454 - Flags: review?(joe)
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Created attachment 558585 [details] [diff] [review]
Does not require PNG_EASY_ACCESS_SUPPORTED

Someone please run this through Try
Attachment #558454 - Attachment is obsolete: true
Attachment #558454 - Flags: review?(joe)
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
Joe, does this need review?
(Assignee)

Comment 15

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

Comment 16

6 years ago
Created attachment 558683 [details] [diff] [review]
Supplied missing right parenthesis
Attachment #558585 - Attachment is obsolete: true
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
I pushed this to Try https://tbpl.mozilla.org/?tree=Try&rev=9e3046874f19
(Reporter)

Comment 19

6 years ago
So is it try passed or failed?
Assignee: ojab → glennrp+bmo
(Reporter)

Comment 20

6 years ago
ping?
(Assignee)

Comment 21

6 years ago
The try report has a few red and orange entries but I don't know what they mean.
You can ignore those reds/oranges -- edmorley starred them all as known intermittent issues. (so, the patch here wasn't responsible for them)
(Assignee)

Comment 23

6 years ago
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.
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 24

6 years ago
Created attachment 568764 [details] [diff] [review]
v04: correct location (image/decoders)

This should be run through "try" again.
Attachment #558683 - Attachment is obsolete: true
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?
(Assignee)

Comment 26

6 years ago
Created attachment 568838 [details] [diff] [review]
v05: Revised checkin message
Attachment #568764 - Attachment is obsolete: true
(Assignee)

Comment 27

6 years ago
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.
Attachment #568838 - Flags: review?(joe)
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!
Attachment #568838 - Flags: review?(joe) → review+
(Assignee)

Comment 29

6 years ago
Created attachment 571851 [details] [diff] [review]
v06: fixed indentation, added braces, joined checkin msg lines
(Assignee)

Comment 30

6 years ago
@joe, please transfer your review+ from v05 to v06 if it looks OK to you now.
Attachment #571851 - Flags: review+
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #568838 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/aff1bd412058
Keywords: checkin-needed
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/aff1bd412058
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 770122
You need to log in before you can comment on or make changes to this bug.