Pepper: PPB_VideoEncoder implementation

ppapi/proxy/video_encoder_resource.cc

Index: ppapi/proxy/video_encoder_resource.cc
diff --git a/ppapi/proxy/video_encoder_resource.cc b/ppapi/proxy/video_encoder_resource.cc
index 5d7913d71c985b6204d1e32af23f9792e7585116..f0c46af3d8e8e9893215d5b0ca75f9b0c1f166ad 100644
--- a/ppapi/proxy/video_encoder_resource.cc
+++ b/ppapi/proxy/video_encoder_resource.cc
@@ -2,19 +2,78 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/shared_memory.h"
+#include "ppapi/c/pp_array_output.h"
+#include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/video_encoder_resource.h"
+#include "ppapi/proxy/video_frame_resource.h"
+#include "ppapi/shared_impl/media_stream_buffer.h"
+#include "ppapi/shared_impl/media_stream_buffer_manager.h"
+#include "ppapi/thunk/enter.h"
 
+using ppapi::proxy::SerializedHandle;
+using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_VideoEncoder_API;
 
 namespace ppapi {
 namespace proxy {
 
+namespace {
+
+void RunCallback(scoped_refptr<TrackedCallback>* callback, int32_t error) {
+  if (!TrackedCallback::IsPending(*callback))
+    return;
+
+  scoped_refptr<TrackedCallback> temp;
+  callback->swap(temp);
+  temp->Run(error);
+}
+
+}  // namespace
+
+VideoEncoderResource::ShmBuffer::ShmBuffer(base::SharedMemoryHandle handle,
+                                           uint32_t id,
+                                           uint32_t size)
+    : shm(new base::SharedMemory(handle, true)), id(id), size(size) {
+  shm->Map(size);
+}
+
+VideoEncoderResource::ShmBuffer::~ShmBuffer() {
+}
+
+VideoEncoderResource::BitstreamBuffer::BitstreamBuffer(uint32_t id,
+                                                       uint32_t size,
+                                                       bool key_frame)
+    : id(id), size(size), key_frame(key_frame) {
+}
+
+VideoEncoderResource::BitstreamBuffer::BitstreamBuffer(
+    const BitstreamBuffer& other)
+    : id(other.id), size(other.size), key_frame(other.key_frame) {
+}
+
+VideoEncoderResource::BitstreamBuffer::~BitstreamBuffer() {
+}
+
 VideoEncoderResource::VideoEncoderResource(Connection connection,
                                            PP_Instance instance)
-    : PluginResource(connection, instance) {
+    : PluginResource(connection, instance),
+      initialized_(false),
+      // Set |encoder_last_error_| to PP_OK after successful initialization.
+      // This makes error checking a little more concise, since we can check
+      // that the encoder has been initialized and hasn't returned an error by
+      // just testing |encoder_last_error_|.
+      encoder_last_error_(PP_ERROR_FAILED),
+      input_frame_count_(0),
+      input_coded_size_(PP_MakeSize(0, 0)),
+      waiting_for_video_frames_(false),
+      buffer_manager_(this) {

[bbudge, 2015/02/10 22:47:38]

Initialize get_bitstream_buffer_data_ to nullptr here.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+  SendCreate(RENDERER, PpapiHostMsg_VideoEncoder_Create());
 }
 
 VideoEncoderResource::~VideoEncoderResource() {
+  Close();

[bbudge, 2015/02/10 22:47:38]

I think you'll need to check if the plugin also called Close() so it doesn't
happen twice.

[llandwerlin-old, 2015/02/11 18:42:24]

Done.

+  ReleaseFrames();

[bbudge, 2015/02/10 22:47:39]

Could ReleaseFrames just be part of Close()?

[llandwerlin-old, 2015/02/11 18:42:24]

Done.

 }
 
 PPB_VideoEncoder_API* VideoEncoderResource::AsPPB_VideoEncoder_API() {
@@ -24,7 +83,25 @@ PPB_VideoEncoder_API* VideoEncoderResource::AsPPB_VideoEncoder_API() {
 int32_t VideoEncoderResource::GetSupportedProfiles(
     const PP_ArrayOutput& output,
     const scoped_refptr<TrackedCallback>& callback) {
-  return PP_ERROR_FAILED;
+  // TODO(llandwerlin): should we prevent concurrent calls?

[bbudge, 2015/02/10 22:47:38]

Yes, we should prevent that.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+  Call<PpapiPluginMsg_VideoEncoder_GetSupportedProfilesReply>(
+      RENDERER, PpapiHostMsg_VideoEncoder_GetSupportedProfiles(),
+      base::Bind(&VideoEncoderResource::OnPluginMsgGetSupportedProfilesReply,
+                 this, output, callback));
+  return PP_OK_COMPLETIONPENDING;
+}
+
+int32_t VideoEncoderResource::GetFramesRequired() {
+  if (encoder_last_error_)
+    return encoder_last_error_;
+  return input_frame_count_;
+}
+
+int32_t VideoEncoderResource::GetFrameCodedSize(PP_Size* size) {
+  if (encoder_last_error_)
+    return encoder_last_error_;
+  *size = input_coded_size_;
+  return PP_OK;
 }
 
 int32_t VideoEncoderResource::Initialize(
@@ -34,45 +111,297 @@ int32_t VideoEncoderResource::Initialize(
     uint32_t initial_bitrate,
     PP_HardwareAcceleration acceleration,
     const scoped_refptr<TrackedCallback>& callback) {
-  return PP_ERROR_FAILED;
-}
-
-int32_t VideoEncoderResource::GetFramesRequired() {
-  return PP_ERROR_FAILED;
-}
+  if (initialized_)
+    return PP_ERROR_FAILED;
+  if (TrackedCallback::IsPending(initialize_callback_))
+    return PP_ERROR_INPROGRESS;
 
-int32_t VideoEncoderResource::GetFrameCodedSize(PP_Size* size) {
-  return PP_ERROR_FAILED;
+  initialize_callback_ = callback;
+  Call<PpapiPluginMsg_VideoEncoder_InitializeReply>(
+      RENDERER, PpapiHostMsg_VideoEncoder_Initialize(
+                    input_format, *input_visible_size, output_profile,
+                    initial_bitrate, acceleration),
+      base::Bind(&VideoEncoderResource::OnPluginMsgInitializeReply, this));
+  return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t VideoEncoderResource::GetVideoFrame(
     PP_Resource* video_frame,
     const scoped_refptr<TrackedCallback>& callback) {
-  return PP_ERROR_FAILED;
+  if (encoder_last_error_)
+    return encoder_last_error_;
+

[bbudge, 2015/02/10 22:47:38]

You'll need some way to throttle the plugin. As it stands, the plugin can call
this method an unlimited number of times without an error, growing the queue
without bound.

I'd recommend returning PP_ERROR_INPROGRESS if we're waiting for the initial set
of frames (waiting_for_video_frames_ == true) or if we are waiting for an
existing frame to become available.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+  pending_get_video_frame_calls_.push_back(
+      std::make_pair(video_frame, callback));
+
+  // Lazily ask for a shared memory buffer in which video frames are allocated.
+  if (buffer_manager_.number_of_buffers() < 1) {

[bbudge, 2015/02/10 22:47:38]

Condition is clearer as == 0.

[llandwerlin-old, 2015/02/11 18:42:24]

Done.

+    // Check that we haven't ask for the buffers already.

[bbudge, 2015/02/10 22:47:38]

nit: s/ask/asked

[llandwerlin-old, 2015/02/11 18:42:24]

Done.

+    if (!waiting_for_video_frames_) {
+      waiting_for_video_frames_ = true;
+      Call<PpapiPluginMsg_VideoEncoder_GetVideoFramesReply>(
+          RENDERER, PpapiHostMsg_VideoEncoder_GetVideoFrames(),
+          base::Bind(&VideoEncoderResource::OnPluginMsgGetVideoFramesReply,
+                     this));
+    }
+    return PP_OK_COMPLETIONPENDING;
+  }
+
+  NotifyGetVideoFrameCallbacks();
+
+  return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t VideoEncoderResource::Encode(
     PP_Resource video_frame,
     PP_Bool force_keyframe,
     const scoped_refptr<TrackedCallback>& callback) {
-  return PP_ERROR_FAILED;
+  if (encoder_last_error_)
+    return encoder_last_error_;
+

[bbudge, 2015/02/10 22:47:38]

We're not throttling the plugin here, but I don't think the plugin would ever be
able to get enough VideoFrames to spam the proxy, so it's OK.

[llandwerlin-old, 2015/02/11 18:42:25]

I guess it could be good not to resend the same frame to the encoder until it
comes back as done.

+  VideoFrameMap::iterator it = video_frames_.find(video_frame);
+  if (it == video_frames_.end())
+    // TODO(llandwerlin): deal MediaStreamVideoTrack's video frames.

[bbudge, 2015/02/10 22:47:38]

s/deal/accept ?

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+    return PP_ERROR_BADRESOURCE;
+
+  scoped_refptr<VideoFrameResource> frame_resource = it->second;
+
+  Call<PpapiPluginMsg_VideoEncoder_EncodeReply>(
+      RENDERER,
+      PpapiHostMsg_VideoEncoder_Encode(frame_resource->GetBufferIndex(),
+                                       PP_ToBool(force_keyframe)),
+      base::Bind(&VideoEncoderResource::OnPluginMsgEncodeReply, this,
+                 callback));
+
+  return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t VideoEncoderResource::GetBitstreamBuffer(
-    PP_BitstreamBuffer* picture,
+    PP_BitstreamBuffer* bitstream_buffer,
     const scoped_refptr<TrackedCallback>& callback) {
-  return PP_ERROR_FAILED;
+  if (encoder_last_error_)
+    return encoder_last_error_;
+  if (TrackedCallback::IsPending(get_bitstream_buffer_callback_))
+    return PP_ERROR_INPROGRESS;
+
+  if (available_bitstream_buffers_.empty()) {
+    get_bitstream_buffer_callback_ = callback;
+    get_bitstream_buffer_data_ = bitstream_buffer;
+    return PP_OK_COMPLETIONPENDING;
+  }
+
+  BitstreamBuffer buffer(available_bitstream_buffers_.front());
+
+  available_bitstream_buffers_.pop_front();
+  WriteBitstreamBuffer(bitstream_buffer, buffer);
+
+  return PP_OK;
 }
 
 void VideoEncoderResource::RecycleBitstreamBuffer(
-    const PP_BitstreamBuffer* picture) {
+    const PP_BitstreamBuffer* bitstream_buffer) {
+  if (encoder_last_error_)
+    return;
+  BitstreamBufferMap::const_iterator iter =
+      bitstream_buffers_map_.find(bitstream_buffer->buffer);
+  if (iter != bitstream_buffers_map_.end())

[bbudge, 2015/02/10 22:47:38]

Need brackets around multiline block here.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+    Post(RENDERER,
+         PpapiHostMsg_VideoEncoder_RecycleBitstreamBuffer(iter->second));
 }
 
 void VideoEncoderResource::RequestEncodingParametersChange(uint32_t bitrate,
                                                            uint32_t framerate) {
+  if (encoder_last_error_)
+    return;
+  Post(RENDERER, PpapiHostMsg_VideoEncoder_RequestEncodingParametersChange(
+                     bitrate, framerate));

[bbudge, 2015/02/10 22:47:38]

Is this indentation the clang formatter?

[llandwerlin-old, 2015/02/11 18:42:25]

Oops...

 }
 
 void VideoEncoderResource::Close() {
+  if (encoder_last_error_)
+    return;
+  // Synchronously call into the host to make sure all in flight
+  // Encode request are properly cancelled. This will also trigger the
+  // NotifyError callback before SyncCall() returns.
+  SyncCall<PpapiPluginMsg_VideoEncoder_CloseReply>(

[bbudge, 2015/02/10 22:47:38]

We avoid sync calls whenever possible. I don't understand why this is needed,
since you don't handle the reply message. I expect the host to eventually reply
to every message that you sent, so all callbacks will eventually be called.

Is there something I'm missing?

[llandwerlin-old, 2015/02/11 18:42:25]

Yes, just so it's convenient, I'm using Call<>(), so I don't have to keep track
of all Encode(), but there is no guarantee these will be called before Close()
returns.

I'll switching to a map to manage this here.

+      RENDERER, PpapiHostMsg_VideoEncoder_Close());
+}
+
+void VideoEncoderResource::OnReplyReceived(
+    const ResourceMessageReplyParams& params,
+    const IPC::Message& msg) {
+  PPAPI_BEGIN_MESSAGE_MAP(VideoEncoderResource, msg)
+  PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(
+      PpapiPluginMsg_VideoEncoder_BitstreamBufferReady,
+      OnPluginMsgBitstreamBufferReady)
+  PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(PpapiPluginMsg_VideoEncoder_NotifyError,
+                                      OnPluginMsgNotifyError)
+  PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(
+      PluginResource::OnReplyReceived(params, msg))
+  PPAPI_END_MESSAGE_MAP()
+}
+
+void VideoEncoderResource::OnPluginMsgGetSupportedProfilesReply(
+    const PP_ArrayOutput& output,
+    const scoped_refptr<TrackedCallback>& callback,
+    const ResourceMessageReplyParams& params,
+    const std::vector<PP_VideoProfileDescription>& profiles) {
+  void* ptr = output.GetDataBuffer(output.user_data, profiles.size(),
+                                   sizeof(PP_VideoProfileDescription));
+
+  if (!ptr) {
+    callback->Run(PP_ERROR_FAILED);
+    return;
+  }
+
+  if (profiles.size() > 0)
+    memcpy(ptr, &profiles[0],
+           profiles.size() * sizeof(PP_VideoProfileDescription));
+  callback->Run(PP_OK);
+}
+
+void VideoEncoderResource::OnPluginMsgInitializeReply(
+    const ResourceMessageReplyParams& params,
+    uint32_t buffer_length,
+    uint32_t input_frame_count,
+    const PP_Size& input_coded_size) {
+  encoder_last_error_ = params.result();
+  if (encoder_last_error_) {
+    RunCallback(&initialize_callback_, encoder_last_error_);
+    return;
+  }
+
+  std::vector<base::SharedMemoryHandle> shm_handles;
+  params.TakeAllSharedMemoryHandles(&shm_handles);
+
+  shm_buffers_.clear();
+  bitstream_buffers_map_.clear();

[bbudge, 2015/02/10 22:47:38]

These should be empty at this point. Perhaps DCHECKs if you think it likely that
this could break in the future. But then I'd probably DCHECK(!initialized_) at
the beginning instead.

[llandwerlin-old, 2015/02/11 18:42:24]

Done.

+  for (uint32_t i = 0; i < shm_handles.size(); ++i) {
+    ShmBuffer* buffer = new ShmBuffer(shm_handles[i], i, buffer_length);
+    shm_buffers_.push_back(buffer);
+    bitstream_buffers_map_.insert(
+        std::make_pair(buffer->shm->memory(), buffer->id));
+  }
+
+  input_frame_count_ = input_frame_count;
+  input_coded_size_ = input_coded_size;
+  initialized_ = true;
+  RunCallback(&initialize_callback_, encoder_last_error_);
+}
+
+void VideoEncoderResource::OnPluginMsgGetVideoFramesReply(
+    const ResourceMessageReplyParams& params,
+    uint32_t frame_count,
+    uint32_t frame_length,
+    const PP_Size& frame_size) {
+  int32_t error = params.result();
+  if (error) {
+    NotifyError(error);
+    return;
+  }
+
+  std::vector<base::SharedMemoryHandle> buffer_handles;
+  params.TakeAllSharedMemoryHandles(&buffer_handles);

[bbudge, 2015/02/10 22:47:38]

Since this is for a single handle, you could use TakeSharedMemoryHandleAtIndex
instead.
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/gamepa...

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+  CHECK(buffer_handles.size() == 1);

[bbudge, 2015/02/10 22:47:38]

I don't think you need to CHECK for the correct number of handles. This code
runs in a less privileged process than the host.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+
+  if (!buffer_manager_.SetBuffers(
+          frame_count, frame_length,
+          make_scoped_ptr(new base::SharedMemory(buffer_handles[0], false)),
+          true)) {
+    NotifyError(PP_ERROR_FAILED);
+    return;
+  }
+
+  waiting_for_video_frames_ = false;

[bbudge, 2015/02/10 22:47:38]

You might want to make this the first line so it isn't stuck at 'true' after a
failure.

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+
+  NotifyGetVideoFrameCallbacks();
+}
+
+void VideoEncoderResource::OnPluginMsgEncodeReply(
+    const scoped_refptr<TrackedCallback>& callback,
+    const ResourceMessageReplyParams& params,
+    uint32_t frame_id) {
+  encoder_last_error_ = params.result();
+  callback->Run(encoder_last_error_);
+  buffer_manager_->EnqueueBuffer(frame_id);
+  NotifyGetVideoFrameCallbacks();
+}
+
+void VideoEncoderResource::OnPluginMsgBitstreamBufferReady(
+    const ResourceMessageReplyParams& params,
+    uint32_t buffer_id,
+    uint32_t buffer_size,
+    bool key_frame) {
+  available_bitstream_buffers_.push_back(
+      BitstreamBuffer(buffer_id, buffer_size, PP_FromBool(key_frame)));

[bbudge, 2015/02/10 22:47:38]

PP_FromBool isn't needed here.

[llandwerlin-old, 2015/02/11 18:42:25]

What's the purpose of PP_FromBool then?

+
+  if (TrackedCallback::IsPending(get_bitstream_buffer_callback_)) {
+    BitstreamBuffer buffer(available_bitstream_buffers_.front());
+    available_bitstream_buffers_.pop_front();
+    WriteBitstreamBuffer(get_bitstream_buffer_data_, buffer);
+    get_bitstream_buffer_data_ = nullptr;
+
+    scoped_refptr<TrackedCallback> callback;
+    get_bitstream_buffer_callback_.swap(callback);
+    callback->Run(PP_OK);
+  }
+}
+
+void VideoEncoderResource::OnPluginMsgNotifyError(
+    const ResourceMessageReplyParams& params,
+    int32_t error) {
+  NotifyError(error);
+}
+
+void VideoEncoderResource::NotifyError(int32_t error) {
+  encoder_last_error_ = error;
+  RunCallback(&initialize_callback_, error);
+  RunCallback(&get_bitstream_buffer_callback_, error);
+  NotifyGetVideoFrameCallbacksError(error);
+}
+
+void VideoEncoderResource::NotifyGetVideoFrameCallbacks() {
+  int32_t frame_id;
+
+  while (!pending_get_video_frame_calls_.empty() &&
+         (frame_id = buffer_manager_.DequeueBuffer()) >= 0) {
+    std::pair<PP_Resource*, scoped_refptr<TrackedCallback> > cb =
+        pending_get_video_frame_calls_.front();
+    pending_get_video_frame_calls_.pop_front();
+
+    scoped_refptr<VideoFrameResource> resource = new VideoFrameResource(
+        pp_instance(), frame_id, buffer_manager_->GetBufferPointer(frame_id));
+    video_frames_.insert(
+        VideoFrameMap::value_type(resource->pp_resource(), resource));
+
+    *cb.first = resource->GetReference();
+    cb.second->Run(PP_OK);
+  }
+}
+
+void VideoEncoderResource::NotifyGetVideoFrameCallbacksError(int32_t error) {

[bbudge, 2015/02/10 22:47:38]

You could just move this loop into NotifyError. Is there a case where it would
be called separately?

[llandwerlin-old, 2015/02/11 18:42:25]

Done.

+  while (!pending_get_video_frame_calls_.empty()) {
+    std::pair<PP_Resource*, scoped_refptr<TrackedCallback> > cb =
+        pending_get_video_frame_calls_.front();
+    pending_get_video_frame_calls_.pop_front();
+    cb.second->Run(error);
+  }
+}
+
+void VideoEncoderResource::WriteBitstreamBuffer(
+    PP_BitstreamBuffer* bitstream_buffer,
+    const BitstreamBuffer& buffer) {
+  bitstream_buffer->size = buffer.size;
+  bitstream_buffer->buffer = shm_buffers_[buffer.id]->shm->memory();
+  bitstream_buffer->key_frame = PP_FromBool(buffer.key_frame);
+}
+
+void VideoEncoderResource::ReleaseFrames() {
+  for (VideoFrameMap::iterator it = video_frames_.begin();
+       it != video_frames_.end(); ++it) {
+    it->second->Invalidate();
+    it->second = nullptr;
+  }
 }
 
 }  // namespace proxy