Add Pepper VideoReader and VideoWriter plumbing

Adds internal APIs, thunks, resources, and resource hosts.
Adds IPC messages to support the new resources.
Adds stubbed out host methods to hold implementation

TBR=tsepez@chromium.org, yzshen@chromium.org

BUG=230980
TESTS=none

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

[bbudge, 2013-04-08 22:11:43]

+juberti +ronghuawu FYI
This isn't ready for review. In particular, I need to add code to create and
manage a pool of ImageData resources for the frames. This should allow Ronghua
to see where the Webkit video stream code has to be hooked up.

[Ronghua Wu (Left Chromium), 2013-04-08 22:34:39]

https://codereview.chromium.org/13771020/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_video_reader_host.cc (right):

https://codereview.chromium.org/13771020/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_video_reader_host.cc:61: // Create an image data
resource and copy the stream's next frame into it.
Will the plugin need to call this GetFrame for every frame? Or can plugin just
register a callback and we will push the frame to plugin when it's ready.

[bbudge, 2013-04-08 22:38:01]

https://codereview.chromium.org/13771020/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_video_reader_host.cc (right):

https://codereview.chromium.org/13771020/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_video_reader_host.cc:61: // Create an image data
resource and copy the stream's next frame into it.
My understanding of this API is that the plugin 'pulls' frames from the stream.
I guess that means you should retrieve the most recent frame from the stream
here. Perhaps that implies keeping the timestamp of the last frame returned to
the plugin so you don't return the same frame more than once. The place to put
all of this state is in this class.

On 2013/04/08 22:34:39, Ronghua Wu wrote:
> Will the plugin need to call this GetFrame for every frame? Or can plugin just
> register a callback and we will push the frame to plugin when it's ready.

[bbudge, 2013-05-01 21:53:39]

Yuzhu for Pepper plumbing.
Tom for IPC review (ppapi_messages.h)

[Tom Sepez, 2013-05-01 22:10:20]

On 2013/05/01 21:53:39, bbudge1 wrote:
> Yuzhu for Pepper plumbing.
> Tom for IPC review (ppapi_messages.h)
Bill, can you point me at where you validate the URL string you're passing
around?

[bbudge, 2013-05-01 22:14:26]

On 2013/05/01 22:10:20, Tom Sepez wrote:
> On 2013/05/01 21:53:39, bbudge1 wrote:
> > Yuzhu for Pepper plumbing.
> > Tom for IPC review (ppapi_messages.h)
> Bill, can you point me at where you validate the URL string you're passing
> around?

Nowhere : ) This used to be an id and was then changed to a URL. I'll change it
to a GURL.

[yzshen1, 2013-05-01 23:18:33]

Only some nits.

