Closed Bug 375741 Opened 13 years ago Closed 13 years ago

Add support for APNG encoding


(Core :: ImageLib, defect)

Not set





(Reporter: Dolske, Assigned: Dolske)




(1 file, 3 obsolete files)

Attached patch Draft patch (obsolete) — Splinter Review
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.
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");

Depends on: 375733
+  // ?: 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.
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.
Attached patch Final patch. (obsolete) — Splinter Review
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?
Attachment #259948 - Attachment is obsolete: true
Attachment #260651 - Flags: review?(pavlov)
Attachment #260651 - Attachment is patch: true
Attachment #260651 - Attachment mime type: application/octet-stream → text/plain
There is no bug for comment #3, the work will be done as part of bug 257197 
Attached patch Unit test for APNG encoder (obsolete) — Splinter Review
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.
[This encoder patch depends on attachment 261579 [details] [diff] [review] to bug 257197 (the main APNG bug).]
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...
Updated patch addresses review comments and the changes andrew made in attachment 261579 [details] [diff] [review].
Attachment #260651 - Attachment is obsolete: true
Attachment #260926 - Attachment is obsolete: true
Attachment #262387 - Flags: review?(pavlov)
Attachment #260651 - Flags: review?(pavlov)
Attachment #262387 - Flags: review?(pavlov) → review+
Attachment #262387 - Flags: superreview+
Attachment #262387 - Flags: review?(asmith15)
Attachment #262387 - Flags: review+
Comment on attachment 262387 [details] [diff] [review]
Address review comments & combine with unit test patch.

looks good
Attachment #262387 - Flags: review?(asmith15) → review+
Whiteboard: [checkin needed]
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
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Flags: in-testsuite+
Depends on: 405982
Depends on: 413424
You need to log in before you can comment on or make changes to this bug.