RESOLVED FIXED 90573
[chromium] Decouple RenderPass drawing from CCRenderSurface
https://bugs.webkit.org/show_bug.cgi?id=90573
Summary [chromium] Decouple RenderPass drawing from CCRenderSurface
Dana Jansens
Reported 2012-07-04 12:42:15 PDT
[chromium] Decouple RenderPass cacheing decisions from CCRenderSurface
Attachments
Patch (70.89 KB, patch)
2012-07-05 16:03 PDT, Dana Jansens
no flags
Patch (76.20 KB, patch)
2012-07-05 16:30 PDT, Dana Jansens
no flags
Patch (76.23 KB, patch)
2012-07-06 11:53 PDT, Dana Jansens
no flags
Patch (96.32 KB, patch)
2012-07-09 14:09 PDT, Dana Jansens
no flags
Patch for landing (96.32 KB, patch)
2012-07-09 14:26 PDT, Dana Jansens
no flags
Dana Jansens
Comment 1 2012-07-05 16:03:13 PDT
Dana Jansens
Comment 2 2012-07-05 16:30:26 PDT
Created attachment 151001 [details] Patch - move the contentsChanged flag into RenderPassDrawQuad instead of RenderPass.
Dana Jansens
Comment 3 2012-07-06 11:53:40 PDT
Created attachment 151107 [details] Patch - rebase and remove unused drawingSurface variable
Adrienne Walker
Comment 4 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?
Dana Jansens
Comment 5 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.
Adrienne Walker
Comment 6 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.
Dana Jansens
Comment 7 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.)
Dana Jansens
Comment 8 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.
Adrienne Walker
Comment 9 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
Dana Jansens
Comment 10 2012-07-09 14:26:58 PDT
Created attachment 151320 [details] Patch for landing
Dana Jansens
Comment 11 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.
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2012-07-09 15:31:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.