Bug 164010 - [WebRTC][OpenWebRTC] Implement device permissions handling solution for owr backend in the UI proces
Summary: [WebRTC][OpenWebRTC] Implement device permissions handling solution for owr b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-26 04:11 PDT by Philippe Normand
Modified: 2016-11-11 05:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.06 KB, patch)
2016-11-07 06:16 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2016-11-10 02:47 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2016-11-10 05:32 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (8.68 KB, patch)
2016-11-11 02:35 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2016-10-26 04:11:06 PDT
Since bug 162147 was fixed our RealtimeMediaSourceCenter is broken because the media devices are enumerated in the instance running in the UI process. The instance running in the Web process has an empty sourceMap hashmap. I think that to solve this we should move the enumeration process in a CaptureDeviceManager implementation, similar to what the Mac port does.

We will also likely need a synchronous OWR API to list devices. The API we have for this right now is asynchronous.
Comment 1 Eric Carlson 2016-10-26 06:15:44 PDT
(In reply to comment #0)
> Since bug 162147 was fixed our RealtimeMediaSourceCenter is broken because
> the media devices are enumerated in the instance running in the UI process.
> The instance running in the Web process has an empty sourceMap hashmap. I
> think that to solve this we should move the enumeration process in a
> CaptureDeviceManager implementation, similar to what the Mac port does.
> 
> We will also likely need a synchronous OWR API to list devices. The API we
> have for this right now is asynchronous.

Or you can make RealtimeMediaSourceCenter::getMediaStreamDevices asynchronous, e.g. it could take a lambda:

    using GetDevicesHandler = std::function<void(Vector<CaptureDevice>&&)>;
    virtual void getMediaStreamDevices(GetDevicesHandler) = 0;
Comment 2 Philippe Normand 2016-10-26 06:19:27 PDT
Ah yes, good idea!
Comment 3 Alejandro G. Castro 2016-11-07 06:16:23 PST
Created attachment 294059 [details]
Patch
Comment 4 Philippe Normand 2016-11-07 07:38:36 PST
Comment on attachment 294059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294059&action=review

> Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:173
> +        m_completionHandler(MediaStreamPrivate::create(audioSources, videoSources));

Hum here I think that either audioSources or videoSources could be NULL. Is it properly handled?
Comment 5 Alejandro G. Castro 2016-11-08 00:18:31 PST
(In reply to comment #4)
> Comment on attachment 294059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294059&action=review
> 
> > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:173
> > +        m_completionHandler(MediaStreamPrivate::create(audioSources, videoSources));
> 
> Hum here I think that either audioSources or videoSources could be NULL. Is
> it properly handled?

Yes, the completion handler basically checks if they are null or not to request audio or video permissions to the user.

Thanks for the review.
Comment 6 Philippe Normand 2016-11-09 08:48:23 PST
This patch doesn't work very well here :( Please wait before landing this.

I started experimenting with a proper CaptureDeviceManager implementation. The main drawback I see so far is that we'd need to list devices once from the UI process and once from the Web process. Also I think we can "forge" device IDs using their name maybe?
Comment 7 Alejandro G. Castro 2016-11-10 02:47:48 PST
Created attachment 294356 [details]
Patch
Comment 8 Philippe Normand 2016-11-10 02:50:15 PST
Comment on attachment 294356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294356&action=review

> Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:148
> +#ifndef NDEBUG

Why this in a ifdef ? test sources can be useful whatever the WebKit build config.
Comment 9 Alejandro G. Castro 2016-11-10 02:55:50 PST
(In reply to comment #8)
> Comment on attachment 294356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294356&action=review
> 
> > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:148
> > +#ifndef NDEBUG
> 
> Why this in a ifdef ? test sources can be useful whatever the WebKit build
> config.

Right, I was considering it a debug feature but I think you are right.
Comment 10 Alejandro G. Castro 2016-11-10 05:32:18 PST
Created attachment 294365 [details]
Patch
Comment 11 WebKit Commit Bot 2016-11-11 01:59:06 PST
Comment on attachment 294365 [details]
Patch

Rejecting attachment 294365 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 294365, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
1 FAILED at 81.
Hunk #2 succeeded at 147 (offset 2 lines).
Hunk #3 succeeded at 176 with fuzz 1 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp.rej
patching file Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Philippe Normand']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2497039
Comment 12 Alejandro G. Castro 2016-11-11 02:18:36 PST
The patch is not valid anymore after r208559, the API of the ValidConstraintsHandler does not use RealtimeMediaSource anymore. This is good because we can avoid creating the dummy source objects in the UI process.
Comment 13 Alejandro G. Castro 2016-11-11 02:35:39 PST
Created attachment 294489 [details]
Patch
Comment 14 WebKit Commit Bot 2016-11-11 05:12:39 PST
Comment on attachment 294489 [details]
Patch

Clearing flags on attachment: 294489

Committed r208587: <http://trac.webkit.org/changeset/208587>
Comment 15 WebKit Commit Bot 2016-11-11 05:12:43 PST
All reviewed patches have been landed.  Closing bug.