1183378 - Add downscale-during-decode support for layerized images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

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

Right now layerized images are always decoded at their intrinsic size, which both wastes memory and triggers issues like bug 803703 and bug 1182929. Let's fix that by adding a size parameter to GetImageContainer.

Comment 2

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

Attachment: nsImageFrame should use GetImageContainerAtSize to use downscale-on-decode with image layers., v1

Comment 3

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

Attachment: nsImageFrame should use GetImageContainerAtSize to use downscale-on-decode with image layers., v2

Comment 4

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

Attachment: 0022-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeSizeFor.patch

Abstract out the logic to compute the image decode size from the dest rect, the frame and an image container.

Comment 5

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

Attachment: 0023-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch

When generating WebRender commands, ensure we use the scaled image containers.

Comment 6

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

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

Comment 7

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

Attachment: 0023-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v2

Updated to pass in the SVG context. I should probably create a helper function like in part 1 to avoid all the code duplication...

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8928661 [details] [diff] [review]
0023-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v2

Looking at this patch we are using ComputeSizeForDecode for the size of the image we want. ComputeSizeForDecode doesn't quite get it right. For one thing it ignores scales that are being animated. This is because it was based on MaybeDecodeForPredictedSize, which is intended to be used before paint time (ie after reflow when we've sized the image) so we get a head start on decoding the image before paint time.

But the code in this patch is executed at paint time, so we should know the exact size of the image and we should request that.

