Use new RTCCameraVideoCapturer in AppRTCMobile.

Use new RTCCameraVideoCapturer in AppRTCMobile.

Because the new API doesn't use constraints, refactors settings to instead have a
video resolution setting.

BUG=webrtc:7177
Review-Url: https://codereview.webrtc.org/2778163005
Cr-Commit-Position: refs/heads/master@{#17545}
Committed: https://chromium.googlesource.com/external/webrtc/+/c522e758f6a364c23e7232d4f402713601f0b35e

[sakal, 2017-03-30 11:58:43]

Patchset #3 (id:40001) has been deleted

[sakal, 2017-03-30 12:22:18]

Patchset #1 (id:1) has been deleted

[sakal, 2017-03-30 12:22:25]

Patchset #1 (id:20001) has been deleted

[sakal, 2017-03-30 12:22:48]

Patchset #1 (id:60001) has been deleted

[sakal, 2017-03-30 12:25:19]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 12:25:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-30 12:28:20]

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

[commit-bot: I haz the power, 2017-03-30 12:28:21]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15623)

[sakal, 2017-03-31 07:57:45]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 07:57:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 08:00:40]

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

[commit-bot: I haz the power, 2017-03-31 08:00:40]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19122)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24422)

[sakal, 2017-03-31 11:03:20]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 11:03:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 11:06:14]

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

[commit-bot: I haz the power, 2017-03-31 11:06:15]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15649)

[sakal, 2017-03-31 12:12:51]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 12:13:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 12:15:10]

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

[commit-bot: I haz the power, 2017-03-31 12:15:11]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19143)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24442)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23253)

[sakal, 2017-03-31 12:23:17]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 12:23:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 12:26:11]

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

[commit-bot: I haz the power, 2017-03-31 12:26:12]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19144)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23254)

[sakal, 2017-03-31 12:59:43]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 12:59:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 13:04:51]

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

[commit-bot: I haz the power, 2017-03-31 13:04:52]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[sakal, 2017-03-31 13:15:04]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 13:15:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-31 13:19:59]

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

[commit-bot: I haz the power, 2017-03-31 13:20:00]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15671)

[sakal, 2017-03-31 13:39:54]

Patchset #5 (id:160001) has been deleted

[sakal, 2017-03-31 13:40:00]

Patchset #4 (id:140001) has been deleted

[sakal, 2017-03-31 13:40:08]

Patchset #1 (id:80001) has been deleted

[sakal, 2017-03-31 13:40:18]

Patchset #1 (id:100001) has been deleted

[sakal, 2017-03-31 13:40:28]

Patchset #1 (id:120001) has been deleted

[sakal, 2017-03-31 13:41:13]

Description was changed from

==========
Use new capturer in AppRTCMobile.

BUG=
==========

to

==========
Use new capturer in AppRTCMobile.

BUG=webrtc:7177
==========

[sakal, 2017-03-31 13:41:13]

sakal@webrtc.org changed reviewers:
+ denicija@webrtc.org, magjed@webrtc.org

[sakal, 2017-03-31 13:41:24]

PTAL

