Issue 298023004: [PPAPI] Compositor API implementation

Peng

The compositor API implementation is ready for review. PTAL. Thanks.

6 years, 6 months ago (2014-05-30 14:36:04 UTC) #1

dmichael (off chromium)

raymes: If you have time, could you be primary on this? Thank you!

6 years, 6 months ago (2014-05-30 21:55:21 UTC) #2

raymes

Sure! On Sat, May 31, 2014 at 7:55 AM, <dmichael@chromium.org> wrote: > raymes: If you ...

6 years, 6 months ago (2014-06-02 01:15:03 UTC) #3

raymes

First pass. I haven't reviewed the example you added yet. Also, I can't vouch for ...

6 years, 6 months ago (2014-06-02 03:51:07 UTC) #4

raymes

Also we will need to have some tests. Take a look at test_graphics_2d.cc and test_graphics_3d.cc. ...

6 years, 6 months ago (2014-06-02 04:01:32 UTC) #5

Also we will need to have some tests. Take a look at test_graphics_2d.cc
and test_graphics_3d.cc. I assume we can have something similar?


On Mon, Jun 2, 2014 at 1:51 PM, <raymes@chromium.org> wrote:

> First pass. I haven't reviewed the example you added yet.
>
> Also, I can't vouch for any of the graphics related code :)
>
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc
> File content/renderer/pepper/pepper_compositor_host.cc (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode69
> content/renderer/pepper/pepper_compositor_host.cc:69: void
> PepperCompositorHost::ViewFlushedPaint() {}
> Why is this function implemented at all?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode103
> content/renderer/pepper/pepper_compositor_host.cc:103:
> ppapi::CompositorLayer* old_layer,
> Can this be const too?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode121
> content/renderer/pepper/pepper_compositor_host.cc:121:
> new_layer->clip_rect.size.height != 0) {
> Would it be better to consider any rect that has an empty area as
> indicating no clip? (new_layer->clip_rect.size.width *
> new_layer->clip_rect.size.height == 0)?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode160
> content/renderer/pepper/pepper_compositor_host.cc:160: do {
> Why not get rid of this do/while and just return from the function in
> the error cases?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode174
> content/renderer/pepper/pepper_compositor_host.cc:174:
> DCHECK_EQ(desc.format, PP_IMAGEDATAFORMAT_RGBA_PREMUL);
> Should we early exit in these cases too?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode215
> content/renderer/pepper/pepper_compositor_host.cc:215: bool changed) {
> Would "reset" be a better parameter name here?
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode220
> content/renderer/pepper/pepper_compositor_host.cc:220:
> DCHECK(!commit_layers_reply_context_.is_valid());
> I think it might be better to return an error in this case.
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode232
> content/renderer/pepper/pepper_compositor_host.cc:232: it !=
> layers.end(); ++it, ++i) {
> Rather than having two iterating variables, it might be better just to
> do for(size_t i = 0; i < layers.size(); ++i) {
>   ppapi::CompositorLayer* new_layer = &layers[i];
>   new_layer->...
> }
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode235
> content/renderer/pepper/pepper_compositor_host.cc:235:
> scoped_refptr<cc::Layer> layer =
> cc_layer (may be slightly more descriptive)
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.cc#newcode247
> content/renderer/pepper/pepper_compositor_host.cc:247: NOTREACHED();
> We should return an error value here.
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.h
> File content/renderer/pepper/pepper_compositor_host.h (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.h#newcode27
> content/renderer/pepper/pepper_compositor_host.h:27: // Assciates this
> device with the given plugin instance. You can pass NULL
> Associates
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_compositor_host.h#newcode73
> content/renderer/pepper/pepper_compositor_host.h:73:
> std::vector<ppapi::CompositorLayer> pp_layers_;
> Some comments for these members would be useful for me since I don't
> know much about the compositor layers :)
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> content/renderer/pepper/pepper_plugin_instance_impl.cc#newcode1173
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1173:
> texture_layer_->SetBounds(position.size());
> Why is this added?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc
> File ppapi/proxy/compositor_resource.cc (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode171
> ppapi/proxy/compositor_resource.cc:171: // Set the source size to
> image'size. It will be used to verify
> to the image's size
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode203
> ppapi/proxy/compositor_resource.cc:203: std::copy(matrix, matrix + 16,
> data_.transform);
> matrix + arraysize(matrix)?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode225
> ppapi/proxy/compositor_resource.cc:225: return PP_ERROR_BADARGUMENT;
> Maybe remove the default because we won't get compile-time errors for
> missing cases (and below).
>
> You can return an error outside of the switch.
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode267
> ppapi/proxy/compositor_resource.cc:267: type ==
> CompositorLayer::TYPE_IMAGE);
> If this is not the case, should we return false here too?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode288
> ppapi/proxy/compositor_resource.cc:288: break;;
> extra ;
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode390
> ppapi/proxy/compositor_resource.cc:390: layer_changed_ = true;
> Does this mean that CommitLayers has to be called before the layers will
> really be reset? This isn't clear in the API definition.
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode412
> ppapi/proxy/compositor_resource.cc:412: (*it)->data().texture.id,
> release_callback));
> I think the indentation is off
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.cc#newcode421
> ppapi/proxy/compositor_resource.cc:421: (*it)->data().image.id,
> release_callback));
> Why not just have a id for every ppapi::CompositorLayer? It would
> simplify this logic a lot. It wouldn't actually require any more space
> because of the union.
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.h
> File ppapi/proxy/compositor_resource.h (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/compositor_resource.h#newcode30
> ppapi/proxy/compositor_resource.h:30: class Layer : public
> base::SupportsWeakPtr<Layer> {
> How come you don't just put this stuff in CompositorLayerResource? I
> think it's more confusing to have an extra layer of indirection. The
> CompositorResource could just hold a reference to the
> CompositorLayerResource
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_messages.h
> File ppapi/proxy/ppapi_messages.h (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_messages.h#newcode12
> ppapi/proxy/ppapi_messages.h:12: #include "base/memory/linked_ptr.h"
> Is this needed?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_param_traits.cc
> File ppapi/proxy/ppapi_param_traits.cc (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_param_traits.cc#newcode734
> ppapi/proxy/ppapi_param_traits.cc:734: NOTREACHED();
> return false?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_param_traits.h
> File ppapi/proxy/ppapi_param_traits.h (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/proxy/ppapi_param_traits.h#newcode11
> ppapi/proxy/ppapi_param_traits.h:11: #include "base/memory/linked_ptr.h"
> is this needed?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/shared_impl/compositor_layer.h
> File ppapi/shared_impl/compositor_layer.h (right):
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/shared_impl/compositor_layer.h#newcode10
> ppapi/shared_impl/compositor_layer.h:10: #include <vector>
> is this needed?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/shared_impl/compositor_layer.h#newcode12
> ppapi/shared_impl/compositor_layer.h:12: #include
> "base/memory/scoped_ptr.h"
> Is this needed?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/shared_impl/compositor_layer.h#newcode16
> ppapi/shared_impl/compositor_layer.h:16: #include
> "ppapi/shared_impl/ppapi_shared_export.h"
> Is this needed?
>
> https://codereview.chromium.org/298023004/diff/120001/
> ppapi/shared_impl/compositor_layer.h#newcode20
> ppapi/shared_impl/compositor_layer.h:20: struct CompositorLayer {
> Could we call this CompositorLayerData. This would make it a bit clearer
> that it's an IPC data struct.
>
> https://codereview.chromium.org/298023004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peng

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc#newcode69 content/renderer/pepper/pepper_compositor_host.cc:69: void PepperCompositorHost::ViewFlushedPaint() {} On 2014/06/02 03:51:08, raymes wrote: > ...

6 years, 6 months ago (2014-06-02 19:01:54 UTC) #6

Peng

On 2014/06/02 04:01:32, raymes wrote: > Also we will need to have some tests. Take ...

6 years, 6 months ago (2014-06-02 19:09:07 UTC) #7

raymes

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc#newcode69 content/renderer/pepper/pepper_compositor_host.cc:69: void PepperCompositorHost::ViewFlushedPaint() {} Ah ok, that seems reasonable then. ...

6 years, 6 months ago (2014-06-03 00:43:22 UTC) #8

Peng

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper/pepper_compositor_host.cc#newcode174 content/renderer/pepper/pepper_compositor_host.cc:174: DCHECK_EQ(desc.format, PP_IMAGEDATAFORMAT_RGBA_PREMUL); On 2014/06/03 00:43:22, raymes wrote: > That's ...

6 years, 6 months ago (2014-06-03 18:32:23 UTC) #9

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:174: DCHECK_EQ(desc.format,
PP_IMAGEDATAFORMAT_RGBA_PREMUL);
On 2014/06/03 00:43:22, raymes wrote:
> That's a good point. But I think we usually just return an error in these
cases
> instead, even if it is validated in the plugin (you can also add a
NOTREACHED()
> so that we crash in debug). Even if the plugin is compromised (or it might
just
> be buggy) it doesn't necessarily mean that we want to crash the renderer too.
> 
> Also we should be careful with this because I don't know if enter.succeeded()
is
> guaranteed to be successful. We don't want a case where the plugin can
> intentionally crash the renderer.

Done.

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:220:
DCHECK(!commit_layers_reply_context_.is_valid());
On 2014/06/03 00:43:22, raymes wrote:
> See the note above.

Done.

https://codereview.chromium.org/298023004/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:247: NOTREACHED();
On 2014/06/03 00:43:22, raymes wrote:
> On 2014/06/02 19:01:55, Peng wrote:
> > On 2014/06/02 03:51:08, raymes wrote:
> > > We should return an error value here.
> > 
> > Same. CompositorLayer() already verifies all layer before sending this IPC.
> > So it should not happen, unless the plugin or renderer process is
compromised.
> > So maybe crash is better?
> > 
> 
> See the note above.

Done.

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:390: layer_changed_ = true;
On 2014/06/03 00:43:22, raymes wrote:
> But the layers won't really be destroyed until CommitLayers is called?

I understand your point. This API only reset layers in plugin process. To commit
the result, plugin need call CommitLayers(). I will update the API definition.

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:412: (*it)->data().texture.id,
release_callback));
On 2014/06/03 00:43:22, raymes wrote:
> I think there should just be 4 space indent from the above line (looks like
> there are 6)

Done.

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:421: (*it)->data().image.id,
release_callback));
On 2014/06/03 00:43:22, raymes wrote:
> But we could just make it a layer ID instead right? It looks like it's just a
> counter value.
> 
> I'm proposing moving the id field into the top level of CompositorLayerData
and
> you could even initialize it on construction. I don't see any downside to
doing
> this? It would simplify this logic here.

Moved it to toplevel structure, and renamed it to resource_id.

Done

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/298023004/diff/120001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:30: class Layer : public
base::SupportsWeakPtr<Layer> {
On 2014/06/03 00:43:22, raymes wrote:
> Could you explain a bit more about the difficulty? I was imagining that
> CompositorResource would just hold a
> vector<scoped_refptr<CompositorLayerResource> > rather than a
> ScopedVector<Layer>. If you really needed a lot of direct access to members
from
> CompositorLayer you could make it a friend class (though it doesn't seem like
> that is a big issue).
> 
> From my perspective this would be a simpler design.

The problem for me is how PP_Resource ref is maintained in pepper. So using
LayerImpl and WeakPtr, I don't need care about PP_Resource ref.

I went through the code of ResourceTracker, and understand the PP_Resource ref,
and Changed the implementation to remove LayerImpl.

Done

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:225: break;
On 2014/06/03 00:43:22, raymes wrote:
> nit: get rid of the default: case altogether (and below)

I tried without default:. The compiler will give a warning (something like: the
switch does not handle all values in the enum).

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:739: return false;;
On 2014/06/03 00:43:22, raymes wrote:
> nit: extra ;

Done.

raymes

Looking much better, thanks! https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper/pepper_compositor_host.cc#newcode171 content/renderer/pepper/pepper_compositor_host.cc:171: <<"Can not access the image's ...

6 years, 6 months ago (2014-06-04 01:11:39 UTC) #10

piman

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc#newcode39 content/renderer/pepper/pepper_compositor_host.cc:39: layer_ = cc::SolidColorLayer::Create(); You're only using this for grouping, ...

6 years, 6 months ago (2014-06-04 13:19:44 UTC) #11

Peng

https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper/pepper_compositor_host.cc#newcode171 content/renderer/pepper/pepper_compositor_host.cc:171: <<"Can not access the image's shared memory!"; On 2014/06/04 ...

6 years, 6 months ago (2014-06-05 00:50:37 UTC) #12

https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/140001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:171: <<"Can not access the
image's shared memory!";
On 2014/06/04 01:11:38, raymes wrote:
> You might as well keep all of these checks in now, maybe just convert them to
> DCHECKs (since they are all checked before calling the function).

Done.

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/298023004/diff/140001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:225: break;
On 2014/06/04 01:11:38, raymes wrote:
> This is because it is missing PP_BLENDMODE_LAST. It's better to add it, and
then
> if someone adds a new blend mode, they will get a compiler warning here. The
> same with other switches.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:39: layer_ =
cc::SolidColorLayer::Create();
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> You're only using this for grouping, maybe just make it a base cc::Layer

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:40:
layer_->SetMasksToBounds(true);
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> Note: this can be expensive if the layer is transformed.
> Possibly better could be to explicitly clip the child layers (by modifying
their
> bounds), but you'll need to pipe the plugin size for that.
> 
> Maybe leave as a TODO.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:55: return true;  // Rebinding
the same device, nothing to do.
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> This can only happen for bound_instance_ == new_instance == NULL. Maybe not
> worth the test since you're just setting a field.

Seems it is not necessary to check this case.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:57: return false;  // Can't
change a bound device.
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> You can replace this by a DCHECK, since we call BindToInstance(NULL) before
> rebinding.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:106: CHECK(!old_layer ||
new_layer->type == old_layer->type);
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> no CHECK. Use DCHECK.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:110:
layer->SetBounds(PP_ToGfxSize((new_layer)->size));
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> no need for extra () around new_layer

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:124:
layer->SetMaskLayer(cc::Layer::Create());
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> Masks are *very* expensive. It's most likely not what you want (also, it won't
> work with a base cc::Layer).
> Maybe SetClipParent?
> But really, for a single clip rect, it's even cheaper to simply adjust the
layer
> bounds.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:170: #endif
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> rather than duping the handle, can we just keep a reference to the
PPB_ImageData
> instead?

I found SharedMemory will closed the handle in dtor. So we have to dup the
handle. See [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared...

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:175:
CHECK(shared_memory->Map(desc.stride * desc.size.height));
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> no CHECK.

If the Map() failed. cc will crash anyway. So what should we do if Map() failed?

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:200: NOTREACHED();
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> Not NOTREACHED(), it can be reached by untrusted plugins.

Sorry. I don't understand. In plugin process, we will return PP_ERROR_INPROGRESS
directly, if there is a pending commitlayers() call.
So here should not be reached, unless there is a bug or the plugin is
compromised, I think. So I put a NOTREACHED() here.
I thought NOREACHED() only crash renderer in debug build. So probably it is OK?

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:206: // Verfiy layers first,
if an error happens, we will return the error to
On 2014/06/04 01:11:38, raymes wrote:
> nit: Verify the layers first

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:207: // plugin and keep
current layers set by previous CommitLayers() unchanged.
On 2014/06/04 01:11:38, raymes wrote:
> nit: by the previous CommitLayers()

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:215: NOTREACHED();
On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> Same here, not NOTREACHED

Same. It should only crash in debug build. And just print an error message in
release build.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:249: return
PP_ERROR_BADARGUMENT;
On 2014/06/04 01:11:38, raymes wrote:
> nit: Could we pull the body of this loop out into a helper function that just
> returns a pp_error code?

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:266: CHECK((old_layer &&
cc_layer) || (!old_layer && !cc_layer));
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> no CHECK. use DCHECK.
> 
> can also be written as DCHECK_EQ(!!old_layer, !!new_layer);

After adding struct LayerData { cc_layer, pp_layer}. It is not an issue anymore.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:276:
cc_layer->SetAnchorPoint(gfx::PointF());
On 2014/06/04 01:11:38, raymes wrote:
> Would it make sense to shift these into UpdateLayer (even if they get called
> multiple times) just to push all of that logic into the one place? I'll leave
it
> up to you 

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.h (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.h:86:
std::vector<ppapi::CompositorLayerData> pp_layers_;
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> Can we use an array of structures? The intent is that both cc_layers_ and
> pp_layers_ have matching elements, it's more readable to have a
> 
> struct LayerData {
>   scoped_refptr<cc::Layer> cc_layer;
>   ppapi::CompositorLayerData pp_layer;
> };
> 
> and a std::vector of those.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:526:
bound_graphics_3d_(NULL),
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> nit: it's a scoped_refptr, no need to initialize explicitly.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:738:
static_cast<scoped_refptr<cc::Layer> >(compositor_layer_);
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> no need for static_cast in either case.
> cc::Layer* layer = texture_layer_ ? texture_layer_ : compositor_layer_;
> 
> Maybe extract into a helper function to that you can reuse in ScrollRect.

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:753:
static_cast<scoped_refptr<cc::Layer> >(compositor_layer_);
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> ditto

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1990: (want_3d_layer ==
layer_is_hardware_) &&
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> you want another early out for want_compositor_layer == !!compositor_later_

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2249: compositor =
static_cast <PepperCompositorHost*>(host);
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> nit: no space after static_cast

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:705: PepperCompositorHost*
bound_compositor_;
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> should this be a scoped_refptr too?

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:68: const struct PP_Size* size) {
On 2014/06/04 01:11:38, raymes wrote:
> nit: the indentation style you had before is more common, if it fits

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:187: return PP_ERROR_BADARGUMENT;
On 2014/06/04 01:11:38, raymes wrote:
> nit: I'm tempted to ask to group the common checks (from here and SetTexture)
> out into a helper function, but I'll leave it up to you.

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:254: default:
On 2014/06/04 01:11:38, raymes wrote:
> as mentioned, let's get rid of all the default cases :)

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:273: return PP_ERROR_BADARGUMENT;
On 2014/06/04 01:11:38, raymes wrote:
> nit: add {}

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:305: type ==
CompositorLayerData::TYPE_IMAGE);
On 2014/06/04 01:11:38, raymes wrote:
> We might as well just test this with an if statement and return an error if
they
> are invalid.

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:338: release_callback->Run(PP_OK);
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> if is_lost is set, you should return failure here.

When is_list is false? Does it means the Graphics3D context is lost, the texture
can not be used anymore?

Done

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.cc (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:67: NOTREACHED();
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> No default case, just omit it to get compiler warnings if a value is added to
> the enum and this code is not updated.

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:91: (*it)->Invalidate();
On 2014/06/04 01:11:38, raymes wrote:
> One problem here is that if IsInProgress() returns true, we won't properly
> invalidate the layers on destruction. This could result in a UAF in the layer.
> We should always invalidate the layers on destruction.

Good catch. Done

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.cc:118: (*it)->data().resource_id,
release_callback));
On 2014/06/04 01:11:38, raymes wrote:
> Note that the ReleaseCallback holds a reference to the
CompositorLayerResource.
> So how about we just hold a reference to the CompositorLayerResource directly?
> This way we wouldn't have to expose ReleaseCallback at all.
> CompositorLayerResource could release the resource in its destructor.
> 
> So release_callback_map_ here would become live_layers_map_ here.

Actually, the ReleaseCallback does not need hold a ref to the CLR. Because for a
single Layer, it could generate several ReleaseCallbacks. For example:(see below
steps)

1) layer.SetImage(image1);
2) compositor.CommitLayers();
3) layer.SetImage(image2);
4) compositor.CommitLayers();

