Bug 112095 – [Texmap] Synchronise layers only if the layer has been changed.

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

[JungJik Lee]

We always append m_layerState to coordinator in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly. This causes renderNext-circular calls continuously.
So this patch checks whether the layer needs to be updated or not, if we need to update the layer, then we append layer state to coordinator.
This will reduce the needless update and IPC calls.

[JungJik Lee]

Created attachment 192633 [details]
Proposal patch

[Dongseong Hwang]

Comment on attachment 192633 [details]
Proposal patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750
> +    bool needToSyncThisLayer = hasChangedLayerState();

great! how about early return here or line 742?

[Noam Rosenthal]

Comment on attachment 192633 [details]
Proposal patch

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

Please correct nitpicks, otherwise patch looks ok.

> Source/WebCore/ChangeLog:14
> +        We always append m_layerState to coordinator in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly.
> +        This causes renderNext-circular calls continuously. 
> +        So this patch checks whether the layer needs to be updated or not,
> +        if we need to update the layer, then we append layer state to coordinator.
> +        This will reduce the needless update and IPC calls.
> +
> +        No new tests (OOPS!).

CoordinatedGraphicsLayer always sets the layer state, which results in an infinite commit/renderNextFrame loop between the web process in the UI process.
This patch breaks the infinite loop by avoiding commit if the layer state hasn't changed.

There is no current facility for automatically testing this, however it doesn't break existing tests.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750
> +    bool needToSyncThisLayer = hasChangedLayerState();

bool layerNeedsSync = needsSync();

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:775
> +bool CoordinatedGraphicsLayer::hasChangedLayerState()

rename to needsSync()

[Gwang Yoon Hwang]

Thanks for reporting.
It is the bug I made. :(

I think this patch can be better using CoordinatedGraphicsLayerState::changeMask

ex)
if (!m_layerState.changeMask)
    return;

in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() or CoordinatedLayerTreeHost::syncLayerState


BTW, I think we need clean up m_shouldSync...series in CoordinatedGraphicsLayerState. We can use LayerState::changeMask instead of that.

[Noam Rosenthal]

(In reply to comment #4)
> Thanks for reporting.
> It is the bug I made. :(
> 
> I think this patch can be better using CoordinatedGraphicsLayerState::changeMask
> 
> ex)
> if (!m_layerState.changeMask)
>     return;
This won't work since you also need to deal with tiles, which don't appear in the changeMask. I think JungJik's solution is ok for now.

[JungJik Lee]

Comment on attachment 192633 [details]
Proposal patch

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

>>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750
>>> +    bool needToSyncThisLayer = hasChangedLayerState();
>> 
>> great! how about early return here or line 742?
> 
> bool layerNeedsSync = needsSync();

what's mean? early return. the states will be changed inside syncFunctions.

[JungJik Lee]

Created attachment 192660 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #5)
> (In reply to comment #4)
> > Thanks for reporting.
> > It is the bug I made. :(
> > 
> > I think this patch can be better using CoordinatedGraphicsLayerState::changeMask
> > 
> > ex)
> > if (!m_layerState.changeMask)
> >     return;
> This won't work since you also need to deal with tiles, which don't appear in the changeMask. I think JungJik's solution is ok for now.

I agree. I'll make another bug to clean up various states in CoordinatedGraphicsLayer.

[Noam Rosenthal]

Comment on attachment 192660 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        (WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly): if hasChangedLayerState is true, append m_layerState.

hasChangedLayerState -> needsSync

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:179
> +    bool needsSync();

This function should be const.

[JungJik Lee]

Created attachment 192705 [details]
Patch

[JungJik Lee]

Created attachment 192714 [details]
Patch

[JungJik Lee]

(In reply to comment #11)
> Created an attachment (id=192714) [details]
> Patch

Add m_movingVisibleRect flag to needsSync.

[Noam Rosenthal]

Comment on attachment 192714 [details]
Patch

This is actually wrong. We might have a m_movingVisibleRect and still not need to sync. This patch would make it so that during a transform animation we'd be constantly syncing.
The right approach for this is close to what ryumiel was suggesting - add an isEmpty() function to GraphicsLayerState and not sync if that returns true.

[JungJik Lee]

(In reply to comment #13)
> (From update of attachment 192714 [details])
> This is actually wrong. We might have a m_movingVisibleRect and still not need to sync. This patch would make it so that during a transform animation we'd be constantly syncing.
> The right approach for this is close to what ryumiel was suggesting - add an isEmpty() function to GraphicsLayerState and not sync if that returns true.

I am not sure that ryumiel was suggesting.
but I think we still need to sync when m_movingVisibleRect is true. because 
when m_movingVisibleRect is true, we get current transform to send UI Process.
maybe the position of m_movingVisibleRect in needsSync is not correct.
it would be "if (layerNeedsSync || hasActiveTransformAnimation)".

[Noam Rosenthal]

Created attachment 193074 [details]
Patch

[JungJik Lee]

(In reply to comment #15)
> Created an attachment (id=193074) [details]
> Patch

it's great. could you test https://bugs.webkit.org/show_bug.cgi?id=112114 with the patch? And please, ignore the description of the test. it's wrong. thank you.

[Noam Rosenthal]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=193074) [details] [details]
> > Patch
> 
> it's great. could you test https://bugs.webkit.org/show_bug.cgi?id=112114 with the patch? And please, ignore the description of the test. it's wrong. thank you.

Seems ok.

[Allan Sandfeld Jensen]

Comment on attachment 193074 [details]
Patch

r=me

[Balazs Kelemen]

Comment on attachment 193074 [details]
Patch

Clearing flags on attachment: 193074

Committed r145806: <http://trac.webkit.org/changeset/145806>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.