[macOS Capture] Lazily instantiate takePhoto()'s _stillImageOutput.

Per https://crbug.com/1116241, always instantiating and adding the
AVCaptureStillImageOutput to the capture session does, in some cases,
result in high CPU load due to the macOS process VTDecoderXPCService
converting pixel formats and producing MJPEG. But this work is only
needed if the ImageCapture.takePhoto() API is invoked, which most
MediaStreamTrack-using web pages never use.

This CL lazily instantiates _stillImageOutput when takePhoto() is
called and keeps it alive until 60 seconds of time has passed without
another takePhoto() call. We keep it alive like this because
instantiation involves noticeable delays that is avoided if takePhoto()
is called after instantiation.

The delays are due to two reasons:
1. Delays inherent to instantiating AVCaptureStillImageOutput.
2. This CL introduces a async delay of 3 seconds after instantiation.
   If we don't wait before taking the photo, the 3A has not stabilized
   and the photo is too dark.
Because performance is critical to the success of WebRTC applications,
the pros are believed to outweigh the cons.

To achieve its goal, this CL:
- Makes it possible to mock the frame receiver.
- Adds helper utils.
- Adds video_capture_device_avfoundation_mac_unittest.mm.
- Replaces a thread checker with a task runner to allow posting tasks.
- Introduces MOCK_TIME in the tests.

Bug: chromium:1124652, chromium:1116241
Change-Id: I91ebcb45d44fda3fcdb613b8907a10dfa89a9615
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388100Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Markus Handell <handellm@google.com>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806083}

media/capture/BUILD.gn

