1452048 - Camera thread hang when trying to reconfig android camera capture

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Andreas Pehrson [:pehrsons]]

+++ This bug was initially created as a clone of Bug #1440255 +++

See bug 1440255 comment 29.

Repros on Android. You need a camera that can do higher resolution than 640x480.

STR:
1. In one tab, load https://jsfiddle.net/pehrsons/r1jz5q9h/
2. Click Start. Approve the permission prompt. It should say "Started" below the video element.
3. In another tab, load https://jsfiddle.net/pehrsons/yrbxrzhq/
4. Click Start. Approve all the permission prompts. On desktop, it's ok to persist the permissions.

Note that in both fiddles you must select the same camera.

Expected:
The first tab should not change resolution and not crash.

Expected:
The second tab should pass all tests. It will say "DONE All 27 cases tested" at the bottom when done.


Actual:
When the second tab tries a higher resolution than 640x480 it hangs. The video in the first tab freezes at this point too.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1452048 - Give Looper lifetime control to external callers.

Review commit: https://reviewboard.mozilla.org/r/234546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234546/

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1452048 - Leave critical section before calling into java StartCapture().

If we stay in the critical section, and the StartCapture() is a reconfiguring
one, we risk deadlocking with IncomingFrame which runs on the camera thread.

Review commit: https://reviewboard.mozilla.org/r/234548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234548/

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1452048 - Read _captureStarted under lock on the camera thread.

It is written under lock on the controlling thread (CamerasParent) in
StartCapture/StopCapture.

Review commit: https://reviewboard.mozilla.org/r/234550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234550/

Comment 4

[Dan Minor [:dminor]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

https://reviewboard.mozilla.org/r/234546/#review240186

This change looks fine, but it is not obvious to me why you want to make it. Please add a line to your commit message with some explanation.

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8965696 [details]
Bug 1452048 - Leave critical section before calling into java StartCapture().

https://reviewboard.mozilla.org/r/234548/#review240188

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 8965697 [details]
Bug 1452048 - Read _captureStarted under lock on the camera thread.

https://reviewboard.mozilla.org/r/234550/#review240194

Comment 7

[Andreas Pehrson [:pehrsons]]

(In reply to Dan Minor [:dminor] from comment #4)
> Comment on attachment 8965695 [details]
> Bug 1452048 - Give Looper lifetime control to external callers.
> 
> https://reviewboard.mozilla.org/r/234546/#review240186
> 
> This change looks fine, but it is not obvious to me why you want to make it.
> Please add a line to your commit message with some explanation.

For completion I'll put a comment here too.

If we quit the looper in stopCaptureOnCameraThread(), and startCaptureOnCameraThread() is trying to reconfig the device by doing a restart with new settings (basically `stopCaptureOnCameraThread(); startCaptureOnCameraThread(newSettings)`), the looper will have quit when we're done. This means no more message can be posted on the camera thread which will run to completion. It will also not be able to deliver any more frames.

This was not intended by the caller of startCapture() and is clearly wrong. We should only quit the looper when the caller intended to not need it anymore.

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234546/diff/1-2/

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965696 [details]
Bug 1452048 - Leave critical section before calling into java StartCapture().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234548/diff/1-2/

Comment 10

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965697 [details]
Bug 1452048 - Read _captureStarted under lock on the camera thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234550/diff/1-2/

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

https://reviewboard.mozilla.org/r/234546/#review240538

Comment 12

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e069a4ddf0748d14114afb0c3bb0c965a02f98c2 -d ea27d33d3920: rebasing 456808:e069a4ddf074 "Bug 1452048 - Give Looper lifetime control to external callers. r=dminor"
merging media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java
warning: conflicts while merging media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234546/diff/2-3/

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965696 [details]
Bug 1452048 - Leave critical section before calling into java StartCapture().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234548/diff/2-3/

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965697 [details]
Bug 1452048 - Read _captureStarted under lock on the camera thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234550/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7108113d5a4d
Give Looper lifetime control to external callers. r=dminor
https://hg.mozilla.org/integration/autoland/rev/729193a3d03a
Leave critical section before calling into java StartCapture(). r=dminor
https://hg.mozilla.org/integration/autoland/rev/a83f3f831cab
Read _captureStarted under lock on the camera thread. r=dminor

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7108113d5a4d
https://hg.mozilla.org/mozilla-central/rev/729193a3d03a
https://hg.mozilla.org/mozilla-central/rev/a83f3f831cab

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

This approval request applies to all patches on this bug.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1299515
[User impact if declined]: Some sites may cause camera capture to hang indefinitely on Android. Using camera capture in more than one tab at the same time may make this worse.
[Is this code covered by automated tests?]: No. We don't have automation with real devices on Android.
[Has the fix been verified in Nightly?]: No. I have verified it locally however.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0.
[List of other uplifts needed for the feature/fix]: Bug 1440255 which is already uplifted.
[Is the change risky?]: There's some risk.
[Why is the change risky/not risky?]: These patches change some threading lifetime thread safety attributes on Android only. There's always risk involved when touching threading mechanisms and the worst-case scenarios are quite bad. These changes are quite simple however, and we're making the code for starting capture have the same thread safety attributes as the code for stopping capture already has. This precedence speaks in favor of taking this.
[String changes made/needed]: None

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

webrtc fix for fennec, beta60+

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b932d9bda916
https://hg.mozilla.org/releases/mozilla-beta/rev/5032bbe4c92f
https://hg.mozilla.org/releases/mozilla-beta/rev/7617bd6c7f2c

Comment 21

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Samsung Galaxy S8 (Android 8.0)

Verified this following the steps provided in Comment 0 on the latest Nightly (2017-04-13) and Beta (60.0b12) builds.

The video in the first tab[1] did not crash when the tests from the second tab[2] were running. All tests passed successfully. Marking this as verified.

[1] https://jsfiddle.net/pehrsons/r1jz5q9h/
[2] https://jsfiddle.net/pehrsons/yrbxrzhq/