Events refactor in Android session based capturing.

Events refactor in Android session based capturing.

In the new implementation session reports all events through
CameraSession.Events interface. CameraCapturer passes these events
forward.

BUG=webrtc:6325
Committed: https://crrev.com/dd0c7f0ea4a432135d06beef40e20560533cb600
Cr-Commit-Position: refs/heads/master@{#14286}

[sakal, 2016-09-15 14:20:25]

Patchset #1 (id:1) has been deleted

[sakal, 2016-09-15 14:20:35]

Patchset #2 (id:40001) has been deleted

[sakal, 2016-09-15 14:20:39]

Patchset #2 (id:60001) has been deleted

[sakal, 2016-09-15 14:20:46]

Patchset #1 (id:20001) has been deleted

[sakal, 2016-09-15 14:20:57]

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

https://codereview.webrtc.org/2331343010/diff/80001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:120: if (session !=
currentSession && currentSession != null) {
I am not sure what we should do here.

[sakal, 2016-09-15 14:21:24]

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

[sakal, 2016-09-15 14:21:24]

PTAL

[magjed_webrtc, 2016-09-15 14:58:06]

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:49: private final
CreateSessionCallback callback;
Any reason for moving these around? Otherwise I would prefer to keep the diff as
small as possible.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:124:
eventsHandler.onCameraClosed();
Now we hold stateLock while making the callbacks. Does that increase the risk of
deadlock?

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:132: if (session !=
currentSession) {
Can we drop frames if we are in an incorrect state, e.g. stopping, as well?

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java (left):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java:124:
checkThread();
Do we call this on another thread now? Is that safe?

[sakal, 2016-09-15 15:08:18]

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:49: private final
CreateSessionCallback callback;
On 2016/09/15 14:58:06, magjed_webrtc wrote:
> Any reason for moving these around? Otherwise I would prefer to keep the diff
as
> small as possible.

I wanted to keep the order consistent everywhere in the code.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:124:
eventsHandler.onCameraClosed();
On 2016/09/15 14:58:06, magjed_webrtc wrote:
> Now we hold stateLock while making the callbacks. Does that increase the risk
of
> deadlock?

If the callbacks invoke method on other threads, deadlocks can happen. I don't
think in practice anyone does so.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:132: if (session !=
currentSession) {
On 2016/09/15 14:58:06, magjed_webrtc wrote:
> Can we drop frames if we are in an incorrect state, e.g. stopping, as well?

I am not sure what you mean. The stopping state is inside the session object.
The session object drops frames itself if it is in stopping state. We should
never be holding a session that is in stopping state anyway.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java (left):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraVideoCapturer.java:124:
checkThread();
On 2016/09/15 14:58:06, magjed_webrtc wrote:
> Do we call this on another thread now? Is that safe?

Yes, we do. It is called from the thread that calls stopCapture. I don't see why
this method wouldn't be thread safe. surfaceTextureHelper is final member.
Handler is final member in surfaceTextureHelper. cameraObserver is final member.
removeCallbacks should be safe to call on multiple threads.

[magjed_webrtc, 2016-09-16 09:00:37]

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:376: if
(captureSession == null) {
We should always clean up here, but maybe you want to fix that in a separate CL?

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:37: new
CameraSession.CreateSessionCallback() {
It would be nice with checkIsOnCameraThread for onDone and onFailure.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:96: new
CameraSession.Events() {
nit: Can you move 'new CameraSession.Events() {' up to the previous line (I
think it fits), and reduce indentation to 2 spaces for the rest of the code?

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:97: public void
onCameraError(CameraSession session, String error) {
It would be nice with checkIsOnCameraThread checks for all these functions, and
comment in Events that they all happen on the camera thread.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:132: if (session !=
currentSession) {
On 2016/09/15 15:08:18, sakal wrote:
> On 2016/09/15 14:58:06, magjed_webrtc wrote:
> > Can we drop frames if we are in an incorrect state, e.g. stopping, as well?
> 
> I am not sure what you mean. The stopping state is inside the session object.
> The session object drops frames itself if it is in stopping state. We should
> never be holding a session that is in stopping state anyway.

I was thinking if the frame dropping in the session could be made redundant by
some check here.

[sakal, 2016-09-16 12:38:59]

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:376: if
(captureSession == null) {
On 2016/09/16 09:00:37, magjed_webrtc wrote:
> We should always clean up here, but maybe you want to fix that in a separate
CL?

I added stop() here now. It doesn't really matter though since CameraCapturer
will call stopCapture() on error anyway.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:37: new
CameraSession.CreateSessionCallback() {
On 2016/09/16 09:00:37, magjed_webrtc wrote:
> It would be nice with checkIsOnCameraThread for onDone and onFailure.

Done.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:96: new
CameraSession.Events() {
On 2016/09/16 09:00:37, magjed_webrtc wrote:
> nit: Can you move 'new CameraSession.Events() {' up to the previous line (I
> think it fits), and reduce indentation to 2 spaces for the rest of the code?

Done.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:97: public void
onCameraError(CameraSession session, String error) {
On 2016/09/16 09:00:37, magjed_webrtc wrote:
> It would be nice with checkIsOnCameraThread checks for all these functions,
and
> comment in Events that they all happen on the camera thread.

Done.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:132: if (session !=
currentSession) {
On 2016/09/16 09:00:37, magjed_webrtc wrote:
> On 2016/09/15 15:08:18, sakal wrote:
> > On 2016/09/15 14:58:06, magjed_webrtc wrote:
> > > Can we drop frames if we are in an incorrect state, e.g. stopping, as
well?
> > 
> > I am not sure what you mean. The stopping state is inside the session
object.
> > The session object drops frames itself if it is in stopping state. We should
> > never be holding a session that is in stopping state anyway.
> 
> I was thinking if the frame dropping in the session could be made redundant by
> some check here.

It is already redundant since we don't keep closed sessions. I kept it for extra
safety. Do you want to remove it?

[sakal, 2016-09-16 12:42:03]

Patchset #5 (id:160001) has been deleted

[magjed_webrtc, 2016-09-16 12:59:19]

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:376: if
(captureSession == null) {
On 2016/09/16 12:38:58, sakal wrote:
> On 2016/09/16 09:00:37, magjed_webrtc wrote:
> > We should always clean up here, but maybe you want to fix that in a separate
> CL?
> 
> I added stop() here now. It doesn't really matter though since CameraCapturer
> will call stopCapture() on error anyway.

Yeah probably, but it's nice if this session is self contained and doesn't rely
on someone else cleaning up. We also have the 'if (currentSession != session)'
check which sounds like we can miss to call stopCapture() sometimes.

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/CameraCapturer.java (right):

https://codereview.webrtc.org/2331343010/diff/100001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/CameraCapturer.java:132: if (session !=
currentSession) {
On 2016/09/16 12:38:58, sakal wrote:
> On 2016/09/16 09:00:37, magjed_webrtc wrote:
> > On 2016/09/15 15:08:18, sakal wrote:
> > > On 2016/09/15 14:58:06, magjed_webrtc wrote:
> > > > Can we drop frames if we are in an incorrect state, e.g. stopping, as
> well?
> > > 
> > > I am not sure what you mean. The stopping state is inside the session
> object.
> > > The session object drops frames itself if it is in stopping state. We
should
> > > never be holding a session that is in stopping state anyway.
> > 
> > I was thinking if the frame dropping in the session could be made redundant
by
> > some check here.
> 
> It is already redundant since we don't keep closed sessions. I kept it for
extra
> safety. Do you want to remove it?

Good, no keep it.

https://codereview.webrtc.org/2331343010/diff/180001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/180001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:385: stop();
I would like to flatten the number of function indirections and different code
paths. If we add null checks in stopInternal(), would it be possible to write
this function like:

final boolean startFailure = (cameraDevice == null);
state = SessionState.STOPPED;
stopInternal();
if (startFailure) {
  callback.onFailure(error);
} else {
  events.onCameraError(this, error);
}
?

[sakal, 2016-09-16 13:17:56]

Patchset #6 (id:200001) has been deleted

[sakal, 2016-09-16 13:25:16]

https://codereview.webrtc.org/2331343010/diff/180001/webrtc/api/android/java/...
File webrtc/api/android/java/src/org/webrtc/Camera2Session.java (right):

https://codereview.webrtc.org/2331343010/diff/180001/webrtc/api/android/java/...
webrtc/api/android/java/src/org/webrtc/Camera2Session.java:385: stop();
On 2016/09/16 12:59:19, magjed_webrtc wrote:
> I would like to flatten the number of function indirections and different code
> paths. If we add null checks in stopInternal(), would it be possible to write
> this function like:
> 
> final boolean startFailure = (cameraDevice == null);
> state = SessionState.STOPPED;
> stopInternal();
> if (startFailure) {
>   callback.onFailure(error);
> } else {
>   events.onCameraError(this, error);
> }
> ?

Done.

[magjed_webrtc, 2016-09-16 13:50:48]

lgtm

[sakal, 2016-09-19 09:04:56]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-09-19 09:05:00]

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

[sakal, 2016-09-19 09:33:59]

The CQ bit was unchecked by sakal@webrtc.org

[sakal, 2016-09-19 09:40:03]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-09-19 09:40:04]

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/2331343010/#ps240001 (title:
"Remove CameraSession parameter from onCameraOpening.")

[commit-bot: I haz the power, 2016-09-19 09:40: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, 2016-09-19 11:37:19]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2016-09-19 11:37:29]

Description was changed from

==========
Events refactor in Android session based capturing.

In the new implementation session reports all events through
CameraSession.Events interface. CameraCapturer passes these events
forward.

BUG=webrtc:6325
==========

to

==========
Events refactor in Android session based capturing.

In the new implementation session reports all events through
CameraSession.Events interface. CameraCapturer passes these events
forward.

BUG=webrtc:6325

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

[commit-bot: I haz the power, 2016-09-19 11:37:30]

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