Seth Fowler [:seth] [:s2h] Assignee
	Description • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 9 years ago

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

	David Flanagan [:djf]
	Comment 16 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 17 • 9 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 18 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 19 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 20 • 9 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 21 • 9 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 22 • 9 years ago

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?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 23 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 24 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 26 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 27 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 28 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 29 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 30 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 31 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 32 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 33 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 34 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 35 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 36 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 37 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 38 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 39 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 40 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 41 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 42 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 43 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 44 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 45 • 9 years ago
bugherder uplift

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

	Carsten Book [:Tomcat]
	Comment 46 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 47 • 9 years ago
bugherder uplift

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

	Loic
	Comment 48 • 8 years ago

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?

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 49 • 8 years ago

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.