1118087 - Correctly sync decode even if an imgFrame is partially decoded

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.)

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Try job here:

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

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: 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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31abccd4bd7

Comment 9

[Seth Fowler [:seth] [:s2h]]

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

Comment 10

[Tom S]

Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3cc2c90893

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: Correctly sync decode even if an imgFrame is partially decoded

Here's the rebased version.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de7cd37e48e8