Bug 83287 – [chromium] Surface replica should have a separate quad in the render pass

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Surface replica should have a separate quad in the render pass

[Dana Jansens]

Created attachment 135842 [details]
Patch

[Adrienne Walker]

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?

[Dana Jansens]

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.

[Stephen White]

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?

[Stephen White]

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).

[Dana Jansens]

Created attachment 135958 [details]
Patch

added fixme to cache the filtered texture

[Dana Jansens]

(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.

[Adrienne Walker]

Comment on attachment 135958 [details]
Patch

Thanks for this refactoring.  R=me.

[WebKit Review Bot]

Comment on attachment 135958 [details]
Patch

Clearing flags on attachment: 135958

Committed r113453: <http://trac.webkit.org/changeset/113453>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.