Add support for more resolutions on iOS/macOS

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 331db6d9557d4a6d74dd84d2692bc346ae396037..3514fd128947e2623d3b0446c2d1b2b365c21051 100644
--- a/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
+++ b/webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm
@@ -27,20 +27,39 @@
 #include "webrtc/base/thread.h"
 #include "webrtc/common_video/include/corevideo_frame_buffer.h"
 
-// TODO(tkchin): support other formats.
-static NSString *const kDefaultPreset = AVCaptureSessionPreset640x480;
-static NSString *const kIPhone4SPreset = AVCaptureSessionPreset352x288;
-static cricket::VideoFormat const kDefaultFormat =
-    cricket::VideoFormat(640,
-                         480,
-                         cricket::VideoFormat::FpsToInterval(30),
-                         cricket::FOURCC_NV12);
-// iPhone4S is too slow to handle 30fps.
-static cricket::VideoFormat const kIPhone4SFormat =
-    cricket::VideoFormat(352,
-                         288,
-                         cricket::VideoFormat::FpsToInterval(15),
-                         cricket::FOURCC_NV12);
+struct AVCaptureSessionPresetResolution {

[tkchin_webrtc, 2016/08/11 16:51:52]

Some notes about this file:

ObjC++ is gnarly and should be avoided as much as possible except to interop
with WebRTC's video APIs.
This file has both ObjC and C++ impls because previously everything was in one
directory, and we wanted to hide the internal ObjC impl because no one should be
using it. This is now a relic because we have a public header directory now
(Framework/Headers/WebRTC). It would be better to split out
RTCAVFoundationVideoCapturerInternal into its own file in this Classes/ private
directory, and only use ObjC-isms there (ie use NSObject containers instead of
structs). webrtc::AVFoundationVideoCapturer can then be just the ObjC++ bridge
between ObjC and C++.

Essentially, in objc/ dirs, avoid C++ unless you absolutely have to use it,
otherwise we'll end up in a confusing ObjC++ tangle.

[kthelgason, 2016/08/11 17:42:04]

That's good to know, thanks. I'll revise this. Perhaps splitting the classes up
between files should be done in a separate CL?

+  NSString *sessionPreset;
+  int width;
+  int height;
+  cricket::VideoFormat getVideoFormat(int framerate) {
+    return cricket::VideoFormat(height, width,
+                                cricket::VideoFormat::FpsToInterval(framerate),
+                                cricket::FOURCC_NV12);
+  }
+};
+
+static const AVCaptureSessionPresetResolution kSupportedPresets[] = {

[tkchin_webrtc, 2016/08/11 16:51:52]

Not all of these is supported for every device. The presets need to be queried
at runtime. This file should be mac-compatible as well, so while we're adding
these, might as well add some of the rest.

kAvailablePresets?

[kthelgason, 2016/08/11 17:42:04]

Acknowledged.

+  { AVCaptureSessionPreset352x288, 352, 288},
+  { AVCaptureSessionPreset640x480, 640, 480},
+  { AVCaptureSessionPreset1280x720, 1280, 720}
+};
+
+static AVCaptureSessionPresetResolution const kDefaultPreset =
+  kSupportedPresets[1];
+static AVCaptureSessionPresetResolution const kIPhone4SPreset =
+  kSupportedPresets[0];
+
+// mapping from cricket::VideoFormat to AVCaptureSession presets
+static NSString *GetSessionPresetForVideoFormat(
+  const cricket::VideoFormat& format) {
+  for (const auto preset : kSupportedPresets) {
+    if (format.width == preset.width && format.height == preset.height) {
+      return preset.sessionPreset;
+    }
+  }
+  // Nothing found, use default
+  return kDefaultPreset.sessionPreset;
+}
 
 // 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
@@ -361,10 +380,10 @@ static cricket::VideoFormat const kIPhone4SFormat =
     captureSession.usesApplicationAudioSession = NO;
   }
 #endif
