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

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

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.

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!

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

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.

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?

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.

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.

(In reply to neil@parkwaycc.co.uk 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.

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

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.

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

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

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

Remove unintended throbber change.

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

Repushed. It was the other patch in that push that caused the assertion issue. https://hg.mozilla.org/integration/mozilla-inbound/rev/9c499c83036c

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

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.

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

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

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

(In reply to neil@parkwaycc.co.uk from comment #26) > My test cases animate in a Gecko 23 build, yes. Thanks!