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

[magjed_webrtc, 2016/10/03 15:32:34]

nit: add dot at end of every sentence in comments.

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+// 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) {
+  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) {
+  return range.minFrameRate <= fps && range.maxFrameRate >= fps;
+}
+
+// Returns filtered array of device formats based on predefined constraints our
+// stack imposes.
+static NSArray<AVCaptureDeviceFormat *>* GetEligibleDeviceFormats(

[kthelgason, 2016/10/03 13:57:54]

nit: location of * is inconsistent, should stick to function name.

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+    const AVCaptureDevice *device,
+    int supportedFps) {
+  NSMutableArray<AVCaptureDeviceFormat *> *eligibleDeviceFormats =
+      [NSMutableArray array];
+
+  for (AVCaptureDeviceFormat *format in device.formats) {

[kthelgason, 2016/10/03 13:57:53]

Can we make this const?

+    // 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

[kthelgason, 2016/10/03 13:57:53]

ditto

+             .videoSupportedFrameRateRanges) {
+      if (IsFrameRateWithinRange(supportedFps, frameRateRange)) {
+        [eligibleDeviceFormats addObject:format];
+        continue;

[magjed_webrtc, 2016/10/03 15:32:34]

nit: Use break instead of continue to avoid adding the same format multiple
times. The set filtering will take care of it later anyway, but still.

+      }
     }
   }
