Bug 63760 - [chromium] Compositor must reserve all textures before drawing
Summary: [chromium] Compositor must reserve all textures before drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-30 15:31 PDT by Vangelis Kokkevis
Modified: 2011-07-01 12:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.50 KB, patch)
2011-06-30 16:05 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (14.25 KB, patch)
2011-06-30 16:34 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2011-06-30 15:31:03 PDT
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.
Comment 1 Vangelis Kokkevis 2011-06-30 16:05:39 PDT
Created attachment 99387 [details]
Patch
Comment 2 James Robinson 2011-06-30 16:21:51 PDT
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
Comment 3 Vangelis Kokkevis 2011-06-30 16:34:28 PDT
Created attachment 99395 [details]
Patch
Comment 4 Vangelis Kokkevis 2011-06-30 16:35:34 PDT
(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.
Comment 5 James Robinson 2011-06-30 17:20:25 PDT
(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...
Comment 6 James Robinson 2011-07-01 11:35:29 PDT
Comment on attachment 99395 [details]
Patch

Actually, I was mistaken - prepareToUpdate() calls invalidateTiles(), which doesn't actually invalidate any tiles.
Comment 7 WebKit Review Bot 2011-07-01 12:25:14 PDT
Comment on attachment 99395 [details]
Patch

Clearing flags on attachment: 99395

Committed r90260: <http://trac.webkit.org/changeset/90260>
Comment 8 WebKit Review Bot 2011-07-01 12:25:20 PDT
All reviewed patches have been landed.  Closing bug.