Correctly sync decode even if an imgFrame is partially decoded

RESOLVED FIXED in mozilla37

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
In the post-SurfaceCache world there are two cases for sync decoding.

Case 1: LookupFrame doesn't find the frame and we have FLAG_SYNC_DECODE set. In this situation we will run a sync decode in WantDecodedFrames and everything works fine.

Case 2: LookupFrame *does* find the frame, but it's not yet fully decoded. (imgFrame->ImageComplete() returns false). Right now LookupFrame doesn't realize that the frame isn't fully decoded, and returns it. We failed to sync decode!

In practice this doesn't bite us much because RasterImage::Draw calls SyncDecode before it calls LookupFrame. That means we could only hit this if we called LookupFrame with FLAG_SYNC_DECODE set via another path (GetFrame, for example) while an async decode was running but not yet complete.

However, in bug 1079627, we're going to remove the call to SyncDecode in RasterImage::Draw and rely upon LookupFrame to handle sync decoding correctly. That means that we'd hit this a lot more. So we'll fix it now.

The implementation strategy is simple. From the perspective of the decoder, all that has changed is that it's now required to call either imgFrame::Finish() (which we already called in PostFrameStop) or imgFrame::Abort() (which we now call when we encounter an error). These calls signal that the imgFrame is complete and we aren't going to draw to it anymore.

Then, when code like LookupFrame wants to sync decode, it just has to call imgFrame::WaitUntilComplete(). WaitUntilComplete returns immediately if the imgFrame is already complete, but if not, it blocks until imgFrame::Abort or imgFrame::Finish is called.

With this change, sync decoding should *finally* be guaranteed to always work, which is extremely important.

(This obviously requires that decoding happens off-main-thread and that decoders do not need to block on the main thread to do things like allocate frames. Fortunately, that's the case after bug 1116747 and bug 1116735.)
(Assignee)

Comment 1

3 years ago
Created attachment 8544314 [details] [diff] [review]
Correctly sync decode even if an imgFrame is partially decoded

Here's the patch.

I renamed imgFrame::ImageComplete() to imgFrame::IsImageComplete() as part of this patch, because I thought that the distinction between imgFrame::ImageComplete and imgFrame::Finish was unclear otherwise, and it's really important that you call the right one! (There are multiple assertions to help ensure that you do, though.)
Attachment #8544314 - Flags: review?(tnikkel)
(Assignee)

Comment 2

3 years ago
Created attachment 8544470 [details] [diff] [review]
Correctly sync decode even if an imgFrame is partially decoded

Backporting an important fix from a later patch in my queue: we must have all
the source data before we try to wait on a partially decoded imgFrame. Although
doing so would be safe (since both the decoders and the necko events are
delivered off-main-thread), it would mean blocking the main thread on network
I/O, which is unacceptable. Callers which need sync decoding need to wait for
the image's load event first.
Attachment #8544470 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8544314 - Attachment is obsolete: true
Attachment #8544314 - Flags: review?(tnikkel)
Attachment #8544470 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 3

3 years ago
Thanks for the review! Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=8deb81d59a6c
(Assignee)

Comment 4

3 years ago
Created attachment 8546395 [details] [diff] [review]
Correctly sync decode even if an imgFrame is partially decoded

Fixed an issue where we weren't marked mCompositingPrevFrame as finished in
FrameAnimator::DoBlend. As long as I was at it, updated the code at the end of
DoBlend to call imgFrame::Finish instead of imgFrame::ImageUpdated(), as that's
more explicit.
(Assignee)

Updated

3 years ago
Attachment #8544470 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Try job here looks green:

https://tbpl.mozilla.org/?tree=Try&rev=5e5e179f962e

Went ahead and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/710d6d8b8f1c
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b74c9afa3b4b since one of this changes caused test failures on windows like:

https://treeherder.mozilla.org/logviewer.html#?job_id=5263429&repo=mozilla-inbound
and
https://treeherder.mozilla.org/logviewer.html#?job_id=5263747&repo=mozilla-inbound
Flags: needinfo?(seth)
(Assignee)

Comment 7

3 years ago
Created attachment 8546901 [details] [diff] [review]
Correctly sync decode even if an imgFrame is partially decoded

The failure this got backed out for was caused because we didn't properly mark the imgFrame as aborted if creating a DrawTarget failed in imgFrame::InitWithDrawable. I've made sure that we mark the imgFrame aborted on all error paths now.
(Assignee)

Updated

3 years ago
Attachment #8546395 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31abccd4bd7
Flags: needinfo?(seth)
(Assignee)

Comment 9

3 years ago
I realized I never posted it, but here's a try job showing that this patch is 100% green:

https://tbpl.mozilla.org/?tree=Try&rev=a55162c994be
Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3cc2c90893
(Assignee)

Comment 11

3 years ago
Argh! It's frustrating that this keeps getting backed out even with green try jobs. I suspect the problem is actually bug 1117607, though, so I've given up on that bug for now and rebased this bug not to require it.

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/de7cd37e48e8
(Assignee)

Comment 12

3 years ago
Created attachment 8547312 [details] [diff] [review]
Correctly sync decode even if an imgFrame is partially decoded

Here's the rebased version.
(Assignee)

Updated

3 years ago
Attachment #8546901 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/de7cd37e48e8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.