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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: allow flags to be passed to StartDecoding

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: make nsTreeBodyFrame use async notifications

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: make style images use async notifications

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/072d40d19b1f
https://hg.mozilla.org/mozilla-central/rev/0ed01739297a

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e0069c673e
Make style images use async image notifications when requesting decoding. r=aosmond

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing reftest 368020-1.html on OS X and Windows:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=39e0069c673e177f9998f4b0646712242b5699cc
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39898387&repo=mozilla-inbound

Comment 8

[Timothy Nikkel (:tnikkel)]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

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

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f49fba3f4d
Make style images use async image notifications when requesting decoding. r=aosmond

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Timothy Nikkel (:tnikkel)]

backed out the last part for causing bug 1325910
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b3cc1c100caaf6a0d6e332cebf1ba5969cea7b

Comment 14

[Carsten Book [:Tomcat]]

(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

Comment 15

[Carsten Book [:Tomcat]]

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