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..53578a0d4193d0792fe84c815c83f4967e0c592f 100644
--- a/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
+++ b/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
@@ -30,41 +30,66 @@
 #include "webrtc/common_video/include/corevideo_frame_buffer.h"
 #include "webrtc/common_video/rotation.h"
 
-struct AVCaptureSessionPresetResolution {
-  NSString *sessionPreset;
-  int width;
-  int height;
-};
+// Maping from cricket::VideoFormat to AVCaptureDeviceFormat.

[kthelgason, 2016/09/20 07:00:30]

nit: mapping

+static AVCaptureDeviceFormat *GetDeviceFormatForVideoFormat(const AVCaptureDevice *device,
+                                                            const cricket::VideoFormat& videoFormat) {
+
+  AVCaptureDeviceFormat *desiredDeviceFormat = nil;
+  for (AVCaptureDeviceFormat *deviceFormat in [device formats]) {
+    CMVideoDimensions dimension = CMVideoFormatDescriptionGetDimensions([deviceFormat formatDescription]);
+    FourCharCode code = CMFormatDescriptionGetMediaSubType([deviceFormat formatDescription]);
+
+    if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange &&
+        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) {

[magjed_webrtc, 2016/09/20 08:42:18]

Shouldn't we also look at fps? Maybe it would be beneficial to split out a
cricket::VideoFormat AVFormatToCricketFormat(AVCaptureDeviceFormat*
deviceFormat) function and use 'videoFormat ==
AVFormatToCricketFormat(deviceFormat)' as the check here.

[daniela-webrtc, 2016/09/26 11:03:15]

Acknowledged.

+      if (code == 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::vector<cricket::VideoFormat> GetSupportedVideoFormatsForDevice(AVCaptureDevice *device) {
+  std::vector<cricket::VideoFormat> supportedFormats;
 
-// 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;
+  for (AVCaptureDeviceFormat *deviceFormat in [device formats]) {
+    FourCharCode code = CMFormatDescriptionGetMediaSubType([deviceFormat formatDescription]);
+
+    if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange &&
+        code !=kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) {

[kthelgason, 2016/09/20 07:00:30]

nit: missing space

[daniela-webrtc, 2016/09/23 08:49:17]

Acknowledged.

+      continue;
+    }
+
+    CMVideoDimensions dimension = CMVideoFormatDescriptionGetDimensions([deviceFormat formatDescription]);
+
+    for (AVFrameRateRange* frameRate in [deviceFormat videoSupportedFrameRateRanges]) {
+      Float64 fps = frameRate.maxFrameRate;

[kthelgason, 2016/09/20 07:00:30]

is there a need to introduce this variable?

[daniela-webrtc, 2016/09/26 11:03:15]

Acknowledged.

+      int64_t interval = cricket::VideoFormat::FpsToInterval(fps);
+      cricket::VideoFormat format = cricket::VideoFormat(dimension.width,
+                                                         dimension.height,
+                                                         interval,
+                                                         cricket::FOURCC_NV12);
+
+      std::vector<cricket::VideoFormat>::iterator iterator = std::find(supportedFormats.begin(),

[kthelgason, 2016/09/20 07:00:30]

I'd go for auto here, this type is very unwieldy.

[daniela-webrtc, 2016/09/23 08:49:17]

This will no longer be needed once I implement the supportedFormats as set.
Thanks for the tip!

+                                                                       supportedFormats.end(),
+                                                                       format);
+      if (iterator != supportedFormats.end()) {
+        continue;
+      } else {
+        supportedFormats.push_back(format);
+      }
     }

[kthelgason, 2016/09/20 07:00:30]

We might be better off just using a std::set for supportedFormats, and skipping
the existance check, or using .erase on the vector to get rid of duplicates.

[magjed_webrtc, 2016/09/20 08:42:18]

+1 Go with a std::set, it should simplify the code. You can create the
std::vector for SetSupportedFormats automatically by using a vector output
iterator to std::set_intersection.

[daniela-webrtc, 2016/09/23 08:49:17]

Acknowledged.

   }
-  // 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 +112,9 @@ static NSString *GetSessionPresetForVideoFormat(
 - (instancetype)initWithCapturer:(webrtc::AVFoundationVideoCapturer *)capturer;
 - (AVCaptureDevice *)getActiveCaptureDevice;
 
+- (nullable AVCaptureDevice *)frontCaptureDevice;
+- (nullable AVCaptureDevice *)backCaptureDevice;
+
 // Starts and stops the capture session asynchronously. We cannot do this
 // synchronously without blocking a WebRTC thread.
 - (void)start;
@@ -175,6 +203,15 @@ static NSString *GetSessionPresetForVideoFormat(
   return self.useBackCamera ? _backCameraInput.device : _frontCameraInput.device;
 }
 
+- (AVCaptureDevice *)frontCaptureDevice {
+  return _frontCameraInput.device;
+}
+
+- (AVCaptureDevice *)backCaptureDevice {
+  return _backCameraInput.device;
+}
+
+
 - (dispatch_queue_t)frameQueue {
   if (!_frameQueue) {
     _frameQueue =
@@ -597,32 +634,24 @@ enum AVFoundationVideoCapturerMessageType : uint32_t {
 };
 
 AVFoundationVideoCapturer::AVFoundationVideoCapturer() : _capturer(nil) {
-  // Set our supported formats. This matches kAvailablePresets.
-  _capturer =
-      [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this];
+  _capturer = [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this];
 
-  std::vector<cricket::VideoFormat> supported_formats;
-  int framerate = 30;
+  std::vector<cricket::VideoFormat> frontCameraSupportedVideoFormats =
+  GetSupportedVideoFormatsForDevice([_capturer frontCaptureDevice]);
+  std::vector<cricket::VideoFormat> backCameraSupportedVideoFormats =

[kthelgason, 2016/09/20 07:00:30]

I would use auto for all these types, I try to use it as much as I can. I don't
know if other people here have the same opinion, but it's up to you. 

[magjed_webrtc, 2016/09/20 08:42:18]

See https://google.github.io/styleguide/cppguide.html#auto. I would not use auto
here, because the type is not clear from the local context (in the same
expression or within a few lines). I also don't think the type is particularly
cluttered, and for me, it adds clarity.

+  GetSupportedVideoFormatsForDevice([_capturer backCaptureDevice]);
 
-#if TARGET_OS_IPHONE
-  if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
-    set_enable_video_adapter(false);
-    framerate = 15;
-  }
-#endif
+  std::sort(frontCameraSupportedVideoFormats.begin(), frontCameraSupportedVideoFormats.end());
+  std::sort(backCameraSupportedVideoFormats.begin(), backCameraSupportedVideoFormats.end());
 
-  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;
+  std::set_intersection(frontCameraSupportedVideoFormats.begin(),
+                        frontCameraSupportedVideoFormats.end(),
+                        backCameraSupportedVideoFormats.begin(),
+                        backCameraSupportedVideoFormats.end(),
+                        std::back_inserter(intersectionVideoFormats));
 
-  SetSupportedFormats(supported_formats);
+  SetSupportedFormats(intersectionVideoFormats);
 }
 
 AVFoundationVideoCapturer::~AVFoundationVideoCapturer() {
@@ -640,17 +669,26 @@ cricket::CaptureState AVFoundationVideoCapturer::Start(
     return cricket::CaptureState::CS_FAILED;
   }
 
-  NSString *desiredPreset = GetSessionPresetForVideoFormat(format);
-  RTC_DCHECK(desiredPreset);
+  AVCaptureDeviceFormat *deviceFormat =
+  GetDeviceFormatForVideoFormat([_capturer getActiveCaptureDevice], format);

[magjed_webrtc, 2016/09/20 08:42:18]

You need to indent the new line with 4 spaces.
https://google.github.io/styleguide/cppguide.html#Function_Calls. Try running
'git cl format' which will fix stuff like this automagically. That commands
works very well for C++, but be aware that it might have some problems with ObjC
code, so a tip is to commit first before running it and then cherry-pick the
changes you actually want.

[daniela-webrtc, 2016/09/26 11:03:15]

Acknowledged.

+  AVCaptureDevice *device = [_capturer getActiveCaptureDevice];
+  AVCaptureSession *session = [_capturer captureSession];
+  NSError *error = nil;
 
-  [_capturer.captureSession beginConfiguration];
-  if (![_capturer.captureSession canSetSessionPreset:desiredPreset]) {
-    LOG(LS_ERROR) << "Unsupported video format.";
-    [_capturer.captureSession commitConfiguration];
-    return cricket::CaptureState::CS_FAILED;
+  [session beginConfiguration]; // the session to which the receiver's AVCaptureDeviceInput is added.

[kthelgason, 2016/09/20 07:00:30]

Is this necessary when only re-configuring the device and not the session?

[daniela-webrtc, 2016/09/23 08:49:17]

Yes. Because this essentially causes the session to be re-configured as well (as
per Apple's documentations and examples)

+  if ([device lockForConfiguration:&error]) {
+    @try {
+      [device setActiveFormat:deviceFormat];
+    } @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