Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device configurations

media/capture/video/fake_video_capture_device.cc

Index: media/capture/video/fake_video_capture_device.cc
diff --git a/media/capture/video/fake_video_capture_device.cc b/media/capture/video/fake_video_capture_device.cc
index 7e46f68cab708272d74618b7b1b964b765df1556..e899464164485b20a98e97a941570a9b4a6e3210 100644
--- a/media/capture/video/fake_video_capture_device.cc
+++ b/media/capture/video/fake_video_capture_device.cc
@@ -29,6 +29,7 @@
 namespace media {
 
 namespace {
+
 // Sweep at 600 deg/sec.
 static const float kPacmanAngularVelocity = 600;
 // Beep every 500 ms.
@@ -39,54 +40,76 @@ static const float kGradientFrequency = 1.f / 5;
 static const double kMinZoom = 100.0;
 static const double kMaxZoom = 400.0;
 static const double kZoomStep = 1.0;
-static const double kInitialZoom = 100.0;
-
-static const gfx::Size kSupportedSizesOrderedByIncreasingWidth[] = {
-    gfx::Size(96, 96), gfx::Size(320, 240), gfx::Size(640, 480),
-    gfx::Size(1280, 720), gfx::Size(1920, 1080)};
-static const int kSupportedSizesCount =
-    arraysize(kSupportedSizesOrderedByIncreasingWidth);
-
-static gfx::Size SnapToSupportedSize(const gfx::Size& requested_size) {
-  for (const gfx::Size& supported_size :
-       kSupportedSizesOrderedByIncreasingWidth) {
-    if (requested_size.width() <= supported_size.width())
-      return supported_size;
-  }
-  return kSupportedSizesOrderedByIncreasingWidth[kSupportedSizesCount - 1];
-}
-
-// Represents the current state of a FakeVideoCaptureDevice.
-// This is a separate struct because read-access to it is shared with several
-// collaborating classes.
-struct FakeDeviceState {
-  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;
+enum class PixelFormatMatchType {
+  EXACT,
+  SUPPORTED_THROUGH_CONVERSION,
+  INCOMPATIBLE
 };
 
-// Paints a "pacman-like" animated circle including textual information such
-// as a frame count and timer.
-class PacmanFramePainter {
- public:
-  enum class Format { I420, SK_N32, Y16 };
-  PacmanFramePainter(Format pixel_format,
-                     const FakeDeviceState* fake_device_state);
-
-  void PaintFrame(base::TimeDelta elapsed_time, uint8_t* target_buffer);
+PixelFormatMatchType DetermineFormatMatchType(
+    media::VideoPixelFormat supported_format,
+    media::VideoPixelFormat requested_format) {
+  if (requested_format == media::PIXEL_FORMAT_I420) {

[emircan, 2017/03/01 18:58:58]

Merge to a single if statement.

[chfremer, 2017/03/01 21:55:48]

Done.

+    if (supported_format == media::PIXEL_FORMAT_MJPEG)
+      return PixelFormatMatchType::SUPPORTED_THROUGH_CONVERSION;
+  }
+  return (requested_format == supported_format)
+             ? PixelFormatMatchType::EXACT
+             : PixelFormatMatchType::INCOMPATIBLE;
+}
 
- private:
-  void DrawGradientSquares(base::TimeDelta elapsed_time,
-                           uint8_t* target_buffer);
+bool CurrentFormatMatchIsWorseThanBest(PixelFormatMatchType current_match,
+                                       PixelFormatMatchType best_match) {
+  if (current_match == PixelFormatMatchType::INCOMPATIBLE) {

[emircan, 2017/03/01 18:58:58]

I don't think comparing like this is scalable. Can you just do |current_match >
best_match| since you actually ranked them in declaration?

[chfremer, 2017/03/01 21:55:48]

Done.

+    return best_match == PixelFormatMatchType::EXACT ||
+           best_match == PixelFormatMatchType::SUPPORTED_THROUGH_CONVERSION;
+  }
+  if (current_match == PixelFormatMatchType::SUPPORTED_THROUGH_CONVERSION)
+    return best_match == PixelFormatMatchType::EXACT;
+  return false;
+}
 
-  void DrawPacman(base::TimeDelta elapsed_time, uint8_t* target_buffer);
+int FindIndexOfClosestSupportedFormat(
+    const VideoCaptureFormat& requested_format,
+    const VideoCaptureFormats& supported_formats) {
+  int best_index = 0;
+  PixelFormatMatchType best_format_match = PixelFormatMatchType::INCOMPATIBLE;
+  int best_width_mismatch = std::numeric_limits<int>::max();
+  float best_frame_rate_mismatch = std::numeric_limits<float>::max();
+  for (int i = 0; i < static_cast<int>(supported_formats.size()); i++) {
+    const auto& supported_format = supported_formats[i];
+    PixelFormatMatchType current_format_match = DetermineFormatMatchType(
+        supported_format.pixel_format, requested_format.pixel_format);
+    if (CurrentFormatMatchIsWorseThanBest(current_format_match,
+                                          best_format_match)) {
+      continue;
+    }
+    if (supported_format.frame_size.width() <
+        requested_format.frame_size.width())
+      continue;
+    const int current_width_mismatch = supported_format.frame_size.width() -
+                                       requested_format.frame_size.width();
+    if (current_width_mismatch > best_width_mismatch)
+      continue;
+    const float current_frame_rate_mismatch =
+        std::abs(supported_format.frame_rate - requested_format.frame_rate);
+    if (current_width_mismatch < best_width_mismatch) {
+      best_width_mismatch = current_width_mismatch;
+      best_frame_rate_mismatch = current_frame_rate_mismatch;
+      best_index = i;
+      continue;
+    }
+    DCHECK_EQ(best_frame_rate_mismatch, current_frame_rate_mismatch);
+    if (current_frame_rate_mismatch < best_frame_rate_mismatch) {
+      best_frame_rate_mismatch = current_frame_rate_mismatch;
+      best_index = i;
+    }
+  }
+  return best_index;
+}
 
-  const Format pixel_format_;
-  const FakeDeviceState* fake_device_state_ = nullptr;
-};
+}  // anonymous namespace
 
 // Paints and delivers frames to a client, which is set via Initialize().
 class FrameDeliverer {
@@ -100,10 +123,6 @@ class FrameDeliverer {
     client_ = std::move(client);
     device_state_ = device_state;
   }
-  virtual void Uninitialize() {
-    client_.reset();
-    device_state_ = nullptr;
-  }
   virtual void PaintAndDeliverNextFrame(base::TimeDelta timestamp_to_paint) = 0;
 
  protected:
@@ -134,7 +153,6 @@ class OwnBufferFrameDeliverer : public FrameDeliverer {
   void Initialize(VideoPixelFormat pixel_format,
                   std::unique_ptr<VideoCaptureDevice::Client> client,
                   const FakeDeviceState* device_state) override;
-  void Uninitialize() override;
   void PaintAndDeliverNextFrame(base::TimeDelta timestamp_to_paint) override;
 
  private:
@@ -158,7 +176,6 @@ class JpegEncodingFrameDeliverer : public FrameDeliverer {
   ~JpegEncodingFrameDeliverer() override;
 
   // Implementation of FrameDeliveryStrategy
-  void Uninitialize() override;
   void PaintAndDeliverNextFrame(base::TimeDelta timestamp_to_paint) override;
 
  private:
@@ -166,129 +183,57 @@ class JpegEncodingFrameDeliverer : public FrameDeliverer {
   std::vector<unsigned char> jpeg_buffer_;
 };
 
-// Implements the photo functionality of a VideoCaptureDevice
-class FakePhotoDevice {
- public:
-  FakePhotoDevice(std::unique_ptr<PacmanFramePainter> 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<PacmanFramePainter> 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 FakeVideoCaptureDevice : public VideoCaptureDevice {
- public:
-  FakeVideoCaptureDevice(
-      std::unique_ptr<FrameDeliverer> frame_delivery_strategy,
-      std::unique_ptr<FakePhotoDevice> photo_device,
-      std::unique_ptr<FakeDeviceState> device_state);
-  ~FakeVideoCaptureDevice() override;
-
-  // VideoCaptureDevice implementation.
-  void AllocateAndStart(const VideoCaptureParams& params,
-                        std::unique_ptr<Client> client) override;
-  void StopAndDeAllocate() override;
-  void GetPhotoCapabilities(GetPhotoCapabilitiesCallback callback) override;
-  void SetPhotoOptions(mojom::PhotoSettingsPtr settings,
-                       SetPhotoOptionsCallback callback) override;
-  void TakePhoto(TakePhotoCallback callback) override;
-
- private:
-  void BeepAndScheduleNextCapture(base::TimeTicks expected_execution_time);
-  void OnNextFrameDue(base::TimeTicks expected_execution_time, int session_id);
-
-  const std::unique_ptr<FrameDeliverer> frame_deliverer_;
-  const std::unique_ptr<FakePhotoDevice> photo_device_;
-  const std::unique_ptr<FakeDeviceState> device_state_;
-  int current_session_id_ = 0;
-
-  // Time when the next beep occurs.
-  base::TimeDelta beep_time_;
-  // Time since the fake video started rendering frames.
-  base::TimeDelta elapsed_time_;
-
-  base::ThreadChecker thread_checker_;
-
-  // FakeVideoCaptureDevice post tasks to itself for frame construction and
-  // needs to deal with asynchronous StopAndDeallocate().
-  base::WeakPtrFactory<FakeVideoCaptureDevice> weak_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeVideoCaptureDevice);
-};
-
-}  // anonymous namespace
-
-// static
-void FakeVideoCaptureDeviceMaker::GetSupportedSizes(
-    std::vector<gfx::Size>* supported_sizes) {
-  for (int i = 0; i < kSupportedSizesCount; i++)
-    supported_sizes->push_back(kSupportedSizesOrderedByIncreasingWidth[i]);
-}
+FrameDelivererFactory::FrameDelivererFactory(
+    FakeVideoCaptureDevice::DeliveryMode delivery_mode,
+    const FakeDeviceState* device_state)
+    : delivery_mode_(delivery_mode), device_state_(device_state) {}
 
-// static
-std::unique_ptr<VideoCaptureDevice> FakeVideoCaptureDeviceMaker::MakeInstance(
-    PixelFormat pixel_format,
-    DeliveryMode delivery_mode,
-    float frame_rate) {
-  auto device_state = base::MakeUnique<FakeDeviceState>(
-      kInitialZoom, frame_rate,
-      static_cast<media::VideoPixelFormat>(pixel_format));
+std::unique_ptr<FrameDeliverer> FrameDelivererFactory::CreateFrameDeliverer(
+    const VideoCaptureFormat& format) {
   PacmanFramePainter::Format painter_format;
-  switch (pixel_format) {
-    case PixelFormat::I420:
+  switch (format.pixel_format) {
+    case PIXEL_FORMAT_I420:
       painter_format = PacmanFramePainter::Format::I420;
       break;
-    case PixelFormat::Y16:
+    case PIXEL_FORMAT_Y16:
       painter_format = PacmanFramePainter::Format::Y16;
       break;
-    case PixelFormat::MJPEG:
+    case PIXEL_FORMAT_MJPEG:
       painter_format = PacmanFramePainter::Format::SK_N32;
       break;
+    default:
+      NOTREACHED();
+      painter_format = PacmanFramePainter::Format::I420;
+  }
+  auto frame_painter =
+      base::MakeUnique<PacmanFramePainter>(painter_format, device_state_);
+
+  FakeVideoCaptureDevice::DeliveryMode delivery_mode = delivery_mode_;
+  if (format.pixel_format == PIXEL_FORMAT_MJPEG &&
+      delivery_mode_ ==
+          FakeVideoCaptureDevice::DeliveryMode::USE_CLIENT_PROVIDED_BUFFERS) {
+    DLOG(WARNING) << "PIXEL_FORMAT_MJPEG cannot be used in combination with "
+                  << "USE_CLIENT_PROVIDED_BUFFERS. Switching to "
+                     "USE_DEVICE_INTERNAL_BUFFERS.";

[emircan, 2017/03/01 18:58:58]

We should still return an error here to propagate proper use.

[chfremer, 2017/03/01 21:55:48]

I tried out your suggestions but then ended up reverting.
The reason is that properly supporting an error case for this requires adding
quite a bit of complexity. 

In addition to returning nullptr here, we would have to make sure the
FakeVideoCaptureDeviceFactory::GetSupportedFormats() strips out all MJPEG
formats when using USE_CLIENT_PROVIDED_BUFFERS. And then, we would probably want
to strip those cases out already when they are set in one of the configuration
calls, but it would be bad to do that silently. So we would have to add an error
case to the configuration API. And then we would need unit test cases for all
this.

Considering this, I feel the silent switching to USE_DEVICE_INTERNAL_BUFFERS is
probably the better choice.

[emircan, 2017/03/02 20:28:18]

Sounds good.

+    delivery_mode =
+        FakeVideoCaptureDevice::DeliveryMode::USE_DEVICE_INTERNAL_BUFFERS;
   }
-  auto video_frame_painter =
-      base::MakeUnique<PacmanFramePainter>(painter_format, device_state.get());
 
-  std::unique_ptr<FrameDeliverer> frame_delivery_strategy;
   switch (delivery_mode) {
-    case DeliveryMode::USE_DEVICE_INTERNAL_BUFFERS:
-      if (pixel_format == PixelFormat::MJPEG) {
-        frame_delivery_strategy = base::MakeUnique<JpegEncodingFrameDeliverer>(
-            std::move(video_frame_painter));
+    case FakeVideoCaptureDevice::DeliveryMode::USE_DEVICE_INTERNAL_BUFFERS:
+      if (format.pixel_format == PIXEL_FORMAT_MJPEG) {
+        return base::MakeUnique<JpegEncodingFrameDeliverer>(
+            std::move(frame_painter));
       } else {
-        frame_delivery_strategy = base::MakeUnique<OwnBufferFrameDeliverer>(
-            std::move(video_frame_painter));
-      }
-      break;
-    case DeliveryMode::USE_CLIENT_PROVIDED_BUFFERS:
-      if (pixel_format == PixelFormat::MJPEG) {
-        DLOG(ERROR) << "PixelFormat::MJPEG cannot be used in combination with "
-                    << "USE_CLIENT_PROVIDED_BUFFERS.";
-        return nullptr;
+        return base::MakeUnique<OwnBufferFrameDeliverer>(
+            std::move(frame_painter));
       }
-      frame_delivery_strategy = base::MakeUnique<ClientBufferFrameDeliverer>(
-          std::move(video_frame_painter));
-      break;
+    case FakeVideoCaptureDevice::DeliveryMode::USE_CLIENT_PROVIDED_BUFFERS:
+      return base::MakeUnique<ClientBufferFrameDeliverer>(
+          std::move(frame_painter));
   }
-
-  auto photo_frame_painter = base::MakeUnique<PacmanFramePainter>(
-      PacmanFramePainter::Format::SK_N32, device_state.get());
-  auto photo_device = base::MakeUnique<FakePhotoDevice>(
-      std::move(photo_frame_painter), device_state.get());
-
-  return base::MakeUnique<FakeVideoCaptureDevice>(
-      std::move(frame_delivery_strategy), std::move(photo_device),
-      std::move(device_state));
+  NOTREACHED();
+  return nullptr;
 }
 
 PacmanFramePainter::PacmanFramePainter(Format pixel_format,
@@ -427,9 +372,11 @@ void PacmanFramePainter::DrawPacman(base::TimeDelta elapsed_time,
   }
 }
 
-FakePhotoDevice::FakePhotoDevice(std::unique_ptr<PacmanFramePainter> painter,
-                                 const FakeDeviceState* fake_device_state)
-    : painter_(std::move(painter)), fake_device_state_(fake_device_state) {}
+FakePhotoDevice::FakePhotoDevice(
+    std::unique_ptr<PacmanFramePainter> sk_n32_painter,
+    const FakeDeviceState* fake_device_state)
+    : sk_n32_painter_(std::move(sk_n32_painter)),
+      fake_device_state_(fake_device_state) {}
 
 FakePhotoDevice::~FakePhotoDevice() = default;
 
@@ -440,7 +387,7 @@ void FakePhotoDevice::TakePhoto(VideoCaptureDevice::TakePhotoCallback callback,
       PIXEL_FORMAT_ARGB, fake_device_state_->format.frame_size);
   std::unique_ptr<uint8_t[]> buffer(new uint8_t[required_sk_n32_buffer_size]);
   memset(buffer.get(), 0, required_sk_n32_buffer_size);
-  painter_->PaintFrame(elapsed_time, buffer.get());
+  sk_n32_painter_->PaintFrame(elapsed_time, buffer.get());
   mojom::BlobPtr blob = mojom::Blob::New();
   const gfx::PNGCodec::ColorFormat encoding_source_format =
       (kN32_SkColorType == kRGBA_8888_SkColorType) ? gfx::PNGCodec::FORMAT_RGBA
@@ -459,10 +406,12 @@ void FakePhotoDevice::TakePhoto(VideoCaptureDevice::TakePhotoCallback callback,
 }
 
 FakeVideoCaptureDevice::FakeVideoCaptureDevice(
-    std::unique_ptr<FrameDeliverer> frame_delivery_strategy,
+    const VideoCaptureFormats& supported_formats,
+    std::unique_ptr<FrameDelivererFactory> frame_deliverer_factory,
     std::unique_ptr<FakePhotoDevice> photo_device,
     std::unique_ptr<FakeDeviceState> device_state)
-    : frame_deliverer_(std::move(frame_delivery_strategy)),
+    : supported_formats_(supported_formats),
+      frame_deliverer_factory_(std::move(frame_deliverer_factory)),
       photo_device_(std::move(photo_device)),
       device_state_(std::move(device_state)),
       weak_factory_(this) {}
@@ -476,10 +425,16 @@ void FakeVideoCaptureDevice::AllocateAndStart(
     std::unique_ptr<VideoCaptureDevice::Client> client) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  const int closest_supported_format_index = FindIndexOfClosestSupportedFormat(
+      params.requested_format, supported_formats_);
+  const VideoCaptureFormat& selected_format =
+      supported_formats_[closest_supported_format_index];

[emircan, 2017/03/01 18:58:58]

Why don't we make the function FindClosestSupportedFormat() since we do not have
any other use for the index?

[chfremer, 2017/03/01 21:55:48]

Done.

+
   beep_time_ = base::TimeDelta();
   elapsed_time_ = base::TimeDelta();
-  device_state_->format.frame_size =
-      SnapToSupportedSize(params.requested_format.frame_size);
+  frame_deliverer_ =
+      frame_deliverer_factory_->CreateFrameDeliverer(selected_format);
+  device_state_->format.frame_size = selected_format.frame_size;
   frame_deliverer_->Initialize(device_state_->format.pixel_format,
                                std::move(client), device_state_.get());
   current_session_id_++;
@@ -491,7 +446,7 @@ void FakeVideoCaptureDevice::StopAndDeAllocate() {
 
   // Invalidate WeakPtr to stop the perpetual scheduling of tasks.
   weak_factory_.InvalidateWeakPtrs();
-  frame_deliverer_->Uninitialize();
+  frame_deliverer_.reset();
 }
 
 void FakeVideoCaptureDevice::GetPhotoCapabilities(
@@ -574,11 +529,6 @@ void OwnBufferFrameDeliverer::Initialize(
       pixel_format, device_state->format.frame_size)]);
 }
 
-void OwnBufferFrameDeliverer::Uninitialize() {
-  FrameDeliverer::Uninitialize();
-  buffer_.reset();
-}
-
 void OwnBufferFrameDeliverer::PaintAndDeliverNextFrame(
     base::TimeDelta timestamp_to_paint) {
   if (!client())
@@ -631,12 +581,6 @@ JpegEncodingFrameDeliverer::JpegEncodingFrameDeliverer(
 
 JpegEncodingFrameDeliverer::~JpegEncodingFrameDeliverer() = default;
 
-void JpegEncodingFrameDeliverer::Uninitialize() {
-  FrameDeliverer::Uninitialize();
-  sk_n32_buffer_.clear();
-  jpeg_buffer_.clear();
-}
-
 void JpegEncodingFrameDeliverer::PaintAndDeliverNextFrame(
     base::TimeDelta timestamp_to_paint) {
   if (!client())