Bug 107099 – Coordinated Graphics: Remove m_pendingSyncBackingStores in LayerTreeRenderer.

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

[Dongseong Hwang]

LayerTreeRenderer difficultly uses m_pendingSyncBackingStores to set a backing
store to TextureMapperLayer, although LayerTreeRenderer easily sets a direct
composited image to TextureMapperLayer. It is because LayerTreeRenderer directly
set a backing store to TextureMapperLayer, instead of GraphicsLayerTextureMapper.

This patch makes LayerTreeRenderer set a backing store to GraphicsLayerTextureMapper
and then GraphicsLayerTextureMapper will set a backing store to TextureMapperLayer
like other members.

[Dongseong Hwang]

Created attachment 183130 [details]
Patch

[Dongseong Hwang]

Created attachment 183153 [details]
Patch

[Dongseong Hwang]

Created attachment 183330 [details]
Patch

[Dongseong Hwang]

Created attachment 183335 [details]
Patch

[Noam Rosenthal]

Please put it up for review when it applies :)

[Dongseong Hwang]

Created attachment 184659 [details]
Patch

[Dongseong Hwang]

Created attachment 184704 [details]
Patch

[Dongseong Hwang]

(In reply to comment #5)
> Please put it up for review when it applies :)

Now could you review? :)

[Noam Rosenthal]

Comment on attachment 184704 [details]
Patch

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

I'm fine with this, needs a WK2 owner to review; they might want more explanations...

> Source/WebKit2/ChangeLog:15
> +        LayerTreeRenderer difficultly uses m_pendingSyncBackingStores to set a backing
> +        store to TextureMapperLayer, although LayerTreeRenderer easily sets a direct
> +        composited image to TextureMapperLayer. It is because LayerTreeRenderer directly
> +        set a backing store to TextureMapperLayer, instead of GraphicsLayerTextureMapper.
> +
> +        This patch makes LayerTreeRenderer set a backing store to GraphicsLayerTextureMapper
> +        and then GraphicsLayerTextureMapper will set a backing store to TextureMapperLayer
> +        like other members.

Allow me to propose an alternative wording for the explanation :)
Instead of queuing the setting of backing stores in LayerTreeRenderer, and then setting them directly to TextureMapperLayer,
we allow GraphicsLayerTextureMapper's existing queuing mechanism to handle that.
Instead of a m_pendingSyncBackingStores queue, we have a m_backingStores queue which can be applied much more easily to the layer tree.

[Dongseong Hwang]

Created attachment 185117 [details]
Patch

[Dongseong Hwang]

(In reply to comment #9)
> (From update of attachment 184704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184704&action=review
> 
> I'm fine with this, needs a WK2 owner to review; they might want more explanations...

Thank you for your favor as well as review!

I'll ask WK2 owner. Now this bug depends on Bug 105787.

[Dongseong Hwang]

Created attachment 185169 [details]
Patch

[Benjamin Poulain]

Comment on attachment 185169 [details]
Patch

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

I am okay with this for WebKit2, with comments.

Noam, your call for the r+.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:159
> -    RefPtr<TextureMapperBackingStore> m_backingStore;
> +    TextureMapperBackingStore* m_backingStore;

Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer.

Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:414
> +    CoordinatedBackingStore* backingStore = 0;
> +    BackingStoreMap::iterator it = m_backingStores.find(graphicsLayer);
> +    if (it != m_backingStores.end())
>          backingStore = it->value.get();
>      return backingStore;

This is what HashMap::get() is for.

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 185169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review
> 
> I am okay with this for WebKit2, with comments.
> 
> Noam, your call for the r+.

Thank you for review!

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:159
> > -    RefPtr<TextureMapperBackingStore> m_backingStore;
> > +    TextureMapperBackingStore* m_backingStore;
> 
> Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer.
> Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted.

m_releasedCoordinatedBackingStores is already existing and this bug uses it.
I agree that m_releasedCoordinatedBackingStores is convoluted, so I will remove this twisted lifecycle management after Bug 103854.
It is because Bug 103854 will remove the time gap between setting platform layer and flushing layer scene info.

> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:414
> > +    CoordinatedBackingStore* backingStore = 0;
> > +    BackingStoreMap::iterator it = m_backingStores.find(graphicsLayer);
> > +    if (it != m_backingStores.end())
> >          backingStore = it->value.get();
> >      return backingStore;
> 
> This is what HashMap::get() is for.

Oh, I had put lots of above redundant code. someday I'll get rid of all. Thank you.

[Dongseong Hwang]

Created attachment 185396 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 185169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review
> 
> I am okay with this for WebKit2, with comments.
> 
> Noam, your call for the r+.

Noam, could you review this bug?

[Benjamin Poulain]

Comment on attachment 185396 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:420
> +    if (m_backingStores.get(graphicsLayer))

To test if a key exists in a HashMap/HashSet, the correct thing to do is HashMap::contains().

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:435
> +    if (!m_backingStores.get(graphicsLayer))
>          return;
> -    }
>  
> -    if (!layer->backingStore())
> -        return; // The layer has no backing store (and no pending addition).
> -
> -    ASSERT(!m_pendingSyncBackingStores.contains(layer));
> -    m_pendingSyncBackingStores.add(layer, 0);
> +    m_releasedCoordinatedBackingStores.append(m_backingStores.take(graphicsLayer));
> +    toGraphicsLayerTextureMapper(graphicsLayer)->setBackingStore(0);

This particular code should be using iterators. Otherwise you are doing get() twice.

[Dongseong Hwang]

Created attachment 185400 [details]
Patch

[Dongseong Hwang]

(In reply to comment #17)
> (From update of attachment 185396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185396&action=review
> To test if a key exists in a HashMap/HashSet, the correct thing to do is HashMap::contains().
> This particular code should be using iterators. Otherwise you are doing get() twice.

Oh, you are right! Done.

(In reply to comment #13)
> (From update of attachment 185169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review
> Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer.
> Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted.

After rethinking, I understand this patch does not need to use m_releasedCoordinatedBackingStores. TextureMapperLayer still have RefPtr<TextureMapperBackingStore>.
As I mentioned, m_releasedImageBackings is still alive and I'll kill in Bug 108296.

[Noam Rosenthal]

Comment on attachment 185400 [details]
Patch

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

Some nitpick in the changelog, otherwise good to go. Also seems that you have addressed all of Benjamin's concerns.

> Source/WebCore/ChangeLog:12
> +        because of dead code now.

because of dead code now -> because they are no longer used.

[Dongseong Hwang]

Created attachment 185407 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185407 [details]
Patch for landing

Clearing flags on attachment: 185407

Committed r141232: <http://trac.webkit.org/changeset/141232>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.