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]
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 | Review
Final patch. (26.00 KB, patch)
2007-04-04 17:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Unit test for APNG encoder (14.53 KB, patch)
2007-04-07 17:35 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | 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 | Review

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