-  NSString *preset = kDefaultPreset;
+  NSString *preset = kDefaultPreset.sessionPreset;
 #if TARGET_OS_IPHONE
   if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
-    preset = kIPhone4SPreset;
+    preset = kIPhone4SPreset.sessionPreset;
   }
 #endif
   if (![captureSession canSetSessionPreset:preset]) {
@@ -577,21 +596,27 @@ struct AVFoundationFrame {
 
 AVFoundationVideoCapturer::AVFoundationVideoCapturer()
     : _capturer(nil), _startThread(nullptr) {
-  // Set our supported formats. This matches preset.
+  // Set our supported formats. This matches kSupportedPresets
+  _capturer =
+    [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this];
+
   std::vector<cricket::VideoFormat> supported_formats;
+  int framerate = 30;

[tkchin_webrtc, 2016/08/11 16:51:52]

There's no guarantee that you will get these frame rates. The presets will set a
default and later if you want to use something different you need to set it on
the device.

[kthelgason, 2016/08/11 17:42:04]

I understand. I'd pretty much come to that conclusion as well, but I kept it
like this as the previous code sets these framerates explicitly. Would you
suggest removing this always using 30 for the `cricket::VideoFormat`s?

+
 #if TARGET_OS_IPHONE
   if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
-    supported_formats.push_back(cricket::VideoFormat(kIPhone4SFormat));
     set_enable_video_adapter(false);
-  } else {
-    supported_formats.push_back(cricket::VideoFormat(kDefaultFormat));
+    framerate = 15;
   }
-#else
-  supported_formats.push_back(cricket::VideoFormat(kDefaultFormat));
 #endif
+
+  for (auto preset : kSupportedPresets) {
+    if ([_capturer.captureSession canSetSessionPreset:preset.sessionPreset]) {

[tkchin_webrtc, 2016/08/11 17:17:55]

I'm not sure we want to go down the preset route just yet for doing supported
formats, we probably should check what else the preset is setting. The usual way
of iterating formats is:
https://developer.apple.com/library/ios/documentation/AVFoundation/Reference/...

+      supported_formats.push_back(preset.getVideoFormat(framerate));
+    }
+  }
+
   SetSupportedFormats(supported_formats);
-  _capturer =
-      [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this];
 }
 
 AVFoundationVideoCapturer::~AVFoundationVideoCapturer() {
@@ -608,10 +633,16 @@ cricket::CaptureState AVFoundationVideoCapturer::Start(
     LOG(LS_ERROR) << "The capturer is already running.";
     return cricket::CaptureState::CS_FAILED;
   }
-  if (format != kDefaultFormat && format != kIPhone4SFormat) {
-    LOG(LS_ERROR) << "Unsupported format provided.";
+
+  NSString *desiredPreset = GetSessionPresetForVideoFormat(format);
+
+  [_capturer.captureSession beginConfiguration];
+  if (![_capturer.captureSession canSetSessionPreset:desiredPreset]) {
+    LOG(LS_ERROR) << "Unsupported video format.";

[tkchin_webrtc, 2016/08/11 16:51:52]

do you need beginconfiguration to check session preset? Calls to begin should be
matched with commit.

[kthelgason, 2016/08/11 17:42:04]

From Apple's documentation: "After calling beginConfiguration, you can for
example add or remove outputs, alter the sessionPreset, or configure individual
capture input or output properties". 

I will add a `commitConfiguration` call before the return in the failure case as
well.

     return cricket::CaptureState::CS_FAILED;
   }
+  _capturer.captureSession.sessionPreset = desiredPreset;
+  [_capturer.captureSession commitConfiguration];
 
   // Keep track of which thread capture started on. This is the thread that
   // frames need to be sent to.
@@ -619,6 +650,8 @@ cricket::CaptureState AVFoundationVideoCapturer::Start(
   _startThread = rtc::Thread::Current();
 
   SetCaptureFormat(&format);
+
+

[tkchin_webrtc, 2016/08/11 16:51:52]

nit: remove blank line

[kthelgason, 2016/08/11 17:42:04]

Acknowledged.

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