make mCumulativeResolution on FrameMetrics include css transform scale with webrender
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
People
(Reporter: tnikkel, Assigned: tnikkel)
Details
Attachments
(1 obsolete file)
|
Assignee | |
Comment 1•3 years ago
|
||
With non-wr cumulative resolution contained the presshell resolution, css transform scales, and so called extra resolution. The extra resolution came about from FrameLayerBuilder code called ChooseScale that sometimes chose to render at a larger resolution so that we didn't have to re-render on every frame for animating scales for example.
Bug 1485834 is responsible for the current code and it looks it was just getting zooming to work at all on android with webrender. It put the presshell resolution into mCumulativeResolution. This would make zooming work as before this mCumulativeResolution would be 1. This is because mCumulativeResolution came from the ContainerParameters, which are an implementation detail of non-wr that wr didn't have. So it looks like bug 1485834 just used what was handy (the presshell resolution) to get zooming to work.
Making this use GetTransformToAncestorScale means that we are the same as non-wr except for the extra resolution.
I'm not sure that non-wr should have used the extra resolution, I think it would have been better off using GetTransformToAncestorScale as well.
Using the scale from GetTransformToAncestorScale is what several other places do already: CalculateBasicFrameMetrics, and the code to compute our display port rect from the margins data, so this should be more consistent.
More generally speaking, I think that other than the notable exceptions below, mCumulativeResolution only needs to be set to the presshell resolution, and it is okay to include the css transform scale or extra resolution as long as we do apply and un-apply it consistently. Most places that use mCumulativeResolution multiply it in one place and then divide it in another.
The main exception to that is the display port size in the following places:
- layer pixels computed using mCumulativeResolution are compared against prefs that are in layer pixels in ExpandDisplayPortToDangerZone
- GetDisplayportAlignmentMultiplier uses layer pixels computed using mCumulativeResolution to determine alignment
- CalculatePendingDisplayPort sets the final displayport margins in screen pixels computed using mCumulativeResolution
- GetDisplayPortFromMarginsData computes the actual displayport rect we use for painting using GetTransformToAncestorScale
Ideally this should match how we calculate the margins in CalculatePendingDisplayPort.
One will note that after this patch extra resolution is always 1 (I plan to remove it in a followup), one might wonder if that is true with webrender. It is true that webrender still has ChooseScale
and it uses when it encounters content that it cannot natively render so it is rasterized or recorded in the content process. So content can get rasterized or recorded with extra resolution, however it doesn't make it to the wr side in a structured fashion. The items in question are sent to wr as "images" (blob images) with a rect that is equal to their layout rect, without any transforms, resolution, or extra resolution applied. The "images" themselves do contain the extra resolution but they would be drawn using that non-transformed layout rect, and in this case since they have a transform, that transform would be sent to wr on the stacking context pushed for that transform and then that transform would be applied. Wr would not except apz to know about this extra resolution and would not expect values it gets from apz to include this extra resolution.
Now inside wr it could also do something similar where it renders at a resolution that differs from the presshell resolution + css transform scale, but that too would have to be completely internal to wr and it could not expect anyone outside of wr to know about.
The other thing that simplifies this is that the only scrollframe that can have resolution (root content doc root scroll frame) cannot also be transformed.
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
|
||
Hmm, the inputs to AsyncPanZoomController::ConvertScrollbarPoint are the same, but we divide by the zoom so the outputs are different. The input point is transformed by aMouseInput.TransformToLocal(aApzc->GetTransformToThis());. Hmm, not sure what is going wrong here yet.
|
|
Assignee | |
Comment 5•3 years ago
|
||
There's also bug 1702859 on file where webrender doesn't handle resolution with sub frame scrollbars.
|
|
Assignee | |
Comment 6•3 years ago
|
||
From webrender
HitTestingTreeNode (1387c7090) APZC (0) g=(l=0x100000009) r=({ }) t=([ I ]) c=(none)
HitTestingTreeNode (1387caab0) APZC (0) g=(l=0x100000009) r=({ }) t=([ I ]) c=(none)
HitTestingTreeNode (10cf3f100) APZC (1693fe000) g=({ l=0x100000009, p=8, v=5 }) r=({ }) t=([ I ]) c=(none)
HitTestingTreeNode (138958e90) APZC (1693fe000) g=({ l=0x100000009, p=8, v=5 }) r=({ }) t=([ I ]) c=(none) scrollbar scrollthumb
HitTestingTreeNode (138959080) APZC (1693fe000) g=({ l=0x100000009, p=8, v=5 }) r=({ }) t=([ I ]) c=(none) scrollbar scrollthumb
HitTestingTreeNode (1387ca2f0) APZC (1693fe000) g=({ l=0x100000009, p=8, v=5 }) r=({ }) t=([ 0.7 0; 0 0.7; 326 266; ]) c=(none)
HitTestingTreeNode (1387ca8c0) APZC (1693fb000) g=({ l=0x100000009, p=8, v=6 }) r=({ }) t=([ I ]) c=([0,0,970,800])
Note that the scrollthumb nodes share apzc (1693fe000) with the node that has the 0.7 scale transform on it. You'd think this would trip the "Two layers that scroll together have different ancestor transforms" assert, but i does not, maybe because it's a scrollbar part.
|
|
Assignee | |
Comment 7•3 years ago
|
||
When webrender is disabled (layers) there is no scale transform in the HTTN at all.
|
||
Comment 8•3 years ago
•
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
Note that the scrollthumb nodes share apzc (1693fe000) with the node that has the 0.7 scale transform on it. You'd think this would trip the "Two layers that scroll together have different ancestor transforms" assert, but i does not, maybe because it's a scrollbar part.
The reason it does not is that the "ancestor transform" excludes the transform on the scrollable node itself -- it starts at the scrollable node's parent, and goes until the common ancestor of the nodes sharing the APZC.
The reason for this is that the APZC's async transform applies on top of the scrollable node's own ("CSS") transform, and the purpose of the restriction is so that async scrolling does not move different content by different amounts visually. So, if the nodes sharing the APZC each apply different local transforms, then the async scroll is applied to each, and everything from then on to the root is the same, then things are fine.
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
When webrender is disabled (layers) there is no scale transform in the HTTN at all.
I suspect because that transform comes from multiplying a pre- (or post-, I forget) scale with a base transform which cancel out?
|
|
Assignee | |
Comment 9•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #8)
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
Note that the scrollthumb nodes share apzc (1693fe000) with the node that has the 0.7 scale transform on it. You'd think this would trip the "Two layers that scroll together have different ancestor transforms" assert, but i does not, maybe because it's a scrollbar part.
The reason it does not is that the "ancestor transform" excludes the transform on the scrollable node itself -- it starts at the scrollable node's parent, and goes until the common ancestor of the nodes sharing the APZC.
The reason for this is that the APZC's async transform applies on top of the scrollable node's own ("CSS") transform, and the purpose of the restriction is so that async scrolling does not move different content by different amounts visually. So, if the nodes sharing the APZC each apply different local transforms, then the async scroll is applied to each, and everything from then on to the root is the same, then things are fine.
Thanks for the explanation.
The scrollbars are also inside the transform in this case, so the HTTN not having the 0.7 scale transform seems like a (the?) bug
|
|
Assignee | |
Comment 10•3 years ago
|
||
Page structure:
<div scale 0.7>
<scrollable div>
The test tries to drag the scrollable on the scrollable div.
In both webrender and layers the apzc we use in SetupScrollbarDrag is the apzc for the scrollable div. This seems wrong because the scrollbars are outside of that apzc. Perhaps this is setup this way so that events targetting those scrollbars go to the apzc they control.
We then use GetTransformToThis on the apzc to transform the input coords from screen space to the apzc. With layers this is a transform with a scale of 1 and a translate. In the layer tree where ever there is a scale of 0.7 it's cancelled out with a preScale of 1/0.7 on the layer, so I guess it makes sense that the GetTransformToThis transform of the apzc has a scale of 1. With webrender this is a transform with scale 1/0.7.
GetTransformToThis's comment says "Returns the matrix that transforms points from global screen space into this APZC's ParentLayer space", so we must be considering the parent layer space to be different things? In the layers case the parent layer space is outside of the transform? In the webrender the parentlayer space is inside the transform?
So in summary:
layers
TransformToLocal(GetTransformToThis) scales by 1 then ConvertScrollbarPoint scales by 1/0.7 (frame metrics zoom contains css scale)
webrender
TransformToLocal(GetTransformToThis) scales by 1/0.7 then ConvertScrollbarPoint scales by 1 (frame metrics zoom does not contain css scale)
In the layers case for any input that is not targeted at a scrollbar we will use TransformToLocal(GetTransformToThis) but not ConvertScrollbarPoint so how do coords get to the right place?
|
|
||
Comment 11•3 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
In both webrender and layers the apzc we use in SetupScrollbarDrag is the apzc for the scrollable div. This seems wrong because the scrollbars are outside of that apzc. Perhaps this is setup this way so that events targetting those scrollbars go to the apzc they control.
Yup, hitting a scrollbar node deliberately targets the APZC scrolled by that scrollbar.
That's implemented here for the internal hit-test and here for the WR hit-test.
We then use GetTransformToThis on the apzc to transform the input coords from screen space to the apzc. With layers this is a transform with a scale of 1 and a translate. In the layer tree where ever there is a scale of 0.7 it's cancelled out with a preScale of 1/0.7 on the layer, so I guess it makes sense that the GetTransformToThis transform of the apzc has a scale of 1. With webrender this is a transform with scale 1/0.7.
GetTransformToThis's comment says "Returns the matrix that transforms points from global screen space into this APZC's ParentLayer space", so we must be considering the parent layer space to be different things? In the layers case the parent layer space is outside of the transform? In the webrender the parentlayer space is inside the transform?
The idea with Layers was that the transform from an APZC up to the root (so the inverse of GetTransformToThis) is the transform that the compositor applies to the rendered pixels.
If the scroll frame is subject to, say, a rotation transform, then that's part of what the compositor applies.
But if the scroll frame is subject to a scale transform, then rather than rendering the pixels at the original resolution and then having the compositor scale them up, we render the pixels at a different resolution that accounts for the scale, so e.g. in the case of a scale(0.7), there are fewer pixels to begin with. To avoid the final rendered image having the scale applied twice, the scale component of the compositor transform is cancelled out via the pre-scale.
So, I guess the Layers interpretation of ParentLayer coordinate system was "the coordinate system of the rendered pixels (with the rendering including a resolution to account for any enclosing scale), plus any async transform applied, prior to any enclosing transforms applied by the compositor (which exclude the scale component)".
We could choose to match that interpretation exactly with WebRender (which would imply matching the pre-scale logic), or we could choose to do something different that's simpler in a WebRender world.
So in summary:
layers
TransformToLocal(GetTransformToThis) scales by 1 then ConvertScrollbarPoint scales by 1/0.7 (frame metrics zoom contains css scale)
webrender
TransformToLocal(GetTransformToThis) scales by 1/0.7 then ConvertScrollbarPoint scales by 1 (frame metrics zoom does not contain css scale)In the layers case for any input that is not targeted at a scrollbar we will use TransformToLocal(GetTransformToThis) but not ConvertScrollbarPoint so how do coords get to the right place?
TransformToLocal(GetTransformToThis) gives us ParentLayer pixels, which includes the zoom, which includes the cumulative resolution, which includes the resolution to account for the scale from the enclosing transform. So, if we're e.g. targeting the CSS pixels of the scrolled content, we'll divide by the zoom and get our 1/0.7 scale there.
|
|
Assignee | |
Comment 12•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #11)
So, I guess the Layers interpretation of ParentLayer coordinate system was "the coordinate system of the rendered pixels (with the rendering including a resolution to account for any enclosing scale), plus any async transform applied, prior to any enclosing transforms applied by the compositor (which exclude the scale component)".
We could choose to match that interpretation exactly with WebRender (which would imply matching the pre-scale logic), or we could choose to do something different that's simpler in a WebRender world.
Thanks for the explanation, that makes a lot more sense.
With webrender we are currently using these transforms in order to target OOP ifs with fission for the parent to child matrix, and we don't have any notion of pixels being rendered at a scale, so unless we have a separate parallel set of transforms that work differently I think it makes sense to keep webrender going in the direction of having "transforms of coords" rather than the layers view of having "transforms of rendered pixels". I'm open to other ideas of course.
|
|
||
Comment 13•3 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #12)
With webrender we are currently using these transforms in order to target OOP ifs with fission for the parent to child matrix, and we don't have any notion of pixels being rendered at a scale, so unless we have a separate parallel set of transforms that work differently I think it makes sense to keep webrender going in the direction of having "transforms of coords" rather than the layers view of having "transforms of rendered pixels".
That makes sense.
Would that mean excluding the css scale from the cumulative resolution? (If so, we may have just discovered the justification for the decision in this patch not to include it; too bad it wasn't written down.)
|
|
Assignee | |
Comment 14•3 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #13)
(In reply to Timothy Nikkel (:tnikkel) from comment #12)
With webrender we are currently using these transforms in order to target OOP ifs with fission for the parent to child matrix, and we don't have any notion of pixels being rendered at a scale, so unless we have a separate parallel set of transforms that work differently I think it makes sense to keep webrender going in the direction of having "transforms of coords" rather than the layers view of having "transforms of rendered pixels".
That makes sense.
Would that mean excluding the css scale from the cumulative resolution? (If so, we may have just discovered the justification for the decision in this patch not to include it; too bad it wasn't written down.)
Maybe. But then our display port sizing will be off. So we'd have to change that code to factor in the transform somehow.
Or if we still wanted to have the css scale in the cumulative resolution we'd need to re-write some amount of code to handle that. Not sure how much code that is. Some of this code might be incorrect with the different way that webrender transforms work even if we don't make a change to cumulative resolution (there is bug 1702859 that seems like it's underlying issue would lie in this area).
|
|
Assignee | |
Comment 15•3 years ago
|
||
I'm going to mark this bug as invalid. It seems like this is not the approach we want to go with moving forward.
It seemed like this was the best way forward because it most closely matched what we did with layers for many years. But new information learned here indicates that webrender changes things significantly enough that a different approach with webrender might be best, and at the very least continuing the same approach we've been using with webrender (also for years, but significantly less years than layers) is almost certain to uncover less underlying bugs.
Specifically, we learned that there is code depending on cumulative resolution being the presshell resolution in our scrollbar code (ConvertScrollbarPoint, but also our async scroll thumb transformer seems to need some changes in the presence of transforms). We also learned including the transform scale in cumulative resolution will cause us to render incorrectly a testcase that animates scale from 0 to 1 on the compositor: the cumulative resolution will be 0 and stay zero during the animation because we don't invalidate on the content main thread. We could probably fix this by special casing 0 scales to be 1 in case they animate, but this then moves into the territory that ChooseScale did, where we try to pick a better scale to draw at, but this code shouldn't be trying to do that, that kind of stuff is encapsulated inside webrender, and we don't want to try keep in sync with webrender if possible. These issues, and the prospect there might be further issues we haven't discovered yet lead me to think this is not the best path forward.
Bug 1731929 will clean up these fields to make a bit more logical sense.
|
|
||
Updated•3 years ago
|
Description
•