Abstract PPAPI's ImageData behind webrtc::DesktopFrame interface

Abstract PPAPI's ImageData behind webrtc::DesktopFrame interface

This abstracts away RectangleUpdateDecoder's direct use of the the former class.  It will ease the development of non--Web app client implementations.

BUG=255309
TBR=brettw (third_party/webrtc dependency)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210113

[solb, 2013-07-01 21:11:00]

I didn't use webrtc::DesktopFrame as the bug suggests because I need to be able
to create an implementation that manually allocates the memory chunk.

[Wez, 2013-07-01 21:15:40]

On 2013/07/01 21:11:00, solb wrote:
> I didn't use webrtc::DesktopFrame as the bug suggests because I need to be
able
> to create an implementation that manually allocates the memory chunk.

DesktopFrame is really just an interface w/ some standard members.  It should
support derived implementations that manually allocate the memory just fine -
that's what BasicDesktopFrame does, in fact.

[garykac, 2013-07-01 23:50:36]

lgtm, and I'd rather not add a dependency on webrtc here just to get an Image
wrapper class.

But let's double-check with wez before landing this.

https://codereview.chromium.org/18233015/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/18233015/diff/1/remoting/remoting.gyp#newcode...
remoting/remoting.gyp:1949: 'client/plugin/pepper_image_buffer.cc',
Are you using tabs here?

[Wez, 2013-07-02 00:06:08]

On 2013/07/01 23:50:36, garykac wrote:
> lgtm, and I'd rather not add a dependency on webrtc here just to get an Image
> wrapper class.

We already depend on webrtc for the capturers, region class, etc.

Let's use DesktopFrame here unless there's a good reason why that won't work.

[solb, 2013-07-03 19:42:40]

On 2013/07/02 00:06:08, Wez wrote:
> On 2013/07/01 23:50:36, garykac wrote:
> > lgtm, and I'd rather not add a dependency on webrtc here just to get an
Image
> > wrapper class.
> 
> We already depend on webrtc for the capturers, region class, etc.
> 
> Let's use DesktopFrame here unless there's a good reason why that won't work.

I switched back to webrtc::DesktopFrame.  Does this look better?

[Wez, 2013-07-03 20:01:23]

https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_desktop_frame.h (right):

https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:1: #ifndef
REMOTING_CLIENT_PLUGIN_PEPPER_IMAGE_BUFFER_H_
New files need copyright notice.

