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;
-};
+// 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) {
 
-#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
+  AVCaptureDeviceFormat *desiredDeviceFormat = nil;
+  for (AVCaptureDeviceFormat *deviceFormat in [device formats]) {
+    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 &&
+        code !=kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) {
+      continue;
+    }
+
+    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;
+      }
     }
   }
-  // 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::vector<cricket::VideoFormat> GetSupportedVideoFormatsForDevice(AVCaptureDevice *device) {
+  std::vector<cricket::VideoFormat> supportedFormats;
+
+  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.

+  }
+  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;
+- (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 402 matching lines @@
 
 @end
 
 namespace webrtc {
 
 enum AVFoundationVideoCapturerMessageType : uint32_t {
   kMessageTypeFrame,
 };
 
 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() {
   _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);

[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
   // 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);
@@ skipping 92 matching lines @@
       buffer = rotated_buffer;
     }
   }
 
   OnFrame(cricket::WebRtcVideoFrame(buffer, rotation,
                                     translated_camera_time_us, 0),
           captured_width, captured_height);
 }
 
 }  // namespace webrtc