allow StartDecoding to use async notifications and use it in two places

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

No description provided.
Attachment #8810685 - Flags: review?(aosmond) → review+
Attachment #8810686 - Flags: review?(aosmond) → review+
Attachment #8810687 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/072d40d19b1f
Allow flags to be passed to StartDecoding for the sole purpose of allowing async notifications to be requested. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed01739297a
Make nsTreeBodyFrame use async image notifications during painting. r=aosmond
Keywords: leave-open
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e0069c673e
Make style images use async image notifications when requesting decoding. r=aosmond
Keywords: leave-open
The code in nsImageRenderer::PrepareImage is called during painting and calls StartDecoding. It then checks if the image is decoded. With async notifications this check will report the image as not decoded even if the StartDecoding call completely decoded the image. Presumably, then we will invalidate after that paint, and repaint the area of the image. My guess is this partial paint of the page causes small pixel differences (as opposed to painting the entire page at once). The reftest is already fuzzed somewhat similarly.

But it's really a shame that we have to paint again even though the image is decoded, kind of defeats the point of even doing decoding during painting at all. So maybe we should add a way to check if an image is decoded that should only be used in situations such as this. Introducing a way to check state that will report different results from what has been sent out via imgINotificationObserver::Notify is not ideal (we want them to always be in sync).

Or we should do no decoding during paint at all (since we can't benefit from it and thus it blocks painting for no reason).
Depends on: 1325296
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> The code in nsImageRenderer::PrepareImage is called during painting and
> calls StartDecoding. It then checks if the image is decoded. With async
> notifications this check will report the image as not decoded even if the
> StartDecoding call completely decoded the image. Presumably, then we will
> invalidate after that paint, and repaint the area of the image. My guess is
> this partial paint of the page causes small pixel differences (as opposed to
> painting the entire page at once). The reftest is already fuzzed somewhat
> similarly.

Based on my investigation this seems to be correct.

> But it's really a shame that we have to paint again even though the image is
> decoded, kind of defeats the point of even doing decoding during painting at
> all. So maybe we should add a way to check if an image is decoded that
> should only be used in situations such as this. Introducing a way to check
> state that will report different results from what has been sent out via
> imgINotificationObserver::Notify is not ideal (we want them to always be in
> sync).

Returning a result from the StartDecoding call seems to be a reasonable way to do this. But this won't prevent the invalidations from happening after the current paint, and this resulting in another paint. The benefit is that the image will be drawn in the current paint and we won't have to wait until the next paint.

Writing a patch for this exposed bug 1325296. With bug 1325296 fixed the patches fix the failure of comment 7.
Flags: needinfo?(tnikkel)
Depends on: 1325297
(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> > But it's really a shame that we have to paint again even though the image is
> > decoded, kind of defeats the point of even doing decoding during painting at
> > all. So maybe we should add a way to check if an image is decoded that
> > should only be used in situations such as this. Introducing a way to check
> > state that will report different results from what has been sent out via
> > imgINotificationObserver::Notify is not ideal (we want them to always be in
> > sync).
> 
> Returning a result from the StartDecoding call seems to be a reasonable way
> to do this. But this won't prevent the invalidations from happening after
> the current paint, and this resulting in another paint. The benefit is that
> the image will be drawn in the current paint and we won't have to wait until
> the next paint.

I put those patches in bug 1325297 so as not to include too many changes in this one bug.
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f49fba3f4d
Make style images use async image notifications when requesting decoding. r=aosmond
https://hg.mozilla.org/mozilla-central/rev/c2f49fba3f4d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1325910
backed out the last part for causing bug 1325910
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b3cc1c100caaf6a0d6e332cebf1ba5969cea7b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> backed out the last part for causing bug 1325910
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> f4b3cc1c100caaf6a0d6e332cebf1ba5969cea7b

also backout landed now now on central
https://hg.mozilla.org/mozilla-central/rev/e5c12e3f9e1d
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.