Add PPAPI tests for VideoSource and VideoDestination resources.

Add PPAPI tests for VideoSource and VideoDestination resources.

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

[bbudge, 2013-05-07 20:48:48]

To run these, build browser_tests and use
--gtest_filter="OutOfProcessPPAPITest.VideoSource" or VideoDestination.
The tests fail now because the stream_url's in the GetFrame and PutFrame methods
aren't recognized by the renderer process.

[bbudge, 2013-05-09 21:24:08]

Raymes for proxy host and resource (and tests if you'd like).
Ronghua for tests.

[bbudge, 2013-05-09 21:39:19]

+Chris for IPC review (ppapi_messages.h)

[bbudge, 2013-05-09 21:40:25]

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

https://codereview.chromium.org/15039009/diff/3001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1867: std::string /* image_data_desc */,
We're using string to marshal a C struct from the host
(pepper_video_source_host.cc) to the plugin (video_source_resource.cc)

[bbudge, 2013-05-09 21:40:25]

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

https://codereview.chromium.org/15039009/diff/3001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1867: std::string /* image_data_desc */,
We're using string to marshal a C struct from the host
(pepper_video_source_host.cc) to the plugin (video_source_resource.cc)

[palmer, 2013-05-09 21:55:13]

https://codereview.chromium.org/15039009/diff/3001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/15039009/diff/3001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_source_host.cc:183:
memcpy(&(image_desc_data)[0], &image_desc, sizeof(PP_ImageDataDesc));
Isn't there a way to describe to the IPC layer how to serialize new object
types? IPC_STRUCT_TRAITS_BEGIN, _MEMBER, et c., from
ipc/param_traits_write_macros.h?

Barring that, I would suggest passing the members of the struct, if few and
simple, as distinct parameters, but that doesn't seem possible here (nested
structs).

Barring that, I'd use a std::vector<uint8_t> rather than a string.

[Ronghua Wu (Left Chromium), 2013-05-09 22:22:52]

https://codereview.chromium.org/15039009/diff/3001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/15039009/diff/3001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:1259:
TEST_PPAPI_OUT_OF_PROCESS(VideoSource)
do we still need  TEST_PPAPI_NACL?

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
File ppapi/tests/test_video_destination.cc (right):

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:9: #include <string.h>
do we want string.h instead of string?

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:47: "PPB_VideoDestination_Private
interface not available");
do we need to return false here?

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:99: {
just curious, why do we need a { } here?

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_sou...
File ppapi/tests/test_video_source.cc (right):

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_sou...
ppapi/tests/test_video_source.cc:9: #include <string.h>
string?

[bbudge, 2013-05-09 22:35:35]

https://codereview.chromium.org/15039009/diff/3001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/15039009/diff/3001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:1259:
TEST_PPAPI_OUT_OF_PROCESS(VideoSource)
No, because they will fail without a proxy.
On 2013/05/09 22:22:52, Ronghua Wu wrote:
> do we still need  TEST_PPAPI_NACL?

https://codereview.chromium.org/15039009/diff/3001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/15039009/diff/3001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_source_host.cc:183:
memcpy(&(image_desc_data)[0], &image_desc, sizeof(PP_ImageDataDesc));
On 2013/05/09 21:55:13, Chromium Palmer wrote:
> Isn't there a way to describe to the IPC layer how to serialize new object
> types? IPC_STRUCT_TRAITS_BEGIN, _MEMBER, et c., from
> ipc/param_traits_write_macros.h?
> 
> Barring that, I would suggest passing the members of the struct, if few and
> simple, as distinct parameters, but that doesn't seem possible here (nested
> structs).
> 
> Barring that, I'd use a std::vector<uint8_t> rather than a string.

Done.

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
File ppapi/tests/test_video_destination.cc (right):

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:9: #include <string.h>
On 2013/05/09 22:22:52, Ronghua Wu wrote:
> do we want string.h instead of string?

Done.

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:47: "PPB_VideoDestination_Private
interface not available");
On 2013/05/09 22:22:52, Ronghua Wu wrote:
> do we need to return false here?

