Make startCapture return synchronously to avoid Chrome no-responding on N9 with fast tab switching.

On Nexus9, if you open two webrtc tabs and switch them very quickly, then chrome will stop responding soon.
The reason is tab switching on Android will cause capture start/stop accordingly on the OnWasHidden/OnWasShown events. On N9 it's Camera2 API deployed. And with Camera2, capture will start asynchronously. If stopCapture comes too soon, it will fail and the next startCapture will cause the Camera2 into error status(You can't start capture twice in a row with Camera2 API) and freezes Chrome.

This is a simple/straight patch to make the startCapture return synchronously. Please help to advice if this is proper here, or what other ideas you may have.

BUG=501790
Test=Manual Test

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

[braveyao, 2015-08-18 10:18:31]

braveyao@chromium.org changed reviewers:
+ magjed@chromium.org, mcasas@chromium.org

[braveyao, 2015-08-18 10:18:32]

Please help to take a look and advise!

[magjed_chromium, 2015-08-19 14:32:50]

How about you replace |mOpeningCamera|, |mConfiguringCamera|, and
|mCaptureStarted| with a new camera state enum. Something like:
private static enum CameraState {STARTING, STARTED, STOPPING, STOPPED}. 
Then in every function that is about to make a state change, you first block and
wait until the camera is in the expected state. E.g. in startCapture() you add:
synchronized (mStateLock) {
  while (mCameraState != STOPPED) {
    try {
      mStateLock.wait();
    } catch (InterruptedException ex) {
      Log.e(TAG, ex);
    }
  }
  mCameraState = STARTING;
}
and after a state change has occurred, like
CameraCaptureSession.StateCallback.onConfigured(), you add:
synchronized (mStateLock) {
  mCameraState = STARTED;
  mStateLock.notifyAll();
}
The benefit of this is that functions like startCapture() can still be
asynchronous in normal use. Only when you queue multiple calls will it start to
block.

[braveyao, 2015-08-20 08:07:22]

On 2015/08/19 14:32:50, magjed wrote:
> How about you replace |mOpeningCamera|, |mConfiguringCamera|, and
> |mCaptureStarted| with a new camera state enum. Something like:
> private static enum CameraState {STARTING, STARTED, STOPPING, STOPPED}. 
> Then in every function that is about to make a state change, you first block
and
> wait until the camera is in the expected state. E.g. in startCapture() you
add:
> synchronized (mStateLock) {
>   while (mCameraState != STOPPED) {
>     try {
>       mStateLock.wait();
>     } catch (InterruptedException ex) {
>       Log.e(TAG, ex);
>     }
>   }
>   mCameraState = STARTING;
> }
> and after a state change has occurred, like
> CameraCaptureSession.StateCallback.onConfigured(), you add:
> synchronized (mStateLock) {
>   mCameraState = STARTED;
>   mStateLock.notifyAll();
> }
> The benefit of this is that functions like startCapture() can still be
> asynchronous in normal use. Only when you queue multiple calls will it start
to
> block.

magjed@, thanks for the suggestion. It makes the procedure of capture starting
more clear. And also I realized one fault that it's better to notify the event
in all error callback too. 
But I can't understand the comment "The benefit of this is that functions like
startCapture() can still be
asynchronous in normal use. Only when you queue multiple calls will it start to
block.". I can't see how we can keep startCapture asynchronous in normal use?
And there will be no multiple calls to start/stopCapture to the same instance.
The capture_device implementation will allocate a new instance to each
startCapture invocation.
The only way to keep startCapture asynchronous as I understand is to add the
lock.wait() in StopCapture(). So in normal use, there is enough time for the
state to be ready as STARTED. Only with fast tab switching, we will block in
stopCapture() to wait the camera ready to be stopped. It just doesn't look so
good... Your opinion?

[magjed_chromium, 2015-08-20 08:50:47]

On 2015/08/20 08:07:22, braveyao wrote:
> On 2015/08/19 14:32:50, magjed wrote:
> > How about you replace |mOpeningCamera|, |mConfiguringCamera|, and
> > |mCaptureStarted| with a new camera state enum. Something like:
> > private static enum CameraState {STARTING, STARTED, STOPPING, STOPPED}. 
> > Then in every function that is about to make a state change, you first block
> and
> > wait until the camera is in the expected state. E.g. in startCapture() you
> add:
> > synchronized (mStateLock) {
> >   while (mCameraState != STOPPED) {
> >     try {
> >       mStateLock.wait();
> >     } catch (InterruptedException ex) {
> >       Log.e(TAG, ex);
> >     }
> >   }
> >   mCameraState = STARTING;
> > }
> > and after a state change has occurred, like
> > CameraCaptureSession.StateCallback.onConfigured(), you add:
> > synchronized (mStateLock) {
> >   mCameraState = STARTED;
> >   mStateLock.notifyAll();
> > }
> > The benefit of this is that functions like startCapture() can still be
> > asynchronous in normal use. Only when you queue multiple calls will it start
> to
> > block.
> 
> magjed@, thanks for the suggestion. It makes the procedure of capture starting
> more clear. And also I realized one fault that it's better to notify the event
> in all error callback too. 
> But I can't understand the comment "The benefit of this is that functions like
> startCapture() can still be
> asynchronous in normal use. Only when you queue multiple calls will it start
to
> block.". I can't see how we can keep startCapture asynchronous in normal use?
> And there will be no multiple calls to start/stopCapture to the same instance.
> The capture_device implementation will allocate a new instance to each
> startCapture invocation.
> The only way to keep startCapture asynchronous as I understand is to add the
> lock.wait() in StopCapture(). So in normal use, there is enough time for the
> state to be ready as STARTED. Only with fast tab switching, we will block in
> stopCapture() to wait the camera ready to be stopped. It just doesn't look so
> good... Your opinion?