2) and 3) will generate two different release_callbacks for image1 and image2.
So we can not only keep a layer in map.

Done. Do not hold a ref of CLR in release_callback.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_resource.h (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_resource.h:73: mutable int32_t last_resource_id_;
On 2014/06/04 01:11:39, raymes wrote:
> Why is this mutable?

In CompositorLayerResource(CLR), we use a const CompositorResource pointer
(Because CLR does not own CompositorResource, using a const pointer makes it
more clear). And the CLR will call GenerateResourceId(). So GenerateResourceId()
should be a const method. To achieve that, we need mark last_resource_id_
mutable. WDYT?

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
File ppapi/shared_impl/compositor_layer_data.h (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:17: typedef int8_t
Mailbox[GL_MAILBOX_SIZE_CHROMIUM];
On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> just use gpu::Mailbox. It also has existing ParamTraits.
It has a constructor. The compiler does not allow put it in a union. :(

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:51: // Becasue texture is the biggest
struct in the union, so
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> typo becasue -> because

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:52: // we only need set it to 0.
On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> Technically, this is undefined behavior. Also, fragile.
> 
> If you give a name to the union, you can memset it.

If I give it a name, I think I can not use union member directly anymore. For
example:

data.color.red -> data.union_name.color.red.

Do you have a better idea to achieve it?

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:64: int32_t resource_id;
On 2014/06/04 01:11:39, raymes wrote:
> On the same note as my previous comments, I think this could just be called
> layer_id. The host could notify the plugin when it is finished with a layer_id
> (instead of a resource_id) and then the CompositorResource could just drop its
> references to it.

Actually it is not a layer id. One single layer can generate many resource id. 
Each SetImage or SetTexture call will generate a resource id to present the
resource (texture or image).

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:73: } color;
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> If you define those as actual structs, you can use the IPC_STRUCT_TRAITS_*
> macros which are a lot more maintainable that the individual ParamTraits.

Unless using a struct includes all fields in struct color, texture, and image, I
think we can not avoid ParamTraits. But it will add some overhead for IPC
message.

Thought?

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:79: PP_FloatRect source_rect;
On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> if you use gfx::RectF, ParamTraits already exist.

Seems pepper does not use any gfx structs in IPC.

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:87: PP_FloatRect source_rect;
On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> same here

Seems pepper does not use any gfx structs in IPC.

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...

raymes

https://codereview.chromium.org/298023004/diff/230001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/230001/content/renderer/pepper/pepper_compositor_host.cc#newcode182 content/renderer/pepper/pepper_compositor_host.cc:182: } What I meant was to pull out just ...

6 years, 6 months ago (2014-06-06 00:47:00 UTC) #13

raymes

Oops what I mean is local functions in an anonymous namespace :) On Fri, Jun ...

6 years, 6 months ago (2014-06-06 01:25:34 UTC) #14

danakj

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc#newcode200 content/renderer/pepper/pepper_compositor_host.cc:200: NOTREACHED(); On 2014/06/05 00:50:38, Peng wrote: > On 2014/06/04 ...

6 years, 6 months ago (2014-06-06 14:54:32 UTC) #15

Peng

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc#newcode200 content/renderer/pepper/pepper_compositor_host.cc:200: NOTREACHED(); On 2014/06/06 14:54:31, danakj wrote: > On 2014/06/05 ...

6 years, 6 months ago (2014-06-06 19:29:44 UTC) #16

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:200: NOTREACHED();
On 2014/06/06 14:54:31, danakj wrote:
> On 2014/06/05 00:50:38, Peng wrote:
> > On 2014/06/04 13:19:42, piman OOO back 2014-6-09 wrote:
> > > Not NOTREACHED(), it can be reached by untrusted plugins.
> > 
> > Sorry. I don't understand. In plugin process, we will return
> PP_ERROR_INPROGRESS
> > directly, if there is a pending commitlayers() call.
> > So here should not be reached, unless there is a bug or the plugin is
> > compromised, I think. So I put a NOTREACHED() here.
> > I thought NOREACHED() only crash renderer in debug build. So probably it is
> OK?
> 
> NOTREACHED() means "it's impossible to get here". But if a compromised plugin
> can make us get there, then there should not be a NOTREACHED(), we should
> instead handle the case correctly anyway (and probably test that).

Remove NOTREACHED(). Done

https://codereview.chromium.org/298023004/diff/230001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/230001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:182: }
On 2014/06/06 00:47:00, raymes wrote:
> What I meant was to pull out just the body of the loop into the function. So
you
> would have 
> for (size_t i = 0; i < layers.size(); ++i) {
>  if (VerifyCommittedLayers(layers[i]) != PP_OK)
>    return ...
> 
> I think this will make it much easier to read than at present where there are
a
> bunch of continues and so on.

Done.

https://codereview.chromium.org/298023004/diff/230001/ppapi/proxy/compositor_...
File ppapi/proxy/compositor_layer_resource.cc (right):

https://codereview.chromium.org/298023004/diff/230001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:319: void
CompositorLayerResource::OnTextureReleased(
On 2014/06/06 00:47:00, raymes wrote:
> Rather than have these as static functions, you can just make them anonymous
> functions

Done.

https://codereview.chromium.org/298023004/diff/230001/ppapi/proxy/compositor_...
ppapi/proxy/compositor_layer_resource.cc:346: void
CompositorLayerResource::OnImageReleased(
On 2014/06/06 00:47:00, raymes wrote:
> Here too.

Done.

piman

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc#newcode175 content/renderer/pepper/pepper_compositor_host.cc:175: CHECK(shared_memory->Map(desc.stride * desc.size.height)); On 2014/06/05 00:50:38, Peng wrote: > ...

6 years, 6 months ago (2014-06-09 18:27:24 UTC) #17

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:175:
CHECK(shared_memory->Map(desc.stride * desc.size.height));
On 2014/06/05 00:50:38, Peng wrote:
> On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> > no CHECK.
> 
> If the Map() failed. cc will crash anyway. So what should we do if Map()
failed?

Return failure and not crash?

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:667:
ParamTraits<ppapi::CompositorLayerData::Type>::Write(m, p.type);
WriteParam(m, p.type) does the type deduction automatically. Simpler to read.

https://codereview.chromium.org/298023004/diff/170001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:703: if
(!ParamTraits<ppapi::CompositorLayerData::Type>::Read(m, iter, &r->type))
Same here, ReadParam

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
File ppapi/shared_impl/compositor_layer_data.h (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:17: typedef int8_t
Mailbox[GL_MAILBOX_SIZE_CHROMIUM];
On 2014/06/05 00:50:38, Peng wrote:
> On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> > just use gpu::Mailbox. It also has existing ParamTraits.
> It has a constructor. The compiler does not allow put it in a union. :(

Then maybe a union is not a good choice.

Alternatives:
- put all fields inline. Overhead is small compared to 1 mailbox + 1 transform
(and layer count is expected to be small).
- put 3 scoped_ptr for each of the fields. You only allocate memory for the
fields you actually use, at the cost of an indirection.

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:52: // we only need set it to 0.
On 2014/06/05 00:50:38, Peng wrote:
> On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> > Technically, this is undefined behavior. Also, fragile.
> > 
> > If you give a name to the union, you can memset it.
> 
> If I give it a name, I think I can not use union member directly anymore.

How is that a problem?

> For example:
> 
> data.color.red -> data.union_name.color.red.
> 
> Do you have a better idea to achieve it?

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:73: } color;
On 2014/06/05 00:50:38, Peng wrote:
> On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> > If you define those as actual structs, you can use the IPC_STRUCT_TRAITS_*
> > macros which are a lot more maintainable that the individual ParamTraits.
> 
> Unless using a struct includes all fields in struct color, texture, and image,
I
> think we can not avoid ParamTraits. But it will add some overhead for IPC
> message.
> 
> Thought?

If you define a struct ColorLayerData { ... }, TextureLayerData { ... },
ImageLayerData { ... }

then you can have
union LayerData {
  ColorLayerData color;
  TextureLayerData texture;
  ImageLayerData image;
} layer_data;

Then you can define the param traits for each of
ColorLayerData/TextureLayerData/ImageLayerData using the IPC_STRUCT_TRAITS_*

Then the ParamTraits for CompositorLayerData become something like

switch (p.type) {
  case TYPE_COLOR:
    WriteParam(m, p.layer_data.color);
    break;
  case TYPE_TEXTURE:
    WriteParam(m, p.layer_data.texture);
    break;
  case TYPE_IMAGE:
    WriteParam(m, p.layer_data.image);
    break;
}

And similar for the Read.

https://codereview.chromium.org/298023004/diff/190001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:61: NOTREACHED();
Then just DCHECK. No need to pay the cost of doing the check in release.

raymes

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. Why do we need ...

6 years, 6 months ago (2014-06-10 05:00:37 UTC) #18

Peng

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. On 2014/06/10 05:00:37, raymes ...

6 years, 6 months ago (2014-06-10 11:44:22 UTC) #19

Peng

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper/pepper_compositor_host.cc#newcode175 content/renderer/pepper/pepper_compositor_host.cc:175: CHECK(shared_memory->Map(desc.stride * desc.size.height)); On 2014/06/09 18:27:24, piman wrote: > ...

6 years, 6 months ago (2014-06-10 16:20:28 UTC) #20

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:175:
CHECK(shared_memory->Map(desc.stride * desc.size.height));
On 2014/06/09 18:27:24, piman wrote:
> On 2014/06/05 00:50:38, Peng wrote:
> > On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> > > no CHECK.
> > 
> > If the Map() failed. cc will crash anyway. So what should we do if Map()
> failed?
> 
> Return failure and not crash?

Done

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:230: NOTREACHED();
On 2014/06/04 13:19:42, piman wrote:
> same here

Done.

https://codereview.chromium.org/298023004/diff/170001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:239: NOTREACHED();
On 2014/06/04 13:19:43, piman wrote:
> same here

Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
File ppapi/shared_impl/compositor_layer_data.h (right):

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:17: typedef int8_t
Mailbox[GL_MAILBOX_SIZE_CHROMIUM];
On 2014/06/09 18:27:24, piman wrote:
> On 2014/06/05 00:50:38, Peng wrote:
> > On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> > > just use gpu::Mailbox. It also has existing ParamTraits.
> > It has a constructor. The compiler does not allow put it in a union. :(
> 
> Then maybe a union is not a good choice.
> 
> Alternatives:
> - put all fields inline. Overhead is small compared to 1 mailbox + 1 transform
> (and layer count is expected to be small).
> - put 3 scoped_ptr for each of the fields. You only allocate memory for the
> fields you actually use, at the cost of an indirection.

Use 3 scoped_ptr. Done.

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:52: // we only need set it to 0.
On 2014/06/09 18:27:24, piman wrote:
> On 2014/06/05 00:50:38, Peng wrote:
> > On 2014/06/04 13:19:44, piman OOO back 2014-6-09 wrote:
> > > Technically, this is undefined behavior. Also, fragile.
> > > 
> > > If you give a name to the union, you can memset it.
> > 
> > If I give it a name, I think I can not use union member directly anymore.
> 
> How is that a problem?
> 
> > For example:
> > 
> > data.color.red -> data.union_name.color.red.
> > 
> > Do you have a better idea to achieve it?

I feel it looks a little weird. Anyway, change it to use 3 scoped_ptr. Done

https://codereview.chromium.org/298023004/diff/170001/ppapi/shared_impl/compo...
ppapi/shared_impl/compositor_layer_data.h:73: } color;
On 2014/06/09 18:27:24, piman wrote:
> On 2014/06/05 00:50:38, Peng wrote:
> > On 2014/06/04 13:19:43, piman OOO back 2014-6-09 wrote:
> > > If you define those as actual structs, you can use the IPC_STRUCT_TRAITS_*
> > > macros which are a lot more maintainable that the individual ParamTraits.
> > 
> > Unless using a struct includes all fields in struct color, texture, and
image,
> I
> > think we can not avoid ParamTraits. But it will add some overhead for IPC
> > message.
> > 
> > Thought?
> 
> If you define a struct ColorLayerData { ... }, TextureLayerData { ... },
> ImageLayerData { ... }
> 
> then you can have
> union LayerData {
>   ColorLayerData color;
>   TextureLayerData texture;
>   ImageLayerData image;
> } layer_data;
> 
> Then you can define the param traits for each of
> ColorLayerData/TextureLayerData/ImageLayerData using the IPC_STRUCT_TRAITS_*
> 
> Then the ParamTraits for CompositorLayerData become something like
> 
> switch (p.type) {
>   case TYPE_COLOR:
>     WriteParam(m, p.layer_data.color);
>     break;
>   case TYPE_TEXTURE:
>     WriteParam(m, p.layer_data.texture);
>     break;
>   case TYPE_IMAGE:
>     WriteParam(m, p.layer_data.image);
>     break;
> }
> 
> And similar for the Read.

Done. Now we only has ParamTraits for the 4x4 matrix, because we can not use
gfx::Transform which uses skia.

https://codereview.chromium.org/298023004/diff/190001/content/renderer/pepper...
File content/renderer/pepper/pepper_compositor_host.cc (right):

https://codereview.chromium.org/298023004/diff/190001/content/renderer/pepper...
content/renderer/pepper/pepper_compositor_host.cc:61: NOTREACHED();
On 2014/06/09 18:27:24, piman wrote:
> Then just DCHECK. No need to pay the cost of doing the check in release.

Done.

piman

https://codereview.chromium.org/298023004/diff/290001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/290001/content/renderer/pepper/pepper_compositor_host.cc#newcode99 content/renderer/pepper/pepper_compositor_host.cc:99: static_cast<base::SharedMemoryHandle>(_dup(handle)); I don't think you can use _dup to ...

6 years, 6 months ago (2014-06-10 22:37:06 UTC) #21

Peng

https://codereview.chromium.org/298023004/diff/290001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/290001/content/renderer/pepper/pepper_compositor_host.cc#newcode99 content/renderer/pepper/pepper_compositor_host.cc:99: static_cast<base::SharedMemoryHandle>(_dup(handle)); On 2014/06/10 22:37:06, piman wrote: > I don't ...

6 years, 6 months ago (2014-06-10 23:03:32 UTC) #22

raymes

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. But isn't it ok ...

6 years, 6 months ago (2014-06-11 00:21:06 UTC) #23

Peng

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. On 2014/06/11 00:21:06, raymes ...

6 years, 6 months ago (2014-06-11 01:43:11 UTC) #24

raymes

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. Ok that seems reasonable. ...

6 years, 6 months ago (2014-06-12 02:31:25 UTC) #25

raymes

On 2014/06/12 02:31:25, raymes wrote: > https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc > File ppapi/proxy/compositor_layer_resource.cc (right): > > https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 > ...

6 years, 6 months ago (2014-06-12 02:33:32 UTC) #26

Peng

https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc File ppapi/proxy/compositor_layer_resource.cc (right): https://codereview.chromium.org/298023004/diff/270001/ppapi/proxy/compositor_layer_resource.cc#newcode221 ppapi/proxy/compositor_layer_resource.cc:221: ScopedPPResource(pp_resource()), // Keep layer alive. On 2014/06/12 02:31:25, raymes ...

6 years, 6 months ago (2014-06-12 14:24:36 UTC) #27

Peng

+dcheng +nasko for ppapi/proxy/ppapi_messages.h PTAL. Thanks.

6 years, 6 months ago (2014-06-12 14:27:11 UTC) #28

nasko

Couple of comments. https://codereview.chromium.org/298023004/diff/430001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/430001/content/renderer/pepper/pepper_compositor_host.cc#newcode323 content/renderer/pepper/pepper_compositor_host.cc:323: new scoped_ptr<base::SharedMemory>[layers.size()]); nit: You should verify ...

6 years, 6 months ago (2014-06-13 17:23:01 UTC) #30

piman

https://codereview.chromium.org/298023004/diff/430001/ppapi/proxy/compositor_resource.cc File ppapi/proxy/compositor_resource.cc (right): https://codereview.chromium.org/298023004/diff/430001/ppapi/proxy/compositor_resource.cc#newcode113 ppapi/proxy/compositor_resource.cc:113: DCHECK(it != release_callback_map_.end()) << On 2014/06/13 17:23:01, nasko wrote: ...

6 years, 6 months ago (2014-06-13 17:30:05 UTC) #31

nasko

https://codereview.chromium.org/298023004/diff/430001/ppapi/proxy/compositor_resource.cc File ppapi/proxy/compositor_resource.cc (right): https://codereview.chromium.org/298023004/diff/430001/ppapi/proxy/compositor_resource.cc#newcode113 ppapi/proxy/compositor_resource.cc:113: DCHECK(it != release_callback_map_.end()) << On 2014/06/13 17:30:05, piman wrote: ...

6 years, 6 months ago (2014-06-13 17:38:49 UTC) #32

Peng

https://codereview.chromium.org/298023004/diff/430001/content/renderer/pepper/pepper_compositor_host.cc File content/renderer/pepper/pepper_compositor_host.cc (right): https://codereview.chromium.org/298023004/diff/430001/content/renderer/pepper/pepper_compositor_host.cc#newcode323 content/renderer/pepper/pepper_compositor_host.cc:323: new scoped_ptr<base::SharedMemory>[layers.size()]); On 2014/06/13 17:23:01, nasko wrote: > nit: ...

6 years, 6 months ago (2014-06-13 18:04:06 UTC) #33