Bug 109494 - AudioDestination::create() needs extra device identification information for live/local input streams
Summary: AudioDestination::create() needs extra device identification information for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on: 110192
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-11 14:33 PST by Chris Rogers
Modified: 2013-02-22 12:51 PST (History)
13 users (show)

See Also:


Attachments
Patch (22.24 KB, patch)
2013-02-11 14:46 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (20.65 KB, patch)
2013-02-12 13:12 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch for discussion (17.66 KB, patch)
2013-02-18 07:27 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2013-02-19 16:08 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2013-02-19 17:13 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (17.31 KB, patch)
2013-02-19 17:39 PST, Chris Rogers
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2013-02-11 14:33:50 PST
AudioDestination::create() needs extra device identification information for live/local input streams
Comment 1 Chris Rogers 2013-02-11 14:46:23 PST
Created attachment 187682 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-11 14:49:09 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Build Bot 2013-02-11 17:58:50 PST
Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16485685
Comment 4 EFL EWS Bot 2013-02-11 19:00:44 PST
Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16495506
Comment 5 Build Bot 2013-02-12 10:19:12 PST
Comment on attachment 187682 [details]
Patch

Attachment 187682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16531042
Comment 6 Chris Rogers 2013-02-12 13:12:07 PST
Created attachment 187915 [details]
Patch
Comment 7 James Robinson 2013-02-12 13:48:39 PST
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?
Comment 8 Chris Rogers 2013-02-12 14:31:25 PST
(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.
Comment 9 James Robinson 2013-02-12 17:16:15 PST
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.
Comment 10 Tommy Widenflycht 2013-02-13 00:38:49 PST
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.
Comment 11 Chris Rogers 2013-02-13 10:52:59 PST
(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.
Comment 12 Chris Rogers 2013-02-13 16:55:45 PST
(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.
Comment 13 Tommy Widenflycht 2013-02-18 07:27:57 PST
Created attachment 188884 [details]
Patch for discussion
Comment 14 Tommy Widenflycht 2013-02-18 07:28:44 PST
I would be much, much happier if we could do something like in my patch-for-discussion instead.
Comment 15 Chris Rogers 2013-02-18 10:59:44 PST
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
Comment 16 Chris Rogers 2013-02-18 11:11:31 PST
Yes, Tommy, I agree your patch looks much better.
Comment 17 Tommy Widenflycht 2013-02-19 00:25:27 PST
(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...
Comment 18 Chris Rogers 2013-02-19 16:08:57 PST
Created attachment 189194 [details]
Patch
Comment 19 Chris Rogers 2013-02-19 16:12:47 PST
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();
Comment 20 Build Bot 2013-02-19 16:58:10 PST
Comment on attachment 189194 [details]
Patch

Attachment 189194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16635130
Comment 21 Chris Rogers 2013-02-19 17:13:00 PST
Created attachment 189210 [details]
Patch
Comment 22 Build Bot 2013-02-19 17:18:50 PST
Comment on attachment 189210 [details]
Patch

Attachment 189210 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16634212
Comment 23 Chris Rogers 2013-02-19 17:39:36 PST
Created attachment 189214 [details]
Patch
Comment 24 Tommy Widenflycht 2013-02-22 01:34:37 PST
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.
Comment 25 James Robinson 2013-02-22 10:35:45 PST
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.
Comment 26 James Robinson 2013-02-22 10:39:24 PST
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
Comment 27 Chris Rogers 2013-02-22 12:51:10 PST
Committed r143781: <http://trac.webkit.org/changeset/143781>