Bug 83530 – [chromium] Fold LayerChromium::updateCompositorResources into main update

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

[James Robinson]

[chromium] Fold LayerChromium::updateCompositorResources into main update

[James Robinson]

Created attachment 136352 [details]
Patch

[James Robinson]

Has a dependency on https://bugs.webkit.org/show_bug.cgi?id=83514.  Seems to work except for a few unit tests in TiledLayerChromium - layout tests all pass.

The basic idea here is we don't need an impl-thread updateCompositorResources pass since layers are queuing their compositor texture operations on CCTextureUpdater anyway.  This folds paintContentsIfDirty() / updateCompositorResources() into one function called update() that just does the right thing.

[James Robinson]

This fails TiledLayerChromiumTest.invalidateFromPrepare and TiledLayerChromiumTest.visibleContentOpaqueRegion but I'm not quite sure why yet.  Still debugging.

[Dana Jansens]

Comment on attachment 136352 [details]
Patch

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

very cool :)

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:431
> +    m_updateRect.scale(widthScale, heightScale);

should use scaleNonUniform() here I think.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-541
> -            tile->m_updateRect = sourceRect;

Probably a copy-paste thing, but why is this removed? This is used to tell that the tile was updated this frame for Tile::isDirtyForCurrentFrame().

What if the updateRect was non-empty in the loop above, but the sourceRect became empty here?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:578
>      // Abort if we have already prepared a paint or run out of memory.

is "prepared a paint" the right wording still here?

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:-89
> -    if (!m_drawingBuffer)

should the update() function early-out if prepareBackBuffer() failed?

[James Robinson]

Comment on attachment 136352 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:431
>> +    m_updateRect.scale(widthScale, heightScale);
> 
> should use scaleNonUniform() here I think.

That's for matrices, this is a FloatRect so all scales are 2d

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-541
>> -            tile->m_updateRect = sourceRect;
> 
> Probably a copy-paste thing, but why is this removed? This is used to tell that the tile was updated this frame for Tile::isDirtyForCurrentFrame().
> 
> What if the updateRect was non-empty in the loop above, but the sourceRect became empty here?

I removed it because I didn't see any callers further down, but it looks like isDirtyForCurrentFrame() is called in pushPropertiesTo so we probably do need to preserve this state until the next update() call.  Will fix

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:578
>      // Abort if we have already prepared a paint or run out of memory.

Nope, I think "painted" is better here. Fixed.

>> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:-89
>> -    if (!m_drawingBuffer)
> 
> should the update() function early-out if prepareBackBuffer() failed?

prepareBackBuffer() can't fail. we bail out of this function if we don't have an m_drawingBuffer since drawsContent() checks for that

[James Robinson]

Created attachment 136366 [details]
Patch

[Dana Jansens]

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

>>> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:-89
>>> -    if (!m_drawingBuffer)
>> 
>> should the update() function early-out if prepareBackBuffer() failed?
> 
> prepareBackBuffer() can't fail. we bail out of this function if we don't have an m_drawingBuffer since drawsContent() checks for that

forgive the cluelessness, but I don't see a check for m_drawingBuffer in drawsContent() or anywhere else. drawsContent() is just checking m_contextLost. how is it being checkded right now?

Secondly, now that I look at it. We check drawsContent() and return if false. But we actually set m_contextLost later in this function. Is that order correct?

[James Robinson]

