1207355 - Rely on size prediction to trigger decoding for non-CSS images

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

Description

[Seth Fowler [:seth] [:s2h]]

Now that DDD is enabled for all types of images, and we have size prediction for content images, we should rely upon size prediction to trigger decoding. As it stands, there are *many* places in the code that call RequestDecode() or StartDecoding() and force an unnecessary decode at the intrinsic size.

In this bug we'll:

- Remove all calls to RequestDecode().
- Remove imgIContainer::RequestDecode().
- Remove imgIRequest::RequestDecode().

We'll keep StartDecoding() to support CSS images until we have size prediction for them as well.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Stop requesting decodes in nsDocument. r=tn

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Stop requesting decodes in nsImageLoadingContent. r=tn

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Request decodes intelligently in nsImageFrame. r=tn

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Request decodes intelligently in nsBulletFrame. r=tn

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Request decodes intelligently in MultipartImage. r=tn

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 6) - Only respect StartDecoding() in imgRequest/imgRequestProxy. r=tn

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 7) - Only trigger intrinsic size decode in FinalizeDecoder() if StartDecoding() was called. r=tn

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 8) - Remove imgIContainer::RequestDecode() and imgIRequest::RequestDecode(). r=tn

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deaeb6c63753

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82b2695f347

Comment 11

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=257ee09531a3

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1a55e60df0

Comment 13

[Seth Fowler [:seth] [:s2h]]

Wow, still more. After bug 1209703, the border-image issues are gone, but it looks like there are still some problems with table backgrounds. Fortunately that shouldn't require *nearly* as much code to fix as border-image did, because table backgrounds have the infrastructure for sync decoding in place already. There must just be some cases that got missed last time around - I notice these are mostly border-collapse-related issues. (And interestingly, they're mostly existing intermittents, as well.)

Comment 14

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1f8963c3043

Comment 15

[Seth Fowler [:seth] [:s2h]]

OK, bug 1213465 looks to have cleared up the last remaining issues. Try looks green.

Comment 16

[David Flanagan [:djf]]

Bug 1211393 is a 2.5+ blocker. Moving that 2.5+ flag here just for our internal blocker tracking purposes

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664565 [details] [diff] [review]
(Part 1) - Stop requesting decodes in nsDocument. r=tn

The MaybeDecodeForPredictedSize calls in nsImageFrame likely mean that we will have a size-specific decode request already by the time we call nsDocument::AddImage. So that StartDecoding call is likely not needed.

The RequestDecode call in LockEnumerator for when an entire document starts locking it's images (ie tab switch) I can't see what would replace it. Maybe we should do a size-specific decode request in PresShell::UpdateImageLockingState for the visible images?

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664566 [details] [diff] [review]
(Part 2) - Stop requesting decodes in nsImageLoadingContent. r=tn

MaybeDecodeForPredictedSize in nsImageFrame likely handle all the cases we care about where this would start a decode.

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664568 [details] [diff] [review]
(Part 4) - Request decodes intelligently in nsBulletFrame. r=tn

I wouldn't call this "intelligently": the size we draw the bullet image at is not taken into account.

Side note: it seems a little clumsy to have to get the width/height just to pass to RequestDecodeForSize. Might be better to have a request decode function which just asks for the intrinsic size. That way it is easy to find the places where we are not being intelligent about the requested decode size, and we can work to eliminate them (or decide it is not worth it).

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664572 [details] [diff] [review]
(Part 8) - Remove imgIContainer::RequestDecode() and imgIRequest::RequestDecode(). r=tn

>diff --git a/image/test/unit/image_load_helpers.js b/image/test/unit/image_load_helpers.js
>index 5fc5965..e8d9a29 100644
>--- a/image/test/unit/image_load_helpers.js
>+++ b/image/test/unit/image_load_helpers.js
>-    try {
>-      aRequest.requestDecode();
>-    } catch (e) {
>-      do_print("requestDecode threw " + e);
>-    }
>-

The test doesn't need a decode to test things properly?

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664571 [details] [diff] [review]
(Part 7) - Only trigger intrinsic size decode in FinalizeDecoder() if StartDecoding() was called. r=tn

Why don't we want to trigger a decode if RequestDecodeForSize was called?

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8664570 [details] [diff] [review]
(Part 6) - Only respect StartDecoding() in imgRequest/imgRequestProxy. r=tn