https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:20: class PepperDesktopFrame :
public webrtc::DesktopFrame {
Do you really need a class this complex?  Can't you just have:

static DesktopSize ppSizeToDesktopSize(pp::Size size) {...}

class PepperDesktopFrame : public DesktopFrame {
  PepperDesktopFrame(pp::ImageData image)
    : DesktopFrame(ppSizeToDesktopSize(image.size()),
                   image.stride(),
                   image.data(),
                   NULL),
      image_(image) { }
  pp::ImageData image_;
};

Since pp::ImageData ref-counts the underlying resource internally, and you
really just want to adapt from its interface to DesktopFrame?

https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:27: PepperDesktopFrame(pp::Size*
size,
Why pass these by reference?

https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:28: pp::ImageData* buffer,
When passing objects by reference, use scoped_ptr<Foo> as the parameter/return
value type.

[solb, 2013-07-03 21:04:56]

On 2013/07/03 20:01:23, Wez wrote:
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> File remoting/client/plugin/pepper_desktop_frame.h (right):
> 
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_desktop_frame.h:1: #ifndef
> REMOTING_CLIENT_PLUGIN_PEPPER_IMAGE_BUFFER_H_
> New files need copyright notice.
> 
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_desktop_frame.h:20: class PepperDesktopFrame :
> public webrtc::DesktopFrame {
> Do you really need a class this complex?  Can't you just have:
> 
> static DesktopSize ppSizeToDesktopSize(pp::Size size) {...}
> 
> class PepperDesktopFrame : public DesktopFrame {
>   PepperDesktopFrame(pp::ImageData image)
>     : DesktopFrame(ppSizeToDesktopSize(image.size()),
>                    image.stride(),
>                    image.data(),
>                    NULL),
>       image_(image) { }
>   pp::ImageData image_;
> };
> 
> Since pp::ImageData ref-counts the underlying resource internally, and you
> really just want to adapt from its interface to DesktopFrame?
> 
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_desktop_frame.h:27: PepperDesktopFrame(pp::Size*
> size,
> Why pass these by reference?
> 
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_desktop_frame.h:28: pp::ImageData* buffer,
> When passing objects by reference, use scoped_ptr<Foo> as the parameter/return
> value type.

Thanks, Wez, for helping me to simplify this.  Sorry about the last patchset's
being so silly; I guess I didn't read the headers carefully enough.

[solb, 2013-07-03 21:35:36]

On 2013/07/03 21:04:56, solb wrote:
> On 2013/07/03 20:01:23, Wez wrote:
> >
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> > File remoting/client/plugin/pepper_desktop_frame.h (right):
> > 
> >
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> > remoting/client/plugin/pepper_desktop_frame.h:1: #ifndef
> > REMOTING_CLIENT_PLUGIN_PEPPER_IMAGE_BUFFER_H_
> > New files need copyright notice.
> > 
> >
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> > remoting/client/plugin/pepper_desktop_frame.h:20: class PepperDesktopFrame :
> > public webrtc::DesktopFrame {
> > Do you really need a class this complex?  Can't you just have:
> > 
> > static DesktopSize ppSizeToDesktopSize(pp::Size size) {...}
> > 
> > class PepperDesktopFrame : public DesktopFrame {
> >   PepperDesktopFrame(pp::ImageData image)
> >     : DesktopFrame(ppSizeToDesktopSize(image.size()),
> >                    image.stride(),
> >                    image.data(),
> >                    NULL),
> >       image_(image) { }
> >   pp::ImageData image_;
> > };
> > 
> > Since pp::ImageData ref-counts the underlying resource internally, and you
> > really just want to adapt from its interface to DesktopFrame?
> > 
> >
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> > remoting/client/plugin/pepper_desktop_frame.h:27:
PepperDesktopFrame(pp::Size*
> > size,
> > Why pass these by reference?
> > 
> >
>
https://codereview.chromium.org/18233015/diff/10001/remoting/client/plugin/pe...
> > remoting/client/plugin/pepper_desktop_frame.h:28: pp::ImageData* buffer,
> > When passing objects by reference, use scoped_ptr<Foo> as the
parameter/return
> > value type.
> 
> Thanks, Wez, for helping me to simplify this.  Sorry about the last patchset's
> being so silly; I guess I didn't read the headers carefully enough.

Is this ready to commit?

[Wez, 2013-07-03 21:46:39]

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_desktop_frame.h (right):

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:17: class PepperDesktopFrame :
public webrtc::DesktopFrame {
This class is simple enough that you may as well define it in an anonymous
namespace within pepper_view.cc.

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:19: // Constructor.
nit: Please provide a more descriptive comment, e.g. "Wraps the supplied
pp::ImageData."

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_view.cc (right):

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:201: new pp::ImageData(instance_,
You don't need to use new here; pp::ImageData itself is just a handle to a
ref-counted resource.

[Lambros (do not use), 2013-07-03 21:53:25]

Just some drive-by nits.

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_desktop_frame.h (right):

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:5: #ifndef
REMOTING_CLIENT_PLUGIN_PEPPER_IMAGE_BUFFER_H_
nit: ..._DESKTOP_FRAME_H_

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:19: // Constructor.
nit: I think it's better to remove this comment. It is clearly a ctor, and also
the use of a raw pointer already documents non-ownership of the pointer, so I'm
not sure there's anything left to usefully say in a comment here. :)

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:20:
PepperDesktopFrame(pp::ImageData* buffer);
nit: single-arg ctor should be marked explicit.

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:23: pp::ImageData& data_object();
nit: Plain getter for member |foo_| should be named get_foo(), and should be
marked const (which is OK here, the pointer itself is not mutated). Optionally,
you can inline the definition here if you like.

Also, we tend to prefer pointers over non-const references in Chrome code.

[Wez, 2013-07-03 21:56:52]

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_desktop_frame.h (right):

https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_desktop_frame.h:26: pp::ImageData* buffer_;
This should be pp::ImageData, not pp::ImageData* since you want the underlying
resource to stay alive while this DesktopFrame exists; pp::ImageData is itself a
reference to a ref-counted resource.

[solb, 2013-07-03 22:14:49]

On 2013/07/03 21:56:52, Wez wrote:
>
https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
> File remoting/client/plugin/pepper_desktop_frame.h (right):
> 
>
https://codereview.chromium.org/18233015/diff/19001/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_desktop_frame.h:26: pp::ImageData* buffer_;
> This should be pp::ImageData, not pp::ImageData* since you want the underlying
> resource to stay alive while this DesktopFrame exists; pp::ImageData is itself
a
> reference to a ref-counted resource.

Wez and Lambros:
Done.

[Lambros, 2013-07-03 22:39:45]

https://codereview.chromium.org/18233015/diff/23002/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_view.cc (right):

https://codereview.chromium.org/18233015/diff/23002/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:39: inline const pp::ImageData&
get_buffer();
nit: Add another const after get_buffer().

[solb, 2013-07-03 23:27:32]

On 2013/07/03 22:39:45, Lambros wrote:
>
https://codereview.chromium.org/18233015/diff/23002/remoting/client/plugin/pe...
> File remoting/client/plugin/pepper_view.cc (right):
> 
>
https://codereview.chromium.org/18233015/diff/23002/remoting/client/plugin/pe...
> remoting/client/plugin/pepper_view.cc:39: inline const pp::ImageData&
> get_buffer();
> nit: Add another const after get_buffer().

Oops.

[Wez, 2013-07-03 23:43:48]

lgtm

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_view.cc (right):

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:32: // ImageBuffer that uses PPAPI to
allocate space for a raw image.
ImageBuffer that ... ->DesktopFrame that wraps a supplied pp::ImageData.

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:39: inline const pp::ImageData&
get_buffer() const;
nit: We don't normally bother with inline for cases like this; the compiler can
already work out it's inlineable.

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:39: inline const pp::ImageData&
get_buffer() const;
get_buffer -> buffer

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:45:
PepperDesktopFrame::PepperDesktopFrame(const pp::ImageData& buffer)
nit: You can move this inline in the class declaration to save some boilerplate.

https://codereview.chromium.org/18233015/diff/29001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.cc:234: false));
Cleaner to declare pp::ImageData image_data, and then new
PepperDesktopFrame(image_data) here, which would also allow you to get rid of
the static cast.

https://codereview.chromium.org/18233015/diff/29001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/18233015/diff/29001/remoting/remoting.gyp#new...
remoting/remoting.gyp:2751: # it can be included in multiple .gyp files.
What's happened here? Is Reitveld going nuts?

[commit-bot: I haz the power, 2013-07-04 00:02:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/18233015/33001

[Wez, 2013-07-04 00:11:05]

Great! Thanks for cleaning this up. :D

[commit-bot: I haz the power, 2013-07-04 00:16:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-04 00:20:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/18233015/33001

[commit-bot: I haz the power, 2013-07-04 02:46:51]

Change committed as 210113