Bug 83287

Summary: [chromium] Surface replica should have a separate quad in the render pass
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, enne, jamesr, piman, reveman, senorblanco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 81227    
Attachments:
Description Flags
Patch
none
Patch none

Description Dana Jansens 2012-04-05 09:50:54 PDT
[chromium] Surface replica should have a separate quad in the render pass
Comment 1 Dana Jansens 2012-04-05 09:54:40 PDT
Created attachment 135842 [details]
Patch
Comment 2 Adrienne Walker 2012-04-05 11:17:27 PDT
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 3 Dana Jansens 2012-04-05 11:21:22 PDT
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 4 Stephen White 2012-04-05 13:57:39 PDT
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 5 Stephen White 2012-04-05 14:32:47 PDT
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).
Comment 6 Dana Jansens 2012-04-05 18:30:33 PDT
Created attachment 135958 [details]
Patch

added fixme to cache the filtered texture
Comment 7 Dana Jansens 2012-04-06 08:01:31 PDT
(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 8 Adrienne Walker 2012-04-06 09:53:57 PDT
Comment on attachment 135958 [details]
Patch

Thanks for this refactoring.  R=me.
Comment 9 WebKit Review Bot 2012-04-06 10:29:28 PDT
Comment on attachment 135958 [details]
Patch

Clearing flags on attachment: 135958

Committed r113453: <http://trac.webkit.org/changeset/113453>
Comment 10 WebKit Review Bot 2012-04-06 10:29:32 PDT
All reviewed patches have been landed.  Closing bug.