Bug 23361 – Optimize images in layer when using accelerated compositing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

When a composited RenderLayer is created for a single <img>, then we can do some optimizations that we'll track in this bug.

[Dean Jackson]

Created attachment 28916 [details]
initial patch for coding review

Simon,

Here's a first pass. Pls check for style and approach. I'll add a ChangeLog and (manual) tests in a sec

[Simon Fraser (smfr)]

Comment on attachment 28916 [details]
initial patch for coding review

> diff --git a/WebCore/rendering/RenderImage.cpp b/WebCore/rendering/RenderImage.cpp

> +void RenderImage::notifyFinished(CachedResource* newImage)
> +{
> +    if (documentBeingDestroyed())
> +        return;
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((newImage == m_cachedImage) && layer()) {

It's slightly faster to test hasLayer(), not layer().

> +// A layer can use an inner content layer if the render layer's object is a replaced object and has no children.
> +// This allows the GraphicsLayer to display the RenderLayer contents directly; it's used for images and video.

Not video.

> +    // Video always uses an inner layer (for now).
> +    if (renderObject->isVideo())
> +        return true;

Remove this.

> +        // FIXME: video can go here once we hook up accelerated compositing for video
> +        /* if (renderer()->isVideo()) {
> +            drawsContent = hasBoxDecorations || renderer()->style()->hasBackground();
> +        } */

Remove this.

> +        if (renderer()->isImage()) {
> +            RenderImage* imageRenderer = (RenderImage*)renderer();
> +            if (imageRenderer && imageRenderer->cachedImage() && imageRenderer->cachedImage()->image())
> +                m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());
> +            
> +            drawsContent = false;
> +        }
> +        
> +        if (rendererHasBackground()) {
> +            m_graphicsLayer->setBackgroundColor(rendererBackgroundColor());
> +            hasImageBackgroundColor = true;
> +        }
> +    } else {
> +        m_graphicsLayer->clearContents();
> +        drawsContent = true;
> +    }
> +    
> +    if (isSimpleContainerCompositingLayer())
>          drawsContent = false;

I don't think we need to call isSimpleContainerCompositingLayer() if we've already determined that this
is a simple image layer.

