Bug 74719 - TextureMapper: Simplify transform manipulations.
Summary: TextureMapper: Simplify transform manipulations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks: 75918
  Show dependency treegraph
 
Reported: 2011-12-16 08:38 PST by Jocelyn Turcotte
Modified: 2012-01-13 06:40 PST (History)
2 users (show)

See Also:


Attachments
Patch (12.53 KB, patch)
2011-12-16 08:39 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2012-01-12 09:06 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (12.64 KB, patch)
2012-01-13 06:19 PST, Jocelyn Turcotte
noam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>