AudioDestination::create() needs extra device identification information for live/local input streams
Created attachment 187682 [details] Patch
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 on attachment 187682 [details] Patch Attachment 187682 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16485685
Comment on attachment 187682 [details] Patch Attachment 187682 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16495506
Comment on attachment 187682 [details] Patch Attachment 187682 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16531042
Created attachment 187915 [details] Patch
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?
(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.
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 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.
(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.
(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.
Created attachment 188884 [details] Patch for discussion
I would be much, much happier if we could do something like in my patch-for-discussion instead.
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
Yes, Tommy, I agree your patch looks much better.
(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...
Created attachment 189194 [details] Patch
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 on attachment 189194 [details] Patch Attachment 189194 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16635130
Created attachment 189210 [details] Patch
Comment on attachment 189210 [details] Patch Attachment 189210 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16634212
Created attachment 189214 [details] Patch
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 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 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
Committed r143781: <http://trac.webkit.org/changeset/143781>