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;
-};
+static const int kFramesPerSecond = 30;

[magjed_webrtc, 2016/09/27 14:36:10]

Can you add a todo, something like:
// TODO(denicija): Support higher framerates. See http://crbug/webrtc/6355 for
more info.
You can put it in my name if you don't want to own it.

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

 
-#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
+// Maping from cricket::VideoFormat to AVCaptureDeviceFormat.

[tkchin_webrtc, 2016/09/27 13:39:34]

nit: Mapping

[daniela-webrtc, 2016/09/28 15:29:27]

Done.

+static AVCaptureDeviceFormat* GetDeviceFormatForVideoFormat(
+    const AVCaptureDevice* device,
+    const cricket::VideoFormat& videoFormat) {
+  AVCaptureDeviceFormat* desiredDeviceFormat = nil;

[tkchin_webrtc, 2016/09/27 13:39:34]

if treating this as a C/C++ function, should be desired_device_format
likewise for other vars here
Otherwise just move the *

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+  for (AVCaptureDeviceFormat* deviceFormat in [device formats]) {

[tkchin_webrtc, 2016/09/27 13:39:33]

dot syntax for properties. Here and elsewhere

[daniela-webrtc, 2016/09/28 15:29:27]

Done.

+    CMVideoDimensions dimension =
+        CMVideoFormatDescriptionGetDimensions([deviceFormat formatDescription]);
+    FourCharCode code =
+        CMFormatDescriptionGetMediaSubType([deviceFormat formatDescription]);
 
-// 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;
+    if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange &&

[tkchin_webrtc, 2016/09/27 13:39:33]

If we care about only selectively picking out NV12 here, we could probably also
pick whether we want full or video. When do we need full range? Doesn't matter
for this CL really, feel free to keep both, but would be good to understand.

[magjed_webrtc, 2016/09/27 14:36:10]

Ah, true. We hardcode the pixel format to FullRange in videoDataOutput so we can
probably do it here as well. That would simplify the code a bit.

[daniela-webrtc, 2016/09/28 15:29:26]

Not sure I follow. The reason why we allow both full and video range is because
some devices only support video range (i.e macs). This check here is only an
early return so that we don't loop through formats that are neither. 
Then at line 60 we prioritize FullRange over VideoRange.
But if unclear I might introduce helper methods like isSubMediaTypeSupported()
or something along those lines. Is that you were thinking? 

+        code != kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) {
+      continue;
+    }
+
+    if (videoFormat.width == dimension.width &&
+        videoFormat.height == dimension.height) {
+      for (AVFrameRateRange* framerate in
+           [deviceFormat videoSupportedFrameRateRanges]) {
+        if (!(framerate.minFrameRate >= kFramesPerSecond &&

[tkchin_webrtc, 2016/09/27 13:39:34]

perhaps frameRate.minFrameRate < kFramesPerSecond || frameRate.maxFrameRate >
kFramesPerSecond
up to you

[daniela-webrtc, 2016/09/28 15:29:26]

Great catch. Thanks

+              framerate.maxFrameRate <= kFramesPerSecond)) {

[magjed_webrtc, 2016/09/27 14:36:10]

I think this check is incorrect, the comparison is inversed. Swap '<=' with '>='
or do like Zeke suggests. Also, I think we should base this on
videoFormat.framerate() rather than kFramesPerSecond.

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+          continue;
+        }
+      }
+      if (code == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) {
+        // this is the preferred format so no need to wait for better option

[tkchin_webrtc, 2016/09/27 13:39:34]

nit: Capitalize first letter of comment and end with period.
here and elsewhere

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+        return deviceFormat;
+      } else {
+        // this is good candidate, but let's wait for something better
+        desiredDeviceFormat = deviceFormat;

[magjed_webrtc, 2016/09/27 14:36:10]

I'm wondering how we should select between multiple ranges that include 30 fps,
for example 2-30 fps range and 5-240 fps range. In that case we probably want
2-30 fps. I think in general we want a low lower fps bound to allow the camera
to compensate for brightness. Does anyone know if the upper fps bound matter at
all if we cap the fps to 30?

+      }
     }
   }