>@@ -1049,17 +1049,17 @@ imgRequest::FinishPreparingForNewPart(const NewPartResult& aResult)
>     MOZ_ASSERT(progressTracker->HasImage());
>   }
> 
>   if (aResult.mShouldResetCacheEntry) {
>     ResetCacheEntry();
>   }
> 
>   if (IsDecodeRequested()) {
>-    aResult.mImage->RequestDecode();
>+    aResult.mImage->StartDecoding();
>   }
> }

Didn't we have a problem recently with multipart images and decoding not being able to keep up? This would make that worse, no?

>@@ -203,25 +203,16 @@ imgRequestProxy::ChangeOwner(imgRequest* aNewOwner)
>-  // Were we decoded before?
>-  bool wasDecoded = false;
>-  nsRefPtr<ProgressTracker> progressTracker = GetProgressTracker();
>-  if (progressTracker->HasImage() &&
>-      progressTracker->GetImageStatus() &
>-        imgIRequest::STATUS_FRAME_COMPLETE) {
>-    wasDecoded = true;
>-  }
>-

Why get rid of this?

Comment 23

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #17)
> Comment on attachment 8664565 [details] [diff] [review]
> (Part 1) - Stop requesting decodes in nsDocument. r=tn
> 
> The MaybeDecodeForPredictedSize calls in nsImageFrame likely mean that we
> will have a size-specific decode request already by the time we call
> nsDocument::AddImage. So that StartDecoding call is likely not needed.

Yeah, IMO it's definitely not needed. (And just to clarify for anyone who may be reading besides Tim and me: this patch removes it.)

> The RequestDecode call in LockEnumerator for when an entire document starts
> locking it's images (ie tab switch) I can't see what would replace it. Maybe
> we should do a size-specific decode request in
> PresShell::UpdateImageLockingState for the visible images?

I expected that visibility would take care of that, but actually maybe images stay marked as visible in background tabs? If so, yeah, we should trigger MaybeDecodeForPredictedSize() in UpdateImageLockingState().

Comment 24

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #19)
> Comment on attachment 8664568 [details] [diff] [review]
> (Part 4) - Request decodes intelligently in nsBulletFrame. r=tn
> 
> I wouldn't call this "intelligently": the size we draw the bullet image at
> is not taken into account.

Yeah, I know. Just kinda hard to name commits sometimes. =) It's no worse than it was before, at least. Eventually we'll want to have size prediction for everything, but bullets aren't high on my list of priorities.

> Side note: it seems a little clumsy to have to get the width/height just to
> pass to RequestDecodeForSize. Might be better to have a request decode
> function which just asks for the intrinsic size. That way it is easy to find
> the places where we are not being intelligent about the requested decode
> size, and we can work to eliminate them (or decide it is not worth it).

Agreed! The API I intend in the long term is that there is only one method, RequestDecode(), which replaces RequestDecode(), RequestDecodeForSize(), and StartDecoding(). That method will have the signature:

RequestDecode(const Maybe<IntSize>& aSize, uint32_t aFlags)

You'd pass Nothing() to decode at the intrinsic size.

My plan was to switch to this API once I was able to get rid of both RequestDecode() and StartDecoding().

Comment 25

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #20)
> The test doesn't need a decode to test things properly?

That's generic code shared by all the xpcshell tests, but none of them actually depend on it, so we can just remove it.

Comment 26

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #21)
> Comment on attachment 8664571 [details] [diff] [review]
> (Part 7) - Only trigger intrinsic size decode in FinalizeDecoder() if
> StartDecoding() was called. r=tn
> 
> Why don't we want to trigger a decode if RequestDecodeForSize was called?

It's absolutely necessary that we do that, because we do not want to trigger a decode at the intrinsic size just because RequestDecodeForSize() was called before the size was available. (And eventually I plan on removing the |mWantFullDecode| mechanism totally, because stuff that uses size prediction will never use it. In the long term it'll be an error to do this.) Right now, after these patches land, the only case where we try to trigger decoding before the size is available is for CSS images, via StartDecoding(); that's why I moved the mWantFullDecode stuff directly into StartDecoding().

Comment 27

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #22)
> Didn't we have a problem recently with multipart images and decoding not
> being able to keep up? This would make that worse, no?

