Bug 65708 – Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Marrin]

Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState

[Chris Marrin]

Created attachment 102952 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 102952 [details]
Patch

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

I wish we could test this; I see a number of issues (not taking perspective into account, possible scrolling issues).

> Source/WebCore/platform/graphics/GraphicsLayer.h:368
> +    virtual void syncCompositingState(const FloatRect&) { }

I think it's worth naming the parameter in a C-style comment.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:836
> +        state.translate(-anchorPoint().x(), -anchorPoint().y(), -anchorPoint().z(), accumulation);
> +        state.applyTransform(transform(), accumulation);
> +        state.translate(anchorPoint().x(), anchorPoint().y(), anchorPoint().z(), accumulation);

Rather than having to add  TransformState::translate() for this, can't you just compute the transform taking the anchor point into account, then apply it?

Also, you're ignoring childTransform here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:842
> +    if (masksToBounds()) {
> +        // Replace the quad in the TransformState with one that is clipped to this layer's bounds

I think you might want to assert here that accumulation == FlattenTransform, otherwise state.lastPlanarQuad might be in some ancestor's coordinate system.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:96
> +@interface CATiledLayer(GraphicsLayerCAPrivate)
> +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary *)dict;
> +- (BOOL)canDrawConcurrently;
> +- (void)setCanDrawConcurrently:(BOOL)flag;
> +@end

You should #ifdef this for not Leopard and not SnowLeopard.

> Source/WebCore/platform/graphics/transforms/TransformState.cpp:62
> +void TransformState::translate(float x, float y, float z, TransformAccumulation accumulate)

I don't think this is necessary.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:210
> +    if (GraphicsLayer* rootLayer = rootGraphicsLayer()) {
> +        FrameView* frameView = m_renderView ? m_renderView->frameView() : 0;
> +        if (frameView)
> +            rootLayer->syncCompositingState(frameView->frameRect());
> +    }

Is frameRect() the right thing? How is scrolling taken into account?

[Chris Marrin]

(In reply to comment #2)
> (From update of attachment 102952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102952&action=review
> 
> I wish we could test this; I see a number of issues (not taking perspective into account, possible scrolling issues).
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:368
> > +    virtual void syncCompositingState(const FloatRect&) { }
> 
> I think it's worth naming the parameter in a C-style comment.
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:836
> > +        state.translate(-anchorPoint().x(), -anchorPoint().y(), -anchorPoint().z(), accumulation);
> > +        state.applyTransform(transform(), accumulation);
> > +        state.translate(anchorPoint().x(), anchorPoint().y(), anchorPoint().z(), accumulation);
> 
> Rather than having to add  TransformState::translate() for this, can't you just compute the transform taking the anchor point into account, then apply it?

I will do that...

> 
> Also, you're ignoring childTransform here.
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:842
> > +    if (masksToBounds()) {
> > +        // Replace the quad in the TransformState with one that is clipped to this layer's bounds
> 
> I think you might want to assert here that accumulation == FlattenTransform, otherwise state.lastPlanarQuad might be in some ancestor's coordinate system.

Will do

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:96
> > +@interface CATiledLayer(GraphicsLayerCAPrivate)
> > +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary *)dict;
> > +- (BOOL)canDrawConcurrently;
> > +- (void)setCanDrawConcurrently:(BOOL)flag;
> > +@end
> 
> You should #ifdef this for not Leopard and not SnowLeopard.

Ok

> 
> > Source/WebCore/platform/graphics/transforms/TransformState.cpp:62
> > +void TransformState::translate(float x, float y, float z, TransformAccumulation accumulate)
> 
> I don't think this is necessary.

I'll make the change above

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:210
> > +    if (GraphicsLayer* rootLayer = rootGraphicsLayer()) {
> > +        FrameView* frameView = m_renderView ? m_renderView->frameView() : 0;
> > +        if (frameView)
> > +            rootLayer->syncCompositingState(frameView->frameRect());
> > +    }
> 
> Is frameRect() the right thing? How is scrolling taken into account?

For WK2, scrolling in a GraphicsLayer below this, so it should handle it correctly. This is the area I have to change for iOS, since it is like WK1, which does scrolling differently.

[Chris Marrin]

Created attachment 102994 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 102994 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        This is in support of <rdar://problem/9706222> and <rdar://problem/9434765>

No need to mention radar numbers.

> Source/WebCore/page/FrameView.cpp:709
> +            fullScreenLayer->syncCompositingState(frameRect());

Please add a comment that this only works on platforms where RLC uses a scroll layer.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:831
> +void GraphicsLayerCA::recursiveCommitChanges(TransformState& state, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale)
>  {
> +    TransformState::TransformAccumulation accumulation = preserves3D() ? TransformState::AccumulateTransform : TransformState::FlattenTransform;
> +    state.move(m_position.x(), m_position.y(), accumulation);

This code needs to maintain stack of TransformStates, otherwise your state is wrong when you come back out of children.

[Chris Marrin]

Created attachment 103294 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 103294 [details]
Patch

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

r=me but please do the localState thing.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:831
> -void GraphicsLayerCA::recursiveCommitChanges(float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale)
> +void GraphicsLayerCA::recursiveCommitChanges(TransformState& state, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale)
>  {
> +    // Save the state before sending down to kids and restore it after
> +    TransformState savedTransformState = state;

The more natural way to do this would be to have

TransformState localState = state;
and pass localState to kids.

Then your TransformState& param can be const.

[Chris Marrin]

Committed r92651: <http://trac.webkit.org/changeset/92651>