@@ -392,6 +392,11 @@ test("capture_unittests") {
     "video/linux/camera_config_chromeos_unittest.cc",
     "video/linux/v4l2_capture_delegate_unittest.cc",
     "video/linux/video_capture_device_factory_linux_unittest.cc",
+    "video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.h",
+    "video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.mm",
+    "video/mac/test/video_capture_test_utils_mac.h",
+    "video/mac/test/video_capture_test_utils_mac.mm",
+    "video/mac/video_capture_device_avfoundation_mac_unittest.mm",
     "video/mac/video_capture_device_factory_mac_unittest.mm",
     "video/mac/video_capture_device_mac_unittest.mm",
     "video/video_capture_device_client_unittest.cc",

media/capture/video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.h

+// Copyright 2020 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 MEDIA_CAPTURE_VIDEO_MAC_TEST_MOCK_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_FRAME_RECEIVER_MAC_H_
+#define MEDIA_CAPTURE_VIDEO_MAC_TEST_MOCK_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_FRAME_RECEIVER_MAC_H_
+
+#include "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace media {
+
+class MockVideoCaptureDeviceAVFoundationFrameReceiver
+    : public VideoCaptureDeviceAVFoundationFrameReceiver {
+ public:
+  MockVideoCaptureDeviceAVFoundationFrameReceiver();
+  ~MockVideoCaptureDeviceAVFoundationFrameReceiver() override;
+
+  MOCK_METHOD(void,
+              ReceiveFrame,
+              (const uint8_t* video_frame,
+               int video_frame_length,
+               const VideoCaptureFormat& frame_format,
+               const gfx::ColorSpace color_space,
+               int aspect_numerator,
+               int aspect_denominator,
+               base::TimeDelta timestamp),
+              (override));
+
+  MOCK_METHOD(void,
+              OnPhotoTaken,
+              (const uint8_t* image_data,
+               size_t image_length,
+               const std::string& mime_type),
+              (override));
+
+  MOCK_METHOD(void, OnPhotoError, (), (override));
+
+  MOCK_METHOD(void,
+              ReceiveError,
+              (VideoCaptureError error,
+               const base::Location& from_here,
+               const std::string& reason),
+              (override));
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_MAC_TEST_MOCK_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_FRAME_RECEIVER_MAC_H_
\ No newline at end of file

media/capture/video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.mm

+// Copyright 2020 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.
+
+#import "media/capture/video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.h"
+
+namespace media {
+
+MockVideoCaptureDeviceAVFoundationFrameReceiver::
+    MockVideoCaptureDeviceAVFoundationFrameReceiver() = default;
+
+MockVideoCaptureDeviceAVFoundationFrameReceiver::
+    ~MockVideoCaptureDeviceAVFoundationFrameReceiver() = default;
+
+}  // namespace media

media/capture/video/mac/test/video_capture_test_utils_mac.h

+// Copyright 2020 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 MEDIA_CAPTURE_VIDEO_MAC_TEST_VIDEO_CAPTURE_TEST_UTILS_MAC_H_
+#define MEDIA_CAPTURE_VIDEO_MAC_TEST_VIDEO_CAPTURE_TEST_UTILS_MAC_H_
+
+#import <Foundation/Foundation.h>
+#include <memory>
+
+#include "base/callback_forward.h"
+#include "media/capture/video/mac/video_capture_device_factory_mac.h"
+
+namespace media {
+
+// Video capture code on MacOSX must run on a CFRunLoop enabled thread
+// for interaction with AVFoundation.
+// In order to make the test case run on the actual message loop that has
+// been created for this thread, we need to run it inside a RunLoop. This is
+// required, because on MacOS the capture code must run on a CFRunLoop
+// enabled message loop.
+void RunTestCase(base::OnceClosure test_case);
+
+std::vector<VideoCaptureDeviceInfo> GetDevicesInfo(
+    VideoCaptureDeviceFactoryMac* video_capture_device_factory);
+// If there are no devices, nil is returned.
+NSString* GetFirstDeviceId();
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_MAC_TEST_VIDEO_CAPTURE_TEST_UTILS_MAC_H_

media/capture/video/mac/test/video_capture_test_utils_mac.mm

+// Copyright 2020 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.
+
+#import "media/capture/video/mac/test/video_capture_test_utils_mac.h"
+
+#import <Foundation/Foundation.h>
+#include <memory>
+
+#include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
+#include "base/test/gmock_callback_support.h"
+#include "base/test/task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "media/capture/video/mac/video_capture_device_factory_mac.h"
+
+namespace media {
+
+void RunTestCase(base::OnceClosure test_case) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI,
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME);
+  base::RunLoop run_loop;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(
+                     [](base::RunLoop* run_loop, base::OnceClosure* test_case) {
+                       std::move(*test_case).Run();
+                       run_loop->Quit();
+                     },
+                     &run_loop, &test_case));
+  run_loop.Run();
+}
+
+std::vector<VideoCaptureDeviceInfo> GetDevicesInfo(
+    VideoCaptureDeviceFactoryMac* video_capture_device_factory) {
+  std::vector<VideoCaptureDeviceInfo> descriptors;
+  base::RunLoop run_loop;
+  video_capture_device_factory->GetDevicesInfo(base::BindLambdaForTesting(
+      [&descriptors, &run_loop](std::vector<VideoCaptureDeviceInfo> result) {
+        descriptors = std::move(result);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+  return descriptors;
+}
+
+NSString* GetFirstDeviceId() {
+  VideoCaptureDeviceFactoryMac video_capture_device_factory;
+  std::vector<VideoCaptureDeviceInfo> devices_info =
+      GetDevicesInfo(&video_capture_device_factory);
+  if (devices_info.empty())
+    return nil;
+  return [NSString
+      stringWithUTF8String:devices_info.front().descriptor.device_id.c_str()];
+}
+
+}  // namespace media

media/capture/video/mac/video_capture_device_avfoundation_mac.h

@@ -16,6 +16,27 @@
 
 namespace media {
 class VideoCaptureDeviceMac;
+
+class CAPTURE_EXPORT VideoCaptureDeviceAVFoundationFrameReceiver {
+ public:
+  virtual ~VideoCaptureDeviceAVFoundationFrameReceiver() = default;
+
+  virtual void ReceiveFrame(const uint8_t* video_frame,
+                            int video_frame_length,
+                            const VideoCaptureFormat& frame_format,
+                            const gfx::ColorSpace color_space,
+                            int aspect_numerator,
+                            int aspect_denominator,
+                            base::TimeDelta timestamp) = 0;
+  virtual void OnPhotoTaken(const uint8_t* image_data,
+                            size_t image_length,
+                            const std::string& mime_type) = 0;
+  virtual void OnPhotoError() = 0;
+  virtual void ReceiveError(VideoCaptureError error,
+                            const base::Location& from_here,
+                            const std::string& reason) = 0;
+};
+
 }  // namespace media
 
 // Class used by VideoCaptureDeviceMac (VCDM) for video and image capture using
@@ -47,6 +68,7 @@ class VideoCaptureDeviceMac;
 //    the VideoCaptureDeviceAVFoundation object.
 //
 //
+CAPTURE_EXPORT
 @interface VideoCaptureDeviceAVFoundation
     : NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> {
  @private
@@ -59,7 +81,7 @@ class VideoCaptureDeviceMac;
   base::scoped_nsobject<AVCaptureDeviceFormat> _bestCaptureFormat;
 
   base::Lock _lock;  // Protects concurrent setting and using |frameReceiver_|.
-  media::VideoCaptureDeviceMac* _frameReceiver;  // weak.
+  media::VideoCaptureDeviceAVFoundationFrameReceiver* _frameReceiver;  // weak.
 
   base::scoped_nsobject<AVCaptureSession> _captureSession;
 
@@ -71,16 +93,27 @@ class VideoCaptureDeviceMac;
 
   // An AVDataOutput specialized for taking pictures out of |captureSession_|.
   base::scoped_nsobject<AVCaptureStillImageOutput> _stillImageOutput;
+  size_t _takePhotoStartedCount;
+  size_t _takePhotoPendingCount;
+  size_t _takePhotoCompletedCount;
+  bool _stillImageOutputWarmupCompleted;
+  std::unique_ptr<base::WeakPtrFactory<VideoCaptureDeviceAVFoundation>>
+      _weakPtrFactoryForTakePhoto;
 
-  base::ThreadChecker _main_thread_checker;
+  // For testing.
+  base::RepeatingCallback<void()> _onStillImageOutputStopped;
+
+  scoped_refptr<base::SingleThreadTaskRunner> _mainThreadTaskRunner;
 }
 
 // Initializes the instance and the underlying capture session and registers the
 // frame receiver.
-- (id)initWithFrameReceiver:(media::VideoCaptureDeviceMac*)frameReceiver;
+- (id)initWithFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver;
 
 // Sets the frame receiver.
-- (void)setFrameReceiver:(media::VideoCaptureDeviceMac*)frameReceiver;
+- (void)setFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver;
 
 // Sets which capture device to use by name, retrieved via |deviceNames|. Once
 // the deviceId is known, the library objects are created if needed and
@@ -111,6 +144,9 @@ class VideoCaptureDeviceMac;
 // -stopCapture.
 - (void)takePhoto;
 
+- (void)setOnStillImageOutputStoppedForTesting:
+    (base::RepeatingCallback<void()>)onStillImageOutputStopped;
+
 @end
 
 #endif  // MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_MAC_H_

media/capture/video/mac/video_capture_device_avfoundation_mac.mm

@@ -15,6 +15,7 @@
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "media/base/timestamp_constants.h"
@@ -26,14 +27,24 @@
 #include "services/video_capture/public/uma/video_capture_service_event.h"
 #include "ui/gfx/geometry/size.h"
 
+namespace {
+
+constexpr int kTimeToWaitBeforeStoppingStillImageCaptureInSeconds = 60;
+
+}  // anonymous namespace
+
 @implementation VideoCaptureDeviceAVFoundation
 
 #pragma mark Public methods
 
-- (id)initWithFrameReceiver:(media::VideoCaptureDeviceMac*)frameReceiver {
+- (id)initWithFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver {
   if ((self = [super init])) {
-    DCHECK(_main_thread_checker.CalledOnValidThread());
+    _mainThreadTaskRunner = base::ThreadTaskRunnerHandle::Get();
     DCHECK(frameReceiver);
+    _weakPtrFactoryForTakePhoto =
+        std::make_unique<base::WeakPtrFactory<VideoCaptureDeviceAVFoundation>>(
+            self);
     [self setFrameReceiver:frameReceiver];
     _captureSession.reset([[AVCaptureSession alloc] init]);
   }
@@ -41,11 +52,15 @@
 }
 
 - (void)dealloc {
+  [self stopStillImageOutput];
   [self stopCapture];
+  _weakPtrFactoryForTakePhoto = nullptr;
+  _mainThreadTaskRunner = nullptr;
   [super dealloc];
 }
 
-- (void)setFrameReceiver:(media::VideoCaptureDeviceMac*)frameReceiver {
+- (void)setFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver {
   base::AutoLock lock(_lock);
   _frameReceiver = frameReceiver;
 }
@@ -53,15 +68,14 @@
 - (BOOL)setCaptureDevice:(NSString*)deviceId
             errorMessage:(NSString**)outMessage {
   DCHECK(_captureSession);
-  DCHECK(_main_thread_checker.CalledOnValidThread());
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
 
   if (!deviceId) {
     // First stop the capture session, if it's running.
     [self stopCapture];
     // Now remove the input and output from the capture session.
     [_captureSession removeOutput:_captureVideoDataOutput];
-    if (_stillImageOutput)
-      [_captureSession removeOutput:_stillImageOutput];
+    [self stopStillImageOutput];
     if (_captureDeviceInput) {
       DCHECK(_captureDevice);
       [_captureSession stopRunning];
@@ -96,12 +110,6 @@
   }
   [_captureSession addInput:_captureDeviceInput];
 
-  // Create and plug the still image capture output. This should happen in
-  // advance of the actual picture to allow for the 3A to stabilize.
-  _stillImageOutput.reset([[AVCaptureStillImageOutput alloc] init]);
-  if (_stillImageOutput && [_captureSession canAddOutput:_stillImageOutput])
-    [_captureSession addOutput:_stillImageOutput];
-
   // Create a new data output for video. The data output is configured to
   // discard late frames by default.
   _captureVideoDataOutput.reset([[AVCaptureVideoDataOutput alloc] init]);
@@ -124,8 +132,8 @@
 - (BOOL)setCaptureHeight:(int)height
                    width:(int)width
                frameRate:(float)frameRate {
-  DCHECK(![_captureSession isRunning] &&
-         _main_thread_checker.CalledOnValidThread());
+  DCHECK(![_captureSession isRunning]);
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
 
   _frameWidth = width;
   _frameHeight = height;
@@ -188,7 +196,7 @@
 }
 
 - (BOOL)startCapture {
-  DCHECK(_main_thread_checker.CalledOnValidThread());
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
   if (!_captureSession) {
     DLOG(ERROR) << "Video capture session not initialized.";
     return NO;
@@ -214,55 +222,192 @@
 }
 
 - (void)stopCapture {
-  DCHECK(_main_thread_checker.CalledOnValidThread());
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
+  [self stopStillImageOutput];
   if ([_captureSession isRunning])
     [_captureSession stopRunning];  // Synchronous.
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
 - (void)takePhoto {
-  DCHECK(_main_thread_checker.CalledOnValidThread());
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
   DCHECK([_captureSession isRunning]);
-  if (!_stillImageOutput)
+
+  ++_takePhotoStartedCount;
+
+  // Ready to take a photo immediately?
+  if (_stillImageOutput && _stillImageOutputWarmupCompleted) {
+    [self takePhotoInternal];
     return;
+  }
 
-  DCHECK_EQ(1u, [[_stillImageOutput connections] count]);
-  AVCaptureConnection* const connection =
-      [[_stillImageOutput connections] firstObject];
-  if (!connection) {
+  // Lazily instantiate the |_stillImageOutput| the first time takePhoto() is
+  // called. When takePhoto() isn't called, this avoids JPEG compession work for
+  // every frame. This can save a lot of CPU in some cases (see
+  // https://crbug.com/1116241). However because it can take a couple of second
+  // for the 3A to stabilize, lazily instantiating like may result in noticeable
+  // delays. To avoid delays in future takePhoto() calls we don't delete
+  // |_stillImageOutput| until takePhoto() has not been called for 60 seconds.
+  if (!_stillImageOutput) {
+    // We use AVCaptureStillImageOutput for historical reasons, but note that it
+    // has been deprecated in macOS 10.15[1] in favor of
+    // AVCapturePhotoOutput[2].
+    //
+    // [1]
+    // https://developer.apple.com/documentation/avfoundation/avcapturestillimageoutput
+    // [2]
+    // https://developer.apple.com/documentation/avfoundation/avcapturephotooutput
+    // TODO(https://crbug.com/1124322): Migrate to the new API.
+    _stillImageOutput.reset([[AVCaptureStillImageOutput alloc] init]);
+    if (!_stillImageOutput ||
+        ![_captureSession canAddOutput:_stillImageOutput]) {
+      // Complete this started photo as error.
+      ++_takePhotoPendingCount;
+      {
         base::AutoLock lock(_lock);
+        if (_frameReceiver) {
           _frameReceiver->OnPhotoError();
+        }
+      }
+      [self takePhotoCompleted];
       return;
     }
+    [_captureSession addOutput:_stillImageOutput];
+    // A delay is needed before taking the photo or else the photo may be dark.
+    // 2 seconds was enough in manual testing; we delay by 3 for good measure.
+    _mainThreadTaskRunner->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(
+            [](base::WeakPtr<VideoCaptureDeviceAVFoundation> weakSelf) {
+              [weakSelf.get() takePhotoInternal];
+            },
+            _weakPtrFactoryForTakePhoto->GetWeakPtr()),
+        base::TimeDelta::FromSeconds(3));
+  }
+}
+
+- (void)setOnStillImageOutputStoppedForTesting:
+    (base::RepeatingCallback<void()>)onStillImageOutputStopped {
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
+  _onStillImageOutputStopped = onStillImageOutputStopped;
+}
+
+#pragma mark Private methods
 
+- (void)takePhotoInternal {
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
+  // stopStillImageOutput invalidates all weak ptrs, meaning in-flight
+  // operations are affectively cancelled. So if this method is running, still
+  // image output must be good to go.
+  DCHECK([_captureSession isRunning]);
+  DCHECK(_stillImageOutput);
+  DCHECK([[_stillImageOutput connections] count] == 1);
+  AVCaptureConnection* const connection =
+      [[_stillImageOutput connections] firstObject];
+  DCHECK(connection);
+  _stillImageOutputWarmupCompleted = true;
+
+  // For all photos started that are not yet pending, take photos.
+  while (_takePhotoPendingCount < _takePhotoStartedCount) {
+    ++_takePhotoPendingCount;
     const auto handler = ^(CMSampleBufferRef sampleBuffer, NSError* error) {
+      {
         base::AutoLock lock(_lock);
-    if (!_frameReceiver)
-      return;
+        if (_frameReceiver) {
           if (error != nil) {
             _frameReceiver->OnPhotoError();
-      return;
-    }
-
-    // Recommended compressed pixel format is JPEG, we don't expect surprises.
-    // TODO(mcasas): Consider using [1] for merging EXIF output information:
-    // [1] +(NSData*)jpegStillImageNSDataRepresentation:jpegSampleBuffer;
+          } else {
+            // Recommended compressed pixel format is JPEG, we don't expect
+            // surprises.
+            // TODO(mcasas): Consider using [1] for merging EXIF output
+            // information:
+            // [1]
+            // +(NSData*)jpegStillImageNSDataRepresentation:jpegSampleBuffer;
             DCHECK_EQ(kCMVideoCodecType_JPEG,
                       CMFormatDescriptionGetMediaSubType(
                           CMSampleBufferGetFormatDescription(sampleBuffer)));
 
             char* baseAddress = 0;
             size_t length = 0;
-    media::ExtractBaseAddressAndLength(&baseAddress, &length, sampleBuffer);
-    _frameReceiver->OnPhotoTaken(reinterpret_cast<uint8_t*>(baseAddress),
-                                 length, "image/jpeg");
+            media::ExtractBaseAddressAndLength(&baseAddress, &length,
+                                               sampleBuffer);
+            _frameReceiver->OnPhotoTaken(
+                reinterpret_cast<uint8_t*>(baseAddress), length, "image/jpeg");
+          }
+        }
+      }
+      // Called both on success and failure.
+      _mainThreadTaskRunner->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              [](base::WeakPtr<VideoCaptureDeviceAVFoundation> weakSelf) {
+                [weakSelf.get() takePhotoCompleted];
+              },
+              _weakPtrFactoryForTakePhoto->GetWeakPtr()));
     };
-
     [_stillImageOutput captureStillImageAsynchronouslyFromConnection:connection
                                                    completionHandler:handler];
+  }
 }
 
-#pragma mark Private methods
+- (void)takePhotoCompleted {
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
+  ++_takePhotoCompletedCount;
+  if (_takePhotoStartedCount != _takePhotoCompletedCount)
+    return;
+  // All pending takePhoto()s have completed. If no more photos are taken
+  // within 60 seconds, stop still image output to avoid expensive MJPEG
+  // conversions going forward.
+  _mainThreadTaskRunner->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](base::WeakPtr<VideoCaptureDeviceAVFoundation> weakSelf,
+             size_t takePhotoCount) {
+            VideoCaptureDeviceAVFoundation* strongSelf = weakSelf.get();
+            if (!strongSelf)
+              return;
+            // Don't stop the still image output if takePhoto() was called
+            // while the task was pending.
+            if (strongSelf->_takePhotoStartedCount != takePhotoCount)
+              return;
+            [strongSelf stopStillImageOutput];
+          },
+          _weakPtrFactoryForTakePhoto->GetWeakPtr(), _takePhotoStartedCount),
+      base::TimeDelta::FromSeconds(
+          kTimeToWaitBeforeStoppingStillImageCaptureInSeconds));
+}
+
+- (void)stopStillImageOutput {
+  DCHECK(_mainThreadTaskRunner->BelongsToCurrentThread());
+  if (!_stillImageOutput) {
+    // Already stopped.
+    return;
+  }
+  if (_captureSession) {
+    [_captureSession removeOutput:_stillImageOutput];
+  }
+  _stillImageOutput.reset();
+  _stillImageOutputWarmupCompleted = false;
+
+  // Cancel all in-flight operations.
+  _weakPtrFactoryForTakePhoto->InvalidateWeakPtrs();
+  // Report error for all pending calls that were stopped.
+  size_t pendingCalls = _takePhotoStartedCount - _takePhotoCompletedCount;
+  _takePhotoCompletedCount = _takePhotoPendingCount = _takePhotoStartedCount;
+  {
+    base::AutoLock lock(_lock);
+    if (_frameReceiver) {
+      for (size_t i = 0; i < pendingCalls; ++i) {
+        _frameReceiver->OnPhotoError();
+      }
+    }
+  }
+
+  if (_onStillImageOutputStopped) {
+    // Callback used by tests.
+    _onStillImageOutputStopped.Run();
+  }
+}
 
 // |captureOutput| is called by the capture device to deliver a new frame.
 // AVFoundation calls from a number of threads, depending on, at least, if

media/capture/video/mac/video_capture_device_avfoundation_mac_unittest.mm

+// Copyright 2020 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 "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/run_loop.h"
+#include "base/test/gmock_callback_support.h"
+#import "media/capture/video/mac/test/mock_video_capture_device_avfoundation_frame_receiver_mac.h"
+#import "media/capture/video/mac/test/video_capture_test_utils_mac.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+
+namespace media {
+
+TEST(VideoCaptureDeviceAVFoundationMacTest, TakePhoto) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    EXPECT_CALL(frame_receiver, OnPhotoTaken)
+        .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
+    [captureDevice takePhoto];
+    run_loop.Run();
+  }));
+}
+
+TEST(VideoCaptureDeviceAVFoundationMacTest, StopCaptureWhileTakingPhoto) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    EXPECT_CALL(frame_receiver, OnPhotoError())
+        .WillOnce(base::test::RunClosure(run_loop.QuitClosure()));
+    [captureDevice takePhoto];
+    // There is no risk that takePhoto() has successfully finishes before
+    // stopCapture() because the takePhoto() call involes a PostDelayedTask()
+    // that cannot run until RunLoop::Run() below.
+    [captureDevice stopCapture];
+    run_loop.Run();
+  }));
+}
+
+TEST(VideoCaptureDeviceAVFoundationMacTest, MultiplePendingTakePhotos) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    size_t photos_taken_count = 0;
+    EXPECT_CALL(frame_receiver, OnPhotoTaken)
+        .WillRepeatedly([&photos_taken_count, &run_loop] {
+          ++photos_taken_count;
+          if (photos_taken_count == 3) {
+            run_loop.Quit();
+          }
+        });
+    [captureDevice takePhoto];
+    [captureDevice takePhoto];
+    [captureDevice takePhoto];
+    run_loop.Run();
+  }));
+}
+
+TEST(VideoCaptureDeviceAVFoundationMacTest,
+     StopCaptureWhileMultiplePendingTakePhotos) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    size_t photo_error_count = 0;
+    EXPECT_CALL(frame_receiver, OnPhotoError)
+        .WillRepeatedly([&photo_error_count, &run_loop] {
+          ++photo_error_count;
+          if (photo_error_count == 3) {
+            run_loop.Quit();
+          }
+        });
+    [captureDevice takePhoto];
+    [captureDevice takePhoto];
+    [captureDevice takePhoto];
+    // There is no risk that takePhoto() has successfully finishes before
+    // stopCapture() because the takePhoto() calls involes a PostDelayedTask()
+    // that cannot run until RunLoop::Run() below.
+    [captureDevice stopCapture];
+    run_loop.Run();
+  }));
+}
+
+TEST(VideoCaptureDeviceAVFoundationMacTest,
+     StopStillImageOutputWhenNoLongerTakingPhotos) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    [captureDevice
+        setOnStillImageOutputStoppedForTesting:run_loop.QuitClosure()];
+    base::TimeTicks start_time = base::TimeTicks::Now();
+    [captureDevice takePhoto];
+    // The RunLoop automatically advances mocked time when there are delayed
+    // tasks pending. This allows the test to run fast and still assert how much
+    // mocked time has elapsed.
+    run_loop.Run();
+    auto time_elapsed = base::TimeTicks::Now() - start_time;
+    // Still image output is not stopped until 60 seconds of inactivity, so the
+    // mocked time must have advanced at least this much.
+    EXPECT_GE(time_elapsed.InSeconds(), 60);
+  }));
+}
+
+// This test ensures we don't crash even if we leave operations pending.
+TEST(VideoCaptureDeviceAVFoundationMacTest,
+     TakePhotoAndShutDownWithoutWaiting) {
+  RunTestCase(base::BindOnce([] {
+    NSString* deviceId = GetFirstDeviceId();
+    if (!deviceId) {
+      DVLOG(1) << "No camera available. Exiting test.";
+      return;
+    }
+
+    testing::NiceMock<MockVideoCaptureDeviceAVFoundationFrameReceiver>
+        frame_receiver;
+    base::scoped_nsobject<VideoCaptureDeviceAVFoundation> captureDevice(
+        [[VideoCaptureDeviceAVFoundation alloc]
+            initWithFrameReceiver:&frame_receiver]);
+
+    NSString* errorMessage = nil;
+    ASSERT_TRUE([captureDevice setCaptureDevice:deviceId
+                                   errorMessage:&errorMessage]);
+    ASSERT_TRUE([captureDevice startCapture]);
+
+    [captureDevice takePhoto];
+  }));
+}
+
+}  // namespace media

