1150774 - Use the correct units in nsDisplayImageContainer::ConfigureLayer

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

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

I noticed when working on bug 1150704 that we're taking a dest rect in (implicitly) LayoutDevicePixel units here:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?from=nsDisplayImage::ConfigureLayer#1585

And adding it to an offset in (implicitly) LayerIntPoint units here:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?from=nsDisplayImage::ConfigureLayer#1587

That's a unit mismatch, and unfortunately we seem to be consistently handling units sloppily in ConfigureLayer implementations.

In this bug I'll switch to strongly typed units in ConfigureLayer, and use them to fix this bug.

Comment 1

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

Attachment: (Part 1) - Add helpers to simplify using typed units in ConfigureLayer

Here's part 1. This patch adds various helper methods that will simplify the
code in part 2.

Comment 2

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

Attachment: (Part 2) - Use the correct units in nsDisplayImageContainer::ConfigureLayer and related code

This patch has the meat. The goals here were:

- Correct the computation in ConfigureLayer, which was mixing up
  LayoutDevicePixels and LayerPixels.

- Switch to strongly-typed units in ConfigureLayer to make it harder to make the
  same mistake in the future.

I ended up having to change a few other things to achieve these goals. I
explicitly did not try to completely convert these classes over to
strongly-typed units in this patch, though.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8587789 [details] [diff] [review]
(Part 1) - Add helpers to simplify using typed units in ConfigureLayer

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

r+ with comments addressed. Thanks!

::: layout/base/FrameLayerBuilder.cpp
@@ +2269,5 @@
>                   "Can't be a solid color as well as an image!");
>      if (imageContainer) {
>        nsRefPtr<ImageLayer> imageLayer = CreateOrRecycleImageLayer(data->mLayer);
>        imageLayer->SetContainer(imageContainer);
> +      data->mImage->ConfigureLayer(imageLayer, mParameters);

This hunk should be in Part 2.

::: layout/base/FrameLayerBuilder.h
@@ +90,5 @@
>    float mXScale, mYScale;
> +
> +  LayoutDeviceToLayerScale2D Scale() const {
> +    return LayoutDeviceToLayerScale2D(mXScale, mYScale);
> +  }

Since you're introducing this, could you use it in http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=0160303649ae#837 too?

Thanks!

@@ +104,5 @@
>     */
>    nsIntPoint mOffset;
>  
> +  LayerIntPoint Offset() const {
> +    return LayerPixel::FromUntyped(mOffset);

We've generally been writing these as 'LayerIntPoint::FromUntyped(mOffset)' (LayerIntPoint derives from LayerPixel).

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8587885 [details] [diff] [review]
(Part 2) - Use the correct units in nsDisplayImageContainer::ConfigureLayer and related code

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

::: layout/base/nsDisplayList.cpp
@@ +2499,5 @@
>    // layer pixel boundaries. This should be OK for now.
>  
>    int32_t appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
> +  mDestRect =
> +    LayoutDevicePixel::FromAppUnits(state.mDestArea, appUnitsPerDevPixel);

LayoutDeviceRect::FromAppUnits

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8587885 [details] [diff] [review]
(Part 2) - Use the correct units in nsDisplayImageContainer::ConfigureLayer and related code

>+nsDisplayImage::ConfigureLayer(ImageLayer* aLayer,

>-  const gfxRect destRect = GetDestRect();
>+  const int32_t factor = mFrame->PresContext()->AppUnitsPerDevPixel();
>+  const LayoutDeviceRect destRect =
>+    LayoutDeviceRect::FromAppUnits(GetDestRect(), factor);
> 
>-  gfxPoint p = destRect.TopLeft() + aOffset;
>+  const LayerRect destLayerRect = destRect * aParameters.Scale();
>+  const LayerPoint p = destLayerRect.TopLeft() + aParameters.Offset();
>   Matrix transform = Matrix::Translation(p.x, p.y);
>-  transform.PreScale(destRect.Width() / imageWidth,
>-                     destRect.Height() / imageHeight);
>+  transform.PreScale(destLayerRect.Width() / imageWidth,
>+                     destLayerRect.Height() / imageHeight);
>   aLayer->SetBaseTransform(gfx::Matrix4x4::From2D(transform));
> }

So we are effectively adding a scale by aParameters.Scale() to the matrix transform here. But FrameLayerBuilder adds a postscale with the same scale here

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=10e5dd951795#2839

So isn't this going to double scale the image?

Comment 6

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

(In reply to Timothy Nikkel (:tn) from comment #5)
> So isn't this going to double scale the image?

Yeah. I'll remove the scale and add a comment.

Even more amusing, it was the units for aParameters.Offset() that originally led me to file this bug. It turns out that whenever we hit a code path that actually calls ConfigureLayer(), aParameters.Offset() is zero. That's why we never noticed an issue before: there isn't one.

<sad trombone>

I'm going to update the patch to get rid of the addition of the offset as well, and just assert that it's zero. (With a comment that we need to figure out the tricky units issues here if that ever changes.)

Comment 7

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

Oh, and also, I'm going to add a part 3 with a simple layerized image test that would've caught that double scale bug.

Comment 8

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

Attachment: (Part 2) - Use the correct units in nsDisplayImageContainer::ConfigureLayer and related code

This updated version of part 2 is the same except that we no longer apply the
ContainerLayerParameters scale, per comment 5, and we assert that the
ContainerLayerParameters offset is zero.

Even though the rest of the changes in part 2 were motivated by the issue with
the offset's units, they still seem worth making. At least we're taking a small
step in the direction of using strongly-typed units everywhere.

Comment 9

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

(In reply to Seth Fowler [:seth] from comment #7)
> Oh, and also, I'm going to add a part 3 with a simple layerized image test
> that would've caught that double scale bug.

... or not. Unfortunately, I have not had much luck writing a test for this that fails with the original version of part 2. I've only been able to reproduce a failure when I modify nsDisplayImage to always give images an active layer. =\

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/03c86fb075bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/1620323c1a01

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03c86fb075bd
https://hg.mozilla.org/mozilla-central/rev/1620323c1a01