> +void RenderLayerBacking::rendererContentChanged()
> +{
> +    bool hasBoxDecorations;
> +    if (useInnerContentLayer(hasBoxDecorations) && renderer()->isImage()) {
> +        RenderImage* imageRenderer = (RenderImage*)renderer();
> +        if (imageRenderer &&
> +            imageRenderer->cachedImage() &&
> +            imageRenderer->cachedImage()->image() &&
> +            imageRenderer->cachedImage()->isLoaded()) {
> +            // We have to wait until the image is fully loaded before setting it on the layer.

Not when <rdar://problem/5798760> is fixed. We should think about whether we want to set images
on layers while the image is being loaded.

> +            // This is a no-op if the layer doesn't have an inner layer for the image.
> +            m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());
> +            
> +            // Image animation is "lazy", in that it automatically stops unless someone is drawing
> +            // the image. So we have to kick the animation each time; this has the downside that the
> +            // image will keep animating, even if it's layer is not visible.

its, not it's.

> diff --git a/WebCore/rendering/RenderLayerBacking.h b/WebCore/rendering/RenderLayerBacking.h

> +    // Returns true if we can optimize the RenderLayer to draw the replaced content
> +    // directly into a compositing buffer
> +    bool useInnerContentLayer(bool& hasBoxDecorations) const;

I'm not a big fan of returning hasBoxDecorations as a side-effect of this method. Can it just
be an accessor on the style?

> diff --git a/WebCore/rendering/RenderObject.h b/WebCore/rendering/RenderObject.h
> index c52b1ae..11631b0 100644
> --- a/WebCore/rendering/RenderObject.h
> +++ b/WebCore/rendering/RenderObject.h
> @@ -269,6 +269,7 @@ public:
>      virtual bool isTextControl() const { return false; }
>      virtual bool isTextArea() const { return false; }
>      virtual bool isTextField() const { return false; }
> +    virtual bool isVideo() const { return false; }

I don't think we want this now.

> diff --git a/WebCore/rendering/RenderVideo.h b/WebCore/rendering/RenderVideo.h
> index 43c1e7b..b69c1f7 100644
> --- a/WebCore/rendering/RenderVideo.h
> +++ b/WebCore/rendering/RenderVideo.h
> @@ -41,6 +41,8 @@ public:
>  
>      virtual const char* renderName() const { return "RenderVideo"; }
>  
> +    virtual bool isVideo() const { return true; }

Ditto.

r- for now, but I think it's generally OK.

[Dean Jackson]

> I don't think we need to call isSimpleContainerCompositingLayer() if we've
> already determined that this
> is a simple image layer.

I think we have to - this code is called from a few different places and not always on a simple image.

e.g. updateAfterLayout()

[Dean Jackson]

> I'm not a big fan of returning hasBoxDecorations as a side-effect of this
> method. Can it just be an accessor on the style?

Yeah, this was ugly and not necessary.

I changed it to canUseInnerContentLayer(), taking no parameters.

[Dean Jackson]

> Not when <rdar://problem/5798760> is fixed. We should think about whether we
> want to set images on layers while the image is being loaded.

I think we should leave this in for now. That bug doesn't look like it will be fixed for Leopard anytime soon (or ever).

[Dean Jackson]

Created attachment 28927 [details]
updated patch (with changelog and manual test)

Addressed all review comments. Now with changelog and a manual test.

[Simon Fraser (smfr)]

Comment on attachment 28927 [details]
updated patch (with changelog and manual test)

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 3c2a777..6529e61 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,35 @@
> [phone]  Dean Jackson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

Normally the bug URL goes right here on a line by itself.

> diff --git a/WebCore/manual-tests/resources/simple_image.png b/WebCore/manual-tests/resources/simple_image.png

I think these should be pixel tests, though I'm not sure where they should live. They could go anywhere,
since they will look OK with accel-comp turned off, and it's fine to put a dummy 3d transform in the css.

> diff --git a/WebCore/rendering/RenderLayer.cpp b/WebCore/rendering/RenderLayer.cpp
> index 6ca9174..6861016 100644
> --- a/WebCore/rendering/RenderLayer.cpp
> +++ b/WebCore/rendering/RenderLayer.cpp
> @@ -224,6 +224,15 @@ RenderLayerCompositor* RenderLayer::compositor() const
>      ASSERT(renderer()->view());
>      return renderer()->view()->compositor();
>  }
> +    
> +void RenderLayer::rendererContentChanged()
> +{
> +    if (compositor()->updateLayerCompositingState(this, StyleDifferenceEqual))
> +        compositor()->setCompositingLayersNeedUpdate();     // FIXME: layers will only get updated after the next layout.

I'm not sure that updateLayerCompositingState() here can ever cause the layer to move
between compositing and non-compositing, so the setCompositingLayersNeedUpdate() may not
be required.

> diff --git a/WebCore/rendering/RenderLayerBacking.cpp b/WebCore/rendering/RenderLayerBacking.cpp

> +    // Reject anything that has a border, a border-radius, outline, margin or padding.
> +    bool hasBoxDecorations = hasBorderOutlineOrShadow(style) || style->hasMargin() || style->hasPadding();
> +    
> +    // Only optimize images that are unadorned.
> +    if (renderObject->isImage())
> +        return !hasBoxDecorations;

No point computing hasBoxDecorations if you are not an image.

> @@ -572,9 +602,36 @@ void RenderLayerBacking::detectDrawingOptimizations()
>  {
>      bool drawsContent = true;
>  
> -    if (isSimpleContainerCompositingLayer() || paintingGoesToWindow())
> +    // Check if a replaced layer can be further simplified.
> +    bool hasImageBackgroundColor = false;
> +    if (canUseInnerContentLayer()) {
> +        if (renderer()->isImage()) {
> +            RenderImage* imageRenderer = (RenderImage*)renderer();
> +            if (imageRenderer && imageRenderer->cachedImage() && imageRenderer->cachedImage()->image())
> +                m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());

Hmm, this might set the image before it's fully loaded. Maybe we should just call
rendererContentChanged() here, to avoid code duplication.

> +            drawsContent = false;
> +        }
> +        
> +        if (rendererHasBackground()) {
> +            m_graphicsLayer->setBackgroundColor(rendererBackgroundColor());
> +            hasImageBackgroundColor = true;
> +        }
> +    } else {
> +        m_graphicsLayer->clearContents();
> +        drawsContent = true;
> +    }
> +    
> +    if (isSimpleContainerCompositingLayer())
>          drawsContent = false;

I think this should be an else if, since you can't have canUseInnerContentLayer() and
isSimpleContainerCompositingLayer().

> -
> +    else if (!hasImageBackgroundColor) {
> +        // Clear the background color in case we are swapping away from a simple layer.
> +        m_graphicsLayer->clearBackgroundColor();
> +    }

The logic flow here is confusing. Maybe it should be:

if (canUseInnerContentLayer()) {

} else {
    if (isSimpleContainerCompositingLayer())
        ...
    else if (!hasImageBackgroundColor)
        ...
}

r=me with the test in a better place, and the suggestions above.

[Dean Jackson]

> +void RenderLayer::rendererContentChanged()
>> +{
>> +    if (compositor()->updateLayerCompositingState(this, StyleDifferenceEqual))
> > +        compositor()->setCompositingLayersNeedUpdate();     // FIXME: layers will only get updated after the next layout.

> I'm not sure that updateLayerCompositingState() here can ever cause the layer
> to move
> between compositing and non-compositing, so the
> setCompositingLayersNeedUpdate() may not
> be required.

I think you're right. removing that code.

[Dean Jackson]

Created attachment 28954 [details]
extracted patch for animated gif updates

Simon, please check this small change to RenderImage::imageChanged(). It fixes animated gifs in composited layers.

[Simon Fraser (smfr)]

Comment on attachment 28954 [details]
extracted patch for animated gif updates

> +#if USE(ACCELERATED_COMPOSITING)
> +        if (hasLayer()) {
> +            // tell any potential compositing layers
> +            // that the image needs updating.
> +            layer()->rendererContentChanged();

Sentence case for the comment, please (and you can probably put it on one line).

[Dean Jackson]

Committed r41996
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/Image.h
	A	WebCore/manual-tests/simple-image-compositing.html
	A	WebCore/manual-tests/resources/simple_image.png
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderImage.cpp
	M	WebCore/rendering/RenderLayerBacking.h
	M	WebCore/rendering/RenderImage.h
	M	WebCore/rendering/RenderLayerBacking.cpp
	M	WebCore/rendering/RenderLayer.h
	A	LayoutTests/platform/mac/compositing/direct-image-compositing-expected.txt
	A	LayoutTests/platform/mac/compositing/direct-image-compositing-expected.checksum
	A	LayoutTests/platform/mac/compositing/direct-image-compositing-expected.png
	M	LayoutTests/ChangeLog
	A	LayoutTests/compositing/direct-image-compositing.html
	A	LayoutTests/compositing/resources/simple_image.png