Add AVFoundation video capture support to Mac objc SDK (based on iOS)

Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I could write some if you can point me at a location. Also, I don't have access to the automated tests (I don't think)

BUG=webrtc:3417
Committed: https://crrev.com/fc22e03eb836768c9994162c6c19afba0d96126e
Cr-Commit-Position: refs/heads/master@{#13080}

[afedor, 2016-06-08 02:47:42]

Description was changed from

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

BUG=3417
==========

to

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

BUG=3417
==========

[afedor, 2016-06-08 02:47:43]

adam.fedor@gmail.com changed reviewers:
+ tkchin@webrtc.org

[afedor, 2016-06-08 02:52:09]

Description was changed from

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

BUG=3417
==========

to

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=3417
==========

[tkchin_webrtc, 2016-06-08 19:26:31]

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:224: -
(void)deviceOrientationDidChange:(NSNotification *)notification {
guard this one as well

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:284: NSError
*error = [notification.userInfo objectForKey:
nit: avoid awkward break
NSError *error =
    [notification.userInfo objectForKey:AVCaptureSessionErrorKey];

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/sdk.gyp
File webrtc/sdk/sdk.gyp (right):

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/sdk.gyp#newcode49
webrtc/sdk/sdk.gyp:49: 'link_settings': {
revert. AVFoundation is required by RTCCameraPreviewView

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/sdk.gyp#newcode91
webrtc/sdk/sdk.gyp:91: 'link_settings': {
add the AVFoundation one here for now
eventually it'd be good to move the capture / render bits to their own target

https://codereview.webrtc.org/2046863004/diff/20001/webrtc/sdk/sdk.gyp#newcod...
webrtc/sdk/sdk.gyp:200: '-framework CoreMedia',
ooc what compiler warning do you get without CoreMedia?

[tkchin_webrtc, 2016-06-08 19:30:14]

Just a few nits, this looks like a good start.

We don't have continuous testing for this bit of code because when we tried it
on devices there was a lot of flakiness due to an issue with mediaserverd on
iOS.

Can you run (and possibly update) this test:
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/objc/AppRTCD...

[afedor, 2016-06-08 20:56:48]

Without CodeMedia, I get the link errors:

Undefined symbols for architecture x86_64:
  "_CMSampleBufferDataIsReady", referenced from:

(and three other similar errors)

On 2016/06/08 19:30:14, tkchin_webrtc wrote:

> Can you run (and possibly update) this test:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/objc/AppRTCD...

I ran this and got a SEGFAULT as the code tries to get the wrong argument for a
block from the invocation. I can submit a patch for that, but perhaps the best
place for updating tests is the second part of my patch, where I use this patch
to actually get AppRTCDemo app working (with local video)?

[tkchin_webrtc, 2016-06-08 21:35:55]

hm, no good. Alright, thanks for checking. If it's already broken we don't have
to fix it in this CL.

https://codereview.webrtc.org/2046863004/diff/40001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2046863004/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:287:
[notification.userInfo objectForKey: AVCaptureSessionErrorKey];
nit: indent, and no space after :

it should be 6 spaces from start of line (4 spaces from NSError start), like
what you have for AVCaptureDevice below

https://codereview.webrtc.org/2046863004/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:475:
AVCaptureVideoOrientation orientation = AVCaptureVideoOrientationPortrait;
Technically this shouldn't get called at all, but can you move the guard above
this orientation line, and also have it capture the connection.videoOrientation
line?

[tkchin_webrtc, 2016-06-08 21:36:17]

Description was changed from

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=3417
==========

to

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=webrtc:3417
==========

[tkchin_webrtc, 2016-06-08 22:17:24]

lgtm

I'll commit this once the last couple of nits are addressed

https://codereview.webrtc.org/2046863004/diff/60001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2046863004/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:425:
[AVCaptureDevice defaultDeviceWithMediaType: AVMediaTypeVideo];;
last one I promise :)
no space after :

https://codereview.webrtc.org/2046863004/diff/60001/webrtc/sdk/sdk.gyp
File webrtc/sdk/sdk.gyp (right):

https://codereview.webrtc.org/2046863004/diff/60001/webrtc/sdk/sdk.gyp#newcod...
webrtc/sdk/sdk.gyp:205: '-framework CoreMedia',
alphabetize

[tkchin_webrtc, 2016-06-08 22:34:20]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-06-08 22:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2046863004/#ps80001 (title:
"Alphabatize frameworks. Minor style change")

[afedor, 2016-06-08 22:34:21]

Also, wondering if/when I have to add my name to the AUTHORS file like it says
in the documentation

[commit-bot: I haz the power, 2016-06-08 22:34:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046863004/80001

[commit-bot: I haz the power, 2016-06-08 22:38:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 22:38:40]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14023)

[tkchin_webrtc, 2016-06-08 22:42:44]

On 2016/06/08 22:38:40, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_rel on tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14023)

It looks like the GN build files also have to be updated.
You can add yourself to AUTHORS in this CL.

[afedor, 2016-06-08 22:58:19]

On 2016/06/08 22:42:44, tkchin_webrtc wrote:
> It looks like the GN build files also have to be updated.

Hmm, not sure I understand GN files.  It looks like all the relevant files are
commented out in sdk/BUILD.gn, and if anything the first "if (is_ios)" seems
like it encloses too much

[tkchin_webrtc, 2016-06-08 23:12:18]

On 2016/06/08 22:58:19, adam.fedor wrote:
> On 2016/06/08 22:42:44, tkchin_webrtc wrote:
> > It looks like the GN build files also have to be updated.
> 
> Hmm, not sure I understand GN files.  It looks like all the relevant files are
> commented out in sdk/BUILD.gn, and if anything the first "if (is_ios)" seems
> like it encloses too much

Try rebasing your change. This was just updated a day or two ago.

[afedor, 2016-06-08 23:40:29]

I updated the BUILD.gn file.  Although I'm not sure how to test it.

[tkchin_webrtc, 2016-06-08 23:43:04]

On 2016/06/08 23:40:29, adam.fedor wrote:
> I updated the BUILD.gn file.  Although I'm not sure how to test it.

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/quick_s...

I started the trybots, those will also check it.

[tkchin_webrtc, 2016-06-08 23:53:21]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2016-06-08 23:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2046863004/#ps100001 (title:
"Updated BUILD.gn to match gyp")

[commit-bot: I haz the power, 2016-06-08 23:53:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046863004/100001

[commit-bot: I haz the power, 2016-06-09 00:24:40]

Description was changed from

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=webrtc:3417
==========

to

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=webrtc:3417
==========

[commit-bot: I haz the power, 2016-06-09 00:24:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-09 00:24:49]

Description was changed from

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=webrtc:3417
==========

to

==========
Add AVFoundation video capture support to Mac objc SDK (based on iOS)

The AppRTCDemo app on Mac OSX does not show or send local video streams,
as ACFoundation capture session is not compiled in or implemented in
the appropriate places.  This is the first part of a two-part patch
that implements local capture on the Mac for AppRTCDemo

P.S. This is my first patch to WebRTC.   I didn't see any relevant tests, but I
could write some if you can point me at a location. Also, I don't have access to
the automated tests (I don't think)

BUG=webrtc:3417

Committed: https://crrev.com/fc22e03eb836768c9994162c6c19afba0d96126e
Cr-Commit-Position: refs/heads/master@{#13080}
==========

[commit-bot: I haz the power, 2016-06-09 00:24:50]

Patchset 6 (id:??) landed as
https://crrev.com/fc22e03eb836768c9994162c6c19afba0d96126e
Cr-Commit-Position: refs/heads/master@{#13080}