Thanks!

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:131: PP_Resource
PepperInProcessResourceCreation::CreateVideoDestination(
nit: Usually these NOTIMPLEMENTED Create*() are placed in
resource_creation_impl.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_destination_host.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:38: if
(!host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE))
If we don't have private permission, this host instance won't be created from
the beginning, right?

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:57: return
PP_OK_COMPLETIONPENDING;
nit, optional: you could set context->reply_msg and return PP_ERROR_FAILED
directly.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:42: if
(!host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE))
We won't create this instance from the begining if we don't have private
permission, right?

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:68: // TODO() Check next
available frame's timestamp. Wait until it changes.
Please add a name for the TODO.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1854: double /* timestamp */)
optional, nit: Does it make sense to use PP_TimeTicks here? I think the meaning
is better defined.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1855:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VideoDestination_PutFrameReply)
This is not needed.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:544: instance,
static_cast<PP_ImageDataFormat>(format), &size, clear);
the static_cast is not needed. (and line 547)

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:553: if
(enter_resource.object()->Describe(desc) != PP_TRUE)
In this case and line 557, shall we release the PP_Resource and return 0?

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:589: instance,
nit: The indent is a little bit unusual.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
File ppapi/proxy/video_destination_resource.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:12: #include
"ppapi/shared_impl/array_writer.h"
you probably don't need this.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:15: #include "ppapi/shared_impl/var.h"
this has been included in the .h file.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:22: namespace {
this is not needed.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:12: #include
"ppapi/shared_impl/array_writer.h"
this is not needed.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:21: namespace {
this is not needed.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:85: int32_t result = params.result();
We need to test whether the callback is still pending. If it has been aborted,
|frame| can be invalid. The plugin may have released it.

https://codereview.chromium.org/13771020/diff/19001/ppapi/shared_impl/resource.h
File ppapi/shared_impl/resource.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource.h:73: F(PPB_VideoDestination_Private_API) \
Please order them alphabetically.

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_des...
File ppapi/thunk/ppb_video_destination_private_api.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_des...
ppapi/thunk/ppb_video_destination_private_api.h:23: virtual int32_t Open(PP_Var
stream_url,
nit: please use const &

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_sou...
File ppapi/thunk/ppb_video_source_private_api.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_sou...
ppapi/thunk/ppb_video_source_private_api.h:23: virtual int32_t Open(PP_Var
stream_url,
nit: please use const &

[bbudge, 2013-05-02 21:39:06]

Tom, the stream URLs are sent over as strings. I use GURL to check that they are
valid in the renderer process, in
content/renderer/pepper/pepper_video_destination_host.cc and
pepper_video_source_host.cc. Look in the OnHostMsgOpen methods. Ronghua may have
to add additional validation code when this is hooked up.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_in_process_resource_creation.cc:131: PP_Resource
PepperInProcessResourceCreation::CreateVideoDestination(
On 2013/05/01 23:18:33, yzshen1 wrote:
> nit: Usually these NOTIMPLEMENTED Create*() are placed in
> resource_creation_impl.

Done.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_destination_host.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:38: if
(!host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE))
True. Removed the check. I copied this from other hosts.
On 2013/05/01 23:18:33, yzshen1 wrote:
> If we don't have private permission, this host instance won't be created from
> the beginning, right?

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:57: return
PP_OK_COMPLETIONPENDING;
On 2013/05/01 23:18:33, yzshen1 wrote:
> nit, optional: you could set context->reply_msg and return PP_ERROR_FAILED
> directly.

Done.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:42: if
(!host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE))
On 2013/05/01 23:18:33, yzshen1 wrote:
> We won't create this instance from the begining if we don't have private
> permission, right?

Done.

https://codereview.chromium.org/13771020/diff/19001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:68: // TODO() Check next
available frame's timestamp. Wait until it changes.
On 2013/05/01 23:18:33, yzshen1 wrote:
> Please add a name for the TODO.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1854: double /* timestamp */)
On 2013/05/01 23:18:33, yzshen1 wrote:
> optional, nit: Does it make sense to use PP_TimeTicks here? I think the
meaning
> is better defined.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1855:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VideoDestination_PutFrameReply)
On 2013/05/01 23:18:33, yzshen1 wrote:
> This is not needed.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:544: instance,
static_cast<PP_ImageDataFormat>(format), &size, clear);
On 2013/05/01 23:18:33, yzshen1 wrote:
> the static_cast is not needed. (and line 547)

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:553: if
(enter_resource.object()->Describe(desc) != PP_TRUE)
On 2013/05/01 23:18:33, yzshen1 wrote:
> In this case and line 557, shall we release the PP_Resource and return 0?

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/ppb_image_dat...
ppapi/proxy/ppb_image_data_proxy.cc:589: instance,
On 2013/05/01 23:18:33, yzshen1 wrote:
> nit: The indent is a little bit unusual.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
File ppapi/proxy/video_destination_resource.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:12: #include
"ppapi/shared_impl/array_writer.h"
On 2013/05/01 23:18:33, yzshen1 wrote:
> you probably don't need this.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:15: #include "ppapi/shared_impl/var.h"
On 2013/05/01 23:18:33, yzshen1 wrote:
> this has been included in the .h file.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:22: namespace {
On 2013/05/01 23:18:33, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:12: #include
"ppapi/shared_impl/array_writer.h"
On 2013/05/01 23:18:33, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:21: namespace {
On 2013/05/01 23:18:33, yzshen1 wrote:
> this is not needed.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:85: int32_t result = params.result();
On 2013/05/01 23:18:33, yzshen1 wrote:
> We need to test whether the callback is still pending. If it has been aborted,
> |frame| can be invalid. The plugin may have released it.

Done. I redid the whole callback setup for both resource types.

https://codereview.chromium.org/13771020/diff/19001/ppapi/shared_impl/resource.h
File ppapi/shared_impl/resource.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource.h:73: F(PPB_VideoDestination_Private_API) \
On 2013/05/01 23:18:33, yzshen1 wrote:
> Please order them alphabetically.

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_des...
File ppapi/thunk/ppb_video_destination_private_api.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_des...
ppapi/thunk/ppb_video_destination_private_api.h:23: virtual int32_t Open(PP_Var
stream_url,
On 2013/05/01 23:18:33, yzshen1 wrote:
> nit: please use const &

Done.

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_sou...
File ppapi/thunk/ppb_video_source_private_api.h (right):

https://codereview.chromium.org/13771020/diff/19001/ppapi/thunk/ppb_video_sou...
ppapi/thunk/ppb_video_source_private_api.h:23: virtual int32_t Open(PP_Var
stream_url,
On 2013/05/01 23:18:33, yzshen1 wrote:
> nit: please use const &

Done. For both resource types.

[Tom Sepez, 2013-05-02 21:42:29]

LGTM, but please be sure I'm cc'd on the next patch for the implementation.

[yzshen1, 2013-05-02 22:03:04]

LGTM with a few nits.

Thanks!

https://codereview.chromium.org/13771020/diff/35001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_destination_host.cc (right):

https://codereview.chromium.org/13771020/diff/35001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:61: return
PP_ERROR_FAILED;
If you return a value other than PP_OK_COMPLETIONPENDING, you should set
context->reply_msg instead of line 57-60. Otherwise, the code will hit
assertions because ResourceMessageHandler::RunMessageHandlerAndReply() will try
to send reply for you.

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_destina...
File ppapi/proxy/video_destination_resource.cc (right):

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:92: is_open_ = true;
Maybe we set is_open_ to true only if open is successful?

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:86: is_open_ = true;
It seems better to set is_open_ to true only if the open operation is
successful.

[bbudge, 2013-05-02 23:12:19]

https://codereview.chromium.org/13771020/diff/35001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_destination_host.cc (right):

https://codereview.chromium.org/13771020/diff/35001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_destination_host.cc:61: return
PP_ERROR_FAILED;
On 2013/05/02 22:03:05, yzshen1 wrote:
> If you return a value other than PP_OK_COMPLETIONPENDING, you should set
> context->reply_msg instead of line 57-60. Otherwise, the code will hit
> assertions because ResourceMessageHandler::RunMessageHandlerAndReply() will
try
> to send reply for you.

Let's return PP_OK_COMPLETIONPENDING here for now. I'm not sure how the glue
code will look so we'll be changing this soon. Done here and in
pepper_video_source_host.cc.

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_destina...
File ppapi/proxy/video_destination_resource.cc (right):

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_destina...
ppapi/proxy/video_destination_resource.cc:92: is_open_ = true;
On 2013/05/02 22:03:05, yzshen1 wrote:
> Maybe we set is_open_ to true only if open is successful?

Done.

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_source_...
File ppapi/proxy/video_source_resource.cc (right):

https://codereview.chromium.org/13771020/diff/35001/ppapi/proxy/video_source_...
ppapi/proxy/video_source_resource.cc:86: is_open_ = true;
On 2013/05/02 22:03:05, yzshen1 wrote:
> It seems better to set is_open_ to true only if the open operation is
> successful.

Done.

[commit-bot: I haz the power, 2013-05-03 15:31:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/13771020/61001

[commit-bot: I haz the power, 2013-05-03 15:38:31]

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-05-03 16:26:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/13771020/61001

[commit-bot: I haz the power, 2013-05-03 23:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/13771020/61001

[bbudge, 2013-05-04 00:11:36]

Committed patchset #6 manually as r198258 (presubmit successful).