Bug 91815

Summary: [chromium] Remove redundant surface origin transforms
Product: WebKit Reporter: Adrienne Walker <enne>
Component: New BugsAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, enne, jamesr, kbr, senorblanco, shawnsingh, vollick, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91807    
Bug Blocks: 91823, 92328    
Attachments:
Description Flags
Patch
none
Rebased none

Description Adrienne Walker 2012-07-19 21:27:53 PDT
[chromium] Remove redundant surface origin transforms
Comment 1 Adrienne Walker 2012-07-19 21:29:41 PDT
Created attachment 153400 [details]
Patch
Comment 2 Dana Jansens 2012-07-20 13:45:01 PDT
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
Comment 3 Adrienne Walker 2012-07-25 18:41:02 PDT
Created attachment 154511 [details]
Rebased
Comment 4 Adrienne Walker 2012-07-25 18:53:54 PDT
(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.
Comment 5 Dana Jansens 2012-07-26 09:17:28 PDT
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.
Comment 6 Dana Jansens 2012-07-26 09:23:53 PDT
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.
Comment 7 Adrienne Walker 2012-07-26 09:38:16 PDT
(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.
Comment 8 Dana Jansens 2012-07-26 09:55:49 PDT
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.
Comment 9 Dana Jansens 2012-07-26 09:56:32 PDT
@kbr or @senorblanco for review?
Comment 10 Stephen White 2012-07-26 10:26:32 PDT
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
Comment 11 WebKit Review Bot 2012-07-26 11:00:23 PDT
Comment on attachment 154511 [details]
Rebased

Clearing flags on attachment: 154511

Committed r123771: <http://trac.webkit.org/changeset/123771>
Comment 12 WebKit Review Bot 2012-07-26 11:00:27 PDT
All reviewed patches have been landed.  Closing bug.