That problem occurred because the decoding threads got behind the main thread, and we solved it by forcing a sync decode if we hadn't decoded the previous part by the time the next part came in. This will have no effect on that.

> 
> >@@ -203,25 +203,16 @@ imgRequestProxy::ChangeOwner(imgRequest* aNewOwner)
> >-  // Were we decoded before?
> >-  bool wasDecoded = false;
> >-  nsRefPtr<ProgressTracker> progressTracker = GetProgressTracker();
> >-  if (progressTracker->HasImage() &&
> >-      progressTracker->GetImageStatus() &
> >-        imgIRequest::STATUS_FRAME_COMPLETE) {
> >-    wasDecoded = true;
> >-  }
> >-
> 
> Why get rid of this?

Because it doesn't make any sense in a world with size prediction. A new image implies that the predicted size may be different, and we don't want to just trigger a decode at the intrinsic size. We can't do anything useful at this layer, since size prediction has to happen in layout. At any rate, since this only happens when we're doing cache validation and validation failed, we will not have sent out any notifications yet (they'll be deferred) and thus we don't have to do anything special; layout will request a decode via the usual mechanisms once we send SIZE_AVAILABLE / LOAD_COMPLETE.

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] [:s2h] from comment #25)
> (In reply to Timothy Nikkel (:tn) from comment #20)
> > The test doesn't need a decode to test things properly?
> 
> That's generic code shared by all the xpcshell tests, but none of them
> actually depend on it, so we can just remove it.

But they may not be testing the same things. It seems the request decode was added in http://hg.mozilla.org/mozilla-central/rev/7d0a6850c887 and the commit message mentions changes with regards to decoding.

Comment 29

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #28)
> But they may not be testing the same things. It seems the request decode was
> added in http://hg.mozilla.org/mozilla-central/rev/7d0a6850c887 and the
> commit message mentions changes with regards to decoding.

I have a hard time seeing how removing this code could be an issue if the tests pass. Whether an image is decoded is only observable by whether functions like Draw(), GetFrame(), etc. succeed or fail, or by checking the image's status to see which progress bits are set. I don't really see how one could write a correct test that depended on RequestDecode() already having been called, yet did not fail when the call to RequestDecode() was removed. That's doubly the case because RequestDecode() is *async* and doesn't guarantee that the image actually gets decoded at any particular time, so if a test did depend on it, it'd be an intermittent orange.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Stop requesting decodes in nsDocument.

This new version of part 1 calls MaybeDecodeForPredictedSize() in
UpdateImageLockingState() for all images in the visible list. (Well, all images
with an associated nsImageFrame, anyway.)

Comment 31

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f576013349a8

Comment 32

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Stop requesting decodes in nsDocument.

The changes in the previous version of part 1 needed an additional #include and a friend declaration.

Comment 33

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6df4abcf32

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Stop requesting decodes in nsDocument.

... and an additional do_QueryInterface call. This is what I get for testing the
build locally with other patches applied.

Comment 35

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c56b8dc8273

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] [:s2h] from comment #29)
> (In reply to Timothy Nikkel (:tn) from comment #28)
> > But they may not be testing the same things. It seems the request decode was
> > added in http://hg.mozilla.org/mozilla-central/rev/7d0a6850c887 and the
> > commit message mentions changes with regards to decoding.
> 
> I have a hard time seeing how removing this code could be an issue if the
> tests pass. Whether an image is decoded is only observable by whether
> functions like Draw(), GetFrame(), etc. succeed or fail, or by checking the
> image's status to see which progress bits are set. I don't really see how
> one could write a correct test that depended on RequestDecode() already
> having been called, yet did not fail when the call to RequestDecode() was
> removed. That's doubly the case because RequestDecode() is *async* and
> doesn't guarantee that the image actually gets decoded at any particular
> time, so if a test did depend on it, it'd be an intermittent orange.

It would be running more code paths, and checking that we don't crash or assert.

Comment 37

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4c00dbf49a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcdf51edb121
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc69e37c9b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f2017137e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecebca101fcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e941e0e106a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd950b837fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/aafd6db2fbb4

Comment 38

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c323832f31 for (minus the unrelated 632781-verybig.html, and it's hard to say whether you made the existing generated-content/display-types-01.html more frequent, or it just is more frequent)

https://treeherder.mozilla.org/logviewer.html#?job_id=16459939&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16456754&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16453793&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16453452&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16450273&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16446449&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16442390&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16452625&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16443936&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16452567&repo=mozilla-inbound

Comment 39

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 8) - Remove imgIContainer::RequestDecode() and imgIRequest::RequestDecode().

Added fuzz to a count of reftests that were already intermittent on OS X 10.10,
and which this patch makes more frequent, resulting the backout above.

Comment 40

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5278f641f0c33aecda590bc28607d5e217cee336
Bug 1207355 (Part 1) - Stop requesting decodes in nsDocument. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/1985fbb5e8292e20eb00757cfe0570593059f78c
Bug 1207355 (Part 2) - Stop requesting decodes in nsImageLoadingContent. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc652bd16e80d11ff86042f0ad8391be39c5f5be
Bug 1207355 (Part 3) - Request decodes intelligently in nsImageFrame. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/884aa587c3a80938c9997ca413104143e85acf3f
Bug 1207355 (Part 4) - Request decodes intelligently in nsBulletFrame. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3e7157e07f4fd0122b0a0a3636f410ddc6bd73
Bug 1207355 (Part 5) - Request decodes intelligently in MultipartImage. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3635689afc1a8b96f94571d4593e01a80c39274
Bug 1207355 (Part 6) - Only respect StartDecoding() in imgRequest/imgRequestProxy. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c95c1ebf91e8c3ba276d76f456063fd8c2cdfbf
Bug 1207355 (Part 7) - Only trigger intrinsic size decode in FinalizeDecoder() if StartDecoding() was called. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e19045ba652ca2a5a5fc0e20d6f95293acfa32d
Bug 1207355 (Part 8) - Remove imgIContainer::RequestDecode() and imgIRequest::RequestDecode(). r=tn

Comment 41

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/925600dd61a99865b16294050ee0189cfcf61dff
Bug 1207355 (Followup) - Add fuzz to layout/reftests/generated-content/floated-01.html. r=me

Comment 42

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaad51954fe410b62838479a74eceefb3bd2604
Bug 1207355 (Followup) - Add fuzz to layout/reftests/generated-content tests. r=me

Comment 43

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bac283aad5301f23a0bdbcd7c7f3f92b9da7c24
Bug 1207355 (Followup) - Add yet more fuzz to layout/reftests/generated-content tests. r=me

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5278f641f0c3
https://hg.mozilla.org/mozilla-central/rev/1985fbb5e829
https://hg.mozilla.org/mozilla-central/rev/bc652bd16e80
https://hg.mozilla.org/mozilla-central/rev/884aa587c3a8
https://hg.mozilla.org/mozilla-central/rev/0c3e7157e07f
https://hg.mozilla.org/mozilla-central/rev/f3635689afc1
https://hg.mozilla.org/mozilla-central/rev/4c95c1ebf91e
https://hg.mozilla.org/mozilla-central/rev/2e19045ba652
https://hg.mozilla.org/mozilla-central/rev/925600dd61a9
https://hg.mozilla.org/mozilla-central/rev/cfaad51954fe
https://hg.mozilla.org/mozilla-central/rev/6bac283aad53

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5278f641f0c3
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1985fbb5e829
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bc652bd16e80
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/884aa587c3a8
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0c3e7157e07f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f3635689afc1
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4c95c1ebf91e
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2e19045ba652
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/925600dd61a9
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cfaad51954fe
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6bac283aad53

Comment 46

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7fc223dc5e05
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/db89732be431
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7092a6db0c96
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5aa052134d15
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7f50dc551ed3
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b4061404e673
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/98ebe2f083d7
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/96bf5736d5e9
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1e794c7fab3f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/120debd7c16e
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8f44c3476ce9

Comment 48

[Loic]

There are many regressions in FF45+ after this bug landed. As FF45 is in Beta 6, is it safe to release FF45 with all these (minor?) regressions?

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

Loic: Good question in comment 48!  Definitely nominate a bug for tracking when you have this kind of concern or notice many regressions from a bug. That way a release manager will see it and we can follow up. You could also n-i to a release manager (me, :ritu, or :sylvestre).    I know you often catch hot issues as you do a lot of early triage.