Comment 9

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

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Comment on attachment 8928661 [details] [diff] [review]
> 0023-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v2
> 
> Looking at this patch we are using ComputeSizeForDecode for the size of the
> image we want. ComputeSizeForDecode doesn't quite get it right. For one
> thing it ignores scales that are being animated. This is because it was
> based on MaybeDecodeForPredictedSize, which is intended to be used before
> paint time (ie after reflow when we've sized the image) so we get a head
> start on decoding the image before paint time.
> 
> But the code in this patch is executed at paint time, so we should know the
> exact size of the image and we should request that.

I can make it call GetTransformToAncestorScale easily on the paint time paths, and GetTransformToAncestorScaleExcludingAnimated on the predictive path. However is there another way to determine the "exact size of the image" at paint time, other than what I am doing?

Comment 10

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

Attachment: 0026-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeSizeFor.patch, v2

Comment 11

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

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Comment on attachment 8928661 [details] [diff] [review]
> 0023-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v2
> 
> Looking at this patch we are using ComputeSizeForDecode for the size of the
> image we want. ComputeSizeForDecode doesn't quite get it right. For one
> thing it ignores scales that are being animated. This is because it was
> based on MaybeDecodeForPredictedSize, which is intended to be used before
> paint time (ie after reflow when we've sized the image) so we get a head
> start on decoding the image before paint time.
> 
> But the code in this patch is executed at paint time, so we should know the
> exact size of the image and we should request that.

ComputeSnappedImageDrawingParameters?

Comment 12

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

Attachment: 0027-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeImageCo.patch, v3

Rename and calculate SVG viewport size to get another reftest passing.

Comment 13

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

Attachment: 0028-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v3

Use the renamed ComputeImageContainerDrawingParameters method to also generate the necessary SVG parameters.

Comment 14

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

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

Comment 15

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

(In reply to Andrew Osmond [:aosmond] from comment #14)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=938fa1cbbfbbd83a4bb60c90cb7f27c6e06287f6

Remaining test failures are:

layout/xul/reftest/image-size.xul
layout/reftests/invalidation/image-scrolling-zoom-1-ref.html != ... zoom-1-notref.html

Comment 16

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

Investigation continues but here's an update.

(In reply to Andrew Osmond [:aosmond] from comment #15)
> (In reply to Andrew Osmond [:aosmond] from comment #14)
> > try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=938fa1cbbfbbd83a4bb60c90cb7f27c6e06287f6
> 
> Remaining test failures are:
> 
> layout/xul/reftest/image-size.xul

This is failing because it used a painted layer before due to the scaling, and now it uses the image container directly, and they produce slightly different results. The weird part is all of the GetImageContainerAtSize and Draw calls seem to use the same, native size of the image. So something else is amiss here.

> layout/reftests/invalidation/image-scrolling-zoom-1-ref.html != ...
> zoom-1-notref.html

This is failing because ComputeImageContainerDrawingParameters is returning an empty size for the first few calls, but eventually succeeds.

Comment 17

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

Attachment: 0001-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeImageCo.patch, v4

Fixes layout/reftests/invalidation/image-scrolling-zoom-1-ref.html. It didn't round up to 1 pixel as the minimum dimension of the size like ComputeSnappedImageDrawingParameters does.

Comment 18

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

Attachment: 0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v4

Rebase.

Comment 19

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

Attachment: 0003-Bug-1183378-Part-3.-Increase-WebRender-fuzz-for-layo.patch

Comment 20

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

Attachment: 0004-Bug-1183378-Part-4.-gfxPlatform-CanRenderContentToDa.patch

Comment 21

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

This should be all green.

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

Comment 22

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

Comment on attachment 8930192 [details] [diff] [review]
0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v4

Review of attachment 8930192 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBulletFrame.cpp
@@ +474,5 @@
> +  Maybe<SVGImageContext> svgContext;
> +  gfx::IntSize decodeSize =
> +    nsLayoutUtils::ComputeImageContainerDrawingParameters(mImage, aItem->Frame(), destRect,
> +                                                          flags, svgContext);
> +  if (decodeSize.IsEmpty()) {

In retrospect, I should remove these decodeSize checks. It should never return empty now with the changes to ComputeImageContainerDrawingParameters. I will remove them when I next update the patch.

Comment 23

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

(In reply to Andrew Osmond [:aosmond] from comment #22)
> Comment on attachment 8930192 [details] [diff] [review]
> 0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v4
> 
> Review of attachment 8930192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsBulletFrame.cpp
> @@ +474,5 @@
> > +  Maybe<SVGImageContext> svgContext;
> > +  gfx::IntSize decodeSize =
> > +    nsLayoutUtils::ComputeImageContainerDrawingParameters(mImage, aItem->Frame(), destRect,
> > +                                                          flags, svgContext);
> > +  if (decodeSize.IsEmpty()) {
> 
> In retrospect, I should remove these decodeSize checks. It should never
> return empty now with the changes to ComputeImageContainerDrawingParameters.
> I will remove them when I next update the patch.

Actually OptimalImageSizeForDest can return empty, if decoding hasn't completed. But I think it would be better to let GetImageContainerAtSize fail than have the redundant check which should relatively rarely fail.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8930194 [details] [diff] [review]
0004-Bug-1183378-Part-4.-gfxPlatform-CanRenderContentToDa.patch

I don't think I know enough to review this.

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8930170 [details] [diff] [review]
0001-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeImageCo.patch, v4

I think we have to use the scale factors that we have on the gfxContext/DrawTarget like ComputeSnappedImageDrawingParameters does here

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6707

instead of using GetTransformToAncestorScale. We don't know the scale that FrameLayerBuilder chooses to render at from calling GetTransformToAncestorScale. That information is only available on the gfxContext.

Comment 26

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

(In reply to Timothy Nikkel (:tnikkel) from comment #24)
> Comment on attachment 8930194 [details] [diff] [review]
> 0004-Bug-1183378-Part-4.-gfxPlatform-CanRenderContentToDa.patch
> 
> I don't think I know enough to review this.

Okay. I can really land this as a separate bug, so I'll take it out.

Comment 27

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

Attachment: 0001-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeImageCo.patch, v5

(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> Comment on attachment 8930170 [details] [diff] [review]
> 0001-Bug-1183378-Part-1.-Add-nsLayoutUtils-ComputeImageCo.patch, v4
> 
> I think we have to use the scale factors that we have on the
> gfxContext/DrawTarget like ComputeSnappedImageDrawingParameters does here
> 
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp#6707
> 
> instead of using GetTransformToAncestorScale. We don't know the scale that
> FrameLayerBuilder chooses to render at from calling
> GetTransformToAncestorScale. That information is only available on the
> gfxContext.

AFAIK there is no gfxContext or FrameLayerBuilder to use. I trust your judgement as far as GetTransformToAncestorScale goes -- StackingContextHelper::GetInheritedScale is probably more relevant [1].

[1 -- calculate GetInheritedScale return] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/wr/StackingContextHelper.cpp#43

It is used to create the transform for a gfxContext used on the fallback path [2a-d].

[2a -- fallback calls GetInheritedScale] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/wr/WebRenderCommandBuilder.cpp#515
[2b -- fallback passes scale on for blob images] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/wr/WebRenderCommandBuilder.cpp#573
[2c -- fallback passes scale on without blob images] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/wr/WebRenderCommandBuilder.cpp#608
[2d -- fallback uses scale to calculate transform] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/wr/WebRenderCommandBuilder.cpp#421

Now that I'm using GetInheritedScale, I'm trying to work through the computational differences with ComputeSnappedImageDrawingParameters.

Both calculate (or accept) the dest rect as a LayoutDeviceRect [3a]. If it *didn't* snap [3b], the result is the same as what I do in ComputeImageContainerDrawingParameters -- dest rect * scale, rounded to nearest integer. So the only potential difference would like in gfxContext::UserToDevicePixelSnapped(destRect, true); this call only happens if it is no reflection (flips due to negative scales) and no rotation (axis aligned rectangle).

[3a -- devPixelDest init] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/layout/base/nsLayoutUtils.cpp#6701
[3b -- if it didn't snap, scale] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/layout/base/nsLayoutUtils.cpp#6734

First UserToDevicePixelSnapped checks if pixel snapping is disabled [4a]; if it is it will fallback to the method in [3b]. This seems to be for printing, which I don't think we are really worrying about with WebRender right now. It will also fallback if we aren't ignoring the scale and it is not 1x, but we explicitly pass ignoreScale = true, so that code does nothing [4b].

[4a -- is pixel snapping disabled] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/thebes/gfxContext.cpp#390
[4b -- ignore the scale] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/thebes/gfxContext.cpp#399

Next we calculate the corners of the dest rect after transforming [4c]. If these have anything but a scale or translation, it would fail at [4d], but we already checked for that before calling UserToDevicePixelSnapped.

[4c -- UserToDevice calls] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/thebes/gfxContext.cpp#405

It then rounds the points to the nearest integer after transforming and creates the "snapped" dest rect based on these points and the property of being axis aligned.

To summarize, if there is any transformation besides a translation and a non-reflective scale, we scale the dest rect size manually using the scale factors, and then round to the nearest integer to be our decode size guess. Otherwise we apply the transformation matrix to the corners of the rectangle, round those points to the nearest integer, use those rounded points to determine the dest rect, and *then* take the size of that dest rect to be our decode size guess.

Is the concern/difference here where the rounding occurs? By transforming the points of the rectangle and then rounding, we can get a subtle +/- 1 pixel difference in the dimensions?

Comment 28

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

Attachment: 0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v5

Remove return value checks and pass in the stacking context.

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8930584 [details] [diff] [review]
0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v5

We should only be using FLAG_HIGH_QUALITY_SCALING if we are painting to the window, ie aDisplayListBuilder->IsPaintingToWindow().

Comment 30

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

(In reply to Timothy Nikkel (:tnikkel) from comment #29)
> Comment on attachment 8930584 [details] [diff] [review]
> 0002-Bug-1183378-Part-2.-Make-WebRender-command-creation-.patch, v5
> 
> We should only be using FLAG_HIGH_QUALITY_SCALING if we are painting to the
> window, ie aDisplayListBuilder->IsPaintingToWindow().

Thanks, I've incorporated that (or a similar check for nsImageRenderer::FLAG_PAINTING_TO_WINDOW where appropriate).

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad104efc64f
Part 1. Add nsLayoutUtils::ComputeImageContainerDrawingParameters which generates the parameters needed for GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1ac3fa322e
Part 2. Make WebRender command creation use scaled image containers. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/889fcba58af7
Part 3. Increase WebRender fuzz for layout/xul/reftest/image-size.xul. r=tnikkel

Comment 33

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/1ad104efc64f
https://hg.mozilla.org/mozilla-central/rev/fc1ac3fa322e
https://hg.mozilla.org/mozilla-central/rev/889fcba58af7