[chromium] Move compositor quads to Platform/chromium/public
Created attachment 150868 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
This is the latest step in my quad pickling quest. See ChangeLog for detailed explanation. The only remaining problem is that webkit_unit_tests are not compiling because they don't specify WEBKIT_IMPLEMENTATION, which seems odd to me because they liberally use WebCore objects. Anyone have advice on the best way to address that?
Comment on attachment 150868 [details] Patch Attachment 150868 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13138834
> The only remaining problem is that webkit_unit_tests are not compiling because they don't specify WEBKIT_IMPLEMENTATION, which seems odd to me because they liberally use WebCore objects. Anyone have advice on the best way to address that? That should be fixed soon in https://bugs.webkit.org/show_bug.cgi?id=90094
Comment on attachment 150868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150868&action=review > Source/Platform/chromium/public/WebCompositorQuad.h:47 > +// enum is used to "safely" upcast to the derived class. nit:downcast?
Quads are a compositor class and in uber-comp will be serialized to some other process in Chromium. Classes in the Platform directory are abstractions that wrap Chromium code so that WebCore/WebKit can use it. Post-GTFO, serializing quads seems like a Chromium<->Chromium interaction. Maybe I'm missing something obvious, but why does the Platform directory need to be involved?
James suggested using Platform/chromium/public in Bug 88821. GTFO hasn't happened yet and I'd like to see a basically working ubercomp implementation in the next month, as we'll depend on it for various features. Nor should this really conflict with GTFO, as in the early stages of GTFO these files can still be used (perhaps just replacing IntRect with gfx::Rect if CC will end up using those), and in the later stages they can be moved with rename to a CC <-> Chromium communication layer analogous to the WebKit namespace. Ultimately the main point of this patch is to move quads to a serializable format in a publically accessible area and that should be needed regardless of GTFO.
http://www.urbandictionary.com/define.php?term=gtfo ?
(In reply to comment #9) > http://www.urbandictionary.com/define.php?term=gtfo ? GTFO is the codename for the project to factor the Chromium Compositor out of WebCore and into a separate library.
Comment on attachment 150868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150868&action=review Thanks for the explanation and the pointer to the other bug. If you need this in the next month, then this approach definitely makes sense. We can always move things back out of the Platform API later once they're only needed internally again. I'm happy to R+ once this patch passes the CQ, assuming somebody else approves the WebKit API changes. Are you planning to follow up shortly with converting the other quad types and removing the typedefs? > Source/Platform/ChangeLog:31 > + - I converted the Material casting mechanism to materialCast() methods > + living in the derived classes. That way, the WebCompositorQuad header > + doesn't need to know about all its derived classes. This is a nice cleanup. I know zlieber had been wanting to do this. > Source/Platform/chromium/public/WebCompositorQuad.h:30 > +#include "public/WebCompositorSharedQuadState.h" nit: You should remove the public/ here for includes to these sibling files. > Source/WebCore/platform/graphics/chromium/cc/CCCheckerboardDrawQuad.h:29 > +#include "public/WebCompositorQuad.h" nit: Can you change #include "public/WebFoo.h" to #include <public/WebFoo.h>?
Created attachment 151346 [details] Patch
(In reply to comment #11) > Are you planning to follow up shortly with converting the other quad types and removing the typedefs? OK, I updated this patch to convert the other quad types that were already POD (all except CCYUVVideoDrawQuad and CCRenderPassDrawQuad). I'll follow up on those two harder cases and on the mass renaming in future patches. > > Source/Platform/chromium/public/WebCompositorQuad.h:30 > > +#include "public/WebCompositorSharedQuadState.h" > > nit: You should remove the public/ here for includes to these sibling files. > > > Source/WebCore/platform/graphics/chromium/cc/CCCheckerboardDrawQuad.h:29 > > +#include "public/WebCompositorQuad.h" > > nit: Can you change #include "public/WebFoo.h" to #include <public/WebFoo.h>? All done.
(In reply to comment #13) > (In reply to comment #11) > > Are you planning to follow up shortly with converting the other quad types and removing the typedefs? > > OK, I updated this patch to convert the other quad types that were already POD (all except CCYUVVideoDrawQuad and CCRenderPassDrawQuad). I'll follow up on those two harder cases and on the mass renaming in future patches. What is non-POD in CCYUVVIdeoDrawQuad?
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > Are you planning to follow up shortly with converting the other quad types and removing the typedefs? > > > > OK, I updated this patch to convert the other quad types that were already POD (all except CCYUVVideoDrawQuad and CCRenderPassDrawQuad). I'll follow up on those two harder cases and on the mass renaming in future patches. > > What is non-POD in CCYUVVIdeoDrawQuad? struct CCVideoLayerImpl::FramePlane. That's technically POD, but that struct needs to be extracted into the WebKit namespace and the allocateData()/freeData() methods need to be moved elsewhere.
(In reply to comment #15) > struct CCVideoLayerImpl::FramePlane. That's technically POD, but that struct needs to be extracted into the WebKit namespace and the allocateData()/freeData() methods need to be moved elsewhere. I see, thanks :)
Created attachment 151390 [details] Patch
Resolved conflicts.
Comment on attachment 151390 [details] Patch Attachment 151390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13175284
Comment on attachment 151390 [details] Patch R=me for these compositor changes, assuming you get API approval before submitting.
Created attachment 151774 [details] Patch Fixed webkit_unit_tests
Comment on attachment 151774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151774&action=review R=me. aelias, can you get somebody to approve the WebKit API changes before submitting? > Source/WebCore/platform/chromium/support/WebCompositorSharedQuadState.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. nit: 2012. You do this in a few places.
That's a lot of API. I wish jamesr weren't on vacation so he could review this change. Everything looks plausible, but I'm not really an expert in this area. m_sharedQuadState seems like the sketchiest part... I think it's ok to land this patch with the understanding that jamesr might ask for changes when he gets back.
James has given some feedback on an earlier version at Bug 88821 so it shouldn't blindside him, anyway. I agree about the sketchiness of SharedQuadState. The pointer will also require a custom ipc_traits to pickle. The concept exists to save RAM essentially -- the two WebTransformationMatrix'es in it would add 256 bytes per quad, so we would end up with a 1MB quadlist if we had 5000 quads. Given that our typical number of quads is low (20 to 100 I guess?), I'm not sure this would be a disaster, but the last time this came up, there were objections about the RAM usage. Enne/Dana, any thoughts?
Created attachment 151788 [details] Patch Changed copyright years to 2012
Thinking about SharedQuadState further, I think we should keep it, since it's an order-of-magnitude size difference. We may find it introduces a scaling bottleneck if we do come up with an layer type (or website) involving thousands of quads, which is something that would otherwise be legitimate and performant. I think this patch should be ready to submit.
(In reply to comment #24) > James has given some feedback on an earlier version at Bug 88821 so it shouldn't blindside him, anyway. > > I agree about the sketchiness of SharedQuadState. The pointer will also require a custom ipc_traits to pickle. The concept exists to save RAM essentially -- the two WebTransformationMatrix'es in it would add 256 bytes per quad, so we would end up with a 1MB quadlist if we had 5000 quads. Given that our typical number of quads is low (20 to 100 I guess?), I'm not sure this would be a disaster, but the last time this came up, there were objections about the RAM usage. Enne/Dana, any thoughts? Although, there's nothing sketchy about SharedQuadState in *this* patch. It's just what it means for pickling in a future patch. I totally agree that CCSharedQuadState is possibly a premature optimization to avoid "fat" quads, but at the time that got added there was some general agreement that the memory savings were worth a small amount of extra complexity. I could be convinced to remove it, but I'd like to keep it for now too.
Created attachment 151801 [details] Patch Resolved conflicts with Bug 90843
Comment on attachment 151801 [details] Patch R=me again.
Comment on attachment 151801 [details] Patch Clearing flags on attachment: 151801 Committed r122383: <http://trac.webkit.org/changeset/122383>
All reviewed patches have been landed. Closing bug.
This is probably related to this CL, not 90843: > FYI. > I fixed a build break in http://trac.webkit.org/changeset/122396 > > I cannot judge whether we should initialize opaque with 'true' or 'false'. Please check it.
Thanks. Not sure why that compiled earlier (compiler quirk?). It doesn't matter whether or not it's true or false, since the default constructor is only for serialization and it will always be overwritten in practice. We can leave it as false.