802143 - ReusableTileStoreOGL miscalculates the content bounds

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

At some point, FrameMetrics was changed to pre-multiplied values of the render resolution and this broke the content bounds calculations in ReusableTileStoreOGL.

This means that tiles that shouldn't be retained end up retained, and vice-versa. Patch incoming.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Fix display-port and content bounds calculation in ReusableTileStoreOGL::DrawTiles

I think this fixes the problems, but I'm not entirely certain - if you could help test this, that'd be great.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 672268 [details] [diff] [review]
Fix display-port and content bounds calculation in ReusableTileStoreOGL::DrawTiles

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

I think it's worth clarify in what space each variables are before landing.

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +217,5 @@
>        const FrameMetrics& parentMetrics = parent->GetFrameMetrics();
>        if (parentMetrics.IsScrollable())
>          scrollableLayer = parent;
>        if (!parentMetrics.mDisplayPort.IsEmpty() && scrollableLayer) {
> +          // Get the display-port bounds

I always have trouble wrapping my head around this kind of code because I don't know in what units each of the measurements are.

// Get the display-port bounds in screen space.

@@ +224,5 @@
> +                                parentMetrics.mDisplayPort.width,
> +                                parentMetrics.mDisplayPort.height);
> +
> +          // Check the scale transform applied to the root layer to determine
> +          // the content resolution.

I'd expect to see a if() when I see 'Check'. Maybe Compute/Calculate.

@@ +230,5 @@
> +          const gfx3DMatrix& rootTransform = rootLayer->GetTransform();
> +          float scaleX = rootTransform.GetXScale();
> +          float scaleY = rootTransform.GetYScale();
> +
> +          // Get the content document bounds

in screen or layer space?

Comment 3

[Benoit Girard (:BenWa)]

I'll be testing the patch today. Up to you if you want to wait for my results or land this.

Comment 4

[Benoit Girard (:BenWa)]

I had the patch in my queue today and it seemed fine. I still saw a few tiny glitches but I have no reason to believe it's coming from this patch.

Comment 5

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/05863d81fa3b - forgot to clarify comments, will push a follow-up.

Comment 6

[Chris Lord [:cwiiis]]

Clarification: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e53249351d

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/05863d81fa3b
https://hg.mozilla.org/mozilla-central/rev/c2e53249351d