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.
Created attachment 99387 [details] Patch
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
Created attachment 99395 [details] Patch
(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.
(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 on attachment 99395 [details] Patch Actually, I was mistaken - prepareToUpdate() calls invalidateTiles(), which doesn't actually invalidate any tiles.
Comment on attachment 99395 [details] Patch Clearing flags on attachment: 99395 Committed r90260: <http://trac.webkit.org/changeset/90260>
All reviewed patches have been landed. Closing bug.