Bug 93174

Created attachment 156509 [details] Patch

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.

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.

(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().

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.