871671 - APNGs with a hidden first frame don't load from chrome: or if their size is 16K or below

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[[e-mail]]

In SeaMonkey we use two animated PNGs:
chrome://communicator/skin/brand/throbber-anim.png
chrome://communicator/skin/brand/throbber-anim16.png

However in my local builds they have stopped displaying in the UI some time in the last week or so. (I don't know whether packaged builds are affected.)

If you enter one of those URLs then the image does not appear, but if you then save the image to a file and open the file the image then appears normally. The images also display normally when served over http e.g. via the given MXR link (provided just in case the image has an error in it).

Comment 1

[Frank Wein [:mcsmurf]]

Additional information: Loading the Firefox loading... animation (chrome://global/skin/icons/loading_16.png) works fine in FF nightly build.

Comment 2

[[e-mail]]

Philip Chee pointed out the line of code that reports the console error. I set a breakpoint on that line and looked at the Decoder object:
mRefCnt: 3
mImage: (set)
mCurrentFrame: (set)
mObserver: (imgStatusTrackerObserver)
mImageMetadata: -1, -1, -1, 32, 32
mImageData: (set)
mImageDataLength: 0x1000
mColormap: (null)
mColormapSize: (unset)
mDecodeFlags: 0
mDecodeDone: false
mDataError: false
mFrameCount: 1
mInvalidRect: 0, 0, 0, 0
mFailCode: NS_OK
mNewFrameData: 0, 0, 0, 32, 32, ARGB32, 0
mNeedsNewFrame: false
mInitialized: true
mSizeDecode: false
mInFrame: false
mIsAnimated: false
mSynchronous: true

As an nsPNGDecoder object it also claims to have an nNumFrames of 0.

Comment 3

[[e-mail]]

I found that I'm hitting this line of code:

http://mxr.mozilla.org/comm-central/source/mozilla/image/decoders/nsPNGDecoder.cpp#640

> 635 #ifdef PNG_APNG_SUPPORTED
> 636   if (png_get_valid(png_ptr, info_ptr, PNG_INFO_acTL))
> 637     png_set_progressive_frame_fn(png_ptr, nsPNGDecoder::frame_info_callback, NULL);
> 638 
> 639   if (png_get_first_frame_is_hidden(png_ptr, info_ptr)) {
> 640     decoder->mFrameIsHidden = true;
> 641   } else {
> 642 #endif
> 643     decoder->CreateFrame(0, 0, width, height, decoder->format);
> 644 #ifdef PNG_APNG_SUPPORTED
> 645   }
> 646 #endif

If I use the debugger to jump to line 643 then the image loads correctly!

Comment 4

[[e-mail]]

... and that's being set because we have a PNG_INFO_acTL but no PNG_INFO_fcTL yet, although the APNG spec on wikimo says that our fcTL should come before the IDAT.

Comment 5

[Frank Wein [:mcsmurf]]

I backed out Bug 869125 locally and the bug is gone when displaying chrome://communicator/skin/brand/throbber-anim.png. I reverted the backout and the bug appears again. Though I'm not really sure what the chrome:// protocol has to do with the bug. But as Comment 4 says this might actually be an error in the image itself.

Comment 6

[[e-mail]]

So, there are two bugs:

1. APNGs don't always load if their IDAT comes before their fcTL. According to Wikimo, this is valid, but results in the first frame being skipped.

2. SeaMonkey's APNGs have their IDAT before their fcTL. I will file a separate bug on this. Anyone know how to rearrange PNG chunks?

Comment 7

[Frank Wein [:mcsmurf]]

Joe: Can you take a look at our analysis here (Comment 4, 5 and 6 are the relevant comments)? We figured out almost everything already (except how to fix the bug if it is one ;) see the URL where you can download the APNG file. We don't know though why it only fails when loading the image via chrome:// URL.

Comment 8

[[e-mail]]

Actually those images have keyframes that isn't part of the animation, so the chunks appear to be correct after all.

(Rather confusingly, you can only see the keyframe by opening the file in an APNG-unaware application, as if you disable animation you just get the first frame data.)

Interestingly if I swap the fcTL and the IDAT I then get an animating image (erroneously including the keyframe of course) but wikimo says that I should have needed two fcTL chunks in that case.

Comment 9

[Glenn Randers-Pehrson]

(In reply to [e-mail] from comment #6)

> 2. SeaMonkey's APNGs have their IDAT before their fcTL. I will file a
> separate bug on this. Anyone know how to rearrange PNG chunks?

Yes, "pngsplit" followed by "cat" will do it.  Pngsplit is distributed
with pngcheck (look in the "gpl" directory).

But there is nothing wrong with IDAT before fcTL; that just means skip
the IDAT frame if showing the animation.

Comment 10

[[e-mail]]

OK, so when loading from file I still hit those places in the decode, so that's not where the problem lies...

Comment 11

[[e-mail]]

OK, so this is weird.

For the chrome: path, OnImageDataComplete calls DoImageDataComplete calls DecodeUntilSizeAvailable calls FinishedSomeDecoding calls SyncDecode calls DecodeSomeData calls WriteToDecoder. All the bytes in the image are written to the decoder, although the decoder only processes the first frame, which gets ignored because it is hidden. However FinishedSomeDecoding thinks the image should be ready because that all of the bytes have been processed. Things go downhill from there on.

For the file: path the decode path is a little more complex. We do eventually wind up in WriteToDecoder, but because we're not doing a size decode, we only decode the first 0x4000. This fools FinishedSomeDecoding and therefore the image process continues OK... as long as your APNG is over 16K of course; if your APNG is 16K or less then you're hosed.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: Only pause the PNG decoder when we really need a new frame.

Some time in the debugger enabled me to track this down. nsPNGDecoder::frame_info_callback doesn't verify that it actually needs a new frame before pausing the PNG decoder. In general, the rule of thumb should be that we call png_process_data_pause iff NeedsNewFrame() returns true, and I've updated both callsites to reflect this. (The additional condition in info_callback seems superfluous.)

Comment 13

[[e-mail]]

Comment on attachment 752028 [details] [diff] [review]
Only pause the PNG decoder when we really need a new frame.

I just tried this out and it seems to fix the problem.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 752028 [details] [diff] [review]
Only pause the PNG decoder when we really need a new frame.

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

Modified Binary File: browser/themes/osx/sync-throbber.png - probably not optimal

I'm a little surprised that this actually makes a difference - which path in CreateFrame() did this go down? What was mNumFrames?

Comment 15

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14)
> I'm a little surprised that this actually makes a difference - which path in
> CreateFrame() did this go down? What was mNumFrames?

I was surprised initially too - that's why I was confused about what was going on in our earlier IRC conversation. The explanation is pretty simple though. We don't create a new frame the first time through info_callback for two reasons: (1) the first frame is hidden, and (2) we have the frame created by the decoding infrastructure sitting around. On the first time though frame_info_callback, then, we still have the initial frame, and that means that CreateFrame doesn't create a new one and NeedsNewFrame will return false at that point. If we unconditionally call png_process_data_pause at that point without checking NeedsNewFrame, we end up bailing out to Decoder::Write. Decoder::Write sees that the decoder has stopped processing the image and that it hasn't requested a new frame, and it returns. However, the caller expects that if Decoder::Write accepted all the data in the image and returns without an error, decoding is done! So we never decode the rest of the image.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14)
> Modified Binary File: browser/themes/osx/sync-throbber.png - probably not
> optimal

Ack - not intended to be part of the patch! Thanks for catching that.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: Only pause the PNG decoder when we really need a new frame.

Remove unintended throbber change.

Comment 18

[Seth Fowler [:seth] [:s2h]]

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

Comment 19

[Ed Morley [:emorley]]

Push backed out for assertions:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fea91386092f

https://hg.mozilla.org/integration/mozilla-inbound/rev/791a6f7be62f

Comment 20

[Seth Fowler [:seth] [:s2h]]

Repushed. It was the other patch in that push that caused the assertion issue.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c499c83036c

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c499c83036c

Comment 22

[Seth Fowler [:seth] [:s2h]]

Looks like this is doing OK on central. The bug it fixes is in firefox23, so this needs to be uplifted.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 752079 [details] [diff] [review]
Only pause the PNG decoder when we really need a new frame.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 869125
User impact if declined: Some APNG images will not decode correctly.
Testing completed (on m-c, etc.): On m-c with no reported issues.
Risk to taking this patch (and alternatives if risky): Very low. The change is small and simple.
String or IDL/UUID changes made by this patch: None.

Comment 24

[Frank Wein [:mcsmurf]]

Pushed: https://hg.mozilla.org/releases/mozilla-aurora/rev/375b868fadf9

Comment 25

[u279076]

Neil, this should be fixed now in Firefox 23. Can you check?

Comment 26

[[e-mail]]

(In reply to Anthony Hughes, Mozilla QA from comment #25)
> Neil, this should be fixed now in Firefox 23. Can you check?

My test cases animate in a Gecko 23 build, yes.

Comment 27

[u279076]

(In reply to [e-mail] from comment #26)
> My test cases animate in a Gecko 23 build, yes.

Thanks!