Closed Bug 1130707 Opened 5 years ago Closed 5 years ago
Make decode-on-draw-only image notifications more robust
Decode-on-draw-only images need to handle onload blocking and unblocking and decoding-related notifications themselves, instead of relying on their decoders, because if they don't give the impression that they're *already* decoded, we frequently won't try to draw them at all. We send the initial set of notifications in RasterImage::BlockOnloadForDecodeOnDraw() (which is invoked via runnable from RasterImage::OnImageDataAvailable), and finish up in RasterImage::OnImageDataComplete(). Unfortunately, the way they're handling these notifications is somewhat fragile right now. Specifically: - Not all necko requests can be retargeted off-main-thread. (Blob URI requests, specifically, cannot be retargeted.) Since we always send a runnable to the main thread to run BlockOnloadForDecodeOnDraw() in OnImageDataAvailable(), when the request didn't get retargeted, OnImageDataComplete() will run *before* BlockOnloadForDecodeOnDraw(). We should not use a runnable if we're already on the main thread. - Even beyond that, the main reason that causes a problem is because BlockOnloadForDecodeOnDraw() doesn't do anything if it detects that OnImageDataComplete() has already run. What it's trying to do is avoid sending BLOCK_ONLOAD notifications if there's nothing that will send UNBLOCK_ONLOAD later, but that doesn't mean it shouldn't send the other notifications. Indeed, even the BLOCK_ONLOAD/UNBLOCK_ONLOAD issue can be handled better. In this bug I'm going to make this code much more robust by: - Running BlockOnloadForDecodeOnDraw() directly on the main thread if we're already there. - Taking advantage of the fact that ProgressTracker will only send each image notification once to simplify the way these notifications are handled and ensure that notifications like FRAME_COMPLETE and DECODE_COMPLETE *always* get sent for decode-on-draw-only images.
Here's the patch. This will resolve the blob-URI-related issues on B2G that resulted from turning on the decode-on-draw-only patch there (see e.g. bug 1130403), and it also makes the code simpler and more robust. We rely on two guarantees: - ProgressTracker never allows duplicate notifications to be sent. - Necko always delivers OnStopRequest (which we receive as OnImageDataComplete). This means that we can simply always deliver all the notifications we want to deliver for decode-on-draw-only images in OnImageDataComplete. Ideally we want to deliver BLOCK_ONLOAD and DECODE_STARTED earlier, so we do that on a best effort basis. (And we'll succeed more often now that we don't use a runnable on the main thread.) But no matter what happens, always sending those notifications in OnImageDataComplete guarantees that decode-on-draw-only images will get drawn if they're visible.
Attachment #8560864 - Flags: review?(tnikkel)
... and no tests ran. Great. There's definitely a bug in how try handles Android tests. Here's a new try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89fd2b7b9ed7
Try looks good.
Attachment #8560864 - Flags: review?(tnikkel) → review+
Thanks for the review! Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a48321b8bd00
Pushed a followup to add a missing return statement in this patch that caused but 1129804 to be backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/268487086aca
Comment on attachment 8560864 [details] [diff] [review] Make decode-on-draw-only image notifications more robust Approval Request Comment [Feature/regressing bug #]: Pulling in downscale-during-decode refactoring to ensure that patch compatibility between 37 and 38+. [User impact if declined]: Already approved. [Describe test coverage new/current, TreeHerder]: In 38 for a long time. [Risks and why]: Low risk; in 38 for a long time. [String/UUID change made/needed]: None.
Attachment #8560864 - Flags: approval-mozilla-aurora?
Pushed to Aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/77af082895db remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/4abf28db48d5
Comment on attachment 8560864 [details] [diff] [review] Make decode-on-draw-only image notifications more robust Backed out for bustage. 37 is now Beta and this bug no longer has approval to land. Seth - Please request Beta approval when you're ready to try landing again.
Attachment #8560864 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment on attachment 8560864 [details] [diff] [review] Make decode-on-draw-only image notifications more robust Approval Request Comment [Feature/regressing bug #]: Bug fix for bug 1119774. [User impact if declined]: Issues displaying images; in particular, a race condition may cause us to sometimes never draw an image, or only draw it after a long delay, even though it has already been decoded. [Describe test coverage new/current, TreeHerder]: In 38 for a long time. [Risks and why]: Low risk; in 38 for a long time. Was already approved for uplift to Aurora, but got backed out from Aurora as collateral damage from another backout, even though there was no problem with this patch. [String/UUID change made/needed]: None.
Attachment #8560864 - Flags: approval-mozilla-beta?
Comment on attachment 8560864 [details] [diff] [review] Make decode-on-draw-only image notifications more robust Approved for Beta with the understanding that this patch was backed out for reasons that don't have to do with this patch specifically. Beta+
Attachment #8560864 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.