Session based capturing for Camera2Capturer.

Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org

Committed: https://crrev.com/d30e0ad3e759493f3765215a76396d828be55462
Cr-Commit-Position: refs/heads/master@{#13723}

[sakal, 2016-07-28 13:31:44]

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

[sakal, 2016-07-28 13:31:45]

Magnus, PTAL.

[magjed_webrtc, 2016-07-28 14:24:50]

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java:46:
Camera2Session.create(
Didn't we decide to call the ctor directly here?

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java:50: getCameraName(),
width, height, framerate);
Pass cameraName as an argument instead.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:43:
switchEventsHandler.onCameraSwitchDone(session.isFrontFacing());
Use enumerator.isFrontFacing(cameraName) instead and remove isFrontFacing from
the session interface.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:54:
openAttemptsRemainingCache = --openAttemptsRemaining;
Is this to avoid calling eventsHandler.onCameraError() while holding the lock?
Otherwise, why not just synchronize all with one statement?

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:130: if (cameraName
== null || cameraName.isEmpty()) {
I think we can remove this functionality now and require that the caller passes
a valid camera name.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:169: private void
createSession(int maxOpenCameraAttempts) {
This extra indirection is just confusing. Please inline the two calls with
openAttemptsRemaining = maxOpenCameraAttempts;
createSessionInternal();
The rest of the logic in this function is not necessary.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:184:
createCameraSession(
I still think it would be better if createCameraSession() is called on the
camera thread directly. You don't need to catch any exceptions though.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:262: if
(switchInProgress) {
This check should be done first, before waiting on sessionOpening.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:283: for (int i = 0;
i < deviceNames.length; i++) {
Use indexOf() instead on the deviceNames list. It would be nice if
getDeviceNames() would return a list directly instead of an array.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java:57: void
stopCapture(boolean waitCameraStop);
Who needs this besides the test? Can't they wait for onCameraClosed() instead?

[sakal, 2016-07-29 13:50:54]

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java:46:
Camera2Session.create(
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> Didn't we decide to call the ctor directly here?

I changed my mind. The reason is I don't want to give the caller reference to
the session before it is ready.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java:50: getCameraName(),
width, height, framerate);
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> Pass cameraName as an argument instead.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:43:
switchEventsHandler.onCameraSwitchDone(session.isFrontFacing());
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> Use enumerator.isFrontFacing(cameraName) instead and remove isFrontFacing from
> the session interface.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:54:
openAttemptsRemainingCache = --openAttemptsRemaining;
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> Is this to avoid calling eventsHandler.onCameraError() while holding the lock?
> Otherwise, why not just synchronize all with one statement?

Maybe it would be okay to hold the lock, I just didn't want to keep the lock for
a long time. Maybe it's premature optimization. Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:130: if (cameraName
== null || cameraName.isEmpty()) {
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> I think we can remove this functionality now and require that the caller
passes
> a valid camera name.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:169: private void
createSession(int maxOpenCameraAttempts) {
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> This extra indirection is just confusing. Please inline the two calls with
> openAttemptsRemaining = maxOpenCameraAttempts;
> createSessionInternal();
> The rest of the logic in this function is not necessary.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:184:
createCameraSession(
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> I still think it would be better if createCameraSession() is called on the
> camera thread directly. You don't need to catch any exceptions though.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:262: if
(switchInProgress) {
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> This check should be done first, before waiting on sessionOpening.

Modified as discussed offline.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:283: for (int i = 0;
i < deviceNames.length; i++) {
On 2016/07/28 14:24:49, magjed_webrtc wrote:
> Use indexOf() instead on the deviceNames list. It would be nice if
> getDeviceNames() would return a list directly instead of an array.

Done.

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
File webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/1/webrtc/api/android/java/src/o...
webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java:57: void
stopCapture(boolean waitCameraStop);
On 2016/07/28 14:24:50, magjed_webrtc wrote:
> Who needs this besides the test? Can't they wait for onCameraClosed() instead?

Yes, I think that might be a better idea.

[magjed_webrtc, 2016-08-08 09:48:53]

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:58: synchronized
(stateLock) {
You can remove this synchronize block now.

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:73:
cameraThreadHandler.postDelayed(new Runnable() {
This will post twice. How about adding a delay argument to
createSessionInternal() so you don't need to post here?

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/androidtests/s...
File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java
(right):

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/androidtests/s...
webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:243:
cameraClosedLock.wait();
Can you use ThreadUtils.waitUninteruptibly?

[sakal, 2016-08-08 12:00:14]

Patchset #4 (id:60001) has been deleted

[sakal, 2016-08-08 12:02:06]

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:58: synchronized
(stateLock) {
On 2016/08/08 09:48:53, magjed_webrtc wrote:
> You can remove this synchronize block now.

Done.

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:73:
cameraThreadHandler.postDelayed(new Runnable() {
On 2016/08/08 09:48:53, magjed_webrtc wrote:
> This will post twice. How about adding a delay argument to
> createSessionInternal() so you don't need to post here?

Done.

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/androidtests/s...
File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java
(right):

https://codereview.webrtc.org/2186253002/diff/40001/webrtc/api/androidtests/s...
webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:243:
cameraClosedLock.wait();
On 2016/08/08 09:48:53, magjed_webrtc wrote:
> Can you use ThreadUtils.waitUninteruptibly?

I made it just pass the InterruptedException like the rest of the code.

[magjed_webrtc, 2016-08-08 12:46:05]

lgtm

[sakal, 2016-08-08 12:47:43]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-08-08 12:47:50]

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-08-08 12:54:59]

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

[commit-bot: I haz the power, 2016-08-08 12:55:00]

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

[sakal, 2016-08-08 12:56:33]

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

[sakal, 2016-08-08 12:56:34]

Need additional approval for 	webrtc/base/java/src/org/webrtc/ThreadUtils.java.

Tommi, PTAL.

[sakal, 2016-08-09 12:59:17]

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

[commit-bot: I haz the power, 2016-08-09 12:59:23]

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-08-09 15:06:20]

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

[commit-bot: I haz the power, 2016-08-09 15:06:21]

Dry run: This issue passed the CQ dry run.

[sakal, 2016-08-10 12:13:39]

Description was changed from

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
==========

to

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

[sakal, 2016-08-10 12:14:08]

Description was changed from

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

to

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
==========

[sakal, 2016-08-10 12:14:19]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-08-10 12:14:19]

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/2186253002/#ps100001 (title:
"Fix tests.")

[commit-bot: I haz the power, 2016-08-10 12:14:25]

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-08-10 12:16:33]

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

[commit-bot: I haz the power, 2016-08-10 12:16:34]

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

[sakal, 2016-08-10 12:23:32]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-08-10 12:23:33]

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/2186253002/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-08-10 12:23:45]

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-08-10 12:28:13]

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

[commit-bot: I haz the power, 2016-08-10 12:28:14]

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

[sakal, 2016-08-10 12:56:18]

Description was changed from

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
==========

to

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

[sakal, 2016-08-10 12:56:31]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-08-10 12:56:37]

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-08-10 12:59:05]

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

[commit-bot: I haz the power, 2016-08-10 12:59:06]

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

[sakal, 2016-08-11 07:51:10]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-08-11 07:51:23]

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-08-11 08:00:41]

Description was changed from

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

to

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

[commit-bot: I haz the power, 2016-08-11 08:00:42]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-08-11 08:00:51]

Description was changed from

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org
==========

to

==========
Session based capturing for Camera2Capturer.

BUG=webrtc:6148
TBR=tommi@webrtc.org

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

[commit-bot: I haz the power, 2016-08-11 08:00:52]

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