Closed Bug 1183378 Opened 5 years ago Closed 3 years ago

Add downscale-during-decode support for layerized images

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: seth, Assigned: aosmond)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(3 files, 11 obsolete files)

942 bytes, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.52 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
13.98 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
Blocks: 1192550
Depends on: 1251804
Blocks: 1255641
Keywords: feature
Assignee: seth.bugzilla → aosmond
Priority: -- → P3
Whiteboard: [gfx-noted]
Depends on: 1368776
See Also: → 1367987
Abstract out the logic to compute the image decode size from the dest rect, the frame and an image container.
Attachment #8898896 - Attachment is obsolete: true
When generating WebRender commands, ensure we use the scaled image containers.
Updated to pass in the SVG context. I should probably create a helper function like in part 1 to avoid all the code duplication...
Attachment #8926977 - Attachment is obsolete: true
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.
(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?
(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?
Rename and calculate SVG viewport size to get another reftest passing.
Attachment #8929413 - Attachment is obsolete: true
Use the renamed ComputeImageContainerDrawingParameters method to also generate the necessary SVG parameters.
Attachment #8928661 - Attachment is obsolete: true
(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
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.
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.
Attachment #8929519 - Attachment is obsolete: true
Rebase.
Attachment #8929521 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8930170 - Flags: review?(tnikkel)
Attachment #8930192 - Flags: review?(tnikkel)
Attachment #8930193 - Flags: review?(tnikkel)
Attachment #8930194 - Flags: review?(tnikkel)
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.
(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 on attachment 8930194 [details] [diff] [review]
0004-Bug-1183378-Part-4.-gfxPlatform-CanRenderContentToDa.patch

I don't think I know enough to review this.
Attachment #8930194 - Flags: review?(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.
Attachment #8930170 - Flags: review?(tnikkel)
(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.
Attachment #8930194 - Attachment is obsolete: true
(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?
Attachment #8930170 - Attachment is obsolete: true
Remove return value checks and pass in the stacking context.
Attachment #8930192 - Attachment is obsolete: true
Attachment #8930192 - Flags: review?(tnikkel)
Attachment #8930583 - Flags: review?(tnikkel)
Attachment #8930584 - Flags: review?(tnikkel)
Attachment #8930193 - Flags: review?(tnikkel) → review+
Attachment #8930583 - Flags: review?(tnikkel) → review+
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().
Attachment #8930584 - Flags: review?(tnikkel) → review+
(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).
Pushed by aosmond@gmail.com:
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
Blocks: 1420279
Blocks: 1337552
Duplicate of this bug: 1416022
Depends on: 1420648
Regressions: 1556156
You need to log in before you can comment on or make changes to this bug.