Bug 102449 – Coordinated Graphics: Remove a texture if an direct composited image is off the viewport.

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

[Dongseong Hwang]

Currently, once uploading textures for composited images, Coordinated Graphics 
does not release the textures until all layers using images are destroyed. 

This patch removes a texture if we don't need to render an image. This mechanism
is similar how TiledBackingStore removes invisible tiles.

When all layers are invisible, we wait 3 seconds to remove the image, because we
want to prevent a transform animation from creating and destroying a texture
over and over again.

[Dongseong Hwang]

Created attachment 174565 [details]
Patch

[Dongseong Hwang]

I tested snowstack http://www.satine.org/research/webkit/snowleopard/snowstack.html

Previously, we create directly composited image textures endlessly.

Now, we create less than 60 image textures at the peak, and then we keep 9 textures after 3 seconds.

[Dongseong Hwang]

Created attachment 174598 [details]
Patch

[Dongseong Hwang]

rebased to upstream.

[Noam Rosenthal]

Comment on attachment 174598 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.h:72
> +    void makeImageBackingInvisible(CoordinatedImageBackingID);

Let's rename this to clearImageBackingContents

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:NaN
>  void CoordinatedImageBacking::update()

Is this called during an animation?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:91
> +    bool changedToVisible = setInvisibleIfNeeded();

This line doesn't make sense from the caller site. Some of the names need refactoring.

[Dongseong Hwang]

Created attachment 174902 [details]
Patch

[Dongseong Hwang]

(In reply to comment #5)
Thanks for review!

> (From update of attachment 174598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174598&action=review
> Let's rename this to clearImageBackingContents
Done.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:NaN
> >  void CoordinatedImageBacking::update()
> 
> Is this called during an animation?

Yes, it called during an animation. CoordinatedImageBacking::update() is called every LayerTreeCoordinator::flushPendingLayerChanges().
We cannot guarantee every frame of UI Process animations calls this function, because UI Process can progress between sending RenderNextFrame message and receiving next DidRenderFrame message.
However, I think CoordinatedImageBacking::update() is called often enough.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:91
> > +    bool changedToVisible = setInvisibleIfNeeded();
> 
> This line doesn't make sense from the caller site. Some of the names need refactoring.

I renamed changeVisibilityIfNeeded(bool& changedToVisible). How about it?

In addition, I add some code to reduce redundant texture updates.
For example, when javascript creates a directly composited img element while another directly composited img exists, the previous patch updates the texture redundantly. This patch handles this case also.

[Noam Rosenthal]

Comment on attachment 174902 [details]
Patch

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

Good progress. See comments.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:503
> +    backingStore->removeTile(1 /* id */);

How about adding a removeAllTiles / clear function ?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:107
> +        // currentFrameUpdatedAlready() can decode an image redundantly, so we check at the last.
> +        // When more than two layers refer the same image, markDirty() can be called even though we already updated.
> +        // For example, javascript can create a directly composited img element while another directly composited img exists.
> +        if (currentFrameUpdatedAlready()) {
> +            m_isDirty = false;
> +            return;
> +        }
> +    }

The comments are more confusing. 
It would be cleaner to replace this hole phrase with 
if (m_nativeImagePtr == m_image->nativeImageForCurrentFrame()) 
   m_isDirty = false
   return

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:143
> +void CoordinatedImageBacking::changeVisibilityIfNeeded(bool& changedToVisible)

updateVisibilityIfNeeded

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h:50
> +    class LayerClient {

How about class Host ?

[Dongseong Hwang]

Created attachment 175047 [details]
Patch

[Dongseong Hwang]

(In reply to comment #8)
> (From update of attachment 174902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174902&action=review
> 
> Good progress. See comments.

Thank you for review!

> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:503
> > +    backingStore->removeTile(1 /* id */);
> 
> How about adding a removeAllTiles / clear function ?

Add removeAllTiles because we can not really clear at this time.

> 
> The comments are more confusing. 
> It would be cleaner to replace this hole phrase with 
> if (m_nativeImagePtr == m_image->nativeImageForCurrentFrame()) 
>    m_isDirty = false
>    return
Done.


> updateVisibilityIfNeeded
Done.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h:50
> > +    class LayerClient {
> 
> How about class Host ?
Good suggestion! Done.

[Noam Rosenthal]

Comment on attachment 175047 [details]
Patch

I believe this is ready!

[Dongseong Hwang]

Comment on attachment 175047 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:189
> +    // CoordinatedImageBacking::LayerClient

Oops, here is nits. I'll update

[Dongseong Hwang]

Created attachment 175049 [details]
Patch

[Dongseong Hwang]

Created attachment 175052 [details]
Patch

[Dongseong Hwang]

Comment on attachment 175052 [details]
Patch

Rebase to upstream because this conflicts Bug 102664

[Noam Rosenthal]

(In reply to comment #15)
> (From update of attachment 175052 [details])
> Rebase to upstream because this conflicts Bug 102664

(When a patch is already review, leave the r flag blank, no need to set it as r+)

[Dongseong Hwang]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 175052 [details] [details])
> > Rebase to upstream because this conflicts Bug 102664
> 
> (When a patch is already review, leave the r flag blank, no need to set it as r+)

Yes. Thanks for explanation!

[WebKit Review Bot]

Comment on attachment 175052 [details]
Patch

Clearing flags on attachment: 175052

Committed r135207: <http://trac.webkit.org/changeset/135207>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.