Bug 109494 – AudioDestination::create() needs extra device identification information for live/local input streams

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

AudioDestination::create() needs extra device identification information for live/local input streams

[Chris Rogers]

Created attachment 187682 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16485685

[EFL EWS Bot]

Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16495506

[Build Bot]

Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16531042

[Chris Rogers]

Created attachment 187915 [details]
Patch

[James Robinson]

Comment on attachment 187915 [details]
Patch

I can't quite follow the flow of the ExtraData object in this patch.  Where is this object allocated, where is it used, and where is it destroyed?

[Chris Rogers]

(In reply to comment #7)
> (From update of attachment 187915 [details])
> I can't quite follow the flow of the ExtraData object in this patch.  Where is this object allocated, where is it used, and where is it destroyed?

Basically what happens is that a MediaStream is created by the embedder as a result of a call to getUserMedia().  This assumes that the user has given permission to provide access to the microphone/audio-input, and/or camera when the user agent asks for access.

In the code-paths touched by this patch, this MediaStream can have one or more tracks, one of which is an audio track.  This audio track contains a MediaStreamSource which has the extra data (set by the embedder once the user gives authorization to the getUserMedia() call).  This "extra data" can contain information depending on which device the user allows access to.  In the case of audio, there may be more than one audio input device, and this "extra data" can contain specific information about which one.  This is just an opaque object, and each embedder/port can handle the details of how this is represented.

Some time after this MediaStream is made available to the client JS code, the JS code may create an AudioNode which represents a source for live/local audio input like this:

var sourceNode = audioContext.createMediaStreamSource(mediaStream);

It's at this time that the AudioContext must potentially re-configure its connection to the audio hardware to enable input, given the specific audio input device.  Up until this point, the audio hardware has only been configured to output audio and not handle input.

[James Robinson]

I think Darin's more familiar with how the ExtraData pattern is used.  Would you mind giving this a once-over, Darin?  I can do a line-by-line review once the general concepts are settled.

[Tommy Widenflycht]

Comment on attachment 187915 [details]
Patch

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

This patch needs serious refactoring. The major issue here is that the ExtraData pattern is a) wrongly used and b) wrongly redesigned. There's memory leaks and illegal typecasts for example. Besides there are existing code that uses the current ExtraData of WebMediaStreamSource will will break so this code will introduce a major regression for WebRTC.

If you need to pass some data you need to find another way.

> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:124
> +    m_private->setExtraData(reinterpret_cast<MediaStreamSource::ExtraData*>(extraData));

Why on earth are you removing the typesafe construct here? The extraData pointer is of type WebMediaStreamSource::ExtraData not MediaStreamSource::ExtraData.

[Chris Rogers]

(In reply to comment #10)
> (From update of attachment 187915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187915&action=review
> 
> This patch needs serious refactoring. The major issue here is that the ExtraData pattern is a) wrongly used and b) wrongly redesigned. There's memory leaks and illegal typecasts for example. Besides there are existing code that uses the current ExtraData of WebMediaStreamSource will will break so this code will introduce a major regression for WebRTC.
> 
> If you need to pass some data you need to find another way.
> 
> > Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:124
> > +    m_private->setExtraData(reinterpret_cast<MediaStreamSource::ExtraData*>(extraData));
> 
> Why on earth are you removing the typesafe construct here? The extraData pointer is of type WebMediaStreamSource::ExtraData not MediaStreamSource::ExtraData.

Tommy, I'm certainly open to suggestions.  But as it stands, the current model for ExtraData is not sufficient for passing an opaque object back and forth between the embedder/port and WebCore internals.  We need to have a "token" which can be passed back and forth, translated in both directions:

embedder->WebCore
WebCore->embedder

In the current model, as I understand the code, it's impossible to translate a MediaStreamSource::ExtraData back into a WebMediaStreamSource::ExtraData which is essential to properly integrate getUserMedia() with the Web Audio API.

This "token" as I call it doesn't even have to be a pointer type, and could simply be an integer typedef, which the embedder can lookup in a map (for example), in such a way that object lifetime isn't even an issue.

Thanks for any help you can offer to work out a good design here.

[Chris Rogers]

(In reply to comment #10)
> (From update of attachment 187915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187915&action=review
> 
> This patch needs serious refactoring. The major issue here is that the ExtraData pattern is a) wrongly used and b) wrongly redesigned. There's memory leaks and illegal typecasts for example. Besides there are existing code that uses the current ExtraData of WebMediaStreamSource will will break so this code will introduce a major regression for WebRTC.

I could be wrong, but I don't think it will break any existing code.  I believe that WebMediaStreamSource::extraData() and setExtraData() will continue to work as before.

[Tommy Widenflycht]

Created attachment 188884 [details]
Patch for discussion

[Tommy Widenflycht]

I would be much, much happier if we could do something like in my patch-for-discussion instead.

[Chris Rogers]

Comment on attachment 188884 [details]
Patch for discussion

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

> Source/WebCore/Modules/webaudio/AudioDestinationNode.h:57
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:79
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h:53
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

> Source/WebCore/platform/audio/AudioDestination.h:49
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:59
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

> Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:61
> +#if ENABLE(MEDIA_STREAM)

can now remove the #if

[Chris Rogers]

Yes, Tommy, I agree your patch looks much better.

[Tommy Widenflycht]

(In reply to comment #16)
> Yes, Tommy, I agree your patch looks much better.

Thanks and I know that it comes with a higher price to the implementor since a map or similar has to be implemented (DeviceID <-> platform specific data). I'm very grateful for this.

Btw, neither mine or your patch can be committed as-is without breaking things due to the changed API in Platform.h; webkit-chromium-webkit-chromium patch chain is needed...

[Chris Rogers]

Created attachment 189194 [details]
Patch

[Chris Rogers]

Tommy, can you please take a look.  I've iterated your last patch. 
Thanks!

p.s. I restored a line which was accidentally removed in your patch:
provider = destination()->localAudioInputProvider();

[Build Bot]

Comment on attachment 189194 [details]
Patch

Attachment 189194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16635130

[Chris Rogers]

Created attachment 189210 [details]
Patch

[Build Bot]

Comment on attachment 189210 [details]
Patch

Attachment 189210 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16634212

[Chris Rogers]

Created attachment 189214 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 189214 [details]
Patch

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

LGTM with just one teensy nit. Sorry for not getting back earlier but I have been completely knocked out by the winter flu :/

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:55
> +    createDestination("");

String() is better I think.

[James Robinson]

Comment on attachment 189214 [details]
Patch

WebKit API looks good.  Tommy's right about String() being better than "", although keep in mind String() generates a null string which isn't the same as an empty string.  emptyString() is the way to produce an empty, non-null string.  WebString preserves the empty/null distinction.

[James Robinson]

Comment on attachment 189214 [details]
Patch

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

> Source/Platform/chromium/public/WebMediaStreamSource.h:96
> +    WEBKIT_EXPORT WebString deviceId() const;
> +    WEBKIT_EXPORT void setDeviceId(const WebString&);

On second thought, why are these WebStrings (aka UCS-2)?  It looks like on the chromium side these are always 1-byte/char strings.  If that's the case, WebCString would be a better fit

[Chris Rogers]

Committed r143781: <http://trac.webkit.org/changeset/143781>