Split FakeVideoCaptureDevice into classes with single responsibility

media/capture/video/fake_video_capture_device.h

Index: media/capture/video/fake_video_capture_device.h
diff --git a/media/capture/video/fake_video_capture_device.h b/media/capture/video/fake_video_capture_device.h
index 3fc05d070de8a0d95ff1ea7575c470fbab6ec958..c8e5487b2c838439ff2971d46b5c4d817d699792 100644
--- a/media/capture/video/fake_video_capture_device.h
+++ b/media/capture/video/fake_video_capture_device.h
@@ -2,9 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Implementation of a fake VideoCaptureDevice class. Used for testing other
-// video capture classes when no real hardware is available.
-
 #ifndef MEDIA_CAPTURE_VIDEO_FAKE_VIDEO_CAPTURE_DEVICE_H_
 #define MEDIA_CAPTURE_VIDEO_FAKE_VIDEO_CAPTURE_DEVICE_H_
 
@@ -22,18 +19,155 @@
 
 namespace media {
 
+class FakeVideoCaptureDevice;
+
+// Encapsulates factory logic to make a working FakeVideoCaptureDevice based
+// on a given target OutputMode and frame rate.
+class CAPTURE_EXPORT FakeVideoCaptureDeviceMaker {

[mcasas, 2017/01/12 03:19:16]

s/FakeVideoCaptureDeviceMaker/FakeVideoCaptureDeviceBuilder/?
Maybe too java-ish but perhaps convey intention better than
Maker.  To follow that Java pattern, the method would be call
Build():
https://en.wikipedia.org/wiki/Builder_pattern 

[chfremer, 2017/01/18 01:29:52]

Actually, I intentionally avoided calling it Builder to avoid confusion with the
Builder pattern, which is not being used here. This class merely exposes a
single static factory method. I guess we could drop the class wrapper and expose
the naked method in the namespace directly. Would you prefer that? Or would you
still prefer ...Builder? I would be okay with either.

[mcasas, 2017/02/15 00:44:19]

Well, if it's a Factory-like thing it should be called
Factory, but since there is a Factory, isn't that a
sign of code being repeated? Maybe some code could be
moved to the Factory.

[chfremer, 2017/02/15 01:30:11]

I would say it is a sign of delegation happening, but not 
a sign of duplication. The thing currently called 
FakeVideoCaptureDeviceFactory delegates to this new class 
for part of the process of fulfilling the CreateDevice() 
method contract. But each class is responsible for a 
different part of that process. 

FakeVideoCaptureDeviceFactory is responsible for knowing 
which DeliveryMode to use and determining which 
VideoPixelFormat to choose. FakeVideoCaptureDeviceMaker 
is responsible for creating instances of 
FakeVideoCaptureDevice using this information. To do this,
it has to instantiate and wire up lots of private parts 
that FakeVideoCaptureDeviceFactory should not need to know 
about.

We can only name one of them 
FakeVideoCaptureDeviceFactory, and for simplicity I would 
opt for letting that name stay with the class that 
currently already has it. The name 
FakeVideoCaptureDeviceMaker is obviously not great.
As long as we can keep the two separate, I am open to 
any alternative naming suggestions.

+ public:
+  enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS };

[mcasas, 2017/01/12 03:19:16]

I see what you mean with DeliveryMode, but the entries
are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/

[chfremer, 2017/01/18 01:29:52]

I chose DeliveryMode as a name to highlight the fact that the different options
use different sub-APIs (of the VideoCaptureDevice::Client interface) to deliver
the frames. Who has ownership of the buffers seems secondary to that. When I
first read the code and saw BufferOwnership, I had no idea what it means, so I
was hoping this rename would be an improvement.

[mcasas, 2017/02/15 00:44:19]

Hmm makes sense, use the API naming and not the
internal detail.  In that case, s/OWN/DEVICE/?
or s/OWN/INTERNAL/ ?

[chfremer, 2017/02/15 01:30:11]

Excellent suggestion. For maximum clarity I'd like to include both words, and
call it USE_DEVICE_INTERNAL_BUFFERS and USE_CLIENT_PROVIDED_BUFFERS.

[mcasas, 2017/02/15 18:23:46]

ok!!

+
+  static std::unique_ptr<VideoCaptureDevice> MakeInstance(
+      VideoPixelFormat pixel_format,
+      DeliveryMode delivery_mode,
+      float frame_rate);
+};
+
+// Represents the current state of a FakeVideoCaptureDevice.
+// This is a separate struct because read-access to it is shared with several
+// collaborating classes.
+struct CAPTURE_EXPORT FakeDeviceState {

[mcasas, 2017/01/12 03:19:16]

Classes only used inside a compilation/linking unit don't 
need CAPTURE_EXPORT, so please remove them where
unnecessary.

[chfremer, 2017/01/18 01:29:52]

Done.

+  FakeDeviceState(float zoom, float frame_rate, VideoPixelFormat pixel_format)
+      : zoom(zoom),
+        format(gfx::Size(), frame_rate, pixel_format, PIXEL_STORAGE_CPU) {}
+
+  uint32_t zoom;
+  VideoCaptureFormat format;
+};
+
+// Paints a frame into the given |target_buffer|.
+class CAPTURE_EXPORT FramePainter {
+ public:
+  virtual void PaintFrame(base::TimeDelta elapsed_time,
+                          uint8_t* target_buffer) = 0;
+};
+
+// Paints a "pacman-like" animated circle including textual information such
+// as a frame count and timer.
+class CAPTURE_EXPORT PacmanFramePainter : public FramePainter {
+ public:
+  // Currently, only the following values are supported for |pixel_format|:
+  //   PIXEL_FORMAT_I420
+  //   PIXEL_FORMAT_Y16
+  //   PIXEL_FORMAT_ARGB
+  PacmanFramePainter(VideoPixelFormat pixel_format,
+                     const FakeDeviceState* fake_device_state);
+
+  // Implementation of FramePainter
+  void PaintFrame(base::TimeDelta elapsed_time,
+                  uint8_t* target_buffer) override;
+
+ private:
+  void DrawGradientSquares(base::TimeDelta elapsed_time,
+                           uint8_t* target_buffer);
+
+  void DrawPacman(base::TimeDelta elapsed_time, uint8_t* target_buffer);
+
+  const VideoPixelFormat pixel_format_;
+  const FakeDeviceState* fake_device_state_ = nullptr;
+};
+
+// Delivers frames to a client, which is set via Initialize().
+class CAPTURE_EXPORT FrameDeliveryStrategy {
+ public:
+  virtual ~FrameDeliveryStrategy() {}
+  virtual void Initialize(VideoPixelFormat pixel_format,
+                          std::unique_ptr<VideoCaptureDevice::Client> client,
+                          const FakeDeviceState* device_state) = 0;
+  virtual void Uninitialize() = 0;
+  virtual uint8_t* PrepareBufferForNextFrame() = 0;
+  virtual void DeliverFrame() = 0;
+};
+
+// Delivers frames using its own buffers via OnIncomingCapturedData().
+class CAPTURE_EXPORT OwnBufferFrameDeliveryStrategy
+    : public FrameDeliveryStrategy {
+ public:
+  OwnBufferFrameDeliveryStrategy();
+  ~OwnBufferFrameDeliveryStrategy() override;
+
+  // Implementation of FrameDeliveryStrategy
+  void Initialize(VideoPixelFormat pixel_format,
+                  std::unique_ptr<VideoCaptureDevice::Client> client,
+                  const FakeDeviceState* device_state) override;
+  void Uninitialize() override;
+  uint8_t* PrepareBufferForNextFrame() override;
+  void DeliverFrame() override;
+
+ private:
+  const FakeDeviceState* device_state_ = nullptr;
+  std::unique_ptr<uint8_t[]> buffer_;
+  std::unique_ptr<VideoCaptureDevice::Client> client_;
+  // The system time when we receive the first frame.
+  base::TimeTicks first_ref_time_;
+};
+
+// Delivers frames using buffers provided by the client via
+// OnIncomingCapturedBuffer().
+class CAPTURE_EXPORT ClientBufferFrameDeliveryStrategy
+    : public FrameDeliveryStrategy {
+ public:
+  ClientBufferFrameDeliveryStrategy();
+  ~ClientBufferFrameDeliveryStrategy() override;
+
+  // Implementation of FrameDeliveryStrategy
+  void Initialize(VideoPixelFormat pixel_format,
+                  std::unique_ptr<VideoCaptureDevice::Client> client,
+                  const FakeDeviceState* device_state) override;
+  void Uninitialize() override;
+  uint8_t* PrepareBufferForNextFrame() override;
+  void DeliverFrame() override;
+
+ private:
+  const FakeDeviceState* device_state_ = nullptr;
+  std::unique_ptr<VideoCaptureDevice::Client> client_;
+  std::unique_ptr<VideoCaptureDevice::Client::Buffer> capture_buffer_;
+  // The system time when we receive the first frame.
+  base::TimeTicks first_ref_time_;

[mcasas, 2017/01/12 03:19:16]

 |device_state_|, |client_| and |first_ref_time_| are
both in ClientBufferFrameDeliveryStrategy and 
OwnBufferFrameDeliveryStrategy, shouldn't they
be moved up to FrameDeliveryStrategy ?

[chfremer, 2017/01/18 01:29:52]

In general, I am trying to avoid inheritance of base classes other than pure
interfaces, because inheritance of base classes causes coupling which leads to
many downsides when scales get larger. I guess in this small case it does not
really hurt that much (other than my idealism).

Done.

+};
+
+// Implements the photo functionality of a VideoCaptureDevice
+class CAPTURE_EXPORT FakePhotoDevice {

[mcasas, 2017/01/12 03:19:16]

All these classes would be better as inner classes of FakeVCD,
and, except FakeVideoCaptureDeviceBuilder, try to 
forward declare them and defer their definition to the 
.cc file.

[chfremer, 2017/01/18 01:29:52]

I don't see what the advantage of nesting classes would be here.
The whole point of this refactoring is to separate things into self-contained
units. Wouldn't nesting everything into FakeVCD then couple everything back
together?
Good point. I moved everything not needed in the header to the cc.

+ public:
+  FakePhotoDevice(std::unique_ptr<FramePainter> argb_painter,
+                  const FakeDeviceState* fake_device_state);
+  ~FakePhotoDevice();
+
+  void GetPhotoCapabilities(
+      VideoCaptureDevice::GetPhotoCapabilitiesCallback callback);
+  void TakePhoto(VideoCaptureDevice::TakePhotoCallback callback,
+                 base::TimeDelta elapsed_time);
+
+ private:
+  const std::unique_ptr<FramePainter> argb_painter_;
+  const FakeDeviceState* const fake_device_state_;
+};
+
+// Implementation of VideoCaptureDevice that generates test frames. This is
+// useful for testing the video capture components without having to use real
+// devices. The implementation schedules delayed tasks to itself to generate and
+// deliver frames at the requested rate.
 class CAPTURE_EXPORT FakeVideoCaptureDevice : public VideoCaptureDevice {
  public:
-  enum class BufferOwnership {
-    OWN_BUFFERS,
-    CLIENT_BUFFERS,
-  };
-
-  FakeVideoCaptureDevice(BufferOwnership buffer_ownership,
-                         float fake_capture_rate,
-                         VideoPixelFormat pixel_format = PIXEL_FORMAT_I420);
+  FakeVideoCaptureDevice(
+      std::unique_ptr<FramePainter> frame_painter,
+      std::unique_ptr<FrameDeliveryStrategy> frame_delivery_strategy,
+      std::unique_ptr<FakePhotoDevice> photo_device,
+      std::unique_ptr<FakeDeviceState> device_state);
   ~FakeVideoCaptureDevice() override;
 
+  static void GetSupportedSizes(std::vector<gfx::Size>* supported_sizes);
+
   // VideoCaptureDevice implementation.
   void AllocateAndStart(const VideoCaptureParams& params,
                         std::unique_ptr<Client> client) override;
@@ -44,35 +178,24 @@ class CAPTURE_EXPORT FakeVideoCaptureDevice : public VideoCaptureDevice {
   void TakePhoto(TakePhotoCallback callback) override;
 
  private:
-  void CaptureUsingOwnBuffers(base::TimeTicks expected_execution_time);
-  void CaptureUsingClientBuffers(base::TimeTicks expected_execution_time);
-  void BeepAndScheduleNextCapture(
-      base::TimeTicks expected_execution_time,
-      const base::Callback<void(base::TimeTicks)>& next_capture);
-
-  // |thread_checker_| is used to check that all methods are called in the
-  // correct thread that owns the object.
-  base::ThreadChecker thread_checker_;
+  static gfx::Size SnapToSupportedSize(const gfx::Size& requested_size);

[mcasas, 2017/01/12 03:19:16]

This doesn't need to be static in the class, can be
a static function in the .cc file.

[chfremer, 2017/01/18 01:29:52]

Done.

+  void BeepAndScheduleNextCapture(base::TimeTicks expected_execution_time);
+  void OnNextFrameDue(base::TimeTicks expected_execution_time, int session_id);
 
-  const BufferOwnership buffer_ownership_;
-  // Frame rate of the fake video device.
-  const float fake_capture_rate_;
-  // Pixel format of all device streams.
-  const VideoPixelFormat pixel_format_;
+  const std::unique_ptr<FramePainter> frame_painter_;
+  const std::unique_ptr<FrameDeliveryStrategy> frame_delivery_strategy_;
+  const std::unique_ptr<FakePhotoDevice> photo_device_;
+  const std::unique_ptr<FakeDeviceState> device_state_;
+  bool device_running_ = false;
+  int current_session_id_ = 0;
 
-  std::unique_ptr<VideoCaptureDevice::Client> client_;
-  // |fake_frame_| is used for capturing on Own Buffers.
-  std::unique_ptr<uint8_t[]> fake_frame_;
   // Time when the next beep occurs.
   base::TimeDelta beep_time_;
   // Time since the fake video started rendering frames.
   base::TimeDelta elapsed_time_;
-  VideoCaptureFormat capture_format_;
 
-  double current_zoom_;
+  base::ThreadChecker thread_checker_;
 
-  // The system time when we receive the first frame.
-  base::TimeTicks first_ref_time_;
   // FakeVideoCaptureDevice post tasks to itself for frame construction and
   // needs to deal with asynchronous StopAndDeallocate().
   base::WeakPtrFactory<FakeVideoCaptureDevice> weak_factory_;