Bug 63760 – [chromium] Compositor must reserve all textures before drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vangelis Kokkevis]

During layer update and paint the compositor currently only reserves textures for new layer tiles or for existing tiles that don't have valid textures.  Tiles that have valid textures don't get their textures reserved until draw time, at which point they may already have been recycled and have no valid contents.

[Vangelis Kokkevis]

Created attachment 99387 [details]
Patch

[James Robinson]

Comment on attachment 99387 [details]
Patch

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

I'm pretty sure this is gonna make us re-upload images on every frame, so r- for that.  Otherwise this seems good but I have questions about some other bits.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:162
> -        m_tiler->prepareToUpdate(paintRect, m_textureUpdater.get());
>      }
> +    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> +    if (layerRect.isEmpty())
> +        return;
> +
> +    m_tiler->prepareToUpdate(layerRect, m_textureUpdater.get());

by moving this out of the if check won't this reupload the entire image layer every frame?  that seems bad...

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:406
> +                IntRect scissorRect = layer->ccLayerImpl()->scissorRect();
>                  targetSurfaceRect.intersect(scissorRect);

nit: you don't really need a scissorRect local any more

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-974
> -    bool isLayerVisible = targetSurfaceRect.intersects(layerRect);
> -    if (!isLayerVisible) {
> -        layer->unreserveContentsTexture();
> -        return;

is this early out not useful any more, not valid, or something else?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1073
> +

nit: unnecessary whitespace change here

[Vangelis Kokkevis]

Created attachment 99395 [details]
Patch

[Vangelis Kokkevis]

(In reply to comment #2)
> (From update of attachment 99387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99387&action=review
> 
> I'm pretty sure this is gonna make us re-upload images on every frame, so r- for that.  Otherwise this seems good but I have questions about some other bits.
> 
> > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:162
> > -        m_tiler->prepareToUpdate(paintRect, m_textureUpdater.get());
> >      }
> > +    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> > +    if (layerRect.isEmpty())
> > +        return;
> > +
> > +    m_tiler->prepareToUpdate(layerRect, m_textureUpdater.get());
> 
> by moving this out of the if check won't this reupload the entire image layer every frame?  that seems bad...

Unless I'm reading the code wrong, updates only happen if tiles are dirty.  Calling prepareToUpdate() doesn't dirty the tiles, unless their textures have been lost. 

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:406
> > +                IntRect scissorRect = layer->ccLayerImpl()->scissorRect();
> >                  targetSurfaceRect.intersect(scissorRect);
> 
> nit: you don't really need a scissorRect local any more

Fixed

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-974
> > -    bool isLayerVisible = targetSurfaceRect.intersects(layerRect);
> > -    if (!isLayerVisible) {
> > -        layer->unreserveContentsTexture();
> > -        return;
> 
> is this early out not useful any more, not valid, or something else?
> 

It could be useful for RenderSurfaces but decided to stay consistent and not try to free up textures half way through drawing (although it's done for RS's)

> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1073
> > +
> 
> nit: unnecessary whitespace change here

Fixed.

[James Robinson]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 99387 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=99387&action=review
> > 
> > I'm pretty sure this is gonna make us re-upload images on every frame, so r- for that.  Otherwise this seems good but I have questions about some other bits.
> > 
> > > Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:162
> > > -        m_tiler->prepareToUpdate(paintRect, m_textureUpdater.get());
> > >      }
> > > +    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> > > +    if (layerRect.isEmpty())
> > > +        return;
> > > +
> > > +    m_tiler->prepareToUpdate(layerRect, m_textureUpdater.get());
> > 
> > by moving this out of the if check won't this reupload the entire image layer every frame?  that seems bad...
> 
> Unless I'm reading the code wrong, updates only happen if tiles are dirty.  Calling prepareToUpdate() doesn't dirty the tiles, unless their textures have been lost. 
> 

LayerTilerChromium::prepareToUpdate() invalidates the passed-in rect:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp#L238

I think I understand where Vangelis is going here and he's going to be out of town for a few weeks so I'll update this patch...

[James Robinson]

Comment on attachment 99395 [details]
Patch

Actually, I was mistaken - prepareToUpdate() calls invalidateTiles(), which doesn't actually invalidate any tiles.

[WebKit Review Bot]

Comment on attachment 99395 [details]
Patch

Clearing flags on attachment: 99395

Committed r90260: <http://trac.webkit.org/changeset/90260>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.