[chromium] Surface replica should have a separate quad in the render pass
Created attachment 135842 [details] Patch
Comment on attachment 135842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135842&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1212 > + if (m_defaultRenderSurface->hasReplica()) > + m_defaultRenderSurface->drawReplica(this); Can you just assert that there's no replica here? I think because of the way the offscreen texture is sized, the replica will never be visible. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:134 > + SkBitmap filterBitmap = applyFilters(layerRenderer); Isn't this an expensive operation? Is there a way to not do it twice?
Comment on attachment 135842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135842&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1212 >> + m_defaultRenderSurface->drawReplica(this); > > Can you just assert that there's no replica here? I think because of the way the offscreen texture is sized, the replica will never be visible. A replica does not need to be outside the bounds of the content, you can make a replica underneath its original via CSS. And if the original is not opaque then you would see it through the original. >> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:134 >> + SkBitmap filterBitmap = applyFilters(layerRenderer); > > Isn't this an expensive operation? Is there a way to not do it twice? Yes :( It uses a shared texture, I think because these two methods are called next to each other, we can just save the bitmap and reuse it. But the truth is that drawing any other filters will destroy this bitmap's texture, IIUC. (And for example background filters would do this.) Is this true Stephen? I remember having to work around this while working on the background filter stuff but its a bit fuzzy now.
Comment on attachment 135842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135842&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:134 >>> + SkBitmap filterBitmap = applyFilters(layerRenderer); >> >> Isn't this an expensive operation? Is there a way to not do it twice? > > Yes :( > > It uses a shared texture, I think because these two methods are called next to each other, we can just save the bitmap and reuse it. But the truth is that drawing any other filters will destroy this bitmap's texture, IIUC. (And for example background filters would do this.) > > Is this true Stephen? I remember having to work around this while working on the background filter stuff but its a bit fuzzy now. Not sure what you're asking. The bitmap returned by applyFilters() should be the sole owner of the texture, unless I've messed something up. But I don't think it's a good idea to stash the bitmap on the RenderSurface and rely on the caller to do the calls in the right order. Wouldn't it be easier to encapsulate the replica checking and drawing into drawContents()? You could have a flags argument which said which parts you want to draw (contents, replica, both). Then you could apply the filters once, get the bitmap and use it in both passes. Would that work?
Comment on attachment 135842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135842&action=review >>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:134 >>>> + SkBitmap filterBitmap = applyFilters(layerRenderer); >>> >>> Isn't this an expensive operation? Is there a way to not do it twice? >> >> Yes :( >> >> It uses a shared texture, I think because these two methods are called next to each other, we can just save the bitmap and reuse it. But the truth is that drawing any other filters will destroy this bitmap's texture, IIUC. (And for example background filters would do this.) >> >> Is this true Stephen? I remember having to work around this while working on the background filter stuff but its a bit fuzzy now. > > Not sure what you're asking. The bitmap returned by applyFilters() should be the sole owner of the texture, unless I've messed something up. But I don't think it's a good idea to stash the bitmap on the RenderSurface and rely on the caller to do the calls in the right order. > > Wouldn't it be easier to encapsulate the replica checking and drawing into drawContents()? You could have a flags argument which said which parts you want to draw (contents, replica, both). Then you could apply the filters once, get the bitmap and use it in both passes. Would that work? OK, after discussing with Dana a bit more, I'd rather not cache the SkBitmap used for filters between draws. I know it's inefficient, but filters-on-reflections seems like a bit of an edge case right now, and I'd like to investigate the texture re-use bug that Dana describes (something wrong in SkBitmap's ownership of the texture). Ideally, I'd like to stash the SkBitmap only when we have a proper invalidation mechnanism: then only invalidate the filtered bitmap when the subtree changes, or the filter animation changes. (And ideally, even break up the filters themselves so they can be individually invalidated).
Created attachment 135958 [details] Patch added fixme to cache the filtered texture
(In reply to comment #5) > OK, after discussing with Dana a bit more, I'd rather not cache the SkBitmap used for filters between draws. I know it's inefficient, but filters-on-reflections seems like a bit of an edge case right now, and I'd like to investigate the texture re-use bug that Dana describes (something wrong in SkBitmap's ownership of the texture). > > Ideally, I'd like to stash the SkBitmap only when we have a proper invalidation mechnanism: then only invalidate the filtered bitmap when the subtree changes, or the filter animation changes. (And ideally, even break up the filters themselves so they can be individually invalidated). Bug filed for the texture issue with repro: http://code.google.com/p/chromium/issues/detail?id=122349 When that is resolved I will happily cache the filtered texture. Until then I also think we should do it this way.
Comment on attachment 135958 [details] Patch Thanks for this refactoring. R=me.
Comment on attachment 135958 [details] Patch Clearing flags on attachment: 135958 Committed r113453: <http://trac.webkit.org/changeset/113453>
All reviewed patches have been landed. Closing bug.