1369108 - Ask for WebRTC app permissions asynchronously on Android

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Currently on Android, we ask for camera permission synchronously on the media thread [1]. It's bad because,

1) It can indefinitely block the media thread

2) The way we ask for permission in VideoCaptureDeviceInfoAndroid.java is Fennec-specific and will fail to work for GeckoView

I think the correct solution is to introduce an earlier step in the gUM chain to ask for permissions asynchronously.

[1] https://dxr.mozilla.org/mozilla-central/rev/7b8937970f9ca85db88cb2496f2112175fd847c8/media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureDeviceInfoAndroid.java#78

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1369108 - 1. Introduce optional device permission step;

Right before `MediaManager::EnumerateRawDevices` posts its enumeration
task to the background thread, add the option to send out a
"getUserMedia:ask-device-permission" notification, whose purpose is to
give frontend UI a chance to ask for device permissions (e.g. permission
to enumerate cameras on Android).

The subject parameter of the notification is an `nsIRunnable` task,
which is passed back to `MediaManager`, after receiving permission,
through the `getUserMedia:got-device-permission` notification. The
notification listener then posts the task and enumeration proceeds as
before.
---
 dom/media/MediaManager.cpp | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

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

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1369108 - 2. Refresh Android camera list when necessary;

Currently, if permission is first denied, the list of cameras is empty.
However, if permission is later granted, the list stays empty because we
never try to refresh the list. This patch causes us to refresh the list
when necessary.
---
 .../trunk/webrtc/modules/video_capture/android/device_info_android.cc    | 1 +
 1 file changed, 1 insertion(+)

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

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1369108 - 3. Implement new device permission code path for Fennec;

Instead of asking for permission in VideoCaptureDeviceInfoAndroid.java,
we now merely check for permission there. The actual permission prompt
now happens in WebrtcUI.js, using the new
"getUserMedia:ask-device-permission" and
"getUserMedia:got-device-permission" notifications.
---
 .../videoengine/VideoCaptureDeviceInfoAndroid.java  |  4 ++--
 mobile/android/app/mobile.js                        |  3 +++
 mobile/android/chrome/content/WebrtcUI.js           | 21 ++++++++++++++++++---
 mobile/android/chrome/content/browser.js            |  3 ++-
 4 files changed, 25 insertions(+), 6 deletions(-)

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

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8873109 [details]
Bug 1369108 - 2. Refresh Android camera list when necessary;

https://reviewboard.mozilla.org/r/144562/#review148808

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8873108 [details]
Bug 1369108 - 1. Introduce optional device permission step;

https://reviewboard.mozilla.org/r/144560/#review148814

::: dom/media/MediaManager.cpp:1759
(Diff revision 1)
> +    const char16_t* const type =
> +      aVideoType != MediaSourceEnum::Camera ?     u"audio" :
> +      aAudioType != MediaSourceEnum::Microphone ? u"video" :
> +                                                  u"all";

find it easier to read with parens

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8873110 [details]
Bug 1369108 - 3. Implement new device permission code path for Fennec;

https://reviewboard.mozilla.org/r/144564/#review149164

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/483b253cdd73
1. Introduce optional device permission step; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c93d5eb308
2. Refresh Android camera list when necessary; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a58469c1801
3. Implement new device permission code path for Fennec; r=esawin

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/483b253cdd73
https://hg.mozilla.org/mozilla-central/rev/54c93d5eb308
https://hg.mozilla.org/mozilla-central/rev/0a58469c1801