1151373 - Predict size at which CSS images will be drawn for downscale-during-decode

Status: NEW

(Core :: Graphics: ImageLib, task, P5)

Description

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

We really want the advantages of downscale-during-decode without the negative aspects of decode-on-draw such as image pop-in. To do that, we have to be able to accurately predict the size of images without drawing them.

In this bug I'll implement size prediction for CSS images - at least, those handled by nsImageRenderer - which will take care of this issue for CSS backgrounds, border-image, and other similar features.

Comment 1

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

Attachment: (Part 1) - Add some image-related helper functions

Part 1 just adds a couple of helper functions we use in part 2.

Comment 2

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

Attachment: (Part 2) - Predict the size of CSS images before drawing

Here's where the real work is done.

- nsImageRenderer::PrepareImage() is updated to return an enum, so callers can
  distinguish between an image having its size available, but not being ready to
  draw, and an image which isn't loaded or has some other issue.

- nsImageRenderer::PrepareImage() doesn't call StartDecoding() itself. Instead,
  its two callers, PrepareBackgroundLayer() and DrawBorderImage(), have been
  updated to compute an appropriate size and pass it to
  nsImageRenderer::MaybeDecodeForPredictedSize(), which requests decoding if
  appropriate.

- nsImageRenderer::MaybeDecodeForPredictedSize() implements a similar algorithm
  to nsImageFrame::MaybeDecodeForPredictedSize() in bug 1151359. In a followup,
  I'll pull out the shared code into a helper function, but I'd prefer that each
  case get reviewed in isolation first, because I think having everything laid
  out in one place will help make it clearer if there are any mistakes.

Comment 3

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

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

Comment 4

[Timothy Nikkel (:tnikkel)]

So this isn't so much "prediction" since this code all runs after we've got a display list already. So it's only slightly before we run FrameLayerBuilder and have the actual values and then paint. Which is still valuable since we can do the decoding off main thread in that time interval, but not as good as the image frame case.

Comment 5

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> So this isn't so much "prediction" since this code all runs after we've got
> a display list already. So it's only slightly before we run
> FrameLayerBuilder and have the actual values and then paint. Which is still
> valuable since we can do the decoding off main thread in that time interval,
> but not as good as the image frame case.

Yeah, that's totally true, and it'd be nice to trigger decoding of these images even earlier if possible.

Really, when I say "prediction", I mean that we're predicting the correct size to decode to without having to actually paint the image (like the decode-on-draw-only approach would need). That lets us pick up stuff that's in the display list, but doesn't actually get painted because it's outside the viewport or occluded. Should solve issues like the B2G Gallery app failing to decode the next and previous photo until you're already swiping to them.

Comment 6

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

Attachment: (Part 2) - Predict the size of CSS images before drawing

This updated version of part 2 includes two changes:

- It now uses GetTransformToAncestorScaleExcludingAnimated(); see bug 1151359.

- It gets rid of an assertion about nsImageRenderer::mIsReady which is no longer
  true. The assertion was causing orange tests on try.

Comment 7

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

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

Comment 8

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

Comment on attachment 8588509 [details] [diff] [review]
(Part 1) - Add some image-related helper functions

Clearing my review requests because I'm working on a total rewrite of these patches.

Comment 9

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

Attachment: (Part 2) - Move shared predictive image decoding computations to nsLayoutUtils::DecodeForPredictedSize

Let's go ahead and move the parts of the predictive image decoding computation
that content and CSS images share into a shared function.

Comment 10

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

Attachment: (Part 3) - Add helper functions and background image change detection to nsStyleBackground

OK, now we can start working on the meat of this bug.

This patch is mostly about adding a new nsChangeHint to indicate that CSS
backgrounds have changed. This requires some rework of
nsStyleBackground::CalcDifference, since it's possible to have one version of a
layer have element type and the other have image type, which requires a special
change hint in both cases. That means we need to structure the code in such a
way that we're combining hints, rather than returning the first hint we find.

