Camera thread hang when trying to reconfig android camera capture

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
Rank:
9
VERIFIED FIXED
Last year
Last year

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

({regression})

60 Branch
mozilla61
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ verified, firefox61+ verified)

Details

Attachments

(3 attachments)

+++ 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 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.
Attachment #8965695 - Flags: review?(dminor)
Comment on attachment 8965696 [details]
Bug 1452048 - Leave critical section before calling into java StartCapture().

https://reviewboard.mozilla.org/r/234548/#review240188
Attachment #8965696 - Flags: review?(dminor) → review+
Comment on attachment 8965697 [details]
Bug 1452048 - Read _captureStarted under lock on the camera thread.

https://reviewboard.mozilla.org/r/234550/#review240194
Attachment #8965697 - Flags: review?(dminor) → review+
(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 on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

https://reviewboard.mozilla.org/r/234546/#review240538
Attachment #8965695 - Flags: review?(dminor) → review+
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)
Pushed by pehrsons@gmail.com:
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 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
Attachment #8965695 - Flags: approval-mozilla-beta?
Comment on attachment 8965695 [details]
Bug 1452048 - Give Looper lifetime control to external callers.

webrtc fix for fennec, beta60+
Attachment #8965695 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.