Camera2 in AppRTC Android Demo.

Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
Committed: https://crrev.com/b224d80166056e46fd9023214f83181f4ff7133d
Cr-Commit-Position: refs/heads/master@{#13391}

[sakal, 2016-06-17 12:23:55]

Description was changed from

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
==========

to

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
==========

[sakal, 2016-06-17 12:23:55]

sakal@webrtc.org changed reviewers:
+ perkj@webrtc.org

[sakal, 2016-06-17 12:24:09]

PTAL

[sakal, 2016-06-20 08:38:33]

sakal@webrtc.org changed reviewers:
+ nisse@webrtc.org

[sakal, 2016-06-20 08:38:34]

nisse, PTAL

[nisse-webrtc, 2016-06-20 09:00:28]

I'm afraid I'm not familiar at all with this code. So I only have a comment on
the UI.

https://codereview.webrtc.org/2077663003/diff/1/webrtc/examples/androidapp/re...
File webrtc/examples/androidapp/res/values/arrays.xml (right):

https://codereview.webrtc.org/2077663003/diff/1/webrtc/examples/androidapp/re...
webrtc/examples/androidapp/res/values/arrays.xml:6: <item>Force enable</item>
I think I'd prefer Enable/Disable/Auto (with Auto replacing current Enable, and
Enable replacing current Force enable). Consistency with other settings is of
course also important, not sure what they look like.

[sakal, 2016-06-27 07:08:36]

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

[magjed_webrtc, 2016-06-29 13:36:45]

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/res/values/arrays.xml (right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/res/values/arrays.xml:3: <string-array
name="enableDisableForce">
This name looks weird. Shouldn't at least 'camera2' be included, e.g.
'camera2Values'?

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/res/values/arrays.xml:4: <item>Enable</item>
I think you should go with 'Default', 'True', 'False' for consistency with the
rest of the code.

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java
(right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:227: boolean
camera1Enabled = !Camera2Enumerator.isSupported() ||
!camera2Option.equals("force");
Shouldn't this be just:
boolean camera1Enabled = !camera2Option.equals("force");
?

Otherwise it looks like we just need one boolean. The purpose with "force" is
that we shouldn't fall back to camera1, right?

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:477:
if (videoCapturer != null) {
There is no need for this null check (with the current code), so I would remove
it, but keep it if you think it will make the code more future proof.

[sakal, 2016-06-30 09:14:53]

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/res/values/arrays.xml (right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/res/values/arrays.xml:3: <string-array
name="enableDisableForce">
On 2016/06/29 13:36:44, magjed_webrtc wrote:
> This name looks weird. Shouldn't at least 'camera2' be included, e.g.
> 'camera2Values'?

Done.

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/res/values/arrays.xml:4: <item>Enable</item>
On 2016/06/29 13:36:45, magjed_webrtc wrote:
> I think you should go with 'Default', 'True', 'False' for consistency with the
> rest of the code.

Done.

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java
(right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:227: boolean
camera1Enabled = !Camera2Enumerator.isSupported() ||
!camera2Option.equals("force");
On 2016/06/29 13:36:45, magjed_webrtc wrote:
> Shouldn't this be just:
> boolean camera1Enabled = !camera2Option.equals("force");
> ?
> 
> Otherwise it looks like we just need one boolean. The purpose with "force" is
> that we shouldn't fall back to camera1, right?

The option in the settings is disabled if Camera2Enumerator.isSupported returns
false. Since changing the option is impossible when it's disabled, we should
ignore it's value. Maybe this logic would be better conveyed by something like:

if (!Camera2Enumerator.isSupported()) {
  camera2Option = "default";
}

What do you think?

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:477:
if (videoCapturer != null) {
On 2016/06/29 13:36:45, magjed_webrtc wrote:
> There is no need for this null check (with the current code), so I would
remove
> it, but keep it if you think it will make the code more future proof.

I'd rather keep it. It used to be Camera2Enumerator createCapturer returned null
if the device only reported legacy support. We might add that back in the
future.

[magjed_webrtc, 2016-06-30 11:21:47]

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java
(right):

https://codereview.webrtc.org/2077663003/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:227: boolean
camera1Enabled = !Camera2Enumerator.isSupported() ||
!camera2Option.equals("force");
On 2016/06/30 09:14:52, sakal wrote:
> On 2016/06/29 13:36:45, magjed_webrtc wrote:
> > Shouldn't this be just:
> > boolean camera1Enabled = !camera2Option.equals("force");
> > ?
> > 
> > Otherwise it looks like we just need one boolean. The purpose with "force"
is
> > that we shouldn't fall back to camera1, right?
> 
> The option in the settings is disabled if Camera2Enumerator.isSupported
returns
> false. Since changing the option is impossible when it's disabled, we should
> ignore it's value. Maybe this logic would be better conveyed by something
like:
> 
> if (!Camera2Enumerator.isSupported()) {
>   camera2Option = "default";
> }
> 
> What do you think?

I think it's an overkill for this demo app to have fallback logic to camera1 if
camera2 fails. We would like to detect camera2 failures anyway so it's better if
they are obvious. So I think it's more straightforward to just:
* Have a true/false "use camera2" checkbox in settings.
* Disable the camera2 option in settings if camera2 is not supported.
* Only have one boolean here "useCamera2 = Camera2Enumerator.isSupported() &&
intent.getBooleanExtra(EXTRA_CAMERA2, true);"
 * Remove the fallback logic and crash instead.

[sakal, 2016-07-01 12:48:24]

I removed the fallback logic and also made the code respected the capture to
texture options. I also removed a couple unused string from the strings.xml
file.

[magjed_webrtc, 2016-07-01 13:55:29]

lgtm

[sakal, 2016-07-04 07:08:01]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-07-04 07:08:05]

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, 2016-07-04 08:00:35]

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

[commit-bot: I haz the power, 2016-07-04 08:00:36]

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

[sakal, 2016-07-05 14:05:11]

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

[commit-bot: I haz the power, 2016-07-05 14:05:16]

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, 2016-07-05 16:06:02]

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

[commit-bot: I haz the power, 2016-07-05 16:06:03]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[sakal, 2016-07-06 07:48:26]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-07-06 07:48:26]

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/2077663003/#ps120001 (title:
"Create peer connection factory with the target context in the test")

[commit-bot: I haz the power, 2016-07-06 07:48:31]

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, 2016-07-06 07:58:33]

Description was changed from

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
==========

to

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
==========

[commit-bot: I haz the power, 2016-07-06 07:58:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-06 07:58:36]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 07:58:43]

Description was changed from

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519
==========

to

==========
Camera2 in AppRTC Android Demo.

Add an option to use Camera2 implemantion of CameraVideoCapturer in
AppRTC Android Demo. It is enabled by default.

BUG=webrtc:5519

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

[commit-bot: I haz the power, 2016-07-06 07:58:44]

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