Bug 74719

Summary: TextureMapper: Simplify transform manipulations.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: kenneth, noam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 75918    
Attachments:
Description Flags
Patch
none
Patch
none
Patch noam: review+

Description Jocelyn Turcotte 2011-12-16 08:38:40 PST
TextureMapper: Simplify transform manipulations.
Comment 1 Jocelyn Turcotte 2011-12-16 08:39:26 PST
Created attachment 119613 [details]
Patch
Comment 2 Noam Rosenthal 2011-12-16 10:00:10 PST
Comment on attachment 119613 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        - Make sure that the replica node has a complete transform and
> +          use it directly instead of keeping a copy in the source.

I can't see how this code would work; Since the replica's parent layer is always null, and you don't check for replicatedLayer.
Someone needs to combine the "regular" layer's transform and the replica's local transform. 
That code was pretty tricky and is tested by many layout tests (compositing/reflection). I see no mention in this changelog that those were tested before+after the changes, which makes me think that these changes would break those reflection tests :)

> Source/WebCore/ChangeLog:18
> +        - Fix the fillsForward transform adjustment in syncAnimations and
> +          make the intention clearer by using setTransform and setOpacity.

Different patch.
Comment 3 Jocelyn Turcotte 2012-01-12 09:06:50 PST
Created attachment 122255 [details]
Patch
Comment 4 Noam Rosenthal 2012-01-12 09:21:56 PST
Comment on attachment 122255 [details]
Patch

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

Good direction, needs some improvements.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69
> +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform)

I don't see why we need to functions. We should just have one function, and use (parent || effectTarget).

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76
> +    m_transforms.target = parentTransform;
> +    m_transforms.target
>          .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
> -        .multiply(m_transforms.base)
> -        .translate3d(-originX, -originY, -m_state.anchorPoint.z());
> +        .multiply(m_transforms.base);

You can do
m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base);
<nitpick/>

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:78
> +    // In case a parent had preserves3D and this layer has not, flatten our children.

We should early-return if we don't have children, after we've translated the target transform back.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:95
> +    // The replica layer isn't parented and needs its transform to be stacked atop ours.
> +    if (m_state.replicaLayer)
> +        m_state.replicaLayer->computeTransformsSelf(m_transforms.target);

Instead, you can call effectTarget from within the replica layer if there's no parent.
Comment 5 Noam Rosenthal 2012-01-12 09:24:08 PST
(In reply to comment #4)
> (From update of attachment 122255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122255&action=review
> 
> Good direction, needs some improvements.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69
> > +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform)
> 
> I don't see why we need to functions. We should just have one function, and use (parent || effectTarget).
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76
> > +    m_transforms.target = parentTransform;
> > +    m_transforms.target
> >          .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
> > -        .multiply(m_transforms.base)
> > -        .translate3d(-originX, -originY, -m_state.anchorPoint.z());
> > +        .multiply(m_transforms.base);
> 
> You can do
> m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base);
> <nitpick/>
I mean
m_transforms.target = 
    TransformationMatrix(parentTransform)
        .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
        .multiply(m_transforms.base);
Comment 6 Noam Rosenthal 2012-01-12 11:54:09 PST
Comment on attachment 122255 [details]
Patch

After discussing this on IRC, I believe that my comments actually don't make this patch any better.
Comment 7 Jocelyn Turcotte 2012-01-13 06:19:57 PST
Created attachment 122421 [details]
Patch

Here, it should be on the right bug. Tried to use webkit-patch upload -g SHA^..SHA, but I guess I'll stick with -g HEAD..
Comment 8 Jocelyn Turcotte 2012-01-13 06:40:15 PST
Committed r104931: <http://trac.webkit.org/changeset/104931>