Closed Bug 1287691 Opened 3 years ago Closed 3 years ago

Yield after decoding each frame of an animated image

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

To support streaming decoding of animated images, we need to be able to decode them frame by frame. In this bug we'll add support for yielding after decoding each frame of an animated image. This is straightforward now that bug 1287246 has landed.
In the first patch we just need to expose LexerResult to code that calls into
Decoder::Decode(). That allows that code to distinguish between a decoder
needing more data (Yield::NEED_MORE_DATA) and a decoder yielding because some
output is ready (Yield::OUTPUT_AVAILABLE).

This patch updates the DecodingTask implementations to deal with LexerResults
and to handle the new type of yield. At the moment, they all ignore
Yield::OUTPUT_AVAILABLE and just loop, so nothing changes behavior-wise.

(Actually, that's not entirely true. We will also notify the decoder's owning
image if there are any new progress bits set between each yield. After parts 2
and 3, that means "between each frame". This is mostly about firing the
FLAG_FRAME_COMPLETE progress bit as early as possible, since that's what tells
layout that at least one frame of the image is available to draw. See part 4 for
a little more about this.)
Attachment #8772302 - Flags: review?(edwin)
This patch implements yielding after each frame in the GIF decoder, using
StreamingLexer's new yielding support. If only every patch were this easy.
Attachment #8772303 - Flags: review?(edwin)
This patch does the same for the PNG decoder. Unfortunately this is a little
more complicated, because we have to set up some infrastructure. In particular:

- We need a way to send LexerTransitions from inside of libpng callbacks to
  nsPNGDecoder::ReadPNGData(). That's how we tell StreamingLexer to yield, or
  notify it that we've entered a terminal state. All we need is a member
  variable on nsPNGDecoder.

- We need to add a struct that saves the parameters of CreateFrame() so we can
  call it after yielding. This is much easier than trying to yield *before* we
  get notified of the new frame by libpng; libpng's API doesn't make that easy.
  We want to delay calling CreateFrame() because the whole point of yielding is
  to let the code that's using the decoder do something useful with the newly
  finished frame. If we called CreateFrame() right away, they'd no longer have
  access to that frame via Decoder::GetCurrentFrameRef(), and other decoder
  state would've changed to suit the new frame.

- Finally, we need to save the size of the last chunk of data that was passed to
  ReadPNGData(). This is just a mistake on my part; I had intended to make the
  API that StreamingLexer offered for yielding during unbuffered reads match
  what libpng needed exactly, but I misremembered what libpng did. =)
  StreamingLexer needs to know how many bytes were consumed, while libpng tells
  you how many bytes *weren't* consumed. I'll fix StreamingLexer to work that
  way, and then we can get rid of |mLastChunkLength|, but I don't think the
  issue is urgent enough to worry about fixing that before landing. It will get
  cleaner soon, though.
Attachment #8772306 - Flags: review?(edwin)
So as mentioned in part 1, we want to let a decoder's owning know about any new
progress bits we encounter after decoding each frame. As I mentioned, this is
mostly about setting FLAG_FRAME_COMPLETE as soon as possible, so layout can
start painting the image and animations can start.

We had an existing hack to do this. This patch removes it, since part 1 handles
this problem much more cleanly.
Attachment #8772307 - Flags: review?(edwin)
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a8e7ea0fe5
(Part 1) - Expose yielding to decoding tasks. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd6857c728d
(Part 2) - Yield after each frame in the GIF decoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd2f3d22e5d
(Part 3) - Yield after each frame in the PNG decoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe259a08d225
(Part 4) - Leave notifying decoding progress for each frame to DecodingTask. r=edwin
backed out due to Windows build failure.
http://hg.mozilla.org/integration/mozilla-inbound/rev/b925c4294d44


https://treeherder.mozilla.org/logviewer.html#?job_id=32151415&repo=mozilla-inbound#L8567

 18:09:06     INFO -  c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\image\decoders\nsPNGDecoder.h(68): error C2220: warning treated as error - no 'object' file generated
18:09:06 INFO - Warning: C4002 in c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\image\decoders\nsPNGDecoder.h: too many actual parameters for macro 'Yield'
...
Blocks: 1288040
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7159dd41f1
(Part 1) - Expose yielding to decoding tasks. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ec31db260e
(Part 2) - Yield after each frame in the GIF decoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/2448661e7725
(Part 3) - Yield after each frame in the PNG decoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f518461663cd
(Part 4) - Leave notifying decoding progress for each frame to DecodingTask. r=edwin
Just realized I forgot to attach the updated version of part 3 that fixes the
Windows build failure. Just needed to rename nsPNGDecoder::Yield() to
nsPNGDecoder::DoYield().
Attachment #8772306 - Attachment is obsolete: true
Flags: needinfo?(mozilla.bugzilla)
Blocks: 1291059
You need to log in before you can comment on or make changes to this bug.