-  // If no matching preset is found, use a default one.
-  return AVCaptureSessionPreset640x480;
+  return desiredDeviceFormat;
+}
+
+// Mapping from AVCaptureDeviceFormat to cricket::VideoFormat for given input
+// device.
+static std::set<cricket::VideoFormat> GetSupportedVideoFormatsForDevice(
+    AVCaptureDevice* device) {

[tkchin_webrtc, 2016/09/27 13:39:33]

ditto pick one of C++/ObjC style conventions to use but don't mix them

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+  std::set<cricket::VideoFormat> supportedFormats;
+
+  for (AVCaptureDeviceFormat* deviceFormat in [device formats]) {
+    FourCharCode code =
+        CMFormatDescriptionGetMediaSubType([deviceFormat formatDescription]);
+
+    if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange &&

[tkchin_webrtc, 2016/09/27 13:39:33]

I would've expected this code to output all possible formats including non NV12
based on the function name. Perhaps add comment to indicate only NV12 formats?

[magjed_webrtc, 2016/09/27 14:36:10]

FYI - We currently only support NV12 because the conversion is hardcoded to
libyuv::NV12ToI420 in CoreVideoFrameBuffer. I also think NV12 is the most native
and fastest format for iOS so probably fine to not support anything else. A
comment would be nice though.

+        code != kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) {
+      continue;
+    }
+
+    CMVideoDimensions dimension =
+        CMVideoFormatDescriptionGetDimensions([deviceFormat formatDescription]);
+

[magjed_webrtc, 2016/09/27 14:36:10]

I think we should have the fps check here as well, to make sure all formats we
return here are allowed in GetDeviceFormatForVideoFormat.

[daniela-webrtc, 2016/09/28 15:29:26]

Agreed. I don't like how I handled the filtering of device formats  in both
these methods (a lot of duplicate code for eligible device formats) and it leads
to inconsistency like this one. 
I'll create separate function that returns only eligible  device formats and
respects all the constraints i.e subMediaType and framerate. I think it will
make the code easier to understand as well.

[daniela-webrtc, 2016/09/30 11:35:59]

I've uploaded a patch with this change. Please let me know what you think and if
it's more readable now.

+    cricket::VideoFormat format = cricket::VideoFormat(
+        dimension.width, dimension.height,
+        cricket::VideoFormat::FpsToInterval(kFramesPerSecond),
+        cricket::FOURCC_NV12);
+    supportedFormats.insert(format);
+  }
+
+  return supportedFormats;
 }
 
 // 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;
 
+- (nullable AVCaptureDevice*)frontCaptureDevice;

[tkchin_webrtc, 2016/09/27 13:39:34]

not sure we need nullability annotations for interfaces that we expect to never
have to interop with swift
If we add this though, we should add the assume_nonnull_begin / end macros in
the right places

[daniela-webrtc, 2016/09/28 15:29:26]

In general I like nullable/nonnull annotations as it helps with static analysis
and also makes the intent clearer.
However it might not be suitable for this CL. I'll remove it for now.

+- (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;

[tkchin_webrtc, 2016/09/27 13:39:33]

nit: Device *)
here and below

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+}
+
+- (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 331 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];

[tkchin_webrtc, 2016/09/27 13:39:34]

Device *newDevice
here and below

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+
+    NSError* error = nil;
+    const cricket::VideoFormat* format = _capturer->GetCaptureFormat();

[tkchin_webrtc, 2016/09/27 13:39:34]

is this thread safe? (Calling from different queue)

[daniela-webrtc, 2016/09/29 11:02:31]

Not sure. If i understand correctly the capture format is only set once and only
at the beginning so this read shouldn't be a problem.
If I'm wrong I'll add some thread safe mechanism

+    AVCaptureDeviceFormat* newFormat =
+        GetDeviceFormatForVideoFormat(newDevice, *format);

[tkchin_webrtc, 2016/09/27 13:39:33]

How expensive is this operation? Is it slow to query for available formats /
should we build a cache instead?

[daniela-webrtc, 2016/09/29 11:02:31]

Shouldn't be too expensive and ideally shouldn't happen too often.

