Add basic implementation for MediaStreamAudioDestinationNode
Created attachment 173403 [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 173403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173403&action=review Below are some random nitpicks. This patch doesn't look quite ready. > Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:69 > + virtual void consumeAudio(const WebKit::WebVector<const float*>&, size_t number_of_frames) { }; number_of_frames -> numberOfFrames > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:375 > + MediaStreamSource* source = audioTrackList->item(0)->component()->source(); Why only the 0th item? > Source/WebCore/Modules/webaudio/AudioContext.cpp:426 > + // FIXME: support optional argument for number of channels. > + // FIXME: default should probably be stereo. Please use complete sentences in comments. > Source/WebCore/Modules/webaudio/AudioContext.cpp:427 > + return MediaStreamAudioDestinationNode::create(this, 1); What does 1 mean here? > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:54 > + RefPtr<MediaStreamSource> source = MediaStreamSource::create("xyz", MediaStreamSource::TypeAudio, "MediaStreamAudioDestinationNode"); "xyz" ??? > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:68 > + return audioTrackList->item(0)->component()->source(); Why item 0? > Source/WebCore/platform/mediastream/MediaStreamSource.h:87 > + // FIXME: deal better with lifetime issues. Please use complete sentences in comments. > Source/WebCore/platform/mediastream/chromium/RTCPeerConnectionHandlerChromium.cpp:189 > + m_webHandler->consumeAudio(busVector, numberOfFrames); Isn't passing numberOfFrames here redundant with busVector since busVector.length() == numberOfFrames?
Comment on attachment 173403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173403&action=review > Source/WebCore/platform/mediastream/MediaStreamSource.h:99 > + RTCPeerConnectionHandler* m_peerHandler; // FIXME: lifetime! I really don't like this. The direct issue here is lifetime which this patch doesn't deal with at all but there is a larger issue here as well: A RTCPeerConnectionHandler has a 1..1 relation with a RTCPeerConnection but a source can be indirectly related to 0..N peer connections, it can even be related more than once to a single peer connection (mad but valid). Therefore it must not have any kind of relation with RTCPeerConnectionHandler. The data channel is the only class that has a RTCPeerConnectionHandler and that is because it belong to, and only to, the RTCPeerConnection it was created on. Also shouldn't this work even without a peer connection by getting a LocalMediaStream, doing something with it and then playing out the result through an <audio> tag for example? This far we have used MediaStreamCenter for communication between the UA and classes other than RTCPeerConnection.
Created attachment 174979 [details] Patch
After discussion with Chris and Per from the chromium side we decided that I would try to rewrite the mediastream code to get rid of the design issues in the previous patch. This is not in any way criticism on Chris but rather a bi-effect of the extremely rich (and therefore complex) API of WebRTC.
Comment on attachment 174979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174979&action=review > Source/Platform/chromium/public/WebMediaStreamSource.h:51 > + virtual void consumeAudio(const WebVector<const float*>&, size_t number_of_frames) = 0; number_of_frames -> numberOfFrames Can you add a comment that explains what numberOfFrames counts? I presume it's the size of the array that the float*s point to. Is the implementation of this function expected to delete these arrays? That might be worth documenting either way. > Source/Platform/chromium/public/WebMediaStreamSource.h:103 > + // Only used if if this is a WebAudio source. > + // The WebMediaStreamSourceConsumer is not owned, and has to be disposed of separately. Is there an obvious time after which it is safe for the embedder to delete the WebMediaStreamSourceConsumer? > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:52 > + m_source = MediaStreamSource::create(ASCIILiteral("WebAudio-") + createCanonicalUUIDString(), MediaStreamSource::TypeAudio, "MediaStreamAudioDestinationNode", MediaStreamSource::ReadyStateLive, true); Does the name of this stream need to be unique? What happens if there's a collision (e.g., maliciously)? > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.idl:27 > + JSGenerateToJSObject Why do you need this attribute? > Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:161 > + ASSERT(bus); > + if (!bus) > + return; Why do we both ASSERT and handle the case when the ASSERT fails? We should either remove the ASSERT or not handle the case when it fails. > Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:176 > + m_private->addConsumer(adoptRef(new ConsumerWrapper(consumer))); ConsumerWrapper should have a create method that hides the adoptRef > Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:185 > + ConsumerWrapper* wrapper = reinterpret_cast<ConsumerWrapper*>((*it).get()); reinterpret_cast? Do you mean static_cast? > Source/WebCore/platform/mediastream/MediaStreamSource.h:97 > + Vector<RefPtr<MediaStreamSourceConsumer> > consumers() { return m_consumers; } This makes a copy of the Vector and churns the ref counts of all the MediaStreamSourceConsumer. Perhaps we should return a reference (or a const reference)? If you really want the copy behavior, perhaps we should instead make this function copy out the vector into an out parameter?
Comment on attachment 174979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174979&action=review Tommy, thanks for taking the initiative to iterate on this! >> Source/Platform/chromium/public/WebMediaStreamSource.h:51 >> + virtual void consumeAudio(const WebVector<const float*>&, size_t number_of_frames) = 0; > > number_of_frames -> numberOfFrames > > Can you add a comment that explains what numberOfFrames counts? I presume it's the size of the array that the float*s point to. Is the implementation of this function expected to delete these arrays? That might be worth documenting either way. Just to help clarify, the size of the vector is the number of audio channels. numberOfFrames is the number of audio frames in the (possibly multi-channel) buffer in a planar format. >> Source/Platform/chromium/public/WebMediaStreamSource.h:103 >> + // The WebMediaStreamSourceConsumer is not owned, and has to be disposed of separately. > > Is there an obvious time after which it is safe for the embedder to delete the WebMediaStreamSourceConsumer? Tommy can answer, but it looks like it's simply a requirement that the embedder must call removeConsumer() before the embedder is allowed to delete it, and that this is the only thing that matters from our point of view. > Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:187 > + m_private->removeConsumer(wrapper); Do we want to break after the first occurrence is found? It looks like m_private->removeConsumer() will modify the |consumers| vector, so it may not be safe to continue iterating on this vector > Source/WebCore/platform/mediastream/MediaStreamSource.cpp:89 > +{ We need to add an auto-locker here and in addConsumer() and removeConsumer() since consumeAudio() is being called on a different thread from the others. In fact, it looks like WebMediaStreamSource::removeConsumer() also accesses this vector so the locking needs to extend to that level. So we'll probably need to expose a lock() and unlock() method (or something similar) > Source/WebCore/platform/mediastream/MediaStreamSource.h:51 > +}; Since this is a very basic abstract interface related to audio and might be useful in other places unrelated to MediaStream, I recommend moving it to WebCore/platform/audio and renaming the class "AudioConsumer" or "AudioDestinationConsumer" This interface is very similar to (but the opposite of) WebCore/platform/AudioSourceProvider where AudioDestinationConsumer represents a "push" model and AudioSourceProvider represents a "pull" model (the flow-control is the opposite) > Source/WebCore/platform/mediastream/MediaStreamSource.h:96 > + void removeConsumer(MediaStreamSourceConsumer*); I recommend we be more explicit, using "audio" in these names, for example: requiresAudioConsumer() addAudioConsumer() removeAudioConsumer() Who knows, maybe we'll add hooks in here for video/graphics later on...
Created attachment 175226 [details] Patch
Comment on attachment 174979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174979&action=review >>> Source/Platform/chromium/public/WebMediaStreamSource.h:51 >>> + virtual void consumeAudio(const WebVector<const float*>&, size_t number_of_frames) = 0; >> >> number_of_frames -> numberOfFrames >> >> Can you add a comment that explains what numberOfFrames counts? I presume it's the size of the array that the float*s point to. Is the implementation of this function expected to delete these arrays? That might be worth documenting either way. > > Just to help clarify, the size of the vector is the number of audio channels. numberOfFrames is the number of audio frames in the (possibly multi-channel) buffer in a planar format. Chris, could you provide a short description? Fixed number_of_frames -> numberOfFrames. >>> Source/Platform/chromium/public/WebMediaStreamSource.h:103 >>> + // The WebMediaStreamSourceConsumer is not owned, and has to be disposed of separately. >> >> Is there an obvious time after which it is safe for the embedder to delete the WebMediaStreamSourceConsumer? > > Tommy can answer, but it looks like it's simply a requirement that the embedder must call removeConsumer() before the embedder is allowed to delete it, > and that this is the only thing that matters from our point of view. Yeah, that's right. The intended use case is that the embedded inherits from this as well as other classes and therefore don't want to be cleaned away here. >> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:52 >> + m_source = MediaStreamSource::create(ASCIILiteral("WebAudio-") + createCanonicalUUIDString(), MediaStreamSource::TypeAudio, "MediaStreamAudioDestinationNode", MediaStreamSource::ReadyStateLive, true); > > Does the name of this stream need to be unique? What happens if there's a collision (e.g., maliciously)? The name is really unimportant and is only provided as information to the web developer. >> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.idl:27 >> + JSGenerateToJSObject > > Why do you need this attribute? Chris? >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:161 >> + return; > > Why do we both ASSERT and handle the case when the ASSERT fails? We should either remove the ASSERT or not handle the case when it fails. Fixed, kept the if statement. >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:176 >> + m_private->addConsumer(adoptRef(new ConsumerWrapper(consumer))); > > ConsumerWrapper should have a create method that hides the adoptRef Done. >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:185 >> + ConsumerWrapper* wrapper = reinterpret_cast<ConsumerWrapper*>((*it).get()); > > reinterpret_cast? Do you mean static_cast? Yes, both seems to work though. >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:187 >> + m_private->removeConsumer(wrapper); > > Do we want to break after the first occurrence is found? It looks like m_private->removeConsumer() will modify the |consumers| vector, so it may not be safe to continue iterating on this vector Fixed, don't think anyone wants to subscribe twice. >> Source/WebCore/platform/mediastream/MediaStreamSource.cpp:89 >> +{ > > We need to add an auto-locker here and in addConsumer() and removeConsumer() since consumeAudio() is being called on a different thread from the others. > > In fact, it looks like WebMediaStreamSource::removeConsumer() also accesses this vector so the locking needs to extend to that level. So we'll > probably need to expose a lock() and unlock() method (or something similar) Done. >> Source/WebCore/platform/mediastream/MediaStreamSource.h:51 >> +}; > > Since this is a very basic abstract interface related to audio and might be useful in other places unrelated to MediaStream, I recommend moving it to WebCore/platform/audio and > renaming the class "AudioConsumer" or "AudioDestinationConsumer" > > This interface is very similar to (but the opposite of) WebCore/platform/AudioSourceProvider > where AudioDestinationConsumer represents a "push" model and AudioSourceProvider represents a "pull" model (the flow-control is the opposite) Done. >> Source/WebCore/platform/mediastream/MediaStreamSource.h:96 >> + void removeConsumer(MediaStreamSourceConsumer*); > > I recommend we be more explicit, using "audio" in these names, for example: > requiresAudioConsumer() > addAudioConsumer() > removeAudioConsumer() > > Who knows, maybe we'll add hooks in here for video/graphics later on... Done. >> Source/WebCore/platform/mediastream/MediaStreamSource.h:97 >> + Vector<RefPtr<MediaStreamSourceConsumer> > consumers() { return m_consumers; } > > This makes a copy of the Vector and churns the ref counts of all the MediaStreamSourceConsumer. Perhaps we should return a reference (or a const reference)? If you really want the copy behavior, perhaps we should instead make this function copy out the vector into an out parameter? I know, I wanted to be ultra safe and this is a not a function that is called many times. Have changed so that it exits after the first hit and changed consumers() to return a const reference.
(In reply to comment #10) > (From update of attachment 174979 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174979&action=review > > >>> Source/Platform/chromium/public/WebMediaStreamSource.h:51 > >>> + virtual void consumeAudio(const WebVector<const float*>&, size_t number_of_frames) = 0; > >> > >> number_of_frames -> numberOfFrames > >> > >> Can you add a comment that explains what numberOfFrames counts? I presume it's the size of the array that the float*s point to. Is the implementation of this function expected to delete these arrays? That might be worth documenting either way. > > > > Just to help clarify, the size of the vector is the number of audio channels. numberOfFrames is the number of audio frames in the (possibly multi-channel) buffer in a planar format. > > Chris, could you provide a short description? > > Fixed number_of_frames -> numberOfFrames. > > >>> Source/Platform/chromium/public/WebMediaStreamSource.h:103 > >>> + // The WebMediaStreamSourceConsumer is not owned, and has to be disposed of separately. > >> > >> Is there an obvious time after which it is safe for the embedder to delete the WebMediaStreamSourceConsumer? > > > > Tommy can answer, but it looks like it's simply a requirement that the embedder must call removeConsumer() before the embedder is allowed to delete it, > > and that this is the only thing that matters from our point of view. > > Yeah, that's right. The intended use case is that the embedded inherits from this as well as other classes and therefore don't want to be cleaned away here. > > >> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:52 > >> + m_source = MediaStreamSource::create(ASCIILiteral("WebAudio-") + createCanonicalUUIDString(), MediaStreamSource::TypeAudio, "MediaStreamAudioDestinationNode", MediaStreamSource::ReadyStateLive, true); > > > > Does the name of this stream need to be unique? What happens if there's a collision (e.g., maliciously)? > > The name is really unimportant and is only provided as information to the web developer. > > >> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.idl:27 > >> + JSGenerateToJSObject > > > > Why do you need this attribute? > > Chris? I'm following the same pattern I've used for the other AudioNodes. Perhaps it's no longer necessary, but I had found (and I can't remember the details) that for either JSC or V8 that JSGenerateToJSObject was necessary or bad things were happening. I'm just trying to keep this new node the same as all the others. > > >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:161 > >> + return; > > > > Why do we both ASSERT and handle the case when the ASSERT fails? We should either remove the ASSERT or not handle the case when it fails. > > Fixed, kept the if statement. > > >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:176 > >> + m_private->addConsumer(adoptRef(new ConsumerWrapper(consumer))); > > > > ConsumerWrapper should have a create method that hides the adoptRef > > Done. > > >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:185 > >> + ConsumerWrapper* wrapper = reinterpret_cast<ConsumerWrapper*>((*it).get()); > > > > reinterpret_cast? Do you mean static_cast? > > Yes, both seems to work though. > > >> Source/WebCore/platform/chromium/support/WebMediaStreamSource.cpp:187 > >> + m_private->removeConsumer(wrapper); > > > > Do we want to break after the first occurrence is found? It looks like m_private->removeConsumer() will modify the |consumers| vector, so it may not be safe to continue iterating on this vector > > Fixed, don't think anyone wants to subscribe twice. > > >> Source/WebCore/platform/mediastream/MediaStreamSource.cpp:89 > >> +{ > > > > We need to add an auto-locker here and in addConsumer() and removeConsumer() since consumeAudio() is being called on a different thread from the others. > > > > In fact, it looks like WebMediaStreamSource::removeConsumer() also accesses this vector so the locking needs to extend to that level. So we'll > > probably need to expose a lock() and unlock() method (or something similar) > > Done. > > >> Source/WebCore/platform/mediastream/MediaStreamSource.h:51 > >> +}; > > > > Since this is a very basic abstract interface related to audio and might be useful in other places unrelated to MediaStream, I recommend moving it to WebCore/platform/audio and > > renaming the class "AudioConsumer" or "AudioDestinationConsumer" > > > > This interface is very similar to (but the opposite of) WebCore/platform/AudioSourceProvider > > where AudioDestinationConsumer represents a "push" model and AudioSourceProvider represents a "pull" model (the flow-control is the opposite) > > Done. > > >> Source/WebCore/platform/mediastream/MediaStreamSource.h:96 > >> + void removeConsumer(MediaStreamSourceConsumer*); > > > > I recommend we be more explicit, using "audio" in these names, for example: > > requiresAudioConsumer() > > addAudioConsumer() > > removeAudioConsumer() > > > > Who knows, maybe we'll add hooks in here for video/graphics later on... > > Done. > > >> Source/WebCore/platform/mediastream/MediaStreamSource.h:97 > >> + Vector<RefPtr<MediaStreamSourceConsumer> > consumers() { return m_consumers; } > > > > This makes a copy of the Vector and churns the ref counts of all the MediaStreamSourceConsumer. Perhaps we should return a reference (or a const reference)? If you really want the copy behavior, perhaps we should instead make this function copy out the vector into an out parameter? > > I know, I wanted to be ultra safe and this is a not a function that is called many times. Have changed so that it exits after the first hit and changed consumers() to return a const reference.
(In reply to comment #11) > > >> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.idl:27 > > >> + JSGenerateToJSObject > > > > > > Why do you need this attribute? > > > > Chris? > > I'm following the same pattern I've used for the other AudioNodes. Perhaps it's no longer necessary, but I had found (and I can't remember the details) > that for either JSC or V8 that JSGenerateToJSObject was necessary or bad things were happening. I'm just trying to keep this new node the same as all > the others. > JSGenerateToJSObject (JSC specific) creates the toJS() function for classes that have a parent if you don't specify it the toJS() of the parent class is used instead which might cause type issues. V8 always creates a toV8().
Created attachment 175954 [details] Patch
I added more/better comments and added checks for main thread in the add/removeConsumer functionality in WebMediaStreamSource to be safer.
Comment on attachment 175954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175954&action=review API change LGTM with some naming nits below. > Source/Platform/chromium/public/WebAudioDestinationConsumer.h:32 > +class WebAudioDestinationConsumer { What does the word "destination" mean here? This seems like it could be sensibly called a WebAudioConsumer. > Source/Platform/chromium/public/WebMediaStreamSource.h:95 > + // Only used if if this is a WebAudio source. We should have an ASSERT that verifies this property.
Comment on attachment 175954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175954&action=review looks good >> Source/Platform/chromium/public/WebAudioDestinationConsumer.h:32 >> +class WebAudioDestinationConsumer { > > What does the word "destination" mean here? This seems like it could be sensibly called a WebAudioConsumer. This seems fine to me since AudioDestinationConsumer is symmetric/consistent with AudioSourceProvider. From WebKit's (WebAudio's) point of view, there are nodes which represent sources and destinations. The naming comes from there. An AudioSourceNode gets its audio data from a provider, and an AudioDestinationNode "sends" its audio data to a consumer > Source/WebCore/platform/mediastream/MediaStreamSource.h:44 > +class RTCPeerConnectionHandler; Is this needed anymore?
Created attachment 176240 [details] Patch
Comment on attachment 175954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175954&action=review >> Source/Platform/chromium/public/WebMediaStreamSource.h:95 >> + // Only used if if this is a WebAudio source. > > We should have an ASSERT that verifies this property. I added ASSERTs in MediaStreamSource.cpp. >> Source/WebCore/platform/mediastream/MediaStreamSource.h:44 >> +class RTCPeerConnectionHandler; > > Is this needed anymore? Nope, removed.
Comment on attachment 176240 [details] Patch API change LGTM. I'll let crogers have the final say.
Comment on attachment 176240 [details] Patch Clearing flags on attachment: 176240 Committed r135985: <http://trac.webkit.org/changeset/135985>
All reviewed patches have been landed. Closing bug.