I meant adding the lock.wait() stuff in all state change function including
stopCapture(). startCapture() should not block in normal use, because we are in
the correct state "STOPPED" already. If we always allocate a new instance for
each startCapture() invocation, maybe this is overkill. But can you move your
blocking from startCapture() to stopCapture() instead? Or is there any way to
abort opening/configuring of the camera in stopCapture() if we are in that
state?

[braveyao, 2015-08-20 09:08:24]

On 2015/08/20 08:50:47, magjed wrote:
> On 2015/08/20 08:07:22, braveyao wrote:
> > On 2015/08/19 14:32:50, magjed wrote:
> > > How about you replace |mOpeningCamera|, |mConfiguringCamera|, and
> > > |mCaptureStarted| with a new camera state enum. Something like:
> > > private static enum CameraState {STARTING, STARTED, STOPPING, STOPPED}. 
> > > Then in every function that is about to make a state change, you first
block
> > and
> > > wait until the camera is in the expected state. E.g. in startCapture() you
> > add:
> > > synchronized (mStateLock) {
> > >   while (mCameraState != STOPPED) {
> > >     try {
> > >       mStateLock.wait();
> > >     } catch (InterruptedException ex) {
> > >       Log.e(TAG, ex);
> > >     }
> > >   }
> > >   mCameraState = STARTING;
> > > }
> > > and after a state change has occurred, like
> > > CameraCaptureSession.StateCallback.onConfigured(), you add:
> > > synchronized (mStateLock) {
> > >   mCameraState = STARTED;
> > >   mStateLock.notifyAll();
> > > }
> > > The benefit of this is that functions like startCapture() can still be
> > > asynchronous in normal use. Only when you queue multiple calls will it
start
> > to
> > > block.
> > 
> > magjed@, thanks for the suggestion. It makes the procedure of capture
starting
> > more clear. And also I realized one fault that it's better to notify the
event
> > in all error callback too. 
> > But I can't understand the comment "The benefit of this is that functions
like
> > startCapture() can still be
> > asynchronous in normal use. Only when you queue multiple calls will it start
> to
> > block.". I can't see how we can keep startCapture asynchronous in normal
use?
> > And there will be no multiple calls to start/stopCapture to the same
instance.
> > The capture_device implementation will allocate a new instance to each
> > startCapture invocation.
> > The only way to keep startCapture asynchronous as I understand is to add the
> > lock.wait() in StopCapture(). So in normal use, there is enough time for the
> > state to be ready as STARTED. Only with fast tab switching, we will block in
> > stopCapture() to wait the camera ready to be stopped. It just doesn't look
so
> > good... Your opinion?
> 
> I meant adding the lock.wait() stuff in all state change function including
> stopCapture(). startCapture() should not block in normal use, because we are
in
> the correct state "STOPPED" already. If we always allocate a new instance for
> each startCapture() invocation, maybe this is overkill. But can you move your
> blocking from startCapture() to stopCapture() instead? Or is there any way to
> abort opening/configuring of the camera in stopCapture() if we are in that
> state?

OK, I see what you mean. And under current situation, it's equivalent to try to
block only in stopCapture in case the stopCapture comes too soon, and call
notify with each state change in those callbacks. I'll try this way then.

BTW: I agree it should be overkilling. But it's another topic. Maybe we can
discuss it later in another thread.

[braveyao, 2015-08-20 11:07:39]

Revised! Please help to take another look!

[magjed_chromium, 2015-08-20 12:02:38]

