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.

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

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.

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.

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

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

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

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.

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.

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

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

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 on attachment 8596308 [details] [diff] [review] (Part 4) - Add nsCSSRendering::DecodeBackgroundImagesForPredictedSize Timothy, David suggested that you take a look at DecodeBackgroundImageLayerForPredictedSize() in this patch.