Replace SessionPresets with AVCaptureDeviceFormats

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

Index: webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
diff --git a/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm b/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
index 5708346ae2bccdee06da02c5eea8d2a13d37d453..6e8b84d5e177715186490bec5e805cc3c25bba69 100644
--- a/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
+++ b/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
@@ -30,41 +30,71 @@
 #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.

+
+// 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]);
+
+    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 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
+    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?

+      }
+    }
+  }
+  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]);
 
-// 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]

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);
   }
-  // If no matching preset is found, use a default one.
-  return AVCaptureSessionPreset640x480;
+
+  return supportedFormats;
 }
 
 // This class used to capture frames using AVFoundation APIs on iOS. It is meant
@@ -87,6 +117,9 @@ static NSString *GetSessionPresetForVideoFormat(
 - (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;
@@ -175,6 +208,14 @@ static NSString *GetSessionPresetForVideoFormat(
   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 =
@@ -526,17 +567,6 @@ static NSString *GetSessionPresetForVideoFormat(
   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
@@ -581,10 +611,26 @@ static NSString *GetSessionPresetForVideoFormat(
       [_captureSession addInput:newInput];
     }
     [self updateOrientation];
-    [_captureSession commitConfiguration];
+    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);
-    [self setMinFrameDuration:CMTimeMake(1, fps)forDevice:newInput.device];
+    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];
   }];
 }
 
@@ -597,32 +643,39 @@ enum AVFoundationVideoCapturerMessageType : uint32_t {
 };
 
 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;
+  std::set<cricket::VideoFormat> backCameraSupportedVideoFormats =
+      GetSupportedVideoFormatsForDevice([_capturer backCaptureDevice]);
+
+  std::vector<cricket::VideoFormat> intersectionVideoFormats;
+  if (backCameraSupportedVideoFormats.size() == 0) {
+    std::copy(frontCameraSupportedVideoFormats.begin(),
+              frontCameraSupportedVideoFormats.end(),
+              std::back_inserter(intersectionVideoFormats));
+    SetSupportedFormats(intersectionVideoFormats);
+    return;
   }
-#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);
-    }
+  if (frontCameraSupportedVideoFormats.size() == 0) {
+    std::copy(backCameraSupportedVideoFormats.begin(),
+              backCameraSupportedVideoFormats.end(),
+              std::back_inserter(intersectionVideoFormats));
+    SetSupportedFormats(intersectionVideoFormats);
+    return;
   }
 
-  SetSupportedFormats(supported_formats);
+  std::set_intersection(frontCameraSupportedVideoFormats.begin(),
+                        frontCameraSupportedVideoFormats.end(),
+                        backCameraSupportedVideoFormats.begin(),
+                        backCameraSupportedVideoFormats.end(),
+                        std::back_inserter(intersectionVideoFormats));
+
+  SetSupportedFormats(intersectionVideoFormats);
 }
 
 AVFoundationVideoCapturer::~AVFoundationVideoCapturer() {
@@ -640,17 +693,32 @@ cricket::CaptureState AVFoundationVideoCapturer::Start(
     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
@@ -659,11 +727,6 @@ cricket::CaptureState AVFoundationVideoCapturer::Start(
   [_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;
 }