Bug 164010 – [WebRTC][OpenWebRTC] Implement device permissions handling solution for owr backend in the UI proces

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

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.

[Eric Carlson]

(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;

[Philippe Normand]

Ah yes, good idea!

[Alejandro G. Castro]

Created attachment 294059 [details]
Patch

[Philippe Normand]

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?

[Alejandro G. Castro]

(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.

[Philippe Normand]

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?

[Alejandro G. Castro]

Created attachment 294356 [details]
Patch

[Philippe Normand]

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.

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 294365 [details]
Patch

[WebKit Commit Bot]

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

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

Created attachment 294489 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 294489 [details]
Patch

Clearing flags on attachment: 294489

Committed r208587: <http://trac.webkit.org/changeset/208587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.