Add VideoCaptureImpl

content/renderer/media/video_capture_impl.h

+// Copyright (c) 2011 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.
+
+#ifndef CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_H_
+#define CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_H_
+
+#include <list>
+#include <map>
+
+#include "base/message_loop_proxy.h"
+#include "base/synchronization/lock.h"
+#include "content/renderer/video_capture_message_filter.h"
+#include "media/base/callback.h"
+#include "media/base/message_loop_factory.h"
+#include "media/video/capture/video_capture.h"
+#include "ui/gfx/surface/transport_dib.h"
+
+class VideoCaptureImpl

[scherkus (not reviewing), 2011/05/14 23:33:42]

class level comment

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+    : public media::VideoCapture,
+      public VideoCaptureMessageFilter::Delegate {
+ private:

[scherkus (not reviewing), 2011/05/14 23:33:42]

nit: public section goes first

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+  enum State {
+    kStarted,
+    kStopping,
+    kStopped
+  };
+
+  struct DIBBuffer {
+   public:
+    DIBBuffer() {}

[scherkus (not reviewing), 2011/05/14 23:33:42]

do you need a default ctor?

otherwise it seems we'll have uninitialized memory (dib) and the dtor will
delete it

[wjia(left Chromium), 2011/05/17 04:00:25]

default ctor is removed.

On 2011/05/14 23:33:42, scherkus wrote:

+    DIBBuffer(TransportDIB* d, media::VideoCapture::VideoFrameBuffer* ptr)

[scherkus (not reviewing), 2011/05/14 23:33:42]

please de-inline the ctor/dtor since this class contains non-trivial data
members (i.e., scoped_refptr)

this results in inlining template instantiation code at every new/delete site

see:
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+        : dib(d),
+          mapped_memory(ptr) {
+    }
+    ~DIBBuffer() {
+      delete dib;
+    }
+
+    TransportDIB* dib;
+    scoped_refptr<media::VideoCapture::VideoFrameBuffer> mapped_memory;
+  };
+  typedef std::list<DIBBuffer*> CachedDIB;

[scherkus (not reviewing), 2011/05/14 23:33:42]

nit: I'd move this typedef down to where you declare the variable

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+
+  typedef std::map<media::VideoCapture::EventHandler*, VideoCaptureCapability>

[scherkus (not reviewing), 2011/05/14 23:33:42]

ditto

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+      ClientInfo;
+
+ public:
+  // media::VideoCapture interface

[scherkus (not reviewing), 2011/05/14 23:33:42]

add period

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+  virtual void StartCapture(media::VideoCapture::EventHandler* handler,
+                            const VideoCaptureCapability& capability);
+  virtual void StopCapture(media::VideoCapture::EventHandler* handler);
+  virtual bool CaptureStarted() { return state_ == kStarted; }
+  virtual int CaptureWidth() { return width_; }
+  virtual int CaptureHeight() { return height_; }
+  virtual int CaptureFrameRate() { return frame_rate_; }

[scherkus (not reviewing), 2011/05/14 23:33:42]

please de-inline all of these methods

virtual methods shouldn't be inlined, see:
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+
+  // VideoCaptureMessageFilter::Delegate interface

[scherkus (not reviewing), 2011/05/14 23:33:42]

add period

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+  virtual void OnBufferReceived(TransportDIB::Handle handle,
+                                base::Time timestamp);
+  virtual void OnStateChanged(const media::VideoCapture::State& state);
+  virtual void OnDeviceInfoReceived(
+      const media::VideoCaptureParams& device_info);
+
+  bool pending_start() {
+    return (new_width_ > 0 && new_height_ > 0);
+  }
+
+ protected:

[scherkus (not reviewing), 2011/05/14 23:33:42]

do you intend to further extend this class?

[wjia(left Chromium), 2011/05/17 04:00:25]

no. moved them to private.

+  friend class VideoCaptureImplManager;
+
+  VideoCaptureImpl(media::VideoCaptureSessionId id,
+                   scoped_refptr<base::MessageLoopProxy> ml_proxy,
+                   VideoCaptureMessageFilter* filter);
+  virtual ~VideoCaptureImpl();
+
+ private:
+  void Init();
+  void DeInit(Task* task);
+  void StopDevice();
+  void RestartCapture();
+  void StartCaptureInternal();
+  void AddDelegateOnIOThread();
+  void RemoveDelegateOnIOThread(Task* task);
+
+  scoped_refptr<VideoCaptureMessageFilter> filter_;
+  media::VideoCaptureSessionId session_id_;
+  scoped_refptr<base::MessageLoopProxy> ml_proxy_;
+  int device_id_;
+
+  // Pool of DIBs.
+  CachedDIB cached_dibs_;
+
+  ClientInfo clients_;
+  std::list<media::VideoCapture::EventHandler*> master_clients_;
+
+  int width_;
+  int height_;
+  int frame_rate_;
+  media::VideoFrame::Format video_type_;
+
+  int new_width_;
+  int new_height_;
+  State state_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoCaptureImpl);
+};
+
+DISABLE_RUNNABLE_METHOD_REFCOUNT(VideoCaptureImpl);
+
+class VideoCaptureImplManager {

[scherkus (not reviewing), 2011/05/14 23:33:42]

please move to separate file + class-level comment

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+ public:
+  VideoCaptureImplManager();
+  virtual ~VideoCaptureImplManager() {}
+
+  static media::VideoCapture* AddDevice(
+      media::VideoCaptureSessionId id,
+      media::VideoCapture::EventHandler* handler);
+  static void RemoveDevice(media::VideoCaptureSessionId id,
+                           media::VideoCapture::EventHandler* handler);
+
+  static VideoCaptureImplManager* GetInstance();
+
+ private:
+  struct Device {
+    Device() : vc(NULL) {}

[scherkus (not reviewing), 2011/05/14 23:33:42]

please de-inline ctor/dtor since struct contains non-trivial data members

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+    Device(VideoCaptureImpl* device, media::VideoCapture::EventHandler* handler)
+        : vc(device) {
+      clients.push_front(handler);
+    }
+
+    VideoCaptureImpl* vc;

[scherkus (not reviewing), 2011/05/14 23:33:42]

sanity check: should Device own this pointer?

[wjia(left Chromium), 2011/05/17 04:00:25]

yes, vc is owned by Device, and essentially managed by ImplManager.

+    std::list<media::VideoCapture::EventHandler*> clients;
+  };
+  typedef std::map<media::VideoCaptureSessionId, Device> Devices;

[scherkus (not reviewing), 2011/05/14 23:33:42]

nit: move to variable declaration

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+
+  void FreeDevice(VideoCaptureImpl* vc);
+
+  Devices devices_;
+  base::Lock lock_;
+  scoped_refptr<base::MessageLoopProxy> ml_proxy_;
+  scoped_ptr<media::MessageLoopFactory> ml_factory_;
+};

[scherkus (not reviewing), 2011/05/14 23:33:42]

DISALLOW_ etc....

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+
+DISABLE_RUNNABLE_METHOD_REFCOUNT(VideoCaptureImplManager);
+
+#endif  // CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_H_