Add support for APNG encoding

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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");
    ...
}

encoder.endImageEncode();
(Assignee)

Updated

10 years ago
Depends on: 375733

Comment 2

10 years ago
+  // ?: 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

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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?
Attachment #259948 - Attachment is obsolete: true
Attachment #260651 - Flags: review?(pavlov)
(Assignee)

Updated

10 years ago
Attachment #260651 - Attachment is patch: true
Attachment #260651 - Attachment mime type: application/octet-stream → text/plain

Comment 5

10 years ago
There is no bug for comment #3, the work will be done as part of bug 257197 
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
[This encoder patch depends on attachment 261579 [details] [diff] [review] to bug 257197 (the main APNG bug).]

Comment 8

10 years ago
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...
(Assignee)

Comment 9

10 years ago
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].
Attachment #260651 - Attachment is obsolete: true
Attachment #260926 - Attachment is obsolete: true
Attachment #262387 - Flags: review?(pavlov)
Attachment #260651 - Flags: review?(pavlov)

Updated

10 years ago
Attachment #262387 - Flags: review?(pavlov) → review+

Updated

10 years ago
Attachment #262387 - Flags: superreview+
Attachment #262387 - Flags: review?(asmith15)
Attachment #262387 - Flags: review+

Comment 10

10 years ago
Comment on attachment 262387 [details] [diff] [review]
Address review comments & combine with unit test patch.

looks good
Attachment #262387 - Flags: review?(asmith15) → review+
(Assignee)

Updated

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Whiteboard: [checkin needed]
Flags: in-testsuite+

Updated

10 years ago
Depends on: 405982
(Assignee)

Updated

10 years ago
Depends on: 413424
You need to log in before you can comment on or make changes to this bug.