Replace SessionPresets with AVCaptureDeviceFormats

webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm

 /*
  *  Copyright 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 12 matching lines @@
 #endif
 
 #include "libyuv/rotate.h"
 
 #include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/thread.h"
 #include "webrtc/common_video/include/corevideo_frame_buffer.h"
 #include "webrtc/common_video/rotation.h"
 
-struct AVCaptureSessionPresetResolution {
-  NSString *sessionPreset;
-  int width;
-  int height;
-};
+// TODO(denicija): add support for higher frame rates.
+// See http://crbug/webrtc/6355 for more info.
+static const int kFramesPerSecond = 30;
 
-#if TARGET_OS_IPHONE
-static const AVCaptureSessionPresetResolution kAvailablePresets[] = {
-  { AVCaptureSessionPreset352x288, 352, 288},
-  { AVCaptureSessionPreset640x480, 640, 480},
-  { AVCaptureSessionPreset1280x720, 1280, 720},
-  { AVCaptureSessionPreset1920x1080, 1920, 1080},
-};
-#else // macOS
-static const AVCaptureSessionPresetResolution kAvailablePresets[] = {
-  { AVCaptureSessionPreset320x240, 320, 240},
-  { AVCaptureSessionPreset352x288, 352, 288},
-  { AVCaptureSessionPreset640x480, 640, 480},
-  { AVCaptureSessionPreset960x540, 960, 540},
-  { AVCaptureSessionPreset1280x720, 1280, 720},
-};
-#endif
+static inline BOOL IsMediaSubTypeSupported(FourCharCode mediaSubType) {

[tkchin_webrtc, 2016/10/05 17:33:12]

odd to have so many static fns as opposed to having methods on the relevant
class, but ok

+  return (mediaSubType == kCVPixelFormatType_420YpCbCr8PlanarFullRange ||
+          mediaSubType == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange);
+}
 
-// Mapping from cricket::VideoFormat to AVCaptureSession presets.
-static NSString *GetSessionPresetForVideoFormat(
-  const cricket::VideoFormat& format) {
-  for (const auto preset : kAvailablePresets) {
-    // Check both orientations
-    if ((format.width == preset.width && format.height == preset.height) ||
-        (format.width == preset.height && format.height == preset.width)) {
-      return preset.sessionPreset;
+static inline BOOL IsFrameRateWithinRange(int fps, AVFrameRateRange* range) {

[tkchin_webrtc, 2016/10/05 17:33:12]

nit: *range

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+  return range.minFrameRate <= fps && range.maxFrameRate >= fps;
+}
+
+// Returns filtered array of device formats based on predefined constraints our
+// stack imposes.
+static NSArray<AVCaptureDeviceFormat *> *GetEligibleDeviceFormats(
+    const AVCaptureDevice *device,
+    int supportedFps) {
+  NSMutableArray<AVCaptureDeviceFormat *> *eligibleDeviceFormats =
+      [NSMutableArray array];
+
+  for (AVCaptureDeviceFormat *format in device.formats) {
+    // Filter out subTypes that we currently don't support in the stack
+    FourCharCode mediaSubType =
+        CMFormatDescriptionGetMediaSubType(format.formatDescription);
+    if (!IsMediaSubTypeSupported(mediaSubType)) {
+      continue;
+    }
+
+    // Filter out frame rate ranges that we currently don't support in the stack
+    for (AVFrameRateRange *frameRateRange in format
+             .videoSupportedFrameRateRanges) {

[tkchin_webrtc, 2016/10/05 17:33:12]

nit: don't split properties across lines
if it doesn't fit in 100 chars, declare a local instead

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+      if (IsFrameRateWithinRange(supportedFps, frameRateRange)) {
+        [eligibleDeviceFormats addObject:format];
+        break;
+      }
     }
   }
-  // If no matching preset is found, use a default one.
-  return AVCaptureSessionPreset640x480;
+
+  return [eligibleDeviceFormats copy];

[tkchin_webrtc, 2016/10/05 17:33:12]

safety is fine, but probably not needed. fine to leave though

+}
+
+// Mapping from cricket::VideoFormat to AVCaptureDeviceFormat.
+static AVCaptureDeviceFormat *GetDeviceFormatForVideoFormat(
+    const AVCaptureDevice *device,
+    const cricket::VideoFormat& videoFormat) {

[tkchin_webrtc, 2016/10/05 17:33:12]

be consistent in where you place & and *
ObjC convention is &videoFormat

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+  AVCaptureDeviceFormat *desiredDeviceFormat = nil;
+  NSArray<AVCaptureDeviceFormat *>* eligibleFormats =

[tkchin_webrtc, 2016/10/05 17:33:12]

nit: *eligibleFormats

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+      GetEligibleDeviceFormats(device, videoFormat.framerate());
+
+  for (AVCaptureDeviceFormat *deviceFormat in eligibleFormats) {
+    CMVideoDimensions dimension =
+        CMVideoFormatDescriptionGetDimensions(deviceFormat.formatDescription);
+    FourCharCode mediaSubType =
+        CMFormatDescriptionGetMediaSubType(deviceFormat.formatDescription);
+
+    if (videoFormat.width == dimension.width &&
+        videoFormat.height == dimension.height) {
+      if (mediaSubType == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) {
+        // This is the preferred format so no need to wait for better option.
+        return deviceFormat;
+      } else {
+        // This is good candidate, but let's wait for something better.
+        desiredDeviceFormat = deviceFormat;
+      }
+    }
+  }
+
+  return desiredDeviceFormat;
+}
+
+// Mapping from AVCaptureDeviceFormat to cricket::VideoFormat for given input
+// device.
+static std::set<cricket::VideoFormat> GetSupportedVideoFormatsForDevice(
+    AVCaptureDevice *device) {
+  std::set<cricket::VideoFormat> supportedFormats;
+
+  NSArray<AVCaptureDeviceFormat *> *eligibleFormats =
+      GetEligibleDeviceFormats(device, kFramesPerSecond);
+
+  for (AVCaptureDeviceFormat *deviceFormat in eligibleFormats) {
+    CMVideoDimensions dimension =
+        CMVideoFormatDescriptionGetDimensions(deviceFormat.formatDescription);
+    cricket::VideoFormat format = cricket::VideoFormat(
+        dimension.width, dimension.height,
+        cricket::VideoFormat::FpsToInterval(kFramesPerSecond),
+        cricket::FOURCC_NV12);
+    supportedFormats.insert(format);
+  }
+
+  return supportedFormats;
+}
+
+// Sets device format for the provided capture device. Returns YES/NO depending on success.
+// TODO(denicija): When this file is split this static method should be reconsidered.
+// Perhaps adding a category on AVCaptureDevice would be better.
+static BOOL SetFormatForCaptureDevice(AVCaptureDevice* device,

[tkchin_webrtc, 2016/10/05 17:33:12]

ditto * placement throughout the fn

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+                                      AVCaptureSession* session,
+                                      const cricket::VideoFormat& format) {
+

[tkchin_webrtc, 2016/10/05 17:33:12]

nit: remove blank line

+  AVCaptureDeviceFormat* deviceFormat =
+      GetDeviceFormatForVideoFormat(device, format);
+  const int fps = cricket::VideoFormat::IntervalToFps(format.interval);
+
+  NSError* error = nil;
+  BOOL success = YES;

[tkchin_webrtc, 2016/10/05 17:33:11]

nit: set this to NO and modify it to YES to save one line

[daniela-webrtc, 2016/10/06 08:40:03]

Since there are different code paths that can cause a failure, it would actually
be more code to start with a NO. (Because of the try-catch block mostly i.e how
do we ensure that exception has not occured) 

+  [session beginConfiguration];
+  if ([device lockForConfiguration:&error]) {
+    @try {
+      device.activeFormat = deviceFormat;
+      device.activeVideoMinFrameDuration = CMTimeMake(1, fps);
+    } @catch (NSException* exception) {
+      RTCLogError(
+          @"Exception occured while setting active format!\n User info:%@",

[tkchin_webrtc, 2016/10/05 17:33:12]

nit: make the errors as short as possible
e.g. Failed to set active format. UserInfo:
e.g. Failed to lock device. Error: %@

[daniela-webrtc, 2016/10/06 08:40:03]

Done.

+          exception.userInfo);
+      success = NO;
+    }
+
+    [device unlockForConfiguration];
+  } else {
+    RTCLogError(
+        @"Error occured while locking device %@. Underlying error: %@",
+        device, error.userInfo);
+    success = NO;
+  }
+  [session commitConfiguration];
+
+  return success;
 }
 
 // This class used to capture frames using AVFoundation APIs on iOS. It is meant
 // to be owned by an instance of AVFoundationVideoCapturer. The reason for this
 // because other webrtc objects own cricket::VideoCapturer, which is not
 // ref counted. To prevent bad behavior we do not expose this class directly.
 @interface RTCAVFoundationVideoCapturerInternal : NSObject
     <AVCaptureVideoDataOutputSampleBufferDelegate>
 
 @property(nonatomic, readonly) AVCaptureSession *captureSession;
 @property(nonatomic, readonly) dispatch_queue_t frameQueue;
 @property(nonatomic, readonly) BOOL canUseBackCamera;
 @property(nonatomic, assign) BOOL useBackCamera;  // Defaults to NO.
 @property(nonatomic, assign) BOOL isRunning;  // Whether the capture session is running.
 @property(atomic, assign) BOOL hasStarted;  // Whether we have an unmatched start.
 
 // We keep a pointer back to AVFoundationVideoCapturer to make callbacks on it
 // when we receive frames. This is safe because this object should be owned by
 // it.
 - (instancetype)initWithCapturer:(webrtc::AVFoundationVideoCapturer *)capturer;
 - (AVCaptureDevice *)getActiveCaptureDevice;

[tkchin_webrtc, 2016/10/05 17:33:11]

I don't recall this bit of code. We don't prefix accessors with "get" in objc

[daniela-webrtc, 2016/10/06 08:40:03]

I noticed that as well, so that's why I renamed the new methods without the
"get". This should be changed in the upcoming refactor

 
+- (nullable AVCaptureDevice *)frontCaptureDevice;

[tkchin_webrtc, 2016/10/05 17:33:12]

I'm surprised this isn't throwing warnings at you since the rest of the decls
don't have nullability attrs.

+- (nullable AVCaptureDevice *)backCaptureDevice;
+
 // Starts and stops the capture session asynchronously. We cannot do this
 // synchronously without blocking a WebRTC thread.
 - (void)start;
 - (void)stop;
 
 @end
 
 @implementation RTCAVFoundationVideoCapturerInternal {
   // Keep pointers to inputs for convenience.
   AVCaptureDeviceInput *_frontCameraInput;
@@ skipping 68 matching lines @@
 }
 
 - (AVCaptureSession *)captureSession {
   return _captureSession;
 }
 
 - (AVCaptureDevice *)getActiveCaptureDevice {
   return self.useBackCamera ? _backCameraInput.device : _frontCameraInput.device;
 }
 
+- (AVCaptureDevice *)frontCaptureDevice {
+  return _frontCameraInput.device;
+}
+
+- (AVCaptureDevice *)backCaptureDevice {
+  return _backCameraInput.device;
+}
+
 - (dispatch_queue_t)frameQueue {
   if (!_frameQueue) {
     _frameQueue =
         dispatch_queue_create("org.webrtc.avfoundationvideocapturer.video",
                               DISPATCH_QUEUE_SERIAL);
     dispatch_set_target_queue(
         _frameQueue,
         dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0));
   }
   return _frameQueue;
@@ skipping 31 matching lines @@
 
 - (BOOL)isRunning {
   rtc::CritScope cs(&_crit);
   return _isRunning;
 }
 
 - (void)setIsRunning:(BOOL)isRunning {
   rtc::CritScope cs(&_crit);
   _isRunning = isRunning;
 }
 

[daniela-webrtc, 2016/10/06 08:40:03]

This is from the rebase

 // Called from WebRTC thread.
 - (void)start {
   if (self.hasStarted) {
     return;
   }
   self.hasStarted = YES;
   [RTCDispatcher dispatchAsyncOnType:RTCDispatcherTypeCaptureSession
                                block:^{
 #if TARGET_OS_IPHONE
      // Default to portrait orientation on iPhone. This will be reset in
@@ skipping 279 matching lines @@
     if (!backCameraInput) {
       RTCLogError(@"Failed to create front camera input: %@",
                   error.localizedDescription);
       return nil;
     }
     _backCameraInput = backCameraInput;
   }
   return _backCameraInput;
 }
 
-- (void)setMinFrameDuration:(CMTime)minFrameDuration
-                  forDevice:(AVCaptureDevice *)device {
-  NSError *error = nil;
-  if (![device lockForConfiguration:&error]) {
-    RTCLogError(@"Failed to lock device for configuration. Error: %@", error.localizedDescription);
-    return;
-  }
-  device.activeVideoMinFrameDuration = minFrameDuration;
-  [device unlockForConfiguration];
-}
-
 // Called from capture session queue.
 - (void)updateOrientation {
 #if TARGET_OS_IPHONE
   switch ([UIDevice currentDevice].orientation) {
     case UIDeviceOrientationPortrait:
       _rotation = webrtc::kVideoRotation_90;
       break;
     case UIDeviceOrientationPortraitUpsideDown:
       _rotation = webrtc::kVideoRotation_270;
       break;
@@ skipping 24 matching lines @@
       newInput = _backCameraInput;
     }
     if (oldInput) {
       // Ok to remove this even if it's not attached. Will be no-op.
       [_captureSession removeInput:oldInput];
     }
     if (newInput) {
       [_captureSession addInput:newInput];
     }
     [self updateOrientation];
+    AVCaptureDevice *newDevice = newInput.device;
+    const cricket::VideoFormat* format = _capturer->GetCaptureFormat();

[tkchin_webrtc, 2016/10/05 17:33:12]

*format

+    SetFormatForCaptureDevice(newDevice, _captureSession, *format);
     [_captureSession commitConfiguration];
-
-    const auto fps = cricket::VideoFormat::IntervalToFps(_capturer->GetCaptureFormat()->interval);
-    [self setMinFrameDuration:CMTimeMake(1, fps)forDevice:newInput.device];
   }];
 }
 
 @end
 
 namespace webrtc {
 
 enum AVFoundationVideoCapturerMessageType : uint32_t {
   kMessageTypeFrame,
 };
 
 AVFoundationVideoCapturer::AVFoundationVideoCapturer() : _capturer(nil) {
-  // Set our supported formats. This matches kAvailablePresets.
   _capturer =
       [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this];
 
-  std::vector<cricket::VideoFormat> supported_formats;
-  int framerate = 30;
+  std::set<cricket::VideoFormat> front_camera_video_formats =
+      GetSupportedVideoFormatsForDevice([_capturer frontCaptureDevice]);
 
-#if TARGET_OS_IPHONE
-  if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
-    set_enable_video_adapter(false);
-    framerate = 15;
+  std::set<cricket::VideoFormat> back_camera_video_formats =
+      GetSupportedVideoFormatsForDevice([_capturer backCaptureDevice]);
+
+  std::vector<cricket::VideoFormat> intersection_video_formats;
+  if (back_camera_video_formats.empty()) {
+    intersection_video_formats.assign(front_camera_video_formats.begin(),
+                                      front_camera_video_formats.end());
+
+  } else if (front_camera_video_formats.empty()) {
+    intersection_video_formats.assign(back_camera_video_formats.begin(),
+                                      back_camera_video_formats.end());
+  } else {
+    std::set_intersection(

[magjed_webrtc, 2016/10/04 13:21:25]

nit: At least according to C++ style you should either write it as:
std::set_intersection(front_camera_video_formats.begin(), ...
                      back_camera_video_formats.begin(), ...
or break and indent 4 spaces:
std::set_intersection(
    front_camera_video_formats.begin(), ...
    back_camera_video_formats.begin(), ...

+                          front_camera_video_formats.begin(), front_camera_video_formats.end(),
+                          back_camera_video_formats.begin(), back_camera_video_formats.end(),
+                          std::back_inserter(intersection_video_formats));
   }
-#endif
-
-  for (const auto preset : kAvailablePresets) {
-    if ([_capturer.captureSession canSetSessionPreset:preset.sessionPreset]) {
-      const auto format = cricket::VideoFormat(
-        preset.width,
-        preset.height,
-        cricket::VideoFormat::FpsToInterval(framerate),
-        cricket::FOURCC_NV12);
-      supported_formats.push_back(format);
-    }
-  }
-
-  SetSupportedFormats(supported_formats);
+  SetSupportedFormats(intersection_video_formats);
 }
 
 AVFoundationVideoCapturer::~AVFoundationVideoCapturer() {
   _capturer = nil;
 }
 
 cricket::CaptureState AVFoundationVideoCapturer::Start(
     const cricket::VideoFormat& format) {
   if (!_capturer) {
     LOG(LS_ERROR) << "Failed to create AVFoundation capturer.";
     return cricket::CaptureState::CS_FAILED;
   }
   if (_capturer.isRunning) {
     LOG(LS_ERROR) << "The capturer is already running.";
     return cricket::CaptureState::CS_FAILED;
   }
 
-  NSString *desiredPreset = GetSessionPresetForVideoFormat(format);
-  RTC_DCHECK(desiredPreset);
+  AVCaptureDevice* device = [_capturer getActiveCaptureDevice];
+  AVCaptureSession* session = _capturer.captureSession;
 
-  [_capturer.captureSession beginConfiguration];
-  if (![_capturer.captureSession canSetSessionPreset:desiredPreset]) {
-    LOG(LS_ERROR) << "Unsupported video format.";
-    [_capturer.captureSession commitConfiguration];
+  if (!SetFormatForCaptureDevice(device, session, format)) {
     return cricket::CaptureState::CS_FAILED;
   }
-  _capturer.captureSession.sessionPreset = desiredPreset;
-  [_capturer.captureSession commitConfiguration];
 
   SetCaptureFormat(&format);
   // This isn't super accurate because it takes a while for the AVCaptureSession
   // to spin up, and this call returns async.
   // TODO(tkchin): make this better.
   [_capturer start];
   SetCaptureState(cricket::CaptureState::CS_RUNNING);
 
-  // Adjust the framerate for all capture devices.
-  const auto fps = cricket::VideoFormat::IntervalToFps(format.interval);
-  AVCaptureDevice *activeDevice = [_capturer getActiveCaptureDevice];
-  [_capturer setMinFrameDuration:CMTimeMake(1, fps)forDevice:activeDevice];
-
   return cricket::CaptureState::CS_STARTING;
 }
 
 void AVFoundationVideoCapturer::Stop() {
   [_capturer stop];
   SetCaptureFormat(NULL);
 }
 
 bool AVFoundationVideoCapturer::IsRunning() {
   return _capturer.isRunning;
@@ skipping 79 matching lines @@
       buffer = rotated_buffer;
     }
   }
 
   OnFrame(cricket::WebRtcVideoFrame(buffer, rotation,
                                     translated_camera_time_us, 0),
           captured_width, captured_height);
 }
 
 }  // namespace webrtc