Bug 65708 - Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState
: Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-08-04 11:43 PST by
Modified: 2011-08-08 16:21 PST (History)


Attachments
Patch (18.97 KB, patch)
2011-08-04 11:51 PST, Chris Marrin
no flags Review Patch | Details | Formatted Diff | Diff
Patch (16.56 KB, patch)
2011-08-04 15:34 PST, Chris Marrin
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.79 KB, patch)
2011-08-08 14:27 PST, Chris Marrin
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-04 11:43:26 PST
Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState
------- Comment #1 From 2011-08-04 11:51:23 PST -------
Created an attachment (id=102952) [details]
Patch
------- Comment #2 From 2011-08-04 12:49:42 PST -------
(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?

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?
------- Comment #3 From 2011-08-04 13:01:25 PST -------
(In reply to comment #2)
> (From update of attachment 102952 [details] [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.
------- Comment #4 From 2011-08-04 15:34:28 PST -------
Created an attachment (id=102994) [details]
Patch
------- Comment #5 From 2011-08-04 16:24:42 PST -------
(From update of attachment 102994 [details])
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.
------- Comment #6 From 2011-08-08 14:27:35 PST -------
Created an attachment (id=103294) [details]
Patch
------- Comment #7 From 2011-08-08 14:33:41 PST -------
(From update of attachment 103294 [details])
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.
------- Comment #8 From 2011-08-08 16:21:21 PST -------
Committed r92651: <http://trac.webkit.org/changeset/92651>