Bug 90573 – [chromium] Decouple RenderPass drawing from CCRenderSurface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Decouple RenderPass cacheing decisions from CCRenderSurface

[Dana Jansens]

Created attachment 150995 [details]
Patch

[Dana Jansens]

Created attachment 151001 [details]
Patch

- move the contentsChanged flag into RenderPassDrawQuad instead of RenderPass.

[Dana Jansens]

Created attachment 151107 [details]
Patch

- rebase and remove unused drawingSurface variable

[Adrienne Walker]

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?

[Dana Jansens]

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.

[Adrienne Walker]

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

[Dana Jansens]

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

[Dana Jansens]

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.

[Adrienne Walker]

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

[Dana Jansens]

Created attachment 151320 [details]
Patch for landing

[Dana Jansens]

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.

[WebKit Review Bot]

Comment on attachment 151320 [details]
Patch for landing

Clearing flags on attachment: 151320

Committed r122160: <http://trac.webkit.org/changeset/122160>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.