add MediaPlayer API for accessing rendered audio stream by the Web Audio API
Created attachment 104220 [details] Patch
Comment on attachment 104220 [details] Patch Attachment 104220 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9424058
Comment on attachment 104220 [details] Patch Attachment 104220 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9424063
Created attachment 104223 [details] Patch
Comment on attachment 104223 [details] Patch Could you please provide a little background in the bug report, the ChangeLog, or both indicating what this patch is intended to do and how far along toward that goal it gets?
This is an initial patch which is part of a prototype implementation of MediaElementAudioSourceNode which I have working (in a local build) in the chromium port so far. It's nice to be able to process/analyse/visualize streamed audio sources in addition to the memory-resident audio assets which the Web Audio API currently uses. One thing which still needs to be addressed is the MediaPlayer object lifetime and thread-safety of this code. Can somebody familiar with MediaPlayer and its relationship with HTMLMediaElement help me with this? We need to be careful to avoid the MediaPlayer getting deleted while the MediaElementAudioSourceNode is still consuming the audio stream since it's not running on the main thread. One naive approach I can think of is to make the MediaPlayer a ThreadSafeRefCounted object, but there may be other approaches...
Comment on attachment 104223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104223&action=review > Source/WebCore/ChangeLog:3 > + add MediaPlayer API for accessing rendered audio stream by the Web Audio API This should be a proper sentence. > Source/WebCore/platform/graphics/MediaPlayer.cpp:883 > +#if PLATFORM(CHROMIUM) > + return m_private->audioSourceProvider(); > +#else > + return 0; > +#endif The base audioSourceProvider() returns 0, why not always call it when WEB_AUDIO is defined?
Please see approach 2 in the Web Audio API specification: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioElementIntegration-section This was recommended by Ian Hickson over approach 1 since it avoids adding a new attribute to HTMLMediaElement. This patch will only currently affect the chromium port because of the ENABLE(WEB_AUDIO) guards. For the other ports (including mac port) if WEB_AUDIO is enabled, it will provide an "empty" implementation which renders silence for this node.
(In reply to comment #7) > > > Source/WebCore/platform/graphics/MediaPlayer.cpp:883 > > +#if PLATFORM(CHROMIUM) > > + return m_private->audioSourceProvider(); > > +#else > > + return 0; > > +#endif > > The base audioSourceProvider() returns 0, why not always call it when WEB_AUDIO is defined? Ok, I can do that. Does that mean I need to add a stub implementation to MediaPlayerPrivateQTKit?
(In reply to comment #9) > > Ok, I can do that. Does that mean I need to add a stub implementation to MediaPlayerPrivateQTKit? No, the base class implementation will return 0 unless overridden.
Created attachment 104252 [details] Patch
Comment on attachment 104223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104223&action=review >> Source/WebCore/ChangeLog:3 >> + add MediaPlayer API for accessing rendered audio stream by the Web Audio API > > This should be a proper sentence. Tried to make this more sentence-like. >> Source/WebCore/platform/graphics/MediaPlayer.cpp:883 >> +#endif > > The base audioSourceProvider() returns 0, why not always call it when WEB_AUDIO is defined? FIXED
(In reply to comment #6) > This is an initial patch which is part of a prototype implementation of MediaElementAudioSourceNode which I have working (in a local build) in the chromium port so far. It's nice to be able to process/analyse/visualize streamed audio sources in addition to the memory-resident audio assets which the Web Audio API currently uses. > > One thing which still needs to be addressed is the MediaPlayer object lifetime and thread-safety of this code. Can somebody familiar with MediaPlayer and its relationship with HTMLMediaElement help me with this? We need to be careful to avoid the MediaPlayer getting deleted while the MediaElementAudioSourceNode is still consuming the audio stream since it's not running on the main thread. One naive approach I can think of is to make the MediaPlayer a ThreadSafeRefCounted object, but there may be other approaches... MediaPlayer *should* be a private to HTMLMediaElement implementation detail. It isn't completely private, but that should change. I don't' think there is any need for this new code to have any knowledge of MediaPlayer, all interaction should be with HTMLMediaElement. I think we can safely assume that it is not thread safe as that hasn't been a requirement and MediaPlayer is just a thin wrapper between HTMLMediaElement and the platform media engine (MediaPlayerPrivate<platform>). HTMLMediaElement has a single MediaPlayer instance, but it has to be able to delete it at any time. Instead of using class magic to try to make it thread safe, I think it may be wiser in the long run to make HTMLMediaElement be responsible for making sure the audio node is aware of when it is allowed to consume samples. Could it work to have API on MediaElementAudioSourceNode so HTMLMediaElement could stop it from consuming any more data. "disconnect" maybe?
Comment on attachment 104252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104252&action=review Marking r- for now until we figure out the threading and lifetime management issues. > Source/WebCore/platform/graphics/MediaPlayer.h:320 > +#if ENABLE(WEB_AUDIO) > + AudioSourceProvider* audioSourceProvider(); > +#endif > + > private: This should be private. > Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:63 > + if (mediaElement() && mediaElement()->player()) > + return mediaElement()->player()->audioSourceProvider(); > + I would rather the audioSourceProvider() function be on HTMLMediaElement so "player()" isn't necessary here.
Created attachment 104567 [details] Patch
Comment on attachment 104252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104252&action=review Hi Eric, thanks for having a look. I've tried to address your comments and had a go at the thread safety issue by adding a lock to HTMLMediaElement >> Source/WebCore/platform/graphics/MediaPlayer.h:320 >> private: > > This should be private. I'm not sure I understand how it can be private, since HTMLMediaElement needs to call audioSourceProvider() >> Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:63 >> + > > I would rather the audioSourceProvider() function be on HTMLMediaElement so "player()" isn't necessary here. I've added an audioSourceProvider() method to HTMLMediaElement (which then internally calls into the player).
Sorry Eric, I just saw this additional comment you made. Once again, thanks for having a look at this. (In reply to comment #13) > > MediaPlayer *should* be a private to HTMLMediaElement implementation detail. It isn't completely private, but that should change. I don't' think there is any need for this new code to have any knowledge of MediaPlayer, all interaction should be with HTMLMediaElement. Agreed, the new patch has the web audio code calling the HTMLMediaElement directly. > > I think we can safely assume that it is not thread safe as that hasn't been a requirement and MediaPlayer is just a thin wrapper between HTMLMediaElement and the platform media engine (MediaPlayerPrivate<platform>). Yes, I see that the MediaPlayer can be created (and old one destroyed) at several places. With this last patch, I've taken the approach of adding a lock to HTMLMediaElement to protect the web audio code from these unpredictable changes. The web audio code needs access to this lock during the time it's accessing the "AudioSourceProvider". In this patch, this lock accessor is called "playerLock()", but we could call it something different which doesn't have the word "player" in it, for example. > > HTMLMediaElement has a single MediaPlayer instance, but it has to be able to delete it at any time. Instead of using class magic to try to make it thread safe, I think it may be wiser in the long run to make HTMLMediaElement be responsible for making sure the audio node is aware of when it is allowed to consume samples. Could it work to have API on MediaElementAudioSourceNode so HTMLMediaElement could stop it from consuming any more data. "disconnect" maybe? It's possible for the HTMLMediaElement to let the audio node be aware of when it's safe. But, since the audio node code is running in another thread, it would be necessary for the HTMLMediaElement to wait until the audio node has received this message before it changes any state. In other words, if the audio node is in the middle of running some code (in the audio thread) which calls into the MediaPlayer engine, then it's not sufficient for the HTMLMediaElement to call the audio node (on the main thread) without acknowledging receipt of the message (handed off from the audio thread). I believe this would be more complex to code and more difficult to understand, so I would recommend a lock approach similar to the one in this latest patch.
Comment on attachment 104567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104567&action=review > Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:86 > + if (!mediaElement()) { > + outputBus->zero(); > + return; > + } > + > + Mutex& playerLock = mediaElement()->playerLock(); > + > + if (playerLock.tryLock()) { > + AudioSourceProvider* provider = sourceProvider(); > + if (provider) > + provider->provideInput(outputBus, numberOfFrames); > + else > + outputBus->zero(); > + playerLock.unlock(); > + } else > + outputBus->zero(); Will the lock keep the MediaPlayer/AudioSourceProvider from being destroyed if the HTMLMediaElement destructor is called during this processing?
(In reply to comment #16) > (From update of attachment 104252 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104252&action=review > > Hi Eric, thanks for having a look. I've tried to address your comments and had a go at the thread safety issue by adding a lock to HTMLMediaElement > > >> Source/WebCore/platform/graphics/MediaPlayer.h:320 > >> private: > > > > This should be private. > > I'm not sure I understand how it can be private, since HTMLMediaElement needs to call audioSourceProvider() > Oops! (In reply to comment #17) > > > > HTMLMediaElement has a single MediaPlayer instance, but it has to be able to delete it at any time. Instead of using class magic to try to make it thread safe, I think it may be wiser in the long run to make HTMLMediaElement be responsible for making sure the audio node is aware of when it is allowed to consume samples. Could it work to have API on MediaElementAudioSourceNode so HTMLMediaElement could stop it from consuming any more data. "disconnect" maybe? > > It's possible for the HTMLMediaElement to let the audio node be aware of when it's safe. But, since the audio node code is running in another thread, it would be necessary for the HTMLMediaElement to wait until the audio node has received this message before it changes any state. In other words, if the audio node is in the middle of running some code (in the audio thread) which calls into the MediaPlayer engine, then it's not sufficient for the HTMLMediaElement to call the audio node (on the main thread) without acknowledging receipt of the message (handed off from the audio thread). I believe this would be more complex to code and more difficult to understand, so I would recommend a lock approach similar to the one in this latest patch. I agree that HTMLMediaElement will have to wait for an ack from the audio node before changing any state, but can't we do that implicitly by putting the lock in MediaElementAudioSourceNode and have the "MediaPlayer is changing" method take the lock, and have the audio node hold the lock during processing? That way disconnect can't happen during a pull, and the logic is all kept in the audio node which is already thread safe.
(In reply to comment #18) > (From update of attachment 104567 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104567&action=review > > > Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:86 > > + if (!mediaElement()) { > > + outputBus->zero(); > > + return; > > + } > > + > > + Mutex& playerLock = mediaElement()->playerLock(); > > + > > + if (playerLock.tryLock()) { > > + AudioSourceProvider* provider = sourceProvider(); > > + if (provider) > > + provider->provideInput(outputBus, numberOfFrames); > > + else > > + outputBus->zero(); > > + playerLock.unlock(); > > + } else > > + outputBus->zero(); > > Will the lock keep the MediaPlayer/AudioSourceProvider from being destroyed if the HTMLMediaElement destructor is called during this processing? The destructor cannot be called during processing because the MediaElementAudioSourceNode keeps a reference to the HTMLMediaElement, so that part should be fine. In other words, the HTMLMediaElement will stay alive as long as the MediaElementAudioSourceNode is alive.
(In reply to comment #19) > (In reply to comment #16) > > (From update of attachment 104252 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=104252&action=review > > > > Hi Eric, thanks for having a look. I've tried to address your comments and had a go at the thread safety issue by adding a lock to HTMLMediaElement > > > > >> Source/WebCore/platform/graphics/MediaPlayer.h:320 > > >> private: > > > > > > This should be private. > > > > I'm not sure I understand how it can be private, since HTMLMediaElement needs to call audioSourceProvider() > > > Oops! > > (In reply to comment #17) > > > > > > HTMLMediaElement has a single MediaPlayer instance, but it has to be able to delete it at any time. Instead of using class magic to try to make it thread safe, I think it may be wiser in the long run to make HTMLMediaElement be responsible for making sure the audio node is aware of when it is allowed to consume samples. Could it work to have API on MediaElementAudioSourceNode so HTMLMediaElement could stop it from consuming any more data. "disconnect" maybe? > > > > It's possible for the HTMLMediaElement to let the audio node be aware of when it's safe. But, since the audio node code is running in another thread, it would be necessary for the HTMLMediaElement to wait until the audio node has received this message before it changes any state. In other words, if the audio node is in the middle of running some code (in the audio thread) which calls into the MediaPlayer engine, then it's not sufficient for the HTMLMediaElement to call the audio node (on the main thread) without acknowledging receipt of the message (handed off from the audio thread). I believe this would be more complex to code and more difficult to understand, so I would recommend a lock approach similar to the one in this latest patch. > > I agree that HTMLMediaElement will have to wait for an ack from the audio node before changing any state, but can't we do that implicitly by putting the lock in MediaElementAudioSourceNode and have the "MediaPlayer is changing" method take the lock, and have the audio node hold the lock during processing? That way disconnect can't happen during a pull, and the logic is all kept in the audio node which is already thread safe. Yes, I can move the lock so that it's owned by the MediaElementAudioSourceNode. I would have to add a setter to HTMLMediaElement so that it knows about the MediaElementAudioSourceNode (so it can get the lock when "MediaPlayer" is changing). But this is probably a good idea anyway, since we should handle the case where HTMLMediaElement refuses to have two or more MediaElementAudioSourceNode objects connected to the same <audio> or <video> element. Tomorrow, I can work on a new patch moving the lock ownership to MediaElementAudioSourceNode and making sure there's only a single MediaElementAudioSourceNode "attached" to a HTMLMediaElement.
Created attachment 104747 [details] Patch
Hi Eric, this latest patch moves the lock into the MediaElementAudioSourceNode as you suggest. AudioContext::createMediaElementSource() now also forbids an HTMLMediaElement from having more than one MediaElementAudioSourceNode attached to it.
Comment on attachment 104747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104747&action=review > Source/WebCore/ChangeLog:7 > + Add MediaPlayer::audioSourceProvider() method for audio stream access by the Web Audio API. > + https://bugs.webkit.org/show_bug.cgi?id=66398 > + > + Reviewed by NOBODY (OOPS!). > + Are there existing tests for this feature? If so, you should mention them here. If not you should definitely have some, even if it is only possible to run them in your own build. > Source/WebCore/html/HTMLMediaElement.h:450 > +#if ENABLE(WEB_AUDIO) > + MediaElementAudioSourceNode* m_audioSourceNode; > +#endif This should have a comment about why this isn't a ref pointer, about why it is safe, lifetime, etc.
Created attachment 104758 [details] Patch
(In reply to comment #24) > (From update of attachment 104747 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104747&action=review > > > Source/WebCore/ChangeLog:7 > > + Add MediaPlayer::audioSourceProvider() method for audio stream access by the Web Audio API. > > + https://bugs.webkit.org/show_bug.cgi?id=66398 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > Are there existing tests for this feature? If so, you should mention them here. > > If not you should definitely have some, even if it is only possible to run them in your own build. Latest patch adds one new test-case to webaudio/mediaelementaudiosourcenode.html I'm trying to play catch-up with the layout tests on the Web Audio API. I have about eight or so right now, but plan on adding many others. I'm happy to get feedback on the existing tests and suggestions for future tests. By the way, it would be great to chat sometime about support for the "mac" port. I've been able to build and run the "mac" port version in Safari on Lion, although I ran into what I think is a simple sandboxing issue when trying to load the spatialization resources (should be easy to fix I imagine). > > > > Source/WebCore/html/HTMLMediaElement.h:450 > > +#if ENABLE(WEB_AUDIO) > > + MediaElementAudioSourceNode* m_audioSourceNode; > > +#endif > > This should have a comment about why this isn't a ref pointer, about why it is safe, lifetime, etc. FIXED
what's the lifetime of a MediaElementAudioSourceNode? is it possible for it to get deleted and detach itself from an HTMLMediaElement? if so I wonder if it's possible to have a race between createMediaPlayer() attempting to grab a lock while MediaElementAudioSourceNode's destructor is calling setAudioSourceNode(0)
(In reply to comment #27) > what's the lifetime of a MediaElementAudioSourceNode? > > is it possible for it to get deleted and detach itself from an HTMLMediaElement? > > if so I wonder if it's possible to have a race between createMediaPlayer() attempting to grab a lock while MediaElementAudioSourceNode's destructor is calling setAudioSourceNode(0) Yes, the MediaElementAudioSourceNode can go away while the HTMLMediaElement still lives, but in this case there should be no race condition since I think HTMLMediaElement's use of the lock and the destructor both happen on the main thread (not multiple threads). I can put as ASSERT(isMainThread()) in HTMLMediaElement::createMediaPlayer() and in the destructor to validate that assumption.
(In reply to comment #28) > (In reply to comment #27) > > what's the lifetime of a MediaElementAudioSourceNode? > > > > is it possible for it to get deleted and detach itself from an HTMLMediaElement? > > > > if so I wonder if it's possible to have a race between createMediaPlayer() attempting to grab a lock while MediaElementAudioSourceNode's destructor is calling setAudioSourceNode(0) > > Yes, the MediaElementAudioSourceNode can go away while the HTMLMediaElement still lives, but in this case there should be no race condition since I think HTMLMediaElement's use of the lock and the destructor both happen on the main thread (not multiple threads). I can put as ASSERT(isMainThread()) in HTMLMediaElement::createMediaPlayer() and in the destructor to validate that assumption. Thanks for the explanation! Having not used ASSERT()s before I'll defer to Eric on best practices.
Created attachment 104886 [details] Patch
(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > what's the lifetime of a MediaElementAudioSourceNode? > > > > > > is it possible for it to get deleted and detach itself from an HTMLMediaElement? > > > > > > if so I wonder if it's possible to have a race between createMediaPlayer() attempting to grab a lock while MediaElementAudioSourceNode's destructor is calling setAudioSourceNode(0) > > > > Yes, the MediaElementAudioSourceNode can go away while the HTMLMediaElement still lives, but in this case there should be no race condition since I think HTMLMediaElement's use of the lock and the destructor both happen on the main thread (not multiple threads). I can put as ASSERT(isMainThread()) in HTMLMediaElement::createMediaPlayer() and in the destructor to validate that assumption. > > Thanks for the explanation! Having not used ASSERT()s before I'll defer to Eric on best practices. I think your point is still valid. I don't think there's a thread safety problem with the current patch, but it's best to bullet-proof ourselves against future changes. So, I've uploaded a new patch with a small change where I explicitly am calling ref() in the lock() method with a corresponding deref() in the unlock() method.
Comment on attachment 104886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104886&action=review > Source/WebCore/ChangeLog:36 > + * html/HTMLMediaElement.cpp: > + (WebCore::HTMLMediaElement::HTMLMediaElement): > + (WebCore::HTMLMediaElement::prepareForLoad): > + (WebCore::HTMLMediaElement::loadNextSourceChild): > + (WebCore::HTMLMediaElement::ensureMediaPlayer): > + (WebCore::HTMLMediaElement::createMediaPlayer): > + (WebCore::HTMLMediaElement::setAudioSourceNode): > + (WebCore::HTMLMediaElement::audioSourceProvider): > + * html/HTMLMediaElement.h: > + (WebCore::HTMLMediaElement::audioSourceNode): > + * platform/graphics/MediaPlayer.cpp: > + (WebCore::MediaPlayer::audioSourceProvider): > + * platform/graphics/MediaPlayer.h: > + * platform/graphics/MediaPlayerPrivate.h: > + (WebCore::MediaPlayerPrivateInterface::audioSourceProvider): > + * webaudio/AudioContext.cpp: > + (WebCore::AudioContext::createMediaElementSource): > + * webaudio/AudioContext.h: > + * webaudio/AudioContext.idl: > + * webaudio/MediaElementAudioSourceNode.cpp: > + (WebCore::MediaElementAudioSourceNode::MediaElementAudioSourceNode): > + (WebCore::MediaElementAudioSourceNode::~MediaElementAudioSourceNode): > + (WebCore::MediaElementAudioSourceNode::process): > + * webaudio/MediaElementAudioSourceNode.h: > + (WebCore::MediaElementAudioSourceNode::lock): > + (WebCore::MediaElementAudioSourceNode::unlock): It would be helpful to have comments here about what changed in each function.
Committed r93903: <http://trac.webkit.org/changeset/93903>