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.)
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.)
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.
Thanks for the review! Try job here: https://tbpl.mozilla.org/?tree=Try&rev=8deb81d59a6c
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.
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
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.
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
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
Created attachment 8547312 [details] [diff] [review] Correctly sync decode even if an imgFrame is partially decoded Here's the rebased version.