Done.

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_des...
ppapi/tests/test_video_destination.cc:99: {
That makes it easier to add later tests, since we can reuse variable names, such
as completion callbacks. Since I don't have any yet, it's a bit speculative so
removing the block. Done.

On 2013/05/09 22:22:52, Ronghua Wu wrote:
> just curious, why do we need a { } here?

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_sou...
File ppapi/tests/test_video_source.cc (right):

https://codereview.chromium.org/15039009/diff/3001/ppapi/tests/test_video_sou...
ppapi/tests/test_video_source.cc:9: #include <string.h>
On 2013/05/09 22:22:52, Ronghua Wu wrote:
> string?

Done.

[Ronghua Wu (Left Chromium), 2013-05-09 22:42:55]

*test* LGTM

[palmer, 2013-05-09 23:48:11]

lgtm

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
File ppapi/tests/test_video_destination.cc (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
ppapi/tests/test_video_destination.cc:42:
PPB_VIDEODESTINATION_PRIVATE_INTERFACE));
Style nit: I think the standard is to indent only 4 additional spaces.

Same thing in a few places below.

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
File ppapi/tests/test_video_destination.h (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
ppapi/tests/test_video_destination.h:33: std::string stream_url_;
Should this be a GURL?

Or I could be wrong!

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_so...
File ppapi/tests/test_video_source.h (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_so...
ppapi/tests/test_video_source.h:33: std::string stream_url_;
GURL?

[bbudge, 2013-05-10 00:13:13]

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
File ppapi/tests/test_video_destination.cc (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
ppapi/tests/test_video_destination.cc:42:
PPB_VIDEODESTINATION_PRIVATE_INTERFACE));
Yeah, I should switch to the clang format soon. Done.
On 2013/05/09 23:48:11, Chromium Palmer wrote:
> Style nit: I think the standard is to indent only 4 additional spaces.
> 
> Same thing in a few places below.

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
File ppapi/tests/test_video_destination.h (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_de...
ppapi/tests/test_video_destination.h:33: std::string stream_url_;
Pepper is supposed to be hermetic, so our tests can't use anything else like
base or googleurl.
On 2013/05/09 23:48:11, Chromium Palmer wrote:
> Should this be a GURL?
> 
> Or I could be wrong!

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_so...
File ppapi/tests/test_video_source.h (right):

https://codereview.chromium.org/15039009/diff/19001/ppapi/tests/test_video_so...
ppapi/tests/test_video_source.h:33: std::string stream_url_;
Same comment as in destination.h.
On 2013/05/09 23:48:11, Chromium Palmer wrote:
> GURL?

[raymes, 2013-05-10 05:32:10]

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

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:65: base::Unretained(this),
I guess this wasn't right before because we can't pass the weak pointer across
threads. Unfortunately I still think this isn't right because the
PepperVideoSourceHost can be destroyed when it tries to call into OnGotFrame. 

One option: have a thread-safe recounted helper object which implements
GotFrame. Pass in a weak pointer to this host on construction.

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:108: int32_t result =
SendGetFrameReply(false);  // Don't send reply on failure.
I think this would be clearer if SendGetFrameReply always sent a reply (even on
failure).

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:182: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_ANDROID)
Please add a TODO to unify this with the pdf resource host.

[bbudge, 2013-05-10 12:31:12]

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

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:65: base::Unretained(this),
The RTC folks say this can't be called after we complete a call to
VIdeoSourceHandler::Close. We do that in our destructor. However, let's be
defensive here. Done.
On 2013/05/10 05:32:10, raymes wrote:
> I guess this wasn't right before because we can't pass the weak pointer across
> threads. Unfortunately I still think this isn't right because the
> PepperVideoSourceHost can be destroyed when it tries to call into OnGotFrame. 
> 
> One option: have a thread-safe recounted helper object which implements
> GotFrame. Pass in a weak pointer to this host on construction. 
>

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:108: int32_t result =
SendGetFrameReply(false);  // Don't send reply on failure.
On 2013/05/10 05:32:10, raymes wrote:
> I think this would be clearer if SendGetFrameReply always sent a reply (even
on
> failure).

Done.

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:182: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2013/05/10 05:32:10, raymes wrote:
> Please add a TODO to unify this with the pdf resource host.

Done.

[bbudge, 2013-05-10 13:52:59]

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

https://codereview.chromium.org/15039009/diff/27001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:65: base::Unretained(this),
While out on my morning run, around mile 2 I realized that you are absolutely
right - this must be done with a ref counted helper. Thanks for the excellent
and prompt reviews.
On 2013/05/10 12:31:12, bbudge1 wrote:
> The RTC folks say this can't be called after we complete a call to
> VIdeoSourceHandler::Close. We do that in our destructor. However, let's be
> defensive here. Done.
> On 2013/05/10 05:32:10, raymes wrote:
> > I guess this wasn't right before because we can't pass the weak pointer
across
> > threads. Unfortunately I still think this isn't right because the
> > PepperVideoSourceHost can be destroyed when it tries to call into
OnGotFrame. 
> > 
> > One option: have a thread-safe recounted helper object which implements
> > GotFrame. Pass in a weak pointer to this host on construction. 
> > 
>

[raymes, 2013-05-10 15:14:47]

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

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:131: void
PepperVideoSourceHost::SendGetFrameReply() {
I think the state is a little hard to follow, although I think it's right. Could
you add a comment (probably in the header) describing the post conditions of
this function? In other words, whether successful or unsuccessful, the function
guarantees:
-a reply message will be sent to the plugin,
-last_frame_ will be released,
-get_frame_pending_ will be false,
-reply_context_ will be reset.

Otherwise it might be clearer to pass in all the members needed to this
function. I think what you have is fine though, a comment just might make it
clearer.

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:151: return
ReportGetFrameError(PP_ERROR_FAILED);
Since ReportGetFrameError() doesn't return a value, it might be better to do 
{
  ReportGetFrameError(..);
  return;
}

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_source_host.h (right):

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.h:25: : public
ppapi::host::ResourceHost {
nit: I think this might fit on the above line

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.h:65: void SendGetFrameReply();
nit: Might be nice to have comments for these. See my comment in the cc file.

[bbudge, 2013-05-10 17:38:21]

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

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:131: void
PepperVideoSourceHost::SendGetFrameReply() {
On 2013/05/10 15:14:48, raymes wrote:
> I think the state is a little hard to follow, although I think it's right.
Could
> you add a comment (probably in the header) describing the post conditions of
> this function? In other words, whether successful or unsuccessful, the
function
> guarantees:
> -a reply message will be sent to the plugin,
> -last_frame_ will be released,
> -get_frame_pending_ will be false,
> -reply_context_ will be reset.
> 
> Otherwise it might be clearer to pass in all the members needed to this
> function. I think what you have is fine though, a comment just might make it
> clearer.

Done.

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.cc:151: return
ReportGetFrameError(PP_ERROR_FAILED);
On 2013/05/10 15:14:48, raymes wrote:
> Since ReportGetFrameError() doesn't return a value, it might be better to do 
> {
>   ReportGetFrameError(..);
>   return;
> }

Done.

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_video_source_host.h (right):

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.h:25: : public
ppapi::host::ResourceHost {
On 2013/05/10 15:14:48, raymes wrote:
> nit: I think this might fit on the above line

Done.

https://codereview.chromium.org/15039009/diff/37001/content/renderer/pepper/p...
content/renderer/pepper/pepper_video_source_host.h:65: void SendGetFrameReply();
On 2013/05/10 15:14:48, raymes wrote:
> nit: Might be nice to have comments for these. See my comment in the cc file.

Done.

[raymes1, 2013-05-10 22:50:27]

lgtm

[commit-bot: I haz the power, 2013-05-10 22:55:54]

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

[yzshen1, 2013-05-10 23:02:36]

test OWNER rubber stamp.

[yzshen1, 2013-05-10 23:02:48]

On 2013/05/10 23:02:36, yzshen1 wrote:
> test OWNER rubber stamp.

LGTM. :)

[commit-bot: I haz the power, 2013-05-11 19:23:40]

Change committed as 199626