-  // If no matching preset is found, use a default one.
-  return AVCaptureSessionPreset640x480;
+
+  return [eligibleDeviceFormats copy];
+}
+
+// Mapping from cricket::VideoFormat to AVCaptureDeviceFormat.
+static AVCaptureDeviceFormat *GetDeviceFormatForVideoFormat(
+    const AVCaptureDevice *device,
+    const cricket::VideoFormat& videoFormat) {
+  AVCaptureDeviceFormat *desiredDeviceFormat = nil;
+  NSArray<AVCaptureDeviceFormat *>* eligibleFormats =
+      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 =

[kthelgason, 2016/10/03 13:57:53]

nit: inconsistent * location.

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+      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 recnosidered.

[magjed_webrtc, 2016/10/03 15:32:34]

nit: reconsidered

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+// Perhaps adding a category on AVCaptureDevice would be better.
+static BOOL SetFormatForCaptureDevice(AVCaptureDevice* device,
+                                      AVCaptureSession* session,
+                                      const cricket::VideoFormat& format) {
+
+  AVCaptureDeviceFormat* deviceFormat =
+      GetDeviceFormatForVideoFormat(device, format);
+  const auto fps = cricket::VideoFormat::IntervalToFps(format.interval);

[magjed_webrtc, 2016/10/03 15:32:34]

Use int instead of auto here.

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+
+  NSError* error = nil;
+  BOOL success = YES;
+  [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:%@",
+          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;
 
+- (nonnull AVCaptureDevice *)frontCaptureDevice;

[daniela-webrtc, 2016/10/04 11:47:03]

This should be nullable.

+- (AVCaptureDevice *)backCaptureDevice;

[daniela-webrtc, 2016/10/04 11:47:03]

This should be nullable

+
 // 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 262 matching lines @@
 }
 
 - (AVCaptureVideoDataOutput *)videoDataOutput {
   if (!_videoDataOutput) {
     // Make the capturer output NV12. Ideally we want I420 but that's not
     // currently supported on iPhone / iPad.
     AVCaptureVideoDataOutput *videoDataOutput =
         [[AVCaptureVideoDataOutput alloc] init];
     videoDataOutput.videoSettings = @{
       (NSString *)kCVPixelBufferPixelFormatTypeKey :
         @(kCVPixelFormatType_420YpCbCr8BiPlanarFullRange)

[magjed_webrtc, 2016/10/03 16:58:53]

Couldn't we remove this?

[daniela-webrtc, 2016/10/04 11:47:03]

The app crashes if this is removed.  It happened because this code is called
before the deviceFormat is assigned. The chosen videoOutput, in this case, is
device native but RTC-not supported format.
I think this can be done as part of the refactoring efforts as I don't think it
would be a minor task.

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

Acknowledged.

     };
     videoDataOutput.alwaysDiscardsLateVideoFrames = NO;
     [videoDataOutput setSampleBufferDelegate:self queue:self.frameQueue];
     _videoDataOutput = videoDataOutput;
   }
   return _videoDataOutput;
 }
 
 - (AVCaptureDevice *)videoCaptureDeviceForPosition:
     (AVCaptureDevicePosition)position {
@@ skipping 48 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

[kthelgason, 2016/10/03 13:57:53]

Maybe we should use #if defined(WEBRTC_IOS), just for consistency with the rest
of the codebase?

[daniela-webrtc, 2016/10/04 13:23:22]

Perhaps as part of the upcoming refactor? 

   switch ([UIDevice currentDevice].orientation) {
     case UIDeviceOrientationPortrait:
       _rotation = webrtc::kVideoRotation_90;
       break;
     case UIDeviceOrientationPortraitUpsideDown:
       _rotation = webrtc::kVideoRotation_270;
       break;
     case UIDeviceOrientationLandscapeLeft:
       _rotation = webrtc::kVideoRotation_180;
       break;
     case UIDeviceOrientationLandscapeRight:
       _rotation = webrtc::kVideoRotation_0;
       break;
     case UIDeviceOrientationFaceUp:
     case UIDeviceOrientationFaceDown:
     case UIDeviceOrientationUnknown:
       // Ignore.
       break;
   }
 #endif
 }
 
 // Update the current session input to match what's stored in _useBackCamera.
 - (void)updateSessionInputForUseBackCamera:(BOOL)useBackCamera {
   [RTCDispatcher dispatchAsyncOnType:RTCDispatcherTypeCaptureSession
                                block:^{
     [_captureSession beginConfiguration];

[kthelgason, 2016/10/03 13:57:54]

No matching call to commitConfiguration

[daniela-webrtc, 2016/10/04 11:47:03]

Good catch!

     AVCaptureDeviceInput *oldInput = _backCameraInput;
     AVCaptureDeviceInput *newInput = _frontCameraInput;
     if (useBackCamera) {
       oldInput = _frontCameraInput;
       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];
-    [_captureSession commitConfiguration];
-
-    const auto fps = cricket::VideoFormat::IntervalToFps(_capturer->GetCaptureFormat()->interval);
-    [self setMinFrameDuration:CMTimeMake(1, fps)forDevice:newInput.device];
+    AVCaptureDevice *newDevice = newInput.device;
+    const cricket::VideoFormat* format = _capturer->GetCaptureFormat();
+    SetFormatForCaptureDevice(newDevice, _captureSession, *format);
   }];
 }
 
 @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;
-  }
-#endif
+  std::set<cricket::VideoFormat> back_camera_video_formats =
+      GetSupportedVideoFormatsForDevice([_capturer backCaptureDevice]);
 
-  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);
-    }
+  std::vector<cricket::VideoFormat> intersection_video_formats;
+  if (back_camera_video_formats.size() == 0) {

[magjed_webrtc, 2016/10/03 15:32:34]

nit: Use *.empty() instead of *.size() == 0.

[daniela-webrtc, 2016/10/04 11:47:03]

Done.

+    std::copy(front_camera_video_formats.begin(),
+              front_camera_video_formats.end(),
+              std::back_inserter(intersection_video_formats));
+    SetSupportedFormats(intersection_video_formats);

[kthelgason, 2016/10/03 13:57:53]

Another option that may be less verbose is to use the std::vector range
constructor here, and just do:
SetSupportedFormats(std::vector<T>(set.begin(), set.end()))
But totally up to you.

[magjed_webrtc, 2016/10/03 15:32:34]

Yet another way to do it:
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(
      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));  
}
SetSupportedFormats(intersection_video_formats);

BTW - will front_camera_video_formats ever be empty? I see that you have marked
frontCaptureDevice nonnull.

[daniela-webrtc, 2016/10/04 11:47:03]

I'll go the assign() route to have one return point in the method.
Regarding the nonnull device format, not sure what happened between patches. It
should be nullable. I think having an unavailable front camera is feasible
scenario.

+    return;
   }
 
-  SetSupportedFormats(supported_formats);
+  if (front_camera_video_formats.size() == 0) {
+    std::copy(back_camera_video_formats.begin(),
+              back_camera_video_formats.end(),
+              std::back_inserter(intersection_video_formats));
+    SetSupportedFormats(intersection_video_formats);
+    return;
+  }
+
+  std::set_intersection(
+      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));
+
+  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