[daniela-webrtc, 2017-04-03 10:16:50]

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDAppClient.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:689: [_delegate appClient:self
didReceiveLocalVideoTrack:_localVideoTrack];
Not sure if we should invoke the delegate with nil here to indicate the
localTrack was not created. If there is such need it's better to have additional
callback like `appClientFailedToCreateLocalVideoTrack` or adjust the existing
method to included error parameter.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:728: - (void)startCapture {
This should be exposed in the interface. Also, it signature can be changed to:
`startCaptureWithDevice:` That way you can ditch the _usingFrontCamera ivar and
move the responsibility of capturing device elsewhere (as it should be since
AppDelegate becomes quite cluttered otherwise).

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:753: AVCaptureDeviceFormat
*selectedFormat;
Preferably explicitly set pointers to nil.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:788: [self startCapture];
[startCapture] should not be here but rather exposed in the interface of this
class so that it can be invoked when convenient (for instance in this case in
the delegate callback `appClient:didReceiveLocalVideoTrack:`.
Also, this way `createLocalVideoTrack`has a lot of side effects and does way
more than it's naming suggests.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h:35: * Returns current video
resolution media constraint string.
Remove media constraint references from documentation.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h:44: * Stores the provided
video resolution media constraint string into the store.
Remove media constraint references from documentation.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m:110: return 0;
Hint: Return NSNotFound
https://developer.apple.com/reference/foundation/foundation_constants/nsnotfound

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m:114: return 0;
Same here

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:181: #pragma
mark - Table view delegate(Media Constraints)
Rename Media Constraints references in this section as well.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:201: NSString
*mediaConstraintsString = self.videoResolutionArray[indexPath.row];
Nit: rename mediaConstraintsString to videoResolution

[sakal, 2017-04-04 09:18:24]

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDAppClient.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:689: [_delegate appClient:self
didReceiveLocalVideoTrack:_localVideoTrack];
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Not sure if we should invoke the delegate with nil here to indicate the
> localTrack was not created. If there is such need it's better to have
additional
> callback like `appClientFailedToCreateLocalVideoTrack` or adjust the existing
> method to included error parameter.

Oops, this was added by accident.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:728: - (void)startCapture {
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> This should be exposed in the interface. Also, it signature can be changed to:
> `startCaptureWithDevice:` That way you can ditch the _usingFrontCamera ivar
and
> move the responsibility of capturing device elsewhere (as it should be since
> AppDelegate becomes quite cluttered otherwise).

Moved to CaptureController as discussed.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:753: AVCaptureDeviceFormat
*selectedFormat;
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Preferably explicitly set pointers to nil.

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:788: [self startCapture];
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> [startCapture] should not be here but rather exposed in the interface of this
> class so that it can be invoked when convenient (for instance in this case in
> the delegate callback `appClient:didReceiveLocalVideoTrack:`.
> Also, this way `createLocalVideoTrack`has a lot of side effects and does way
> more than it's naming suggests.

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h:35: * Returns current video
resolution media constraint string.
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Remove media constraint references from documentation.

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.h:44: * Stores the provided
video resolution media constraint string into the store.
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Remove media constraint references from documentation.

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m:110: return 0;
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Hint: Return NSNotFound
>
https://developer.apple.com/reference/foundation/foundation_constants/nsnotfound

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDSettingsModel.m:114: return 0;
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Same here

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:181: #pragma
mark - Table view delegate(Media Constraints)
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Rename Media Constraints references in this section as well.

Done.

https://codereview.webrtc.org/2778163005/diff/180001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:201: NSString
*mediaConstraintsString = self.videoResolutionArray[indexPath.row];
On 2017/04/03 10:16:50, daniela-webrtc wrote:
> Nit: rename mediaConstraintsString to videoResolution

Done.

[sakal, 2017-04-04 10:37:41]

Description was changed from

==========
Use new capturer in AppRTCMobile.

BUG=webrtc:7177
==========

to

==========
Use new RTCCameraVideoCapturer in AppRTCMobile.

Because the new API doesn't use constraints, refactors settings to instead have
a
video resolution setting.

BUG=webrtc:7177
==========

[daniela-webrtc, 2017-04-04 12:25:21]

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m (right):

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:106:
[_captureController startCapture];
I'd add [_capturerController stopCapture] in [self hangup] on line191 (and
adjust the interface of ARDCaptureController accordingly). Also this means we
can completely remove all references of the capturer in ARDAppClient as well :)

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:172: -
(void)setLocalCapturer:(RTCCameraVideoCapturer *)localCapturer {
I don't see this being called anywhere except on 193. Is this intentional?

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:172: -
(void)setLocalCapturer:(RTCCameraVideoCapturer *)localCapturer {
Also, we don't need to keep reference to the RTCCameraVideoCapturer here. We can
setup the captureSession but we don't need to keep ownership.

[magjed_webrtc, 2017-04-05 12:01:03]

lgtm

[sakal, 2017-04-05 12:35:49]

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m (right):

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:106:
[_captureController startCapture];
On 2017/04/04 12:25:21, daniela-webrtc wrote:
> I'd add [_capturerController stopCapture] in [self hangup] on line191 (and
> adjust the interface of ARDCaptureController accordingly). Also this means we
> can completely remove all references of the capturer in ARDAppClient as well
:)

Done.

https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:172: -
(void)setLocalCapturer:(RTCCameraVideoCapturer *)localCapturer {
On 2017/04/04 12:25:21, daniela-webrtc wrote:
> Also, we don't need to keep reference to the RTCCameraVideoCapturer here. We
can
> setup the captureSession but we don't need to keep ownership.

Done.

[daniela-webrtc, 2017-04-05 13:49:25]

On 2017/04/05 12:35:49, sakal wrote:
>
https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
> File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m
(right):
> 
>
https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
> webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:106:
> [_captureController startCapture];
> On 2017/04/04 12:25:21, daniela-webrtc wrote:
> > I'd add [_capturerController stopCapture] in [self hangup] on line191 (and
> > adjust the interface of ARDCaptureController accordingly). Also this means
we
> > can completely remove all references of the capturer in ARDAppClient as well
> :)
> 
> Done.
> 
>
https://codereview.webrtc.org/2778163005/diff/260001/webrtc/examples/objc/App...
> webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:172: -
> (void)setLocalCapturer:(RTCCameraVideoCapturer *)localCapturer {
> On 2017/04/04 12:25:21, daniela-webrtc wrote:
> > Also, we don't need to keep reference to the RTCCameraVideoCapturer here. We
> can
> > setup the captureSession but we don't need to keep ownership.
> 
> Done.

lgtm

[sakal, 2017-04-05 13:52:08]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-04-05 13:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2778163005/#ps300001 (title:
"Remove localCapturer property from ARDVideoCallViewController.")

[commit-bot: I haz the power, 2017-04-05 13:52:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-05 14:11:20]

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

[commit-bot: I haz the power, 2017-04-05 14:11:21]

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

[magjed_webrtc, 2017-04-05 19:05:04]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-04-05 19:05:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-05 19:17:49]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491419104212250,
"parent_rev": "1ba21eb06339be822ab2074285efb6f2fdd6e2e9", "commit_rev":
"c522e758f6a364c23e7232d4f402713601f0b35e"}

[commit-bot: I haz the power, 2017-04-05 19:17:55]

Description was changed from

==========
Use new RTCCameraVideoCapturer in AppRTCMobile.

Because the new API doesn't use constraints, refactors settings to instead have
a
video resolution setting.

BUG=webrtc:7177
==========

to

==========
Use new RTCCameraVideoCapturer in AppRTCMobile.

Because the new API doesn't use constraints, refactors settings to instead have
a
video resolution setting.

BUG=webrtc:7177

Review-Url: https://codereview.webrtc.org/2778163005
Cr-Commit-Position: refs/heads/master@{#17545}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c522e758f6a364c23e7232d4f...
==========

[commit-bot: I haz the power, 2017-04-05 19:17:57]

Committed patchset #7 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/c522e758f6a364c23e7232d4f...