I like this.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(left):

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:438: if
(mCaptureSession == null) return false;
Is this check not necessary anymore?

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:49:
mCameraState = CameraState.CONFIGURING;
I know no one is waiting for OPENING->CONFIGURING transition, but maybe use
cameraStateChangeNotify(CameraState.CONFIGURING) for consistency anyway?

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:393: if
(mCameraState == CameraState.OPENING
Put this if-statement in a 'synchronized (mCameraStateLock)' block.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:418:
mCameraState = CameraState.OPENING;
cameraStateChangeNotify(CameraState.OPENING);

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:448: if
(mCameraState != CameraState.STARTED
Maybe replace 'if' with 'while' to make it more robust, in case the
mCameraStateLock.notifyAll() is for some other notification.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:457: if
(mCameraState == CameraState.STOPPED) return true;
When can this happen? Multiple stopCapture() requests in a row?

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:474:
mCameraState = CameraState.STOPPED;
cameraStateChangeNotify(CameraState.STOPPED);

[braveyao, 2015-08-21 07:19:56]

Comments are all addressed/answered. PTAL!

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(left):

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:438: if
(mCaptureSession == null) return false;
On 2015/08/20 12:02:38, magjed wrote:
> Is this check not necessary anymore?

No need any more, as I think. Now we only handle STARTED and STOPPED states.
STARTED means the mCaptureSession have value. And as to STOPPED, we won't move
on. So it should be OK.
Anyway it does no harm. So keeping it or not?

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:49:
mCameraState = CameraState.CONFIGURING;
On 2015/08/20 12:02:38, magjed wrote:
> I know no one is waiting for OPENING->CONFIGURING transition, but maybe use
> cameraStateChangeNotify(CameraState.CONFIGURING) for consistency anyway?

Done.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:393: if
(mCameraState == CameraState.OPENING
On 2015/08/20 12:02:37, magjed wrote:
> Put this if-statement in a 'synchronized (mCameraStateLock)' block.

Done.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:418:
mCameraState = CameraState.OPENING;
On 2015/08/20 12:02:38, magjed wrote:
> cameraStateChangeNotify(CameraState.OPENING);

Done.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:448: if
(mCameraState != CameraState.STARTED
On 2015/08/20 12:02:38, magjed wrote:
> Maybe replace 'if' with 'while' to make it more robust, in case the
> mCameraStateLock.notifyAll() is for some other notification.

Done.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:457: if
(mCameraState == CameraState.STOPPED) return true;
On 2015/08/20 12:02:38, magjed wrote:
> When can this happen? Multiple stopCapture() requests in a row?

I firstly noticed the error callback when there are multi startCapture()
request, which is all this issue about. If we call stopCapture() before the
unexpected startCapture, we need the processing here I suppose.

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:474:
mCameraState = CameraState.STOPPED;
On 2015/08/20 12:02:38, magjed wrote:
> cameraStateChangeNotify(CameraState.STOPPED);

Done.

[magjed_chromium, 2015-08-21 08:40:18]

lgtm

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(left):

https://codereview.chromium.org/1294953004/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:438: if
(mCaptureSession == null) return false;
On 2015/08/21 07:19:56, braveyao wrote:
> On 2015/08/20 12:02:38, magjed wrote:
> > Is this check not necessary anymore?
> 
> No need any more, as I think. Now we only handle STARTED and STOPPED states.
> STARTED means the mCaptureSession have value. And as to STOPPED, we won't move
> on. So it should be OK.
> Anyway it does no harm. So keeping it or not? 

Ok, don't keep it.

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:395: ||
mCameraState == CameraState.CONFIGURING) {
I think you can fit the if-statement on one line. The limit is 100 chars.

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:451: &&
mCameraState != CameraState.STOPPED) {
This also fits on the line above.

[braveyao, 2015-08-21 09:53:08]

Thanks so much magjed@!

mcasas@, the OWNER!

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:395: ||
mCameraState == CameraState.CONFIGURING) {
On 2015/08/21 08:40:18, magjed wrote:
> I think you can fit the if-statement on one line. The limit is 100 chars.

Done. Yes it's 98 :)

https://codereview.chromium.org/1294953004/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:451: &&
mCameraState != CameraState.STOPPED) {
On 2015/08/21 08:40:18, magjed wrote:
> This also fits on the line above.

Done.

[mcasas, 2015-08-21 21:22:14]

LGTM % comment+nit below.

https://codereview.chromium.org/1294953004/diff/60001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/1294953004/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:282:
private void cameraStateChangeNotify(CameraState state) {
s/cameraStateChangeNotify/changeCameraStateAndNotify/ ?

https://codereview.chromium.org/1294953004/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java:447: //
even make Chrome no-responding. So wait camera to be STARTED.
nit: reflow this text to 100 columns.

[braveyao, 2015-08-25 03:12:47]

The CQ bit was checked by braveyao@chromium.org

[braveyao, 2015-08-25 03:12:47]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@chromium.org,
mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1294953004/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2015-08-25 03:12:57]

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

[commit-bot: I haz the power, 2015-08-25 04:21:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-25 04:21:53]

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

[battre, 2015-08-25 07:26:37]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1316653002/ by battre@chromium.org.

The reason for reverting is: Attempting a revert to see whether this fixes
WebRtcGetUserMediaBrowserTest.RenderVideoTrackInMultipleTagsAndPause.