Refactor video capture to new design

ppapi/proxy/video_capture_resource.cc

Index: ppapi/proxy/video_capture_resource.cc
diff --git a/ppapi/proxy/video_capture_resource.cc b/ppapi/proxy/video_capture_resource.cc
new file mode 100644
index 0000000000000000000000000000000000000000..21b56eb99e537e131b5aec7e0ec171d3b2885cc3
--- /dev/null
+++ b/ppapi/proxy/video_capture_resource.cc
@@ -0,0 +1,219 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/proxy/video_capture_resource.h"
+
+#include "ppapi/c/dev/ppp_video_capture_dev.h"
+#include "ppapi/proxy/dispatch_reply_message.h"
+#include "ppapi/proxy/plugin_dispatcher.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/proxy/ppb_buffer_proxy.h"
+#include "ppapi/proxy/resource_message_params.h"
+#include "ppapi/shared_impl/tracked_callback.h"
+
+namespace ppapi {
+namespace proxy {
+
+VideoCaptureResource::VideoCaptureResource(
+    Connection connection,
+    PP_Instance instance,
+    PluginDispatcher* dispatcher)
+    : PluginResource(connection, instance),
+      open_state_(BEFORE_OPEN),
+      has_pending_enum_devices_callback_(false) {
+  if (!sent_create_to_renderer())

[yzshen1, 2012/11/06 06:51:40]

You don't need to test this condition.

[victorhsieh, 2012/11/08 09:20:18]

Done.

+    SendCreate(RENDERER, PpapiHostMsg_VideoCapture_Create());
+
+  ppp_video_capture_impl_ = static_cast<const PPP_VideoCapture_Dev*>(
+      dispatcher->local_get_interface()(PPP_VIDEO_CAPTURE_DEV_INTERFACE));
+}
+
+VideoCaptureResource::~VideoCaptureResource() {
+}
+
+void VideoCaptureResource::OnReplyReceived(
+    const ResourceMessageReplyParams& params,
+    const IPC::Message& msg) {
+  if (params.sequence()) {
+    PluginResource::OnReplyReceived(params, msg);
+    return;
+  }
+
+  IPC_BEGIN_MESSAGE_MAP(VideoCaptureResource, msg)
+    PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(
+        PpapiPluginMsg_VideoCapture_OnDeviceInfo,
+        OnDeviceInfo)
+    PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(
+        PpapiPluginMsg_VideoCapture_OnStatus,
+        OnStatus)
+    PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(
+        PpapiPluginMsg_VideoCapture_OnError,
+        OnError)
+    PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL(
+        PpapiPluginMsg_VideoCapture_OnBufferReady,
+        OnBufferReady)
+    PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
+  IPC_END_MESSAGE_MAP()
+}
+
+int32_t VideoCaptureResource::EnumerateDevices(
+    PP_Resource* devices,
+    scoped_refptr<TrackedCallback> callback) {
+  if (has_pending_enum_devices_callback_)
+    return PP_ERROR_INPROGRESS;
+
+  has_pending_enum_devices_callback_ = true;
+
+  Call<PpapiPluginMsg_VideoCapture_EnumerateDevicesReply>(
+      RENDERER,
+      PpapiHostMsg_VideoCapture_EnumerateDevices(),
+      base::Bind(&VideoCaptureResource::OnEnumerateDevicesComplete,
+                 this,
+                 devices,
+                 callback));
+  return PP_OK_COMPLETIONPENDING;
+}
+
+int32_t VideoCaptureResource::Open(
+    const std::string& device_id,
+    const PP_VideoCaptureDeviceInfo_Dev& requested_info,
+    uint32_t buffer_count,
+    scoped_refptr<TrackedCallback> callback) {
+  if (open_state_ != BEFORE_OPEN)
+    return PP_ERROR_FAILED;
+
+  if (TrackedCallback::IsPending(open_callback_))
+    return PP_ERROR_INPROGRESS;
+
+  open_callback_ = callback;
+
+  Call<PpapiPluginMsg_VideoCapture_OpenReply>(
+      RENDERER,
+      PpapiHostMsg_VideoCapture_Open(device_id, requested_info, buffer_count),
+      base::Bind(&VideoCaptureResource::OnOpenComplete, this));
+  return PP_OK_COMPLETIONPENDING;
+}
+
+int32_t VideoCaptureResource::StartCapture() {
+  if (open_state_ != OPENED)
+    return PP_ERROR_FAILED;
+

[yzshen1, 2012/11/06 06:51:40]

I notice that the previous code will do a
SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome here.
Is that not needed any more? Will it cause behavior change of the interface? 

[victorhsieh, 2012/11/08 09:20:18]

SetStatus checks the current state before jumping the the new.  The previous
state allowed of STARTING is STOPPED, which only happens either at the initial
state or any error is happened.

In that case, I add another line of code here in OnMsgOnError to reset the
state.  That should make behavior consistent to the old code.

[yzshen1, 2012/11/10 01:14:40]

What is the reason to keep the status_ state machine at the host side instead of
the plugin side?

On 2012/11/08 09:20:18, Victor Hsieh wrote:

[victorhsieh, 2012/11/13 03:10:53]

The idea to keep status_ on the host when I first did this, is to keep 
OnStarted/OnStopped/etc. still sending status to the plugin only in expected
state.

[yzshen1, 2012/11/14 06:54:16]

IMO, it is easier to ensure the implementation has consistent behavior as the
previous implementation, if status_ is on the other side.
But I am fine with your approach.


On 2012/11/13 03:10:53, Victor Hsieh wrote:

+  buffer_in_use_.clear();
+  Post(RENDERER, PpapiHostMsg_VideoCapture_StartCapture());
+  return PP_OK;
+}
+
+int32_t VideoCaptureResource::ReuseBuffer(uint32_t buffer) {
+  if (buffer >= buffer_in_use_.size() || !buffer_in_use_[buffer])
+    return PP_ERROR_BADARGUMENT;
+  Post(RENDERER, PpapiHostMsg_VideoCapture_ReuseBuffer(buffer));
+  return PP_OK;
+}
+
+int32_t VideoCaptureResource::StopCapture() {
+  if (open_state_ != OPENED)
+    return PP_ERROR_FAILED;
+

[yzshen1, 2012/11/06 06:51:40]

I notice that the previous code will do a
SetStatus(PP_VIDEO_CAPTURE_STATUS_STOPPING, false) and check the outcome here.
Is that not needed any more? Will it cause behavior change of the interface? 

[victorhsieh, 2012/11/08 09:20:18]

Similarly to STARTING, the preceding states that enter STOPPING are STARTING,
STARTED and PAUSED, and that's everything except STOPPED.  The old code returns
PP_ERROR_FAILED if already stopped.  Since status_ is removed, the "if" at the
beginning will cover that.

+  Post(RENDERER, PpapiHostMsg_VideoCapture_StopCapture());
+  return PP_OK;
+}
+
+void VideoCaptureResource::Close() {

[yzshen1, 2012/11/06 06:51:40]

Please make sure the second and subsequent Close() calls are no-ops.
You should put "if (open_state_ == CLOSED) return;" at the beginning.

[victorhsieh, 2012/11/08 09:20:18]

Done.

+  Post(RENDERER, PpapiHostMsg_VideoCapture_Close());
+
+  open_state_ = CLOSED;
+
+  if (TrackedCallback::IsPending(open_callback_))
+    open_callback_->PostAbort();
+}
+
+void VideoCaptureResource::OnDeviceInfo(
+    const ResourceMessageReplyParams& params,
+    const struct PP_VideoCaptureDeviceInfo_Dev& info,
+    const std::vector<HostResource>& buffers,
+    uint32_t buffer_size) {
+  if (!ppp_video_capture_impl_)
+    return;
+
+  std::vector<base::SharedMemoryHandle> handles;
+  params.GetAllSharedMemoryHandles(&handles);
+  DCHECK(handles.size() == buffers.size());

[yzshen1, 2012/11/06 06:51:40]

Please use CHECK().

[victorhsieh, 2012/11/08 09:20:18]

Done.

+
+  scoped_array<PP_Resource> resources(new PP_Resource[buffers.size()]);
+  for (size_t i = 0; i < buffers.size(); ++i) {
+    resources[i] = ppapi::proxy::PPB_Buffer_Proxy::AddProxyResource(

[yzshen1, 2012/11/06 06:51:40]

The resources are leaked!
You should release them after use.
Because according to the document, the plugin is expected to add its own
references to the buffers.
It is your responsibility to release the reference returned by
AddProxyResource().

Please be careful about references. Once we get it wrong, it is hard to find out
later.

[victorhsieh, 2012/11/08 09:20:18]

Done.  Please advise if this is not correct.

+        buffers[i], handles[i], buffer_size);
+  }
+
+  buffer_in_use_ = std::vector<bool>(buffers.size());
+
+  ppp_video_capture_impl_->OnDeviceInfo(
+      pp_instance(),
+      pp_resource(),
+      &info,
+      buffers.size(),
+      resources.get());
+}
+
+void VideoCaptureResource::OnStatus(
+    const ResourceMessageReplyParams& params,
+    uint32_t status) {
+  if (ppp_video_capture_impl_)
+    ppp_video_capture_impl_->OnStatus(pp_instance(), pp_resource(), status);
+}
+
+void VideoCaptureResource::OnError(
+    const ResourceMessageReplyParams& params,
+    uint32_t error_code) {
+  if (ppp_video_capture_impl_)
+    ppp_video_capture_impl_->OnError(pp_instance(), pp_resource(), error_code);
+}
+
+void VideoCaptureResource::OnBufferReady(
+    const ResourceMessageReplyParams& params,
+    uint32_t buffer) {
+  SetBufferInUse(buffer);
+  if (ppp_video_capture_impl_) {
+    ppp_video_capture_impl_->OnBufferReady(pp_instance(), pp_resource(),
+                                           buffer);
+  }
+}
+
+void VideoCaptureResource::OnOpenComplete(
+    const ResourceMessageReplyParams& params) {
+  if (open_state_ == BEFORE_OPEN && params.result() == PP_OK)
+    open_state_ = OPENED;
+
+  // The callback may have been aborted by Close(), or the open operation is
+  // completed synchronously.

[yzshen1, 2012/11/06 06:51:40]

the 'synchronously' part is not correct in this case, please remove it.

[victorhsieh, 2012/11/08 09:20:18]

Done.

+  if (TrackedCallback::IsPending(open_callback_))
+    TrackedCallback::ClearAndRun(&open_callback_, params.result());
+}
+
+void VideoCaptureResource::OnEnumerateDevicesComplete(
+    PP_Resource* devices_output,
+    scoped_refptr<TrackedCallback> callback,
+    const ResourceMessageReplyParams& params,
+    const std::vector<DeviceRefData>& devices) {
+  DCHECK(has_pending_enum_devices_callback_);

[yzshen1, 2012/11/06 06:51:40]

- you should set has_pending_enum_devices_callback_ to false in this method.

- you should test whether |callback| is pending. It may have been aborted, if
the last plugin reference to this resource has gone away. In that case, we must
not access |devices_output|. Because it may be invalid already.

[victorhsieh, 2012/11/08 09:20:18]

Done.

+
+  if (params.result() == PP_OK && devices_output) {
+    devices_data_ = devices;
+
+    // devices_output points to the resource array of PP_ArrayOutput allocated
+    // in VideoCapture_Dev.

[yzshen1, 2012/11/06 06:51:40]

This is not accurate. Users can use c interface directly, it may not be from the
cpp wrapper.

[victorhsieh, 2012/11/08 09:20:18]

Done.

+    *devices_output = PPB_DeviceRef_Shared::CreateResourceArray(
+        OBJECT_IS_PROXY, pp_instance(), devices);
+  }
+
+  TrackedCallback::ClearAndRun(&callback, params.result());
+}
+
+void VideoCaptureResource::SetBufferInUse(uint32_t buffer_index) {
+  DCHECK(buffer_index < buffer_in_use_.size());
+  buffer_in_use_[buffer_index] = true;
+}
+
+}  // namespace proxy
+}  // namespace ppapi