Bug 90573 - [chromium] Decouple RenderPass drawing from CCRenderSurface
Summary: [chromium] Decouple RenderPass drawing from CCRenderSurface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 90848
Blocks: 90702 90825
  Show dependency treegraph
 
Reported: 2012-07-04 12:42 PDT by Dana Jansens
Modified: 2012-07-09 22:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (70.89 KB, patch)
2012-07-05 16:03 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (76.20 KB, patch)
2012-07-05 16:30 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (76.23 KB, patch)
2012-07-06 11:53 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (96.32 KB, patch)
2012-07-09 14:09 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (96.32 KB, patch)
2012-07-09 14:26 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-07-04 12:42:15 PDT
[chromium] Decouple RenderPass cacheing decisions from CCRenderSurface
Comment 1 Dana Jansens 2012-07-05 16:03:13 PDT
Created attachment 150995 [details]
Patch
Comment 2 Dana Jansens 2012-07-05 16:30:26 PDT
Created attachment 151001 [details]
Patch

- move the contentsChanged flag into RenderPassDrawQuad instead of RenderPass.
Comment 3 Dana Jansens 2012-07-06 11:53:40 PDT
Created attachment 151107 [details]
Patch

- rebase and remove unused drawingSurface variable
Comment 4 Adrienne Walker 2012-07-09 11:50:49 PDT
Comment on attachment 151107 [details]
Patch

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

A few random comments, but I like the way this looks in general.

> Source/WebCore/ChangeLog:8
> +        We Remove the managed textures from CCRenderSurface and store them in a

s/We //

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298
> +        // FIXME: Make this id global across all compositors (include a compositor id?)

(Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?)

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:56
> +    static PassOwnPtr<CCRenderPass> create(CCRenderSurface*, unsigned id);

Should id be an int here to match the type in CCLayerImpl::id()?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:60
> +    bool m_contentsChangedSinceLastFrame;

Can you just make this the full damage rect? I suspect that eventually we'll want that information so that if a surface texture is cached we can do a partial update of it.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130
> -        OwnPtr<CCLayerImpl> root = CCLayerImpl::create(0);
> +        OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);

...how were these all passing before?
Comment 5 Dana Jansens 2012-07-09 13:21:44 PDT
Comment on attachment 151107 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298
>> +        // FIXME: Make this id global across all compositors (include a compositor id?)
> 
> (Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?)

We could add the compositor id but I'd rather do it here I think. That said, I found a compositor id in CCProxy, so I can just do that now and no fixme.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:56
>> +    static PassOwnPtr<CCRenderPass> create(CCRenderSurface*, unsigned id);
> 
> Should id be an int here to match the type in CCLayerImpl::id()?

ya, sure. matches compositor id too, which i will mashup with it.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:60
>> +    bool m_contentsChangedSinceLastFrame;
> 
> Can you just make this the full damage rect? I suspect that eventually we'll want that information so that if a surface texture is cached we can do a partial update of it.

Hm ya, can do that.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130
>> +        OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
> 
> ...how were these all passing before?

I think because none of them call drawLayers, they didn't hit that HashMap issue.
Comment 6 Adrienne Walker 2012-07-09 13:30:10 PDT
(In reply to comment #5)
> (From update of attachment 151107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151107&action=review
>
> >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130
> >> +        OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
> > 
> > ...how were these all passing before?
> 
> I think because none of them call drawLayers, they didn't hit that HashMap issue.

Ohhh.

In that case, maybe we should assert that ids are non-zero in CCLayerImpl.  Either that, or you need to use different hash traits on your maps that allow for zero ids.
Comment 7 Dana Jansens 2012-07-09 14:03:26 PDT
Comment on attachment 151107 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298
>>> +        // FIXME: Make this id global across all compositors (include a compositor id?)
>> 
>> (Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?)
> 
> We could add the compositor id but I'd rather do it here I think. That said, I found a compositor id in CCProxy, so I can just do that now and no fixme.

I'd like to punt on this for this patch. Its not needed for it strictly, and there's some work to be done. (SingleThreadProxy has compositorId == -1 always, and its not clear to me that's the correct id to use anyhow.)
Comment 8 Dana Jansens 2012-07-09 14:09:54 PDT
Created attachment 151314 [details]
Patch

Changed the assert in CCLayerImpl from >= 0 to > 0. A bunch of tests had to be updated but it's simple changes. Addressed other comments also.
Comment 9 Adrienne Walker 2012-07-09 14:25:02 PDT
Comment on attachment 151314 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:52
> +    ASSERT(id);

nit: id > 0
Comment 10 Dana Jansens 2012-07-09 14:26:58 PDT
Created attachment 151320 [details]
Patch for landing
Comment 11 Dana Jansens 2012-07-09 14:27:14 PDT
Comment on attachment 151314 [details]
Patch

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

thanks!

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:52
>> +    ASSERT(id);
> 
> nit: id > 0

oo right.
Comment 12 WebKit Review Bot 2012-07-09 15:31:42 PDT
Comment on attachment 151320 [details]
Patch for landing

Clearing flags on attachment: 151320

Committed r122160: <http://trac.webkit.org/changeset/122160>
Comment 13 WebKit Review Bot 2012-07-09 15:31:47 PDT
All reviewed patches have been landed.  Closing bug.