media/capture/video/mac/video_capture_device_factory_mac_unittest.mm

@@ -7,48 +7,18 @@
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
+#import "media/capture/video/mac/test/video_capture_test_utils_mac.h"
 #include "media/capture/video/mac/video_capture_device_mac.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
 
-// Video capture code on MacOSX must run on a CFRunLoop enabled thread
-// for interaction with AVFoundation.
-// In order to make the test case run on the actual message loop that has
-// been created for this thread, we need to run it inside a RunLoop. This is
-// required, because on MacOS the capture code must run on a CFRunLoop
-// enabled message loop.
-void RunTestCase(base::OnceClosure test_case) {
-  base::test::TaskEnvironment task_environment(
-      base::test::TaskEnvironment::MainThreadType::UI);
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(
-                     [](base::RunLoop* run_loop, base::OnceClosure* test_case) {
-                       std::move(*test_case).Run();
-                       run_loop->Quit();
-                     },
-                     &run_loop, &test_case));
-  run_loop.Run();
-}
-
-void GetDevicesInfo(VideoCaptureDeviceFactoryMac* video_capture_device_factory,
-                    std::vector<VideoCaptureDeviceInfo>* descriptors) {
-  base::RunLoop run_loop;
-  video_capture_device_factory->GetDevicesInfo(base::BindLambdaForTesting(
-      [descriptors, &run_loop](std::vector<VideoCaptureDeviceInfo> result) {
-        *descriptors = std::move(result);
-        run_loop.Quit();
-      }));
-  run_loop.Run();
-}
-
 TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesAVFoundation) {
   RunTestCase(base::BindOnce([]() {
     VideoCaptureDeviceFactoryMac video_capture_device_factory;
 
-    std::vector<VideoCaptureDeviceInfo> devices_info;
-    GetDevicesInfo(&video_capture_device_factory, &devices_info);
+    std::vector<VideoCaptureDeviceInfo> devices_info =
+        GetDevicesInfo(&video_capture_device_factory);
     if (devices_info.empty()) {
       DVLOG(1) << "No camera available. Exiting test.";
       return;
@@ -64,8 +34,8 @@ TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesWithNoPanTiltZoomSupport) {
   RunTestCase(base::BindOnce([]() {
     VideoCaptureDeviceFactoryMac video_capture_device_factory;
 
-    std::vector<VideoCaptureDeviceInfo> devices_info;
-    GetDevicesInfo(&video_capture_device_factory, &devices_info);
+    std::vector<VideoCaptureDeviceInfo> devices_info =
+        GetDevicesInfo(&video_capture_device_factory);
     if (devices_info.empty()) {
       DVLOG(1) << "No camera available. Exiting test.";
       return;

media/capture/video/mac/video_capture_device_mac.h

@@ -19,11 +19,10 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#import "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
 #include "media/capture/video/video_capture_device.h"
 #include "media/capture/video_capture_types.h"
 
-@class VideoCaptureDeviceAVFoundation;
-
 namespace base {
 class SingleThreadTaskRunner;
 }
@@ -52,7 +51,9 @@ namespace media {
 
 // Called by VideoCaptureManager to open, close and start, stop Mac video
 // capture devices.
-class VideoCaptureDeviceMac : public VideoCaptureDevice {
+class VideoCaptureDeviceMac
+    : public VideoCaptureDevice,
+      public VideoCaptureDeviceAVFoundationFrameReceiver {
  public:
   explicit VideoCaptureDeviceMac(
       const VideoCaptureDeviceDescriptor& device_descriptor);
@@ -78,19 +79,19 @@ class VideoCaptureDeviceMac : public VideoCaptureDevice {
                     const gfx::ColorSpace color_space,
                     int aspect_numerator,
                     int aspect_denominator,
-                    base::TimeDelta timestamp);
+                    base::TimeDelta timestamp) override;
 
   // Callbacks with the result of a still image capture, or in case of error,
   // respectively. It's safe to call these methods from any thread.
   void OnPhotoTaken(const uint8_t* image_data,
                     size_t image_length,
-                    const std::string& mime_type);
-  void OnPhotoError();
+                    const std::string& mime_type) override;
+  void OnPhotoError() override;
 
   // Forwarder to VideoCaptureDevice::Client::OnError().
   void ReceiveError(VideoCaptureError error,
                     const base::Location& from_here,
-                    const std::string& reason);
+                    const std::string& reason) override;
 
   // Forwarder to VideoCaptureDevice::Client::OnLog().
   void LogMessage(const std::string& message);