(In reply to comment #7)
> View in context: https://bugs.webkit.org/attachment.cgi?id=136352&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:-89
> >>> -    if (!m_drawingBuffer)
> >> 
> >> should the update() function early-out if prepareBackBuffer() failed?
> > 
> > prepareBackBuffer() can't fail. we bail out of this function if we don't have an m_drawingBuffer since drawsContent() checks for that
> 
> forgive the cluelessness, but I don't see a check for m_drawingBuffer in drawsContent() or anywhere else. drawsContent() is just checking m_contextLost. how is it being checkded right now?

Whoops!  That is probably not right!  I'll add a null check back.

> 
> Secondly, now that I look at it. We check drawsContent() and return if false. But we actually set m_contextLost later in this function. Is that order correct?

Lost context is asynchronous - we only hear about it at some point after the context was actually lost.  The placement in ::update() is put after the flush to try to update the state for the next frame.  The idea here is we may try to put up a frame or two after we lost the context, but we want to eventually stop.

[James Robinson]

Created attachment 136372 [details]
Patch

[James Robinson]

Created attachment 136516 [details]
Patch

[Adrienne Walker]

Comment on attachment 136516 [details]
Patch

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

I'm a little ಠ_ಠ about having so many refactoring changes in the same patch (e.g. new update function, renaming member variables in UpdatableTexture, changing trace event names, not clearing surfaces).

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:98
> -    TRACE_EVENT("Canvas2DLayerChromium::paintContentsIfDirty", this, 0);
> +    TRACE_EVENT0("cc", "Canvas2DLayerChromium::paintContentsIfDirty");

s/paintContentsIfDirty/update/

Should all TRACE_EVENT be TRACE_EVENT0("cc", ...)? We seem to do this very inconsistently.  Are you just changing it because you're here?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-715
> -            if (m_requestedUpdateTilesRect.contains(IntPoint(i, j)))
> -                continue;

So, previously, if we had done a normal paint, then we wouldn't consider anything in that paint for idle painting? I guess that hasn't been true since we started considering animations for idle paint.  I'm fairly confident we can remove this check, but can you convince me?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:457
> +    LayerList updateList;
> +    updateList.append(rootLayer);

<3

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-594
> -        layer->clearRenderSurface();

This is a little sketchy, but I think it doesn't end up causing problems for CCLayerTreeHostCommon.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:496
> -        webKitPlatformSupport()->currentThread()->postDelayedTask(m_timeoutTask, 5000);
> +        webKitPlatformSupport()->currentThread()->postDelayedTask(m_timeoutTask, 5000*10000);

?

[James Robinson]

(In reply to comment #11)
> (From update of attachment 136516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136516&action=review
> 
> I'm a little ಠ_ಠ about having so many refactoring changes in the same patch (e.g. new update function, renaming member variables in UpdatableTexture, changing trace event names, not clearing surfaces).
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:98
> > -    TRACE_EVENT("Canvas2DLayerChromium::paintContentsIfDirty", this, 0);
> > +    TRACE_EVENT0("cc", "Canvas2DLayerChromium::paintContentsIfDirty");
> 
> s/paintContentsIfDirty/update/
> 
> Should all TRACE_EVENT be TRACE_EVENT0("cc", ...)? We seem to do this very inconsistently.  Are you just changing it because you're here?

They should all be TRACE_EVENT0("cc", ...) so they get the correct category - all of the TRACE_EVENT() traces are going into the "webkit" category so we can't filter them. I just changed the ones I happened to be near.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-715
> > -            if (m_requestedUpdateTilesRect.contains(IntPoint(i, j)))
> > -                continue;
> 
> So, previously, if we had done a normal paint, then we wouldn't consider anything in that paint for idle painting? I guess that hasn't been true since we started considering animations for idle paint.  I'm fairly confident we can remove this check, but can you convince me?
> 

The behavior here is the same - if we paint anything in the normal paint, then we don't idle paint anything on the layer.  The m_requestUpdateTilesRect checks are redundant with the m_didPaint check in this patch.  Old path:

m_requestUpdateTilesRect and m_paintRect set to the empty rect in prepareToUpdate() at the start of the frame
m_requestUpdateTilesRect and m_paintRect set to non-empty in prepareToUpdateTiles() if we actually do a paint (if we make it to textureUpdater()->prepareToUpdate())
if m_paintRect is non-empty, prepareToUpdateIdle() bails immediately

New path:

m_didPaint set to false at the start of a frame
if we call textureUpdater()->prepareToUpdate(), m_didPaint set to true
if m_didPaint is true, idleUpdateLayerRect bails immediately


> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:457
> > +    LayerList updateList;
> > +    updateList.append(rootLayer);
> 
> <3
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-594
> > -        layer->clearRenderSurface();
> 
> This is a little sketchy, but I think it doesn't end up causing problems for CCLayerTreeHostCommon.

Hmmm - I'm less confident with this part of the change now that I look in detail.  I was exciting about deleting the manual cleanup of the LayerList as a member, but didn't think too much about clearing the layer pointers to the RenderSurfaces. They'll get blown away on the next calculateDrawTransforms...(), but they'll sit around until then...

I'll add this clear back (to the local LayerList in CCLTH::updateLayers).

> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:496
> > -        webKitPlatformSupport()->currentThread()->postDelayedTask(m_timeoutTask, 5000);
> > +        webKitPlatformSupport()->currentThread()->postDelayedTask(m_timeoutTask, 5000*10000);
> 
> ?

Oh whoops, that was so I could gdb.  Will revert before landing.

[James Robinson]

Created attachment 136575 [details]
Patch

[Adrienne Walker]

Comment on attachment 136575 [details]
Patch

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

R=me.  Thanks for the explanation about the needsIdlePaint change.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:98
> -    TRACE_EVENT("Canvas2DLayerChromium::paintContentsIfDirty", this, 0);
> +    TRACE_EVENT0("cc", "Canvas2DLayerChromium::paintContentsIfDirty");

Second verse, same as the first.

[James Robinson]

Committed r113809: <http://trac.webkit.org/changeset/113809>

[Nat Duca]

Comment on attachment 136575 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:416
> +    m_pendingBeginFrameRequest->updater = m_currentTextureUpdaterOnImplThread.get();

Would it be better if we PassOwnPtr this and then the commit can PassOwnPtr it back? I worry about it being in both places at once.