Bug 66398 – Add MediaPlayer::audioSourceProvider() method for audio stream access by the Web Audio API.

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

add MediaPlayer API for accessing rendered audio stream by the Web Audio API

[Chris Rogers]

Created attachment 104220 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104220 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 104220 [details]
Patch

Attachment 104220 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9424063

[Chris Rogers]

Created attachment 104223 [details]
Patch

[Kenneth Russell]

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?

[Chris Rogers]

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...

[Eric Carlson]

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?

[Chris Rogers]

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.

[Chris Rogers]

(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?

[Eric Carlson]

(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.

[Chris Rogers]

Created attachment 104252 [details]
Patch

[Chris Rogers]

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

[Eric Carlson]

(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?

[Eric Carlson]

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.

[Chris Rogers]

Created attachment 104567 [details]
Patch

[Chris Rogers]

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).

[Chris Rogers]

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.

[Eric Carlson]

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?

[Eric Carlson]

(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.

[Chris Rogers]

(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.

[Chris Rogers]

(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.

[Chris Rogers]

Created attachment 104747 [details]
Patch

[Chris Rogers]

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.

[Eric Carlson]

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.

[Chris Rogers]

Created attachment 104758 [details]
Patch

[Chris Rogers]

(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

[Andrew Scherkus]

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)

[Chris Rogers]

(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.

[Andrew Scherkus]

(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.

[Chris Rogers]

Created attachment 104886 [details]
Patch

[Chris Rogers]

(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.

[Eric Carlson]

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.

[Chris Rogers]

Committed r93903: <http://trac.webkit.org/changeset/93903>