Glue code to connect PPAPI proxy to MediaStream sources and destinations.

content/renderer/pepper/pepper_video_source_host.cc

Index: content/renderer/pepper/pepper_video_source_host.cc
diff --git a/content/renderer/pepper/pepper_video_source_host.cc b/content/renderer/pepper/pepper_video_source_host.cc
index 1015ea069c1a484eae06e6787cdb29269efc8767..7d43c2ae15bda24b00ef03c818d66422355278f0 100644
--- a/content/renderer/pepper/pepper_video_source_host.cc
+++ b/content/renderer/pepper/pepper_video_source_host.cc
@@ -6,16 +6,18 @@
 
 #include "base/bind.h"
 #include "content/public/renderer/renderer_ppapi_host.h"
+#include "content/renderer/render_thread_impl.h"

[raymes1, 2013/05/06 16:00:11]

Is this needed? 

[bbudge, 2013/05/06 19:27:50]

Not any more. Done.
On 2013/05/06 16:00:11, raymes1 wrote:

 #include "ppapi/c/pp_errors.h"
 #include "ppapi/host/dispatch_host_message.h"
-#include "ppapi/host/host_message_context.h"
 #include "ppapi/host/ppapi_host.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/ppb_image_data_proxy.h"
 #include "ppapi/shared_impl/scoped_pp_resource.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_image_data_api.h"
-#include "skia/ext/platform_canvas.h"
+#include "third_party/libjingle/source/talk/media/base/videocommon.h"
+#include "third_party/libjingle/source/talk/media/base/videoframe.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "webkit/plugins/ppapi/ppb_image_data_impl.h"
 
 using ppapi::host::HostMessageContext;
