Closed Bug 1207355 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
blocking-b2g 2.5+
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: seth, Assigned: seth, NeedInfo)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [platform][MemShrink])

Attachments

(8 files, 4 obsolete files)

3.39 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.45 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.55 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.87 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.88 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.35 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
15.46 KB, patch
Details | Diff | Splinter Review
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.
Attachment #8664565 - Flags: review?(tnikkel)
Attachment #8664566 - Flags: review?(tnikkel)
Attachment #8664567 - Flags: review?(tnikkel)
Attachment #8664568 - Flags: review?(tnikkel)
Attachment #8664569 - Flags: review?(tnikkel)
Attachment #8664570 - Flags: review?(tnikkel)
Attachment #8664571 - Flags: review?(tnikkel)
Attachment #8664572 - Flags: review?(tnikkel)
Blocks: 1206206
Depends on: 1209703
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.)
Depends on: 1213465
OK, bug 1213465 looks to have cleared up the last remaining issues. Try looks green.
Blocks: 1211393
Bug 1211393 is a 2.5+ blocker. Moving that 2.5+ flag here just for our internal blocker tracking purposes
blocking-b2g: --- → 2.5+
Priority: -- → P1
Whiteboard: [platform]
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 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.
Attachment #8664566 - Flags: review?(tnikkel) → review+
Attachment #8664567 - Flags: review?(tnikkel) → review+
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).
Attachment #8664568 - Flags: review?(tnikkel) → review+
Attachment #8664569 - Flags: review?(tnikkel) → review+
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 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 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?
(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().
(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().
(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.
(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().
(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.
(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.
(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.
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.)
Attachment #8679834 - Flags: review?(tnikkel)
Attachment #8664565 - Attachment is obsolete: true
Attachment #8664565 - Flags: review?(tnikkel)
The changes in the previous version of part 1 needed an additional #include and a friend declaration.
Attachment #8679856 - Flags: review?(tnikkel)
Attachment #8679834 - Attachment is obsolete: true
Attachment #8679834 - Flags: review?(tnikkel)
... and an additional do_QueryInterface call. This is what I get for testing the
build locally with other patches applied.
Attachment #8680126 - Flags: review?(tnikkel)
Attachment #8679856 - Attachment is obsolete: true
Attachment #8679856 - Flags: review?(tnikkel)
Whiteboard: [platform] → [platform][MemShrink]
Attachment #8680126 - Flags: review?(tnikkel) → review+
(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.
Attachment #8664570 - Flags: review?(tnikkel) → review+
Attachment #8664571 - Flags: review?(tnikkel) → review+
Attachment #8664572 - Flags: review?(tnikkel) → review+
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.
Attachment #8664572 - Attachment is obsolete: true
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/925600dd61a99865b16294050ee0189cfcf61dff
Bug 1207355 (Followup) - Add fuzz to layout/reftests/generated-content/floated-01.html. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaad51954fe410b62838479a74eceefb3bd2604
Bug 1207355 (Followup) - Add fuzz to layout/reftests/generated-content tests. r=me
Blocks: 1220052
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bac283aad5301f23a0bdbcd7c7f3f92b9da7c24
Bug 1207355 (Followup) - Add yet more fuzz to layout/reftests/generated-content tests. r=me
Depends on: 1220179
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Depends on: 1234276
Depends on: 1235093
Depends on: 1240680
Depends on: 1246509
Depends on: 1249495
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?
Flags: needinfo?(tnikkel)
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.
Flags: needinfo?(epinal99-bugzilla2)
Flags: needinfo?(epinal99-bugzilla2)
Depends on: 1272178
Depends on: 1273455
Depends on: 1259498
Depends on: 1267379
No longer depends on: 1234276
Depends on: 1395964
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.