Bug 88435

Summary: [chromium] Skip willDraw() and didDraw() on fully occluded layers
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, cc-bugs, dglazkov, enne, jamesr, piman, webkit.review.bot, wjmaclean, zlieber
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 88363    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch none

Description Dana Jansens 2012-06-06 11:10:25 PDT
[chromium] Skip willDraw() on occluded layers, and only call didDraw() when willDraw() was called
Comment 1 Dana Jansens 2012-06-06 11:24:13 PDT
Created attachment 146069 [details]
Patch
Comment 2 Dana Jansens 2012-06-06 11:29:07 PDT
FYI This will allow us to do the texture upload in CCVideoLayerImpl::willDraw().

I will do that in a way that the layer doesn't need to access the 3d context though, using the CCRenderer's interfaces instead.
Comment 3 WebKit Review Bot 2012-06-06 22:26:25 PDT
Comment on attachment 146069 [details]
Patch

Attachment 146069 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12914108

New failing tests:
css3/filters/effect-brightness-hw.html
compositing/reflections/load-video-in-reflection.html
compositing/reflections/reflection-on-composited.html
platform/chromium/compositing/child-layer-3d-sorting.html
css3/filters/effect-invert-hw.html
css3/filters/effect-contrast-hw.html
compositing/reflections/reflection-ordering.html
Comment 4 WebKit Review Bot 2012-06-06 22:26:29 PDT
Created attachment 146198 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Dana Jansens 2012-06-07 09:37:27 PDT
Created attachment 146310 [details]
Patch
Comment 6 Adrienne Walker 2012-06-08 13:43:24 PDT
Comment on attachment 146310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146310&action=review

R=me.  I'm curious how video frame uploads will work in an ubercomp world, but this looks good for now.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:531
> +    // Make the viewport large so that we can have large layers that get considered for occlusion (small layers do not).

Sad to have to bake these heuristic assumptions into tests.
Comment 7 Dana Jansens 2012-06-08 14:00:35 PDT
Comment on attachment 146310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146310&action=review

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:531
>> +    // Make the viewport large so that we can have large layers that get considered for occlusion (small layers do not).
> 
> Sad to have to bake these heuristic assumptions into tests.

Yeh, me too.. We'd have to expose something on CCLTHImpl though otherwise which seemed too much to me. Wdyt is the right thing to do?
Comment 8 Adrienne Walker 2012-06-08 14:15:34 PDT
(In reply to comment #7)
> (From update of attachment 146310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146310&action=review
> 
> >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:531
> >> +    // Make the viewport large so that we can have large layers that get considered for occlusion (small layers do not).
> > 
> > Sad to have to bake these heuristic assumptions into tests.
> 
> Yeh, me too.. We'd have to expose something on CCLTHImpl though otherwise which seemed too much to me. Wdyt is the right thing to do?

Talking out loud, maybe this sort of thing should be a setting, where you could have one test for the setting itself, but other tests could assume a less hairy default behavior.

I don't know that it's that important to fix; I was just remarking that it's unfortunate.
Comment 9 Dana Jansens 2012-06-08 14:24:59 PDT
Comment on attachment 146310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146310&action=review

>>>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:531
>>>> +    // Make the viewport large so that we can have large layers that get considered for occlusion (small layers do not).
>>> 
>>> Sad to have to bake these heuristic assumptions into tests.
>> 
>> Yeh, me too.. We'd have to expose something on CCLTHImpl though otherwise which seemed too much to me. Wdyt is the right thing to do?
> 
> Talking out loud, maybe this sort of thing should be a setting, where you could have one test for the setting itself, but other tests could assume a less hairy default behavior.
> 
> I don't know that it's that important to fix; I was just remarking that it's unfortunate.

Oh! A CCLayerTreeHostSetting sounds nice.. thanks for that. I'll make a bug.
Comment 10 WebKit Review Bot 2012-06-08 14:38:08 PDT
Comment on attachment 146310 [details]
Patch

Clearing flags on attachment: 146310

Committed r119867: <http://trac.webkit.org/changeset/119867>
Comment 11 WebKit Review Bot 2012-06-08 14:38:12 PDT
All reviewed patches have been landed.  Closing bug.