Closed Bug 874572 Opened 11 years ago Closed 11 years ago

getUserMedia - Browser holding lock on device camera despite browser activity in the background and paused

Categories

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

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox24 --- verified

People

(Reporter: aaronmt, Assigned: gcp)

References

Details

(Whiteboard: [getUserMedia][android-gum?])

Attachments

(2 files, 2 obsolete files)

Currently, one can not use the device system camera despite the browser activity being sent to the background and paused. Attempts to launch the device camera yield a runtime exception: 

E/CameraClient(29553): Could not open camera 0: -1
E/CameraClient(29553): initialize: Camera 0: unable to initialize device: Operation not permitted (-1)
I/CameraClient(29553): Destroying camera 0
E/CameraHolder(30157): fail to connect Camera
E/CameraHolder(30157): java.lang.RuntimeException: Fail to connect to camera service
E/CameraHolder(30157): 	at android.hardware.Camera.native_setup(Native Method)
E/CameraHolder(30157): 	at android.hardware.Camera.<init>(Camera.java:340)
E/CameraHolder(30157): 	at android.hardware.Camera.open(Camera.java:302)
E/CameraHolder(30157): 	at com.android.camera.CameraManager.cameraOpen(CameraManager.java:283)
E/CameraHolder(30157): 	at com.android.camera.CameraHolder.open(CameraHolder.java:210)
E/CameraHolder(30157): 	at com.android.camera.Util.openCamera(Util.java:315)
E/CameraHolder(30157): 	at com.android.camera.PhotoModule$CameraStartUpThread.run(PhotoModule.java:316)

i) http://mozilla.github.io/webrtc-landing/gum_test.html
ii) Tap 'Video'
iii) Hit the device home-button
iv) Launch the camera

We can not prohibit users from using core system applications on their devices.

The browser should release the lock on the local media when the browser is paused.

--
HTC One (Android 4.1, Nightly 05/21)
LG Nexus 4 (Android 4.2.2, Nightly 05/21)
Whiteboard: [getUserMedia][blocking-gum-][android-gum+]
To add, force-stopping Firefox also does not release the lock on device camera and microphone. Given the nature of Android and device variety and common OOM'ing; the ability to release all held locks before the browser activity dies.
This should only happen if you have an ongoing camera-call or gum call active on a webpage. This means we should terminate those whenever Firefox is backgrounded? I'm not so sure about that one. Our UI should display a clear notification they're active and being used, unless we want to make using WebRTC impossible without having Firefox in the foreground.

>To add, force-stopping Firefox also does not release the lock on device camera and 
>microphone. Given the nature of Android and device variety and common OOM'ing; the 
>ability to release all held locks before the browser activity dies.

OOM-ing kills Firefox, so there is never an opportunity to release the lock. Are you saying that if Firefox is killed, Android doesn't manage to recover these resources? double-u-tee-eff.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> OOM-ing kills Firefox, so there is never an opportunity to release the lock.
> Are you saying that if Firefox is killed, Android doesn't manage to recover
> these resources? double-u-tee-eff.

Well, yesterday I tried just force-stopping in AppInfo and that seemed to be the case .. at least for me on my Nexus 4. Force-stopped, then attempted to open the Camera; got a run-time exception error and that killed the Gallery application too.
There's an UX question here: if we have an active video stream (for example as part of a WebRTC phone call), and Firefox gets backgrounded, do we terminate the video and audio stream and release the camera, or keep it going.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> There's an UX question here: if we have an active video stream (for example
> as part of a WebRTC phone call), and Firefox gets backgrounded, do we
> terminate the video and audio stream and release the camera, or keep it
> going.

What could the audio and video be doing if it were running but attached to a service that had been killed? It seems to me like we should shut it all down at the same time, unless I am missing something else here.
>What could the audio and video be doing if it were running but attached to a 
>service that had been killed?

I'm not sure what you're asking. If the service has been killed, nothing will happen, and I'd expect Android to recover them for other apps (Android being Android, that may be broken).

The question is what to do when we're backgrounded, which is something different from being killed altogether.
Ah, ok. I was getting "backgrounded" confused with being killed due to insufficient memory, sorry. 

