Bug 93174 – RenderLayerBacking must call GraphicsLayer::setContentsToImage when the image is changed.

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

[Dongseong Hwang]

Currently, layout indirectly call RenderLayerBacking::updateGraphicsLayerConfiguration()
and updateGraphicsLayerConfiguration() indirectly call GraphicsLayer::setContentsToImage().
It burdens all accelerated compositing implementations because each implementation
should check if the image is changed.
This patch calls GraphicsLayer::setContentsToImage() when the contents of the
image is actually changed.
Texmap checks the pointer to the image, not nativeImagePtr, so Texmap currently
draws only the first frame of GIF animations. This patch makes Texmap draw GIF
animations.

[Dongseong Hwang]

Created attachment 156509 [details]
Patch

[Dongseong Hwang]

Created attachment 156510 [details]
Layout Test : Animated GIF on a compositing layer

I created this test in order to test the animated gif on AC, especially Texmap.
It is easy for human to check animated gif rendering, but it is hard to make a complete layout test. I think there is no functionality to capture the specific frame of gif animations in the webkit test runner.
So, I submit this test not for review.

[Simon Fraser (smfr)]

Comment on attachment 156509 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Currently, layout indirectly call RenderLayerBacking::updateGraphicsLayerConfiguration()
> +        and updateGraphicsLayerConfiguration() indirectly call GraphicsLayer::setContentsToImage().
> +        It burdens all accelerated compositing implementations because each implementation
> +        should check if the image is changed.

This comment is confusing. updateGraphicsLayerConfiguration() isn't the place where animated GIFs get updated on compositing layers; that happens via RenderBoxModelObject::contentChanged() which ends up in RenderLayerBacking::contentChanged().

> Source/WebCore/rendering/RenderLayerBacking.cpp:1089
> +    NativeImagePtr newNativeImagePtr = image->nativeImageForCurrentFrame();
> +    if (newNativeImagePtr != m_compositedNativeImagePtr) {
> +        m_compositedNativeImagePtr = newNativeImagePtr;
> +
> +        // This is a no-op if the layer doesn't have an inner layer for the image.
> +        m_graphicsLayer->setContentsToImage(image);

It seems odd to inspect the NativeImagePtr here, but still send the Image down to GraphicsLayer.

This code also makes the assumption that pointer equality on a NativeImagePtr is a valid way to tell if the image is unchanged. That may not hold for all platforms. I think the platform code should continue to make the decision about whether the image has changed.

[Dongseong Hwang]

(In reply to comment #3)
> 
> This comment is confusing. updateGraphicsLayerConfiguration() isn't the place where animated GIFs get updated on compositing layers; that happens via RenderBoxModelObject::contentChanged() which ends up in RenderLayerBacking::contentChanged().

I'm sorry for this confusing. I think it is natural for RenderLayerBacking::contentChanged() to call finally GraphicsLayer::setContentsToImage(), but I don't understand why updateGraphicsLayerConfiguration() needs to call GraphicsLayer::setContentsToImage().
It seems that layout causes redundant texture uploading because layout calls updateGraphicsLayerConfiguration() and then GraphicsLayer::setContentsToImage().
So, I described updateGraphicsLayerConfiguration() indirectly calls GraphicsLayer::setContentsToImage() in the changelog. I regret I should explain it more.

> 
> It seems odd to inspect the NativeImagePtr here, but still send the Image down to GraphicsLayer.

I think so. I thought I would do that after listening your opinion because it is a big change.

> 
> This code also makes the assumption that pointer equality on a NativeImagePtr is a valid way to tell if the image is unchanged. That may not hold for all platforms. I think the platform code should continue to make the decision about whether the image has changed.

I made the mistake of hasty generalization after surveying GraphicsLayerCA, GraphicsLayerChromium and GraphicsLayerTextureMapper.
I think we should call GraphicsLayer::setContentsToImage() when the image is actually changed, especially RenderLayerBacking::contentChanged() is called.
Is it ok for only RenderLayerBacking::contentChanged() to call updateImageContents()? Or which functions does updateGraphicsLayerConfiguration() need to call : m_graphicsLayer->setContentsToImage(image), updateDrawsContent() and image->startAnimation().

[Dongseong Hwang]

I assumed WebCore could know whether the image has actually been changed.
However, I understand we need a big refactoring so that WebCore notifies the callback when the image has actually been changed.
So, I close this bug. I will file another bug to fix gif animation of Texmap.