VideoCaptureDeviceAVFoundation: introduce feature kMacNextGenerationCapturer.

Recently a few changes (crrev.com/c/2391767 and crrev.com/c/2399238) entered the AVFoundation capturer, and it's better if these are controlled by a new base::Feature instead of raw changes, because
1) More evolution is expected,
2) We had one regression introduced (crbug.com/1124884#c7).
3) Additionally, this change enables A/B testing in a few months.

The change introduces a new feature kMacNextGenerationCapturer controlling which implementation (default: new capturer) is in use by the system.

The change also dupes the current _mac-suffixed capturer into a new _legacy-suffixed variant.
In future CLs, last weeks evolution to the _mac capturer will be reverted inside the _legacy variant.

Bug: chromium:1126690
Change-Id: I972663eef38623c9a76e0b2424e048bc24fc7093
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401168
Commit-Queue: Markus Handell <handellm@google.com>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806403}

media/base/media_switches.cc

@@ -654,6 +654,11 @@ const base::Feature MEDIA_EXPORT kMediaFoundationVP8Decoding{
 #endif  // defined(OS_WIN)
 
 #if defined(OS_MAC)
+// Controls whether the next version mac capturer, including power improvements,
+// zero copy operation, and other improvements, is active.
+const base::Feature MEDIA_EXPORT kAVFoundationCaptureV2{
+    "AVFoundationCaptureV2", base::FEATURE_ENABLED_BY_DEFAULT};
+
 const base::Feature MEDIA_EXPORT kVideoToolboxVp9Decoding{
     "VideoToolboxVp9Decoding", base::FEATURE_DISABLED_BY_DEFAULT};
 #endif  // defined(OS_MAC)

media/base/media_switches.h

@@ -223,6 +223,7 @@ MEDIA_EXPORT extern const base::Feature kMediaFoundationVP8Decoding;
 #endif  // defined(OS_WIN)
 
 #if defined(OS_MAC)
+MEDIA_EXPORT extern const base::Feature kAVFoundationCaptureV2;
 MEDIA_EXPORT extern const base::Feature kVideoToolboxVp9Decoding;
 #endif
 

media/capture/BUILD.gn

@@ -162,8 +162,11 @@ component("capture_lib") {
 
   if (is_mac) {
     sources += [
+      "video/mac/video_capture_device_avfoundation_legacy_mac.h",
+      "video/mac/video_capture_device_avfoundation_legacy_mac.mm",
       "video/mac/video_capture_device_avfoundation_mac.h",
       "video/mac/video_capture_device_avfoundation_mac.mm",
+      "video/mac/video_capture_device_avfoundation_protocol_mac.h",
       "video/mac/video_capture_device_avfoundation_utils_mac.h",
       "video/mac/video_capture_device_avfoundation_utils_mac.mm",
       "video/mac/video_capture_device_decklink_mac.h",

media/capture/video/mac/video_capture_device_avfoundation_legacy_mac.h

+// Copyright 2013 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_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_LEGACY_MAC_H_
+#define MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_LEGACY_MAC_H_
+
+#import <AVFoundation/AVFoundation.h>
+#import <Foundation/Foundation.h>
+
+#import "base/mac/scoped_nsobject.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#import "media/capture/video/mac/video_capture_device_avfoundation_protocol_mac.h"
+#include "media/capture/video/video_capture_device.h"
+#include "media/capture/video_capture_types.h"
+
+@interface VideoCaptureDeviceAVFoundationLegacy
+    : NSObject <AVCaptureVideoDataOutputSampleBufferDelegate,
+                VideoCaptureDeviceAVFoundationProtocol> {
+ @private
+  // The following attributes are set via -setCaptureHeight:width:frameRate:.
+  int _frameWidth;
+  int _frameHeight;
+  float _frameRate;
+
+  // The capture format that best matches the above attributes.
+  base::scoped_nsobject<AVCaptureDeviceFormat> _bestCaptureFormat;
+
+  base::Lock _lock;  // Protects concurrent setting and using |frameReceiver_|.
+  media::VideoCaptureDeviceAVFoundationFrameReceiver* _frameReceiver;  // weak.
+
+  base::scoped_nsobject<AVCaptureSession> _captureSession;
+
+  // |captureDevice_| is an object coming from AVFoundation, used only to be
+  // plugged in |captureDeviceInput_| and to query for session preset support.
+  base::scoped_nsobject<AVCaptureDevice> _captureDevice;
+  base::scoped_nsobject<AVCaptureDeviceInput> _captureDeviceInput;
+  base::scoped_nsobject<AVCaptureVideoDataOutput> _captureVideoDataOutput;
+
+  // 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<VideoCaptureDeviceAVFoundationLegacy>>
+      _weakPtrFactoryForTakePhoto;
+
+  // For testing.
+  base::RepeatingCallback<void()> _onStillImageOutputStopped;
+
+  scoped_refptr<base::SingleThreadTaskRunner> _mainThreadTaskRunner;
+}
+@end
+
+#endif  // MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_LEGACY_MAC_H_

media/capture/video/mac/video_capture_device_avfoundation_mac.h

@@ -11,66 +11,16 @@
 #import "base/mac/scoped_nsobject.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
+#import "media/capture/video/mac/video_capture_device_avfoundation_protocol_mac.h"
 #include "media/capture/video/video_capture_device.h"
 #include "media/capture/video_capture_types.h"
 
-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
-// AVFoundation API. This class lives inside the thread created by its owner
-// VCDM.
-//
-//  * Previous to any use, clients (VCDM) must call -initWithFrameReceiver: to
-//    initialise an object of this class and register a |frameReceiver_|.
-//  * Frame receiver registration or removal can also happen via explicit call
-//    to -setFrameReceiver:. Re-registrations are safe and allowed, even during
-//    capture using this method.
-//  * Method -setCaptureDevice: must be called at least once with a device
-//    identifier from +deviceNames. Creates all the necessary AVFoundation
-//    objects on first call; it connects them ready for capture every time.
-//    This method should not be called during capture (i.e. between
-//    -startCapture and -stopCapture).
-//  * -setCaptureWidth:height:frameRate: is called if a resolution or frame rate
-//    different than the by default one set by -setCaptureDevice: is needed.
-//    This method should not be called during capture. This method must be
-//    called after -setCaptureDevice:.
-//  * -startCapture registers the notification listeners and starts the
-//    capture. The capture can be stop using -stopCapture. The capture can be
-//    restarted and restoped multiple times, reconfiguring or not the device in
-//    between.
-//  * -setCaptureDevice can be called with a |nil| value, case in which it stops
-//    the capture and disconnects the library objects. This step is not
-//    necessary.
-//  * Deallocation of the library objects happens gracefully on destruction of
-//    the VideoCaptureDeviceAVFoundation object.
-//
-//
+// TODO(crbug.com/1126690): rename this file to be suffixed by the
+// "next generation" moniker.
 CAPTURE_EXPORT
 @interface VideoCaptureDeviceAVFoundation
-    : NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> {
+    : NSObject <AVCaptureVideoDataOutputSampleBufferDelegate,
+                VideoCaptureDeviceAVFoundationProtocol> {
  @private
   // The following attributes are set via -setCaptureHeight:width:frameRate:.
   int _frameWidth;
@@ -106,44 +56,6 @@ CAPTURE_EXPORT
   scoped_refptr<base::SingleThreadTaskRunner> _mainThreadTaskRunner;
 }
 
-// Initializes the instance and the underlying capture session and registers the
-// frame receiver.
-- (id)initWithFrameReceiver:
-    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver;
-
-// Sets the frame receiver.
-- (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
-// connected for the capture, and a by default resolution is set. If deviceId is
-// nil, then the eventual capture is stopped and library objects are
-// disconnected. Returns YES on success, NO otherwise. If the return value is
-// NO, an error message is assigned to |outMessage|. This method should not be
-// called during capture.
-- (BOOL)setCaptureDevice:(NSString*)deviceId
-            errorMessage:(NSString**)outMessage;
-
-// Configures the capture properties for the capture session and the video data
-// output; this means it MUST be called after setCaptureDevice:. Return YES on
-// success, else NO.
-- (BOOL)setCaptureHeight:(int)height
-                   width:(int)width
-               frameRate:(float)frameRate;
-
-// Starts video capturing and register the notification listeners. Must be
-// called after setCaptureDevice:, and, eventually, also after
-// setCaptureHeight:width:frameRate:. Returns YES on success, NO otherwise.
-- (BOOL)startCapture;
-
-// Stops video capturing and stops listening to notifications.
-- (void)stopCapture;
-
-// Takes a photo. This method should only be called between -startCapture and
-// -stopCapture.
-- (void)takePhoto;
-
 - (void)setOnStillImageOutputStoppedForTesting:
     (base::RepeatingCallback<void()>)onStillImageOutputStopped;
 

media/capture/video/mac/video_capture_device_avfoundation_protocol_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_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_PROTOCOL_MAC_H_
+#define MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_PROTOCOL_MAC_H_
+
+#import <AVFoundation/AVFoundation.h>
+#import <Foundation/Foundation.h>
+
+#import "base/mac/scoped_nsobject.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#include "media/capture/video/video_capture_device.h"
+#include "media/capture/video_capture_types.h"
+
+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
+
+// Protocol used by VideoCaptureDeviceMac for video and image capture using
+// AVFoundation API. Concrete implementation objects live inside the thread
+// created by its owner VideoCaptureDeviceMac.
+@protocol VideoCaptureDeviceAVFoundationProtocol
+
+// Previous to any use, clients must call -initWithFrameReceiver: to
+// initialise an object of this class and register a |frameReceiver_|. This
+// initializes the instance and the underlying capture session and registers the
+// frame receiver.
+- (id)initWithFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver;
+
+// Frame receiver registration or removal can also happen via explicit call
+// to -setFrameReceiver:. Re-registrations are safe and allowed, even during
+// capture using this method.
+- (void)setFrameReceiver:
+    (media::VideoCaptureDeviceAVFoundationFrameReceiver*)frameReceiver;
+
+// Sets which capture device to use by name, retrieved via |deviceNames|.
+// Method -setCaptureDevice: must be called at least once with a device
+// identifier from GetVideoCaptureDeviceNames(). It creates all the necessary
+// AVFoundation objects on the first call; it connects them ready for capture
+// every time. Once the deviceId is known, the library objects are created if
+// needed and connected for the capture, and a by default resolution is set. If
+// |deviceId| is nil, then the eventual capture is stopped and library objects
+// are disconnected. Returns YES on success, NO otherwise. If the return value
+// is NO, an error message is assigned to |outMessage|. This method should not
+// be called during capture (i.e. between -startCapture and -stopCapture).
+- (BOOL)setCaptureDevice:(NSString*)deviceId
+            errorMessage:(NSString**)outMessage;
+
+// Configures the capture properties for the capture session and the video data
+// output; this means it MUST be called after setCaptureDevice:. Return YES on
+// success, else NO.
+- (BOOL)setCaptureHeight:(int)height
+                   width:(int)width
+               frameRate:(float)frameRate;
+
+// Starts video capturing and registers notification listeners. Must be
+// called after setCaptureDevice:, and, eventually, also after
+// setCaptureHeight:width:frameRate:.
+// The capture can be stopped and restarted multiple times, potentially
+// reconfiguring the device in between.
+// Returns YES on success, NO otherwise.
+- (BOOL)startCapture;
+
+// Stops video capturing and stops listening to notifications. Same as
+// setCaptureDevice:nil but doesn't disconnect the library objects. The capture
+// can be
+- (void)stopCapture;
+
+// Takes a photo. This method should only be called between -startCapture and
+// -stopCapture.
+- (void)takePhoto;
+@end
+
+#endif  // MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_PROTOCOL_MAC_H_

media/capture/video/mac/video_capture_device_avfoundation_utils_mac.h

@@ -42,6 +42,10 @@ void ExtractBaseAddressAndLength(char** base_address,
                                  size_t* length,
                                  CMSampleBufferRef sample_buffer);
 
+// Returns implementation class for VideoCaptureDeviceAVFoundation depending
+// on if |kMacNextGenerationCapturer| is enabled or disabled.
+Class GetVideoCaptureDeviceAVFoundationImplementationClass();
+
 }  // namespace media
 
 #endif  // MEDIA_CAPTURE_VIDEO_MAC_VIDEO_CAPTURE_DEVICE_AVFOUNDATION_UTILS_MAC_H_

media/capture/video/mac/video_capture_device_avfoundation_utils_mac.mm

@@ -8,6 +8,9 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
+#include "media/base/media_switches.h"
+#include "media/capture/video/mac/video_capture_device_avfoundation_legacy_mac.h"
+#include "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
 #include "media/capture/video/mac/video_capture_device_factory_mac.h"
 #include "media/capture/video/mac/video_capture_device_mac.h"
 #include "media/capture/video_capture_types.h"
@@ -299,4 +302,11 @@ media::VideoCaptureFormats GetDeviceSupportedFormats(
   return formats;
 }
 
+Class GetVideoCaptureDeviceAVFoundationImplementationClass() {
+  if (base::FeatureList::IsEnabled(media::kAVFoundationCaptureV2)) {
+    return [VideoCaptureDeviceAVFoundation class];
+  }
+  return [VideoCaptureDeviceAVFoundationLegacy class];
+}
+
 }  // namespace media

media/capture/video/mac/video_capture_device_factory_mac_unittest.mm

@@ -6,14 +6,31 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "media/base/media_switches.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 {
 
-TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesAVFoundation) {
+enum class AVFoundationCaptureV2 { kEnabled, kDisabled };
+
+class VideoCaptureDeviceFactoryMacTest
+    : public ::testing::TestWithParam<AVFoundationCaptureV2> {
+ public:
+  VideoCaptureDeviceFactoryMacTest() {
+    scoped_feature_list_.InitWithFeatureState(
+        media::kAVFoundationCaptureV2,
+        /*enabled=*/GetParam() == AVFoundationCaptureV2::kEnabled);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_P(VideoCaptureDeviceFactoryMacTest, ListDevicesAVFoundation) {
   RunTestCase(base::BindOnce([]() {
     VideoCaptureDeviceFactoryMac video_capture_device_factory;
 
@@ -30,7 +47,7 @@ TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesAVFoundation) {
   }));
 }
 
-TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesWithNoPanTiltZoomSupport) {
+TEST_P(VideoCaptureDeviceFactoryMacTest, ListDevicesWithNoPanTiltZoomSupport) {
   RunTestCase(base::BindOnce([]() {
     VideoCaptureDeviceFactoryMac video_capture_device_factory;
 
@@ -45,4 +62,9 @@ TEST(VideoCaptureDeviceFactoryMacTest, ListDevicesWithNoPanTiltZoomSupport) {
   }));
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         VideoCaptureDeviceFactoryMacTest,
+                         ::testing::Values(AVFoundationCaptureV2::kEnabled,
+                                           AVFoundationCaptureV2::kDisabled));
+
 }  // namespace media

media/capture/video/mac/video_capture_device_mac.h

@@ -20,6 +20,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #import "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
+#import "media/capture/video/mac/video_capture_device_avfoundation_protocol_mac.h"
 #include "media/capture/video/video_capture_device.h"
 #include "media/capture/video_capture_types.h"
 
@@ -118,7 +119,8 @@ class VideoCaptureDeviceMac
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   InternalState state_;
 
-  base::scoped_nsobject<VideoCaptureDeviceAVFoundation> capture_device_;
+  base::scoped_nsobject<NSObject<VideoCaptureDeviceAVFoundationProtocol>>
+      capture_device_;
 
   // To hold on to the TakePhotoCallback while the picture is being taken.
   TakePhotoCallback photo_callback_;

media/capture/video/mac/video_capture_device_mac.mm

@@ -29,6 +29,7 @@
 #include "media/base/timestamp_constants.h"
 #include "media/capture/mojom/image_capture_types.h"
 #import "media/capture/video/mac/video_capture_device_avfoundation_mac.h"
+#include "media/capture/video/mac/video_capture_device_avfoundation_utils_mac.h"
 #include "ui/gfx/geometry/size.h"
 
 @implementation DeviceNameAndTransportType
@@ -429,8 +430,8 @@ bool VideoCaptureDeviceMac::Init(VideoCaptureApi capture_api_type) {
   if (capture_api_type != VideoCaptureApi::MACOSX_AVFOUNDATION)
     return false;
 
-  capture_device_.reset(
-      [[VideoCaptureDeviceAVFoundation alloc] initWithFrameReceiver:this]);
+  capture_device_.reset([[GetVideoCaptureDeviceAVFoundationImplementationClass()
+      alloc] initWithFrameReceiver:this]);
 
   if (!capture_device_)
     return false;