Bug 90582 – [chromium] Move compositor quads to Platform/chromium/public

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] Move compositor quads to Platform/chromium/public

[Alexandre Elias]

Created attachment 150868 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexandre Elias]

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?

[WebKit Review Bot]

Comment on attachment 150868 [details]
Patch

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

[Adam Barth]

> 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

[Dana Jansens]

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?

[Adrienne Walker]

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?

[Alexandre Elias]

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.

[Eric Seidel (no email)]

http://www.urbandictionary.com/define.php?term=gtfo ?

[Adam Barth]

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

[Adrienne Walker]

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>?

[Alexandre Elias]

Created attachment 151346 [details]
Patch

[Alexandre Elias]

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

[Dana Jansens]

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

[Alexandre Elias]

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

[Dana Jansens]

(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 :)

[Alexandre Elias]

Created attachment 151390 [details]
Patch

[Alexandre Elias]

Resolved conflicts.

[WebKit Review Bot]

Comment on attachment 151390 [details]
Patch

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

[Adrienne Walker]

Comment on attachment 151390 [details]
Patch

R=me for these compositor changes, assuming you get API approval before submitting.

[Alexandre Elias]

Created attachment 151774 [details]
Patch

Fixed webkit_unit_tests

[Adrienne Walker]

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.

[Adam Barth]

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.

[Alexandre Elias]

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?

[Alexandre Elias]

Created attachment 151788 [details]
Patch

Changed copyright years to 2012

[Alexandre Elias]

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.

[Adrienne Walker]

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

[Alexandre Elias]

Created attachment 151801 [details]
Patch

Resolved conflicts with Bug 90843

[Adrienne Walker]

Comment on attachment 151801 [details]
Patch

R=me again.

[WebKit Review Bot]

Comment on attachment 151801 [details]
Patch

Clearing flags on attachment: 151801

Committed r122383: <http://trac.webkit.org/changeset/122383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

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.

[Alexandre Elias]

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.