In cases where Firefox is still running in the background, we should leave the WebRTC stream on. We want to keep users informed that something is on though, which bug 874401 should help us with (by letting us display an icon in the notification bar and/or turn on a hardware light on the device).
Okay. So what happens if a different application tries to claim access to the camera while FxAndroid holds the camera in use in the background? Should we give up the camera to the other app? Or is this out of control?
(In reply to Jason Smith [:jsmith] from comment #8)
> Should we give up the camera to the other app? Or is this out of control?

Ideally we should give up the camera or mic to whichever foreground app is asking for it. That's how other android apps behave. For example, switching between the instagram camera and the android camera allows whichever app is in the foreground to use the camera.
Android doesn't work this way though. If the foreground app asks Android for the camera it'll just say it's in use and fail. There's no point where we are asked to give it up.

Based on these comments, giving up (at least) camera and probably the microphone when we're backgrounded seems to be the way forward.
This is going to be interesting to fix. We'll have to add an observer for "application-backgrounded" to MediaManager, which on receiving that event will have to walk its internal list of windows, and figure out the corresponding MediaEngine-backends. Each MediaEngine-backend then has a list of known MediaEngineXXXSources. We can extend MediaEngine(Default) to add SuspendAll and ResumeAll methods, but this will require implementing that in all backends (though on non-Android they can probably be NOPs).

To be able to Suspend and Resume the backend implementations will probably have to remember what parameters they were set up with as well.
Hmm...if this is non-trivial to fix, should we continue to block on this? I don't think we blocked on this on desktop with the Skype bug (i.e. a bug where we locked the camera away from Skype).
Whiteboard: [getUserMedia][blocking-gum-][android-gum+] → [getUserMedia][blocking-gum-][android-gum?]
I'd like to know what Chrome does BTW, if their WebRTC on Android is usable by now.
An alternate approach: the current coupling between the Fennec Java code and the WebRTC Java code already contains the stuff needed to pass an application context. This may be enough to allow the WebRTC VideoCapture code to register a set of callbacks and hook those up to the onPause/onResume events in GeckoApp, the moment it's starting the camera.

I initially thought this would be hacky, but after thinking about it more it allows making the entire Gecko C++ stack oblivious to the Android-specific suspend/resume behavior needed, so it's probably a simpler and better solution.

If the WebRTC C++ stack can be oblivious to the onPause/onResume, it also vastly improves the odds that resuming the call when Fennec is foregrounded again will work without much further complication.
Requires the patch in Bug 874546.
Assignee: nobody → gpascutto
Attachment #757410 - Flags: review?(blassey.bugs)
Attachment #757411 - Flags: review?(blassey.bugs)
Whiteboard: [getUserMedia][blocking-gum-][android-gum?] → [getUserMedia][android-gum?]
Attachment #757411 - Flags: review?(blassey.bugs) → review+
Comment on attachment 757410 [details] [diff] [review]
Patch 1. Suspend and resume the camera.

Review of attachment 757410 [details] [diff] [review]:
-----------------------------------------------------------------

Basically good, but I'd like to see a new patch

::: media/webrtc/trunk/webrtc/modules/video_capture/android/java/org/webrtc/videoengine/VideoCaptureAndroid.java
@@ +105,5 @@
>          currentDevice = in_device;
>  
>          try {
>  	  GeckoAppShell.getGeckoInterface().getCameraView().getHolder().addCallback(this);
> +          ThreadUtils.getUiHandler().post(new Runnable() {

I think this ws is wrong

::: mobile/android/base/GeckoApp.java
@@ +215,5 @@
>      public SurfaceView getCameraView() {
>          return cameraView;
>      }
>  
> +    public void addPauseCallback(Runnable callback) {

these are really more listeners than callbacks, so s/Callback/Listener/g

Also, why are they Runnables? I'd prefer that you make a new interface for listeners.
Attachment #757410 - Flags: review?(blassey.bugs) → review-
Make a listener API and replace the callbacks with it. Should be nicely extensible when we want to listen for device orientation changes as well.
Accidentally merged patch 2 into this one.
Attachment #757410 - Attachment is obsolete: true
Attachment #757411 - Attachment is obsolete: true
Attachment #758036 - Flags: review?(blassey.bugs)
Attachment #758036 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/daed2157e09e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: verifyme
QA Contact: jsmith
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 879947
You need to log in before you can comment on or make changes to this bug.