Bug 69034 – [chromium] Track separate scroll deltas on the compositor thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Track separate scroll deltas on the compositor thread

[Adrienne Walker]

Created attachment 109081 [details]
Patch

[Adrienne Walker]

Work in progress.  The important API addition is CCLayerTreeHostImpl::scrollRootLayer, which can be used to scroll the whole page from the thread.

[Adrienne Walker]

Created attachment 109205 [details]
Patch

[Adrienne Walker]

The tests handle the CCLayerTreeHostImpl end of things, but I feel like I also probably need some CCThreadProxy tests to make sure that no scroll deltas are dropped and to test ping-ponging between the two trees.

[Vangelis Kokkevis]

Comment on attachment 109205 [details]
Patch

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

Great stuff!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:130
> +    layerLocalTransform.translate3d(centerOffsetX - scrollDelta.width(), centerOffsetY - scrollDelta.height(), -layer->anchorPointZ());

I believe the scrollDelta must be applied to the Tr[l] transform above, not here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:174
> +        bakeScrollDeltasRecursive(scrollInfo, rootLayer());

Can we avoid walking recursively through the entire layer tree every update to collect the scroll deltas?  Potentially we could store them in an info structure every time they get set on a layer?

[James Robinson]

Comment on attachment 109205 [details]
Patch

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

Left a pile o' comments. I think this is on a great track, just some odds and ends to clean up.

