Bug 91815 – [chromium] Remove redundant surface origin transforms

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Remove redundant surface origin transforms

[Adrienne Walker]

Created attachment 153400 [details]
Patch

[Dana Jansens]

Comment on attachment 153400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review

LVGTM

> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
>      const WebKit::WebTransformationMatrix& screenSpaceTransform() const { return m_screenSpaceTransform; }
>      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }

Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:-61
> -    WebKit::WebTransformationMatrix m_drawTransform;

:D

[Adrienne Walker]

Created attachment 154511 [details]
Rebased

[Adrienne Walker]

(In reply to comment #2)
> (From update of attachment 153400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review
> 
> > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> >      const WebKit::WebTransformationMatrix& screenSpaceTransform() const { return m_screenSpaceTransform; }
> >      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }
> 
> Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?

Theoretically we could, but I have this idea that you might want the surface scale to not be the same as the device scale.  Maybe the owning layer has a ridiculous scale on it that gets propagated to its descendants and so the surface contents are huge but its rect as a contributing surface is small.

[Dana Jansens]

Comment on attachment 153400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review

>>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
>>>      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }
>> 
>> Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?
> 
> Theoretically we could, but I have this idea that you might want the surface scale to not be the same as the device scale.  Maybe the owning layer has a ridiculous scale on it that gets propagated to its descendants and so the surface contents are huge but its rect as a contributing surface is small.

Hm, a CSS scale on a layer doesn't change its contentBounds though. So if the layer did have a huge scale, the contents would all remain the same.

[Dana Jansens]

Comment on attachment 153400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review

>>>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
>>>>      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }
>>> 
>>> Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?
>> 
>> Theoretically we could, but I have this idea that you might want the surface scale to not be the same as the device scale.  Maybe the owning layer has a ridiculous scale on it that gets propagated to its descendants and so the surface contents are huge but its rect as a contributing surface is small.
> 
> Hm, a CSS scale on a layer doesn't change its contentBounds though. So if the layer did have a huge scale, the contents would all remain the same.

More specifically, in the owningLayer case: The owningLayer's transform is always ~identity. If it had a huge scale, then that scale would appear in the drawTransform of the contributing surface. The surface contents would not be affected. I'm trying to imagine a bad case but having trouble..

Owing layer has a child with a scale(100), this makes the surface texture large. And the owning layer has a scale(1/100). In this case you would want the scale of (things drawing into) the surface to be 1/100? But there's at least one thing drawing into the surface at scale(1), the owning layer.  So this would then get scaled by 1/10000 in screen space. Am I missing something here? It feels like we'd never want to use a different scale than device.

[Adrienne Walker]

(In reply to comment #6)
> (From update of attachment 153400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review
> 
> >>>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> >>>>      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }
> >>> 
> >>> Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?
> >> 
> >> Theoretically we could, but I have this idea that you might want the surface scale to not be the same as the device scale.  Maybe the owning layer has a ridiculous scale on it that gets propagated to its descendants and so the surface contents are huge but its rect as a contributing surface is small.
> > 
> > Hm, a CSS scale on a layer doesn't change its contentBounds though. So if the layer did have a huge scale, the contents would all remain the same.
> 
> More specifically, in the owningLayer case: The owningLayer's transform is always ~identity. If it had a huge scale, then that scale would appear in the drawTransform of the contributing surface. The surface contents would not be affected. I'm trying to imagine a bad case but having trouble..
> 
> Owing layer has a child with a scale(100), this makes the surface texture large. And the owning layer has a scale(1/100). In this case you would want the scale of (things drawing into) the surface to be 1/100? But there's at least one thing drawing into the surface at scale(1), the owning layer.  So this would then get scaled by 1/10000 in screen space. Am I missing something here? It feels like we'd never want to use a different scale than device.

The owning layer's transform is not identity.  It includes surface scale in there.  The surface's draw transform is what the layer's draw transform would have been had it not created a surface.  So, assume a surface's draw transform scales the surface by 1/100 when drawing as a contributing surface.  If there's some child of the owner with a drawable content rect of 1000x1000 in that surface, that drawable content rect in the surface's target is only 10x10.  However, to draw those 10x10 pixels, right now we would use a 1000x1000 surface.

My argument is that if you have a surface that draws as a contributing surface at 1/100 scale, shouldn't you likewise use a surface scale that gets closer to a 1:1 ratio between surface pixels and target pixels? It's less wasteful of memory and has better linear interpolation properties, since we don't have mips.

[Dana Jansens]

Comment on attachment 153400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153400&action=review

>>>>>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
>>>>>>      void setScreenSpaceTransform(const WebKit::WebTransformationMatrix& screenSpaceTransform) { m_screenSpaceTransform = screenSpaceTransform; }
>>>>> 
>>>>> Can we remove this too, since its == the owning layer anyway. And we're always passing around Layers (renderTargets) instead of RenderSurface*s now?
>>>> 
>>>> Theoretically we could, but I have this idea that you might want the surface scale to not be the same as the device scale.  Maybe the owning layer has a ridiculous scale on it that gets propagated to its descendants and so the surface contents are huge but its rect as a contributing surface is small.
>>> 
>>> Hm, a CSS scale on a layer doesn't change its contentBounds though. So if the layer did have a huge scale, the contents would all remain the same.
>> 
>> More specifically, in the owningLayer case: The owningLayer's transform is always ~identity. If it had a huge scale, then that scale would appear in the drawTransform of the contributing surface. The surface contents would not be affected. I'm trying to imagine a bad case but having trouble..
>> 
>> Owing layer has a child with a scale(100), this makes the surface texture large. And the owning layer has a scale(1/100). In this case you would want the scale of (things drawing into) the surface to be 1/100? But there's at least one thing drawing into the surface at scale(1), the owning layer.  So this would then get scaled by 1/10000 in screen space. Am I missing something here? It feels like we'd never want to use a different scale than device.
> 
> The owning layer's transform is not identity.  It includes surface scale in there.  The surface's draw transform is what the layer's draw transform would have been had it not created a surface.  So, assume a surface's draw transform scales the surface by 1/100 when drawing as a contributing surface.  If there's some child of the owner with a drawable content rect of 1000x1000 in that surface, that drawable content rect in the surface's target is only 10x10.  However, to draw those 10x10 pixels, right now we would use a 1000x1000 surface.
> 
> My argument is that if you have a surface that draws as a contributing surface at 1/100 scale, shouldn't you likewise use a surface scale that gets closer to a 1:1 ratio between surface pixels and target pixels? It's less wasteful of memory and has better linear interpolation properties, since we don't have mips.

Ah.. right thanks for bearing with me. The scale is in the owning layer drawTransform.. right. This makes lots of sense.

[Dana Jansens]

@kbr or @senorblanco for review?

[Stephen White]

Comment on attachment 154511 [details]
Rebased

I don't really know this code, but the tests pass and I like it when code goes away.  Rubberstamp r=me

[WebKit Review Bot]

Comment on attachment 154511 [details]
Rebased

Clearing flags on attachment: 154511

Committed r123771: <http://trac.webkit.org/changeset/123771>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.