Closed Bug 1130707 Opened 5 years ago Closed 5 years ago

Make decode-on-draw-only image notifications more robust

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file)

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)
Blocks: 1130403
... 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+
https://hg.mozilla.org/mozilla-central/rev/a48321b8bd00
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
Depends on: 1132195
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?
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.
Flags: needinfo?(seth)
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.
Flags: needinfo?(seth)
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+
No longer depends on: 1154004
You need to log in before you can comment on or make changes to this bug.