> Source/WebCore/platform/CrossThreadCopier.h:135
> +    template<typename T> struct CrossThreadCopier<Vector<T> > {

this seems like a fine cross-thread vector copier. however it's not quite what we really want - we are creating a vector of PODs, passing ownership of that vector to a task, and then taking ownership of that vector from the task on the other thread.

can we just stuff a Vector<OwnPtr<ScrollUpdateInfo> > into the task and take out on the other side, and do whatever template voodoo we need in order to make the verifier not yell at us? that means heap allocating the storage for the vector but means we don't have to do any extra walks through the thing to copy it.

i'm not super worried about performance for our particular use case - it's just that this seems like the wrong bit of infrastructure for our use case, so i'm hesitant to suggest it as the default way to move vectors around across threads.

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:69
> +    IntSize maxScroll = contentsSize - viewportSize;
> +    maxScroll.clampNegativeToZero();

could you remind me how scroll offsets work in RTL mode?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:126
> +    m_scrollDelta += scroll;
> +
> +    IntSize maxDelta = m_maxScrollPosition - toSize(m_scrollPosition);
> +    m_scrollDelta = m_scrollDelta.shrunkTo(maxDelta);

mebbe...

m_scrollDelta = (m_scrollDelta + scroll).shrunkTo(maxDelta); ? maybe go component-wise with min()/max()? this just smells a little funky (maybe it's because our size/point/rect libraries are awkward)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:154
> +    void scroll(const IntSize& scroll);

ScrollView.h calls this 'scrollBy' to highlight that it's relative.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.h:59
> +template<> struct CrossThreadCopierBase<false, false, CCLayerTreeHostCommon::ScrollUpdateInfo> : public CrossThreadCopierPassThrough<CCLayerTreeHostCommon::ScrollUpdateInfo> { };

the fact that we have to declare this and declare the vector thing in crossthreadcopier.h seems a tad unfortunate

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:174
> +    if (rootLayer())
> +        bakeScrollDeltasRecursive(scrollInfo, rootLayer());

since we're only doing scroll stuff on the root, can we cheat a bit here and just collect the rootLayer offsets with a FIXME about getting the offsets for other layers? i think it's not yet clear if we want to try to optimize that case or not

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:198
> +    for (size_t i = 0; i < layer->children().size(); ++i)

are we ever gonna have to worry about scroll offsets on mask/reflection layers? we always forget about those

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:88
> +    CCScrollUpdateSet bakeScrollDeltas();

i know we've been using the term 'bake' for this step when discussion the algorithm but now that i see it as a function name it doesn't seem quite so tasty. why would baking the scroll deltas return an UpdateSet? it feels more like this function is calculating something to give us (and as a side effect updating its internal state, but that's the implementation's business)

maybe calculateScrollUpdates()? i'm so boring with naming

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:83
> +class ScopedSetImplThread {
> +public:
> +    ScopedSetImplThread();
> +    ~ScopedSetImplThread();
> +};

Now that this is in the header by itself it seems less obvious that this is purely for debugging and has no impact on how code actually works. Can we try to make this more obvious somehow?  Maybe just put a DEBUG in the name, or use some sort of macro trickery?

We also might as well stick the impl of the c'tor / d'tor in the header, so that in release the compiler can see there's nothing except #ifndef NDEBUG stuff and can really easily compile it all away

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:67
> +    ScopedSetImplThread impl;
> +    CCSettings settings;
> +    OwnPtr<CCLayerTreeHostImpl> hostImpl = CCLayerTreeHostImpl::create(settings);

rather than having this repeated in every TEST you can create a fixture (an object derived from testing::Test) that has all the state you want set up in its c'tor and then declare these tests TEST_F(FixtureType. name) and then use them. This makes the testcases member functions of a subclass of the fixture class so you can access protected members

[Adrienne Walker]

(In reply to comment #6)
> (From update of attachment 109205 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109205&action=review
> 
> Left a pile o' comments. I think this is on a great track, just some odds and ends to clean up.

Thanks for all the comments.  :)

> > Source/WebCore/platform/CrossThreadCopier.h:135
> > +    template<typename T> struct CrossThreadCopier<Vector<T> > {
> 
> this seems like a fine cross-thread vector copier. however it's not quite what we really want - we are creating a vector of PODs, passing ownership of that vector to a task, and then taking ownership of that vector from the task on the other thread.

I'll see what I can do to clean this up.  An OwnPtr seems like a better way to do this.
 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:69
> > +    IntSize maxScroll = contentsSize - viewportSize;
> > +    maxScroll.clampNegativeToZero();
> 
> could you remind me how scroll offsets work in RTL mode?

The contents and viewport are unchanged, but the scroll position just starts all the way to the right.  Within the contents, the origin is in a different place, but the size is the same.  (Well, technically the origin is always in the top left of the initial containing block.)

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:126
> > +    m_scrollDelta += scroll;
> > +
> > +    IntSize maxDelta = m_maxScrollPosition - toSize(m_scrollPosition);
> > +    m_scrollDelta = m_scrollDelta.shrunkTo(maxDelta);
>
> m_scrollDelta = (m_scrollDelta + scroll).shrunkTo(maxDelta); ? maybe go component-wise with min()/max()? this just smells a little funky (maybe it's because our size/point/rect libraries are awkward)

Maybe I'll just add a clamp(min, max) function to IntSize.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:154
> > +    void scroll(const IntSize& scroll);
> 
> ScrollView.h calls this 'scrollBy' to highlight that it's relative.

Will fix for consistency.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:174
> > +    if (rootLayer())
> > +        bakeScrollDeltasRecursive(scrollInfo, rootLayer());
> 
> since we're only doing scroll stuff on the root, can we cheat a bit here and just collect the rootLayer offsets with a FIXME about getting the offsets for other layers? i think it's not yet clear if we want to try to optimize that case or not

Vangelis points this out as well.  Maybe I'll just toss the logic for other layers and just hardcode collecting just the root.  No reason to add logic we'll tear out later.  And, we can just dust these test offs and pull them out again when we need them.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:198
> > +    for (size_t i = 0; i < layer->children().size(); ++i)
> 
> are we ever gonna have to worry about scroll offsets on mask/reflection layers? we always forget about those

No.  Masks and reflection layers will never be scrolled separately.  They have scroll offsets because they're layers, but they won't ever get used because calculateDrawTransformsAndVisibility will ignore them.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:88
> > +    CCScrollUpdateSet bakeScrollDeltas();
> 
> i know we've been using the term 'bake' for this step when discussion the algorithm but now that i see it as a function name it doesn't seem quite so tasty. why would baking the scroll deltas return an UpdateSet? it feels more like this function is calculating something to give us (and as a side effect updating its internal state, but that's the implementation's business)
> 
> maybe calculateScrollUpdates()? i'm so boring with naming

Yeah, I tried a few names for this and couldn't come with anything great.  calculateScrollOffsets just doesn't give me an indication that there are side effects to calling it.  processScrollOffsets() maybe?

> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:83
> > +class ScopedSetImplThread {
> > +public:
> > +    ScopedSetImplThread();
> > +    ~ScopedSetImplThread();
> > +};
> 
> Now that this is in the header by itself it seems less obvious that this is purely for debugging and has no impact on how code actually works. Can we try to make this more obvious somehow?  Maybe just put a DEBUG in the name, or use some sort of macro trickery?

DebugScopedSetImplThread, maybe?

> > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:67
> > +    ScopedSetImplThread impl;
> > +    CCSettings settings;
> > +    OwnPtr<CCLayerTreeHostImpl> hostImpl = CCLayerTreeHostImpl::create(settings);
> 
> rather than having this repeated in every TEST you can create a fixture (an object derived from testing::Test) that has all the state you want set up in its c'tor and then declare these tests TEST_F(FixtureType. name) and then use them. This makes the testcases member functions of a subclass of the fixture class so you can access protected members

Sure.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:130
> > +    layerLocalTransform.translate3d(centerOffsetX - scrollDelta.width(), centerOffsetY - scrollDelta.height(), -layer->anchorPointZ());
> 
> I believe the scrollDelta must be applied to the Tr[l] transform above, not here.

Ack, excellent catch.  This would have bitten us once we started scrolling other layers independently.

[Adrienne Walker]

Created attachment 109372 [details]
Patch

[Adrienne Walker]

This last patch is rebaselined and passes webkit_unit_tests.  I think I've addressed all the review feedback.

Other changes from the last patch include additional tests (two in CCLayerTreeHostTest to test the CCThreadProxy) and a fix to calculating the visibility rect on the compositor thread rather than on the main thread.  This fixes the problem of the root layer getting cut off and not drawing the rest of its tiles.

The remaining known bug (which could be fixed elsewhere) is that scissor rects are getting incorrectly clipped against the original viewport.  You can "easily" see this if you hack in (1) a scrollRootLayer call before drawLayers() and (2) disabling processScrollDeltas() to fake scrolling on the compositor thread.  Then open up http://www.webkit.org/blog/386/3d-transforms/ with a viewport height that cuts off part of the flipping layer.  Although the layer scrolls with the page properly, its scissor rect is not updated to reflect the scrolling and so it continues to be clipped unexpectedly.

If somebody else could take this, fix what needs fixing, and land it, that'd be swell.  :)

[Vangelis Kokkevis]

(In reply to comment #9)
> This last patch is rebaselined and passes webkit_unit_tests.  I think I've addressed all the review feedback.
> 
> Other changes from the last patch include additional tests (two in CCLayerTreeHostTest to test the CCThreadProxy) and a fix to calculating the visibility rect on the compositor thread rather than on the main thread.  This fixes the problem of the root layer getting cut off and not drawing the rest of its tiles.
> 
> The remaining known bug (which could be fixed elsewhere) is that scissor rects are getting incorrectly clipped against the original viewport.  You can "easily" see this if you hack in (1) a scrollRootLayer call before drawLayers() and (2) disabling processScrollDeltas() to fake scrolling on the compositor thread.  Then open up http://www.webkit.org/blog/386/3d-transforms/ with a viewport height that cuts off part of the flipping layer.  Although the layer scrolls with the page properly, its scissor rect is not updated to reflect the scrolling and so it continues to be clipped unexpectedly.
> 
> If somebody else could take this, fix what needs fixing, and land it, that'd be swell.  :)

I can look at fixing the scissor rects.  I owe you one from last time :)

[James Robinson]

Comment on attachment 109372 [details]
Patch

This looks great - I think we should land this as is, and then fix up the scissor rect as another patch on top of this one.

[WebKit Review Bot]

Comment on attachment 109372 [details]
Patch

Clearing flags on attachment: 109372

Committed r96454: <http://trac.webkit.org/changeset/96454>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.