+    const auto fps = cricket::VideoFormat::IntervalToFps(_capturer->GetCaptureFormat()->interval);
+    if ([newDevice lockForConfiguration:&error]) {
+      @try {
+        [newDevice setActiveFormat:newFormat];

[tkchin_webrtc, 2016/09/27 13:39:33]

Use dot syntax where able
.activeFormat =

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+        [newDevice setActiveVideoMinFrameDuration:CMTimeMake(1, fps)];
+      } @catch (NSException* exception) {
+        LOG(LS_ERROR) << [NSString stringWithFormat:@"Exception occured while "

[tkchin_webrtc, 2016/09/27 13:39:34]

We've used RTCLogError as a convenience 

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

[daniela-webrtc, 2016/09/29 11:02:31]

For future reference, do we use RTCLogError in Objc and LOG(LS_ERROR) in cpp
classes?

+                                                    @"setting active "
+                                                    @"format!\n User info:%@",
+                                                    exception.userInfo];
+      }
+      [newDevice unlockForConfiguration];
+    }
     [_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> frontCameraSupportedVideoFormats =

[tkchin_webrtc, 2016/09/27 13:39:34]

c++ style here
so front_camera_supported_video_formats

[daniela-webrtc, 2016/09/28 15:29:26]

Done.

+      GetSupportedVideoFormatsForDevice([_capturer frontCaptureDevice]);
 
-#if TARGET_OS_IPHONE
-  if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
-    set_enable_video_adapter(false);
-    framerate = 15;
-  }
-#endif
+  std::set<cricket::VideoFormat> backCameraSupportedVideoFormats =
+      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> intersectionVideoFormats;
+  if (backCameraSupportedVideoFormats.size() == 0) {
+    std::copy(frontCameraSupportedVideoFormats.begin(),
+              frontCameraSupportedVideoFormats.end(),
+              std::back_inserter(intersectionVideoFormats));
+    SetSupportedFormats(intersectionVideoFormats);
+    return;
   }
 
-  SetSupportedFormats(supported_formats);
+  if (frontCameraSupportedVideoFormats.size() == 0) {
+    std::copy(backCameraSupportedVideoFormats.begin(),
+              backCameraSupportedVideoFormats.end(),
+              std::back_inserter(intersectionVideoFormats));
+    SetSupportedFormats(intersectionVideoFormats);
+    return;
+  }
+
+  std::set_intersection(frontCameraSupportedVideoFormats.begin(),
+                        frontCameraSupportedVideoFormats.end(),
+                        backCameraSupportedVideoFormats.begin(),
+                        backCameraSupportedVideoFormats.end(),
+                        std::back_inserter(intersectionVideoFormats));
+
+  SetSupportedFormats(intersectionVideoFormats);
 }
 
 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);
+  AVCaptureDeviceFormat* deviceFormat =
+      GetDeviceFormatForVideoFormat([_capturer getActiveCaptureDevice], format);
+  const auto fps = cricket::VideoFormat::IntervalToFps(format.interval);
 
-  [_capturer.captureSession beginConfiguration];
-  if (![_capturer.captureSession canSetSessionPreset:desiredPreset]) {
-    LOG(LS_ERROR) << "Unsupported video format.";
-    [_capturer.captureSession commitConfiguration];
-    return cricket::CaptureState::CS_FAILED;
+  AVCaptureDevice* device = [_capturer getActiveCaptureDevice];
+  AVCaptureSession* session = [_capturer captureSession];
+  NSError* error = nil;
+
+  [session beginConfiguration];  // the session to which the receiver's

[tkchin_webrtc, 2016/09/27 13:39:34]

nit: don't split comments out at end of line, place above if it spans multiple
lines

[daniela-webrtc, 2016/09/28 15:29:26]

Actually this comment is not needed. I'll remove it altogether.

[daniela-webrtc, 2016/09/30 11:35:59]

Done.

+                                 // AVCaptureDeviceInput is added.
+  if ([device lockForConfiguration:&error]) {

[magjed_webrtc, 2016/09/27 14:36:10]

Can we extract this out in a common function:
void SetActiveFormat(AVCaptureDevice* device, const cricket::VideoFormat&
videoFormat);
and in updateSessionInputForUseBackCamera we do:
SetActiveFormat([newInput device], *_capturer->GetCaptureFormat());
and here we do:
[session beginConfiguration];
SetActiveFormat([_capturer getActiveCaptureDevice], format);
[session commitConfiguration];
?

[daniela-webrtc, 2016/09/29 11:02:31]

YES! I forgot to do this. Good catch. Thanks

[daniela-webrtc, 2016/09/30 11:35:59]

Done.

+    @try {
+      [device setActiveFormat:deviceFormat];
+      [device setActiveVideoMinFrameDuration:CMTimeMake(1, fps)];
+    } @catch (NSException* exception) {
+      LOG(LS_ERROR) << [NSString
+          stringWithFormat:
+              @"Exception occured while setting active format!\n User info:%@",
+              exception.userInfo];
+      return cricket::CaptureState::CS_FAILED;
+    }
+
+    [device unlockForConfiguration];
   }
-  _capturer.captureSession.sessionPreset = desiredPreset;
-  [_capturer.captureSession commitConfiguration];
+
+  [session 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