Bug 110355 – Get rid of "non-composited contents" in CoordinatedLayerTreeHost

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

The "non composited contents" layer is unncessary - it creates a special-case code path that can be better dealt with by making that a proper GraphicsLayer from the get-go, by returning false from RenderLayerBacking::paintingIntoWindow()

[Dongseong Hwang]

Noam raised this needs in https://bugs.webkit.org/show_bug.cgi?id=110299#c5 :)

[Rafael Brandao]

(In reply to comment #0)
> The "non composited contents" layer is unncessary - it creates a special-case code path that can be better dealt with by making that a proper GraphicsLayer from the get-go, by returning false from RenderLayerBacking::paintingIntoWindow()

Great idea! These extra code paths were very confusing when I was digging into CGfx and I still don't get where we use the "overlay layer" code path or what it is exactly, besides the name sounds pretty straightforward on what it should be. Could you explain it also?

[Noam Rosenthal]

> Great idea! These extra code paths were very confusing when I was digging into CGfx and I still don't get where we use the "overlay layer" code path or what it is exactly, besides the name sounds pretty straightforward on what it should be. Could you explain it also?

The overlay code path is used for example for the tap highlighter.
I believe that in coordinated graphics we should do tap highlighting completely in the UI process and not in this way, but that's a different story from the non-composited contents.

[Dongseong Hwang]

Currently, two CoordinatedTiles use the same UpdateAtlas in the same time. It is a bug that can crash in some platforms.

For example,
1. m_nonCompositedContentLayer->flushCompositingStateForThisLayerOnly();
2. CoordinatedTile::updateBackBuffer()
3. FrameView::paintContents()
4. FrameView::flushCompositingStateForThisFrame()
5. CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
6. CoordinatedTile::updateBackBuffer()

#2 calls UpdateAtlas::beginPaint and #6 also calls UpdateAtlas::beginPaint before #2 is completed.

I think if this bug is resolved, above bug is also resolved.

[Noam Rosenthal]

(In reply to comment #4)
> Currently, two CoordinatedTiles use the same UpdateAtlas in the same time. It is a bug that can crash in some platforms.
> 
> For example,
> 1. m_nonCompositedContentLayer->flushCompositingStateForThisLayerOnly();
> 2. CoordinatedTile::updateBackBuffer()
> 3. FrameView::paintContents()
> 4. FrameView::flushCompositingStateForThisFrame()
> 5. CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
> 6. CoordinatedTile::updateBackBuffer()
> 
> #2 calls UpdateAtlas::beginPaint and #6 also calls UpdateAtlas::beginPaint before #2 is completed.
> 
> I think if this bug is resolved, above bug is also resolved.
You can probably fix this one by changing CoordinatedLayerTreeHost to call 
    bool didSync = m_webPage->corePage()->mainFrame()->view()->flushCompositingStateIncludingSubframes();
before the other flushes.

[Dongseong Hwang]

(In reply to comment #5)
> (In reply to comment #4)
> You can probably fix this one by changing CoordinatedLayerTreeHost to call 
>     bool didSync = m_webPage->corePage()->mainFrame()->view()->flushCompositingStateIncludingSubframes();
> before the other flushes.

Yes, your suggestion can be work well! We can go with your suggestion for a while.

But there are two weak points.
1. tree order is broken.
2. there is a subtle reason that it works well although tree order is broken.
Setting children needs tree order traversal, because setting parent also is performed during setting children and relies heavily on tree order traversal.
But your suggestion works well because "m_nonCompositedContentLayer->flushCompositingStateForThisLayerOnly();" does not flush only for this layer, but also flush all layers in this frame by tree order.
It eventually calls FrameView::paintContents and paintContents eventually calls flushCompositingStateForThisFrame. So m_nonCompositedContentLayer and rootLayer of RenderLayerCompositor are involved well with parent-child relation.

It works well because of the subtle reason, so we can go with your suggestion until this bug is resolved.

[Jae Hyun Park]

(In reply to comment #5)
> (In reply to comment #4)
> > Currently, two CoordinatedTiles use the same UpdateAtlas in the same time. It is a bug that can crash in some platforms.
> > 
> > For example,
> > 1. m_nonCompositedContentLayer->flushCompositingStateForThisLayerOnly();
> > 2. CoordinatedTile::updateBackBuffer()
> > 3. FrameView::paintContents()
> > 4. FrameView::flushCompositingStateForThisFrame()
> > 5. CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
> > 6. CoordinatedTile::updateBackBuffer()
> > 
> > #2 calls UpdateAtlas::beginPaint and #6 also calls UpdateAtlas::beginPaint before #2 is completed.
> > 
> > I think if this bug is resolved, above bug is also resolved.
> You can probably fix this one by changing CoordinatedLayerTreeHost to call 
>     bool didSync = m_webPage->corePage()->mainFrame()->view()->flushCompositingStateIncludingSubframes();
> before the other flushes.

The same problem still exists in iframe cases even if we call 
bool didSync = m_webPage->corePage()->mainFrame()->view()->flushCompositingStateIncludingSubframes(); 
before the other flushes.

[Dongseong Hwang]

(In reply to comment #7)
> The same problem still exists in iframe cases even if we call 
> bool didSync = m_webPage->corePage()->mainFrame()->view()->flushCompositingStateIncludingSubframes(); 
> before the other flushes.

Alright, I missed iframe.

[Noam Rosenthal]

Created attachment 198933 [details]
Patch

[Balazs Kelemen]

Comment on attachment 198933 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        When in force compositing mode, always assume that the main layer nees

needs :)

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:275
> +        if (m_rootCompositingLayer) {
> +            m_state.contentsSize = roundedIntSize(m_rootCompositingLayer->size());
> +            m_state.coveredRect = toCoordinatedGraphicsLayer(m_rootCompositingLayer)->accumulatedCoverRect();
> +        }

Is it ok that we don't have a root layer here (and send the message without these attributes)? Shouldn't it be an assert or an early return earlier?

[Noam Rosenthal]

Comment on attachment 198933 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:275
>> +        }
> 
> Is it ok that we don't have a root layer here (and send the message without these attributes)? Shouldn't it be an assert or an early return earlier?

Not necessarily. In the case of about:blank we'd have a m_rootLayer and no m_rootCompositingLayer.

[Jocelyn Turcotte]

Comment on attachment 198933 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:849
> +    TransformationMatrix combinedTransform = m_layerTransform.combined().inverse();
> +    if (!combinedTransform.isIdentityOrTranslation())
> +        return;

If scaling doesn't have any effect now (if we apply the scale above the root layer) it might in the future. So at least an assert to cover this case would be valuable.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:932
> +    IntRect rect = m_layerTransform.combined().mapRect(coverRect());

Hopefully this won't be too heavy, we really don't need that information for the whole layer tree.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:158
> +    if (m_rootCompositingLayer == graphicsLayer)
> +        return;

We didn't check it before, is it necessary?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:179
>      scheduleLayerFlush();

I think this isn't necessary and should be handled by WebCore through RenderLayerCompositor, isn't it?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:184
>      scheduleLayerFlush();

ditto?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:189
>      scheduleLayerFlush();

ditto?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:274
> +            m_state.coveredRect = toCoordinatedGraphicsLayer(m_rootCompositingLayer)->accumulatedCoverRect();

Maybe it should be the intersection of the covered rects and not the union. This rects carries the information "we know this rect won't display any checkerboard.".

[Noam Rosenthal]

Comment on attachment 198933 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:849
>> +        return;
> 
> If scaling doesn't have any effect now (if we apply the scale above the root layer) it might in the future. So at least an assert to cover this case would be valuable.

Not sure what you mean.
What this code does is to apply the trajectory only for layers with no scaling/rotation/3d.
I think if we apply the scale above the root layer I think it's OK if the trajectory vector stops working. 
I'm not sure what I would assert for here...

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:158
>> +        return;
> 
> We didn't check it before, is it necessary?

Probably not.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:179
>>      scheduleLayerFlush();
> 
> I think this isn't necessary and should be handled by WebCore through RenderLayerCompositor, isn't it?

OK.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:274
>> +            m_state.coveredRect = toCoordinatedGraphicsLayer(m_rootCompositingLayer)->accumulatedCoverRect();
> 
> Maybe it should be the intersection of the covered rects and not the union. This rects carries the information "we know this rect won't display any checkerboard.".

I don't think so... It's enough if one layer has contents at a given rect for it to not have to display checkerboards.
So if the main contents layer has a big cover rect, and has a child with a smaller cover rect, an intersection would give the small cover rect which is wrong in this case.

[Jocelyn Turcotte]

(In reply to comment #13)
> Not sure what you mean.
> What this code does is to apply the trajectory only for layers with no scaling/rotation/3d.
> I think if we apply the scale above the root layer I think it's OK if the trajectory vector stops working. 
> I'm not sure what I would assert for here...

What I mean is that I could see the main content layer, the one that we care having a proper trajectory set, could have a scaled transform if the scale is done withing WebCore at some point. This would break the trajectory logic because of this early return and would be hardly noticeable.

I think what you want here is to apply the trajectory to a layer that is neither rotated or skewed. A scaled layer can still have the trajectory easily applied since it's normalized. But there is currently no easy TransformationMatrix::isIdentityOrTranslationOrScale method to use.

The ASSERT would be ugly and would look like ASSERT(combinedTransform.m11() == 1 || !combinedTransform.isIdentityOrTranslation()), so I guess we can just fix it if it ever breaks.

> I don't think so... It's enough if one layer has contents at a given rect for it to not have to display checkerboards.
> So if the main contents layer has a big cover rect, and has a child with a smaller cover rect, an intersection would give the small cover rect which is wrong in this case.

This would also break the moment that a small layer is in the area since the cover rect can't be bigger than the contents rect, so you're right, it should be a union.

[Jocelyn Turcotte]

(In reply to comment #14)
> ...if the scale is done withing WebCore at some point...

To be precise, I mean the viewport or pixel ratio scaling applied on the page.

[Noam Rosenthal]

Created attachment 199898 [details]
Patch

[Sam Weinig]

WebKit2 changes look fine to me.

[Jocelyn Turcotte]

Comment on attachment 199898 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:839
> +CoordinatedGraphicsLayer* CoordinatedGraphicsLayer::firstDescendantsWithContents()

firstDescendant (without 's')
It's also not quite the first descendant since this is searching depth-first, but I guess we always expect the root layer itself to be returned anyway.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:842
> +    if (m_mainBackingStore)
> +        return this;

I think "if (shouldHaveBackingStore())" would be more reliable, it won't depend that updateContentBuffers() has been called first.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:258
> +            if (CoordinatedGraphicsLayer* contentsLayer = mainContentsLayer())
> +            m_state.coveredRect = contentsLayer->coverRect();

Indentation

[Noam Rosenthal]

Created attachment 199993 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 199993 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:152
>      static void setShouldSupportContentsTiling(bool);
> -
> +    CoordinatedGraphicsLayer* findFirstDescendantWithContentsRecursively();
>  private:

Nit: Missing empty line.

[Noam Rosenthal]

Created attachment 200001 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 200002 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 200002 [details]
Patch

Clearing flags on attachment: 200002

Committed r149292: <http://trac.webkit.org/changeset/149292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This appears to have caused more than 100 failures in the compositing tests on WebKit2 EFL:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r149293%20(11800)/results.html

[Chris Dumez]

(In reply to comment #25)
> This appears to have caused more than 100 failures in the compositing tests on WebKit2 EFL:
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r149293%20(11800)/results.html

All of them seem to have an addition line like this:
+ (drawsContent 1)

I guess simple rebaseline is needed?

[Noam Rosenthal]

Yes, I think this needs a rebaseline.

[Noam Rosenthal]

I think rebaselines are needed both for Qt and EFL.
I need some help with this since my MBP-Retina is not capable of producing the correct results.