Last Comment Bug 375741 - Add support for APNG encoding
: Add support for APNG encoding
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Justin Dolske [:Dolske]
: Milan Sreckovic [:milan]
Depends on: 375733 405982 413424
  Show dependency treegraph
Reported: 2007-03-28 14:10 PDT by Justin Dolske [:Dolske]
Modified: 2008-01-21 17:59 PST (History)
20 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Draft patch (20.34 KB, patch)
2007-03-28 14:10 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Final patch. (26.00 KB, patch)
2007-04-04 17:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Unit test for APNG encoder (14.53 KB, patch)
2007-04-07 17:35 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Address review comments & combine with unit test patch. (40.16 KB, patch)
2007-04-21 21:39 PDT, Justin Dolske [:Dolske]
asmith16: review+
pavlov: superreview+
Details | Diff | Splinter Review

Description User image Justin Dolske [:Dolske] 2007-03-28 14:10:14 PDT
Created attachment 259948 [details] [diff] [review]
Draft patch

We currently have an imgIEncoder interface, and components which implement it for encoding JPEG and PNG images.

However, the interface is insufficient for encoding animations (like APNG). The interface needs to allow the caller to provide pixel data for each frame separately, as well as allowing the caller to provide metadata for each frame (eg delay, disposal method, etc.).

The attached patch is a mostly-finished work in progress. Parsing of the per-frame options isn't done yet, but some light testing show's it's able to generate proper working APNGs.
Comment 1 User image Justin Dolske [:Dolske] 2007-03-28 14:18:11 PDT
Example usage, from JS:

encoder.startImageEncode(width, height, "frames=2;repeat=5;skipfirstframe=no");

while (haveFrames) {
    encoder.addImageFrame(frame.pixels, frame.pixels.length,
             frame.width, frame.height, frame.stride,
             frame.format, "delay=500,dispose=background");

Comment 2 User image Andrew Smith 2007-03-29 19:44:23 PDT
+  // ?: spec says numFrames == # of fcTL chunks. So, if IDAT is skipped, it's not included in the count?

Hm.. that was a mistake in the spec, I fixed it. numFrames includes the hidden frame.

+  // ?: can an apng contain no IDATs?

If I understand the question: since an APNG must be a valid PNG it must contain a valid set of IDATs.
Comment 3 User image Andrew Smith 2007-03-31 17:31:42 PDT
Actually it seems the spec was right about numFrames, it's the pngdecoder implementation that was wrong. I restored the spec to the previous version and will fix the decoder in the next week or two.
Comment 4 User image Justin Dolske [:Dolske] 2007-04-04 17:37:45 PDT
Created attachment 260651 [details] [diff] [review]
Final patch.

Final patch.

For review:
- Should the "transparency=" option only be allowed in startImageEncode()? [And then internally we would use that value for each frame added.] I wasn't clear if it's a per-PNG or per-frame options.
- I'm not terribly familiar with the string api, so ParseOptions() should probably be looked at closely.
- This code assumes Andrew's fix from comment #3 is implemented... Is there a bug # for that, so it can block this bug?
Comment 5 User image Andrew Smith 2007-04-04 19:46:18 PDT
There is no bug for comment #3, the work will be done as part of bug 257197 
Comment 6 User image Justin Dolske [:Dolske] 2007-04-07 17:35:07 PDT
Created attachment 260926 [details] [diff] [review]
Unit test for APNG encoder

I think with will probably need to get updated, as the expected output will change slightly when the frame count issue (see previous comments) is fixed.
Comment 7 User image Justin Dolske [:Dolske] 2007-04-16 13:12:29 PDT
[This encoder patch depends on attachment 261579 [details] [diff] [review] to bug 257197 (the main APNG bug).]
Comment 8 User image Stuart Parmenter 2007-04-19 09:50:09 PDT
Comment on attachment 260651 [details] [diff] [review]
Final patch.

since we changed num_iterations to num_plays you might want to update the patch for that.

It also might be cleaner  to use a hashtable of options internally rather than a string...
Comment 9 User image Justin Dolske [:Dolske] 2007-04-21 21:39:22 PDT
Created attachment 262387 [details] [diff] [review]
Address review comments & combine with unit test patch.

Updated patch addresses review comments and the changes andrew made in attachment 261579 [details] [diff] [review].
Comment 10 User image Andrew Smith 2007-04-22 09:05:21 PDT
Comment on attachment 262387 [details] [diff] [review]
Address review comments & combine with unit test patch.

looks good
Comment 11 User image Phil Ringnalda (:philor) 2007-04-24 22:58:36 PDT
modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp 1.4
modules/libpr0n/encoders/png/nsPNGEncoder.cpp 1.4
modules/libpr0n/encoders/png/nsPNGEncoder.h 1.2
modules/libpr0n/public/imgIEncoder.idl 1.4
modules/libpr0n/test/unit/test_encoder_apng.js 1.1

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