@@ -30,7 +32,8 @@ PepperVideoSourceHost::PepperVideoSourceHost(
     : ResourceHost(host->GetPpapiHost(), instance, resource),
       renderer_ppapi_host_(host),
       weak_factory_(this),
-      last_timestamp_(0) {
+      source_handler_(new content::VideoSourceHandler(NULL)),
+      get_frame_pending_(false) {
 }
 
 PepperVideoSourceHost::~PepperVideoSourceHost() {
@@ -50,24 +53,27 @@ int32_t PepperVideoSourceHost::OnResourceMessageReceived(
   return PP_ERROR_FAILED;
 }
 
-int32_t PepperVideoSourceHost::OnHostMsgOpen(HostMessageContext* context,
-                                             const std::string& stream_url) {
-  GURL gurl(stream_url);
-  if (!gurl.is_valid())
-    return PP_ERROR_BADARGUMENT;
-  // TODO(ronghuawu) Check that gurl is a valid MediaStream video track URL.
-  // TODO(ronghuawu) Open a MediaStream video track.
-  ReplyMessageContext reply_context = context->MakeReplyMessageContext();
-  reply_context.params.set_result(PP_OK);
-  host()->SendReply(reply_context, PpapiPluginMsg_VideoSource_OpenReply());
-  return PP_OK_COMPLETIONPENDING;
+bool PepperVideoSourceHost::GotFrame(cricket::VideoFrame* frame) {
+  // Take ownership of the new frame, and possibly delete any unsent one.
+  base::AutoLock lock(lock_);

[raymes, 2013/05/06 05:11:33]

Usually posting tasks is preferred over locks if it's possible. Take a look at
http://www.chromium.org/developers/design-documents/threading. In this case, you
might post a task to the main thread with the frame pointer. You can cache the
current MessageLoopProxy on construction (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...)

[bbudge, 2013/05/06 19:27:50]

Yes, I grepped and didn't find any resource using locks. Switched to posting a
task. Done.
On 2013/05/06 05:11:33, raymes wrote:

+  last_frame_.reset(frame);
+
+  if (get_frame_pending_) {
+    ppapi::HostResource image_data_resource;
+    PP_TimeTicks timestamp = 0;
+    int32_t result = ConvertFrame(&image_data_resource, &timestamp);

[raymes1, 2013/05/06 16:00:11]

Since we always call ConvertFrame and then SendFrame, could we just tie the two
functions together (e.g. ConvertAndSendFrame()?). Then we wouldn't have to have
the out paramaters. It could just have a return value indicating if the frame
was successfully converted and sent.

[bbudge, 2013/05/06 19:27:50]

The reason I split these is that there were a lot of 'if's with early outs
returning error codes. Without a separate function, I would have to reverse the
sense of these and get deep nesting. Also, in GotFrame, we always send the
reply, but in OnGetFrame we return just an error code on failure. So this
actually turned out to be cleaner.
On 2013/05/06 16:00:11, raymes1 wrote:

+    SendFrame(image_data_resource, timestamp, result);
+  }
+
+  return true;
 }
 
-int32_t PepperVideoSourceHost::OnHostMsgGetFrame(
-    HostMessageContext* context) {
-  ReplyMessageContext reply_context = context->MakeReplyMessageContext();
-  // TODO(ronghuawu) Wait until a frame with timestamp > last_timestamp_ is
-  // available.
+int32_t PepperVideoSourceHost::ConvertFrame(

[yzshen1, 2013/05/06 16:47:28]

We are not supposed to manipulate resource from a different thread. In the
renderer, we are running on a single thread.
(For example, the resource tracker is "SINGLE_THREADED".)

That is the main reason why we should post a task to the main thread.

[bbudge, 2013/05/06 19:27:50]

Done.

+    ppapi::HostResource* image_data_resource,
+    PP_TimeTicks* timestamp) {
+  scoped_ptr<cricket::VideoFrame> frame(last_frame_.release());
+  int32_t width = static_cast<int32_t>(frame->GetWidth());
+  int32_t height = static_cast<int32_t>(frame->GetHeight());

[Tom Sepez, 2013/05/06 17:19:35]

How do we know these don't overflow when you cast a size_t to int32_t?

[bbudge, 2013/05/06 19:27:50]

I'm assuming they're relatively small integers. I'll use checked casts.
Done.
On 2013/05/06 17:19:35, Tom Sepez wrote:

   // Create an image data resource to hold the frame pixels.
   PP_ImageDataDesc desc;
   IPC::PlatformFileForTransit image_handle;
@@ -76,8 +82,8 @@ int32_t PepperVideoSourceHost::OnHostMsgGetFrame(
       ppapi::ScopedPPResource::PassRef(),
       ppapi::proxy::PPB_ImageData_Proxy::CreateImageData(
           pp_instance(),
-          webkit::ppapi::PPB_ImageData_Impl::GetNativeImageDataFormat(),
-          PP_MakeSize(0, 0),
+          PP_IMAGEDATAFORMAT_BGRA_PREMUL,
+          PP_MakeSize(width, height),
           false /* init_to_zero */,
           false /* is_nacl_plugin */,
           &desc, &image_handle, &byte_count));
@@ -95,21 +101,81 @@ int32_t PepperVideoSourceHost::OnHostMsgGetFrame(
   if (!mapper.is_valid())
     return PP_ERROR_FAILED;
 
-  // TODO(ronghuawu) Copy frame pixels to canvas.
+  const SkBitmap* bitmap = image_data->GetMappedBitmap();
+  if (!bitmap)
+    return PP_ERROR_FAILED;
+  uint8_t* bitmap_pixels = static_cast<uint8_t*>(bitmap->getPixels());
+  if (!bitmap_pixels)
+    return PP_ERROR_FAILED;
 
-  ppapi::HostResource image_data_resource;
-  image_data_resource.SetHostResource(pp_instance(), resource.get());
-  double timestamp = 0;
-  reply_context.params.set_result(PP_OK);
+  size_t bitmap_size = bitmap->getSize();
+  frame->ConvertToRgbBuffer(cricket::FOURCC_BGRA,
+                            bitmap_pixels,
+                            bitmap_size,
+                            bitmap->rowBytes());
+
+  image_data_resource->SetHostResource(pp_instance(), resource.get());
+  *timestamp = static_cast<PP_TimeTicks>(frame->GetTimeStamp()) /
+               talk_base::kNumNanosecsPerSec;

[raymes1, 2013/05/06 16:00:11]

nit: 4 space indent

[bbudge, 2013/05/06 19:27:50]

Done.

+  return PP_OK;
+}
+
+void PepperVideoSourceHost::SendFrame(
+    const ppapi::HostResource& image_data_resource,
+    PP_TimeTicks timestamp,
+    int32_t result) {
+  reply_context_.params.set_result(result);
   host()->SendReply(

[yzshen1, 2013/05/06 16:47:28]

host is not guaranteed to be thread safe as well.

[bbudge, 2013/05/06 19:27:50]

Done.

-      reply_context,
+      reply_context_,
       PpapiPluginMsg_VideoSource_GetFrameReply(image_data_resource, timestamp));
-  last_timestamp_ = timestamp;
+
+  reply_context_ = ppapi::host::ReplyMessageContext();
+  get_frame_pending_ = false;
+}
+
+int32_t PepperVideoSourceHost::OnHostMsgOpen(HostMessageContext* context,
+                                             const std::string& stream_url) {
+  GURL gurl(stream_url);
+  if (!gurl.is_valid())
+    return PP_ERROR_BADARGUMENT;
+
+  stream_url_ = gurl.spec();

[yzshen1, 2013/05/06 16:47:28]

nit: better to move it below the failure return at line 144.

[bbudge, 2013/05/06 19:27:50]

Done.

+  if (!source_handler_->Open(stream_url_, this))
+    return PP_ERROR_BADARGUMENT;
+
+  ReplyMessageContext reply_context = context->MakeReplyMessageContext();
+  reply_context.params.set_result(PP_OK);
+  host()->SendReply(reply_context, PpapiPluginMsg_VideoSource_OpenReply());
+  return PP_OK_COMPLETIONPENDING;
+}
+
+int32_t PepperVideoSourceHost::OnHostMsgGetFrame(
+    HostMessageContext* context) {

[raymes1, 2013/05/06 16:00:11]

Should you check that Open() was successful before allowing this to succeed? Or
maybe it doesn't matter.

[bbudge, 2013/05/06 19:27:50]

Done.

+  if (get_frame_pending_)
+    return PP_ERROR_INPROGRESS;
+
+  reply_context_ = context->MakeReplyMessageContext();
+  get_frame_pending_ = true;

[Ronghua Wu (Left Chromium), 2013/05/06 04:42:07]

You need to protect get_frame_pending_ as well. Move the lock to the beginning
of the method.

[bbudge, 2013/05/06 19:27:50]

Done.

+
+  // If a frame is ready, try to convert it and reply.
+  base::AutoLock lock(lock_);
+  if (last_frame_.get()) {
+    ppapi::HostResource image_data_resource;
+    PP_TimeTicks timestamp = 0;
+    int32_t result = ConvertFrame(&image_data_resource, &timestamp);
+    if (result == PP_OK) {
+      SendFrame(image_data_resource, timestamp, result);
+    } else {
+      reply_context_ = ppapi::host::ReplyMessageContext();
+      return result;
+    }
+  }
+
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PepperVideoSourceHost::OnHostMsgClose(HostMessageContext* context) {
-  // TODO(ronghuawu) Close the video stream.
+  source_handler_->Close(stream_url_, this);

[raymes, 2013/05/06 05:11:33]

Do we need to worry about the case where the resource is destroyed before
Close() is called? Should this be run on destruction as well?

[bbudge, 2013/05/06 19:27:50]

I'm not sure whether the deletion of VideoSourceHandler closes, so I added a
Close method and call it from the destructor.
On 2013/05/06 05:11:33, raymes wrote:

   return PP_OK;
 }