As long as we're at it, we also add
nsStyleBackground::Layer::ImageDecodedSizeMightDependOnPositioningAreaSizeChange()
(that's a mouthful!), which a later part will use to check whether a given layer
cares about size changes due to reflow. This is pretty similar to
RenderingMightDependOnPositioningAreaSizeChange(), except that we don't care
about position.

Comment 11

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

Attachment: (Part 4) - Add nsCSSRendering::DecodeBackgroundImagesForPredictedSize

This part adds nsCSSRendering::DecodeBackgroundImagesForPredictedSize(), which
part 5 will use to trigger predictive decoding of CSS images in various places.

DecodeBackgroundImagesForPredictedSize() basically does two things:

- Iterate through the background layers of the given frame and call
  DecodeBackgroundImageLayerForPredictedSize(), which does the actual predictive
  decoding.

- Take note if any of the layers will need to be redecode if reflow changes this
  frame's size, and if so, add a frame property so we can determine its previous
  size. We'll make use of this frame property in part 5.

DecodeBackgroundImageLayerForPredictedSize() does the real work. It's
essentially a cut-down version of nsCSSRendering::PrepareBackgroundLayer() which
only does the computations necessary to predict the drawn size of the current
layer. I was able to share code with nsImageRenderer by splitting some of the
work it does out into functions. I was, unfortunately, a bit less successful in
sharing code with PrepareBackgroundLayer(); to do that cleanly will require a
bit more refactoring than I wanted to do in the context of this bug. Ultimately
this code calls nsLayoutUtils::DecodeForPredictedSize(), which was added in part
2.

It's probably worth explaining the boolean argument added to
nsImageRenderer::PrepareImage(). We don't want to start a decode for every CSS
image at its intrinsic size, which is what the StartDecoding() call in
PrepareImage() would do. For background images we don't need to, because
predictive decoding completely replaces that call. However, we also use
nsImageRenderer for border-image, and I'm not going to implement predictive
image decoding for border-image in this bug. So for now, in the border-image
case we still want to call StartDecoding(), and that's what the boolean argument
enables. This is just a temporary measure until we have predictive image
decoding for everything.

Comment 12

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

Attachment: (Part 5) - Redecode CSS background images when their predicted size changes

In this part everything gets hooked up to do the actual predictive image
decoding. The following places get modified:

- RestyleManager::DoApplyRenderingToTree(), where we redecode background image
  layers when we get the InvalidateBackgroundImages nsChangeHint. (We started
  sending that in part 3 when a style change happens that could affect the
  decoded size of CSS background images for a frame.)

- nsFrame::DidReflow(), where we redecode background image layers when we have a
  PreviousSize frame property and our size changed. The PreviousSize property
  gets set by nsCSSRendering::DecodeBackgroundImagesForPredictedSize() when it
  detects that a size change due to reflow might change the decoded size of
  images. Note that we don't bother doing predictive decoding on the initial
  reflow, because we rely on ImageLoader - see the next item.

- ImageLoader, where we do a predictive decode when an image's size becomes
  available. We don't know whether the image is being used as a CSS background
  image or not, so we always call
  nsCSSRendering::DecodeBackgroundImagesForPredictedSize. That should be
  harmless in the case where the image isn't being used as a background image -
  either the frame has no background images, in which case it does nothing, or
  the frame does have some background images and we harmlessly request a
  duplicate predictive decode for them. Requesting a decode for the same size
  multiple times is a no-op, so this shouldn't be an issue.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8596302 [details] [diff] [review]
(Part 3) - Add helper functions and background image change detection to nsStyleBackground

>+  /**
>+   * This will cause us to redecode background images.
>+   */
>+  nsChangeHint_InvalidateBackgroundImages = 0x800000

It would be useful for this comment to indicate (and it would be helpful for me when reviewing the patches here) when this hint needs to be sent.  Does it need to be sent only when the background image size changes in a way that will not involve reflow?  Or does it need to happen if the image itself changes as well?  Or also if the image size changes as a result of reflow (which isn't something we can predict accurately when we're computing change hints, i.e., without actually doing reflow)?

Comment 14

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

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #13)
> Comment on attachment 8596302 [details] [diff] [review]
> (Part 3) - Add helper functions and background image change detection to
> nsStyleBackground
> 
> >+  /**
> >+   * This will cause us to redecode background images.
> >+   */
> >+  nsChangeHint_InvalidateBackgroundImages = 0x800000
> 
> It would be useful for this comment to indicate (and it would be helpful for
> me when reviewing the patches here) when this hint needs to be sent.

Sure, I'll update the comment in the next version of the patch. But to give an answer right away:

> Does
> it need to be sent only when the background image size changes in a way that
> will not involve reflow?  Or does it need to happen if the image itself
> changes as well?  Or also if the image size changes as a result of reflow
> (which isn't something we can predict accurately when we're computing change
> hints, i.e., without actually doing reflow)?

It needs to be sent if a style change happens that could change either:

(1) the background image itself, or
(2) the size at which the background image is rendered.

It does not need to be sent for changes as a result of reflow, because we check for that when we're actually doing the reflow.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8596302 [details] [diff] [review]
(Part 3) - Add helper functions and background image change detection to nsStyleBackground

Please update the comment per comment 14.

Per the comment in nsChangeHint.h, you need to add this hint
to both nsChangeHint_Hints_NotHandledForDescendants and the big
bitwise-| at the start of NS_HintsNotHandledForDescendantsIn.

>+  if (NS_IsHintSubset(hint, NS_STYLE_HINT_NONE)) {

I think it's much clearer to write "if (hint)".

(I might try to get rid of NS_STYLE_HINT_NONE at some point;
I really don't like 0-constants in bitfield enums.)


r=dbaron with that

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8596308 [details] [diff] [review]
(Part 4) - Add nsCSSRendering::DecodeBackgroundImagesForPredictedSize

I don't follow the removal of the assertion at the start of
nsImageRenderer::ComputeIntrinsicSize.

Should PreviousSize have a more specific name?  Perhaps
PreviousSizeForBackgroundDecode?  (I think that's probably better
because without a more specific name some other caller might be tempted
to use it, which could introduce ordering dependencies about when it
gets updated.  Given the way it's updated, you really want it used only
by this code.)

I think nsCSSRendering::DecodeBackgroundImagesForPredictedSize should
probably bail if aFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW.

nsCSSRendering::DecodeBackgroundImagesForPredictedSize should delete
the PreviousSize property in the early return case.  Otherwise you
could end up with a stale PreviousSize property when there's no longer
a dependency on the size, and keep redecoding for multiple reflows
that don't change size.  (It's possible there might be a more efficient
place to delete it, but it doesn't seem worth worrying about at first
glance.)

>+  const nsPoint offset =
>+    aForFrame->GetOffsetToCrossDoc(nsLayoutUtils::GetReferenceFrame(aForFrame));

Seems like you should just pretend this is (0, 0) rather than computing
it, since all you need in the end is a size.  (It's worth adding a comment
to that effect.)  But the translation really shouldn't be relevant here.


r=dbaron with that, although maybe tn should also have a look at this
patch (especially DecodeBackgroundImageLayerForPredictedSize)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8596310 [details] [diff] [review]
(Part 5) - Redecode CSS background images when their predicted size changes

The change to RestyleManager::ChangeHintToString should be in part 3
instead of this part.

>+  if (backgroundStyle->mImageCount > 0 &&

I believe mImageCount is 1 by default, so I don't think this is
a useful test.  (I don't think it's even possible to have an
mImageCount of 0.)

You do want a useful test here, though.  You might need to write one.

r=dbaron with that, although I should probably have a look at the
test for whether there's an image.

Comment 18

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

Thanks for the reviews!

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #15)
> I think it's much clearer to write "if (hint)".

I didn't know you could do that! I tried "if (hint == NS_STYLE_HINT_NONE)" and that didn't compile because operator== is forbidden for nsChangeHints.

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #16)
> I don't follow the removal of the assertion at the start of
> nsImageRenderer::ComputeIntrinsicSize.

That shouldn't be in there; it snuck in from the version of the patch before the rewrite. I'll remove it.

> Should PreviousSize have a more specific name?  Perhaps
> PreviousSizeForBackgroundDecode?

Yeah, I think that's a good idea.

> I think nsCSSRendering::DecodeBackgroundImagesForPredictedSize should
> probably bail if aFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW.

I think we still need to run the code in that case, because if the image finished loading before the frame gets its first reflow, then we won't have triggered a decode at the correct size. Indeed, we probably won't have triggered a decode at all, because I presume that the frame has size (0,0) before its first reflow, which will make us bail before trying to do any decoding.

> nsCSSRendering::DecodeBackgroundImagesForPredictedSize should delete
> the PreviousSize property in the early return case.

Doh! Good point.
> 
> >+  const nsPoint offset =
> >+    aForFrame->GetOffsetToCrossDoc(nsLayoutUtils::GetReferenceFrame(aForFrame));
> 
> Seems like you should just pretend this is (0, 0) rather than computing
> it, since all you need in the end is a size.  (It's worth adding a comment
> to that effect.)  But the translation really shouldn't be relevant here.

I'll try that out.

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #17)
> I believe mImageCount is 1 by default, so I don't think this is
> a useful test.  (I don't think it's even possible to have an
> mImageCount of 0.)
> 
> You do want a useful test here, though.  You might need to write one.
> 
> r=dbaron with that, although I should probably have a look at the
> test for whether there's an image.

Hmm, OK. I didn't realize it worked that way. I'll rerequest review when I've updated the code appropriately.

Comment 19

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

Comment on attachment 8596308 [details] [diff] [review]
(Part 4) - Add nsCSSRendering::DecodeBackgroundImagesForPredictedSize

Timothy, David suggested that you take a look at DecodeBackgroundImageLayerForPredictedSize() in this patch.

Comment 20

[Andrey Bienkowski]

https://bugzilla.mozilla.org/show_bug.cgi?id=1548706

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

I think the prediction could still be valid for background images but the code likely has changed so much the attached patches are not valid.