Bug 61947 – AudioContext needs non-blocking call to create AudioBuffer from audio file data

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Rogers]

Currently, AudioContext has a createBuffer() method taking an ArrayBuffer of audio file data as an argument.  This is a blocking call on the main JavaScript thread.  There needs to be an alternative method which decodes the audio data on another thread and then calls an event listener when finished.

[Chris Rogers]

Created attachment 97669 [details]
Patch

[Chris Rogers]

I've tested this patch for asynchronous loading in both Chromium and Safari Mac OS X.

This results in a very noticeable improvement in the web audio pages which load multiple audio assets.

I wanted to get this up for review since I think the bulk of the code is in good shape, but there are two issues I'd still like to address:

1.  FIXME to address possible leaks in AsyncAudioDecoder::~AsyncAudioDecoder() related to unprocessed tasks in queue.
2. the error callback should probably be a different callback type which doesn't pass an AudioBuffer back into JavaScript.

[Kenneth Russell]

Comment on attachment 97669 [details]
Patch

This looks good to me modulo the FIXMEs. You can easily give yourself the ability to raise an exception from decodeAudioData by adding "raises(DOMException)" to the IDL. Using one of the pre-defined exception codes is easy; a custom exception requires custom bindings.

[James Robinson]

Comment on attachment 97669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review

Sorry to override but I don't think this is correct.  Do we have to pass refptr<>s to callbacks to the second thread?  That seems likely to be racy since our RefCounted implementation is not threadsafe.

> Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
> +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData, sampleRate, successCallback, errorCallback));
> +    m_queue.append(decodingTask.release()); // note that ownership of the task is effectively taken by the queue.

This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.

[Kenneth Russell]

(In reply to comment #4)
> (From update of attachment 97669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review
> 
> Sorry to override but I don't think this is correct.  Do we have to pass refptr<>s to callbacks to the second thread?  That seems likely to be racy since our RefCounted implementation is not threadsafe.
> 
> > Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
> > +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData, sampleRate, successCallback, errorCallback));
> > +    m_queue.append(decodingTask.release()); // note that ownership of the task is effectively taken by the queue.
> 
> This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.

Note: the DecodingTask is deleted on the main thread, which is the same thread that instantiates it. See AsyncAudioDecoder::DecodingTask::decode() and its use of callOnMainThread. The background thread is only transiently used for the decoding.

[James Robinson]

Comment on attachment 97669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review

>>> Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
>>> +    m_queue.append(decodingTask.release()); // note that ownership of the task is effectively taken by the queue.
>> 
>> This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.
> 
> Note: the DecodingTask is deleted on the main thread, which is the same thread that instantiates it. See AsyncAudioDecoder::DecodingTask::decode() and its use of callOnMainThread. The background thread is only transiently used for the decoding.

m_audioBuffer appears to be created on the decoder thread and then used from the main thread.

[James Robinson]

(+cc levin for the threading stuff, he knows more than i do about what's safe across threads and what isn't)

[Chris Rogers]

(In reply to comment #4)
> (From update of attachment 97669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review
> 
> Sorry to override but I don't think this is correct.  Do we have to pass refptr<>s to callbacks to the second thread?  That seems likely to be racy since our RefCounted implementation is not threadsafe.
> 
> > Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
> > +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData, sampleRate, successCallback, errorCallback));
> > +    m_queue.append(decodingTask.release()); // note that ownership of the task is effectively taken by the queue.
> 
> This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.

Just a note - the interesting object (DecodingTask) is itself not ref-counted.  Instead OwnPtr's are used...

the RefPtr part seems clear enough to me, but David can have a look too...


Here's how the ownership works for the DecodingTask:

1. a DecodingTask is created on the main thread and added to a MessageQueue which takes ownership.
2. AsyncAudioDecoder::runLoop() is on its own thread and re-takes ownership of the DecodingTask by calling waitForMessage()
3. The ownership is passed (still in this decoder thread) to DecodingTask::decode() (basically by making the DecodingTask responsible for deleting itself)
4. DecodingTask::decode() schedules DecodingTask::notifyComplete() to occur on the main thread.
5. on the main thread DecodingTask::notifyComplete() calls any callbacks (success or error) and then deletes itself.
6. Any DecodingTasks which were not dequeued will be deleted when the MessageQueue itself is deleted.

[Chris Rogers]

(In reply to comment #4)
> (From update of attachment 97669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review
> 
> Sorry to override but I don't think this is correct.  Do we have to pass refptr<>s to callbacks to the second thread?  That seems likely to be racy since our RefCounted implementation is not threadsafe.
> 
> > Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67
> > +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData, sampleRate, successCallback, errorCallback));
> > +    m_queue.append(decodingTask.release()); // note that ownership of the task is effectively taken by the queue.
> 
> This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.

To me there clearly are no raciness problems with the RefPtr part due to the nature of the synchronization.

[David Levin]

Comment on attachment 97669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97669&action=review

> Source/WebCore/webaudio/AsyncAudioDecoder.cpp:66
> +    OwnPtr<DecodingTask> decodingTask = adoptPtr(new DecodingTask(audioData, sampleRate, successCallback, errorCallback));

The constructor should be private and a static create method should be exposed which returns a PassOwnPtr<DecodingTask>.

>>>> Source/WebCore/webaudio/AsyncAudioDecoder.cpp:67

>>> 
>>> This worries me because we're passing two RefPtr<>s to another thread.  Is it safe to drop the last reference to these callbacks from somewhere other than the main thread?  The answer is nearly always no, and our RefPtr<> implementation is not thread-safe without some sort of additional synchronization around the refcount.
>> 
>> Note: the DecodingTask is deleted on the main thread, which is the same thread that instantiates it. See AsyncAudioDecoder::DecodingTask::decode() and its use of callOnMainThread. The background thread is only transiently used for the decoding.
> 
> m_audioBuffer appears to be created on the decoder thread and then used from the main thread.

I agree with James who did a most thorough job here.  (I understand the lifetime here and I think it is all correct but it seems fragile in the sense that it would be easy for someone to mess it up later because it is subtle. For example, you need to understand all of the code to know that even though m_successCallback is RefCounted you better not ref count it on the other thread).

I wonder if something that is not refcounted could be used for the data that is passed using AudioBuffer.

> Source/WebCore/webaudio/AsyncAudioDecoder.h:59
> +        WTF_MAKE_NONCOPYABLE(DecodingTask);

WebKit style is to put this immediately after the class declaraction.

class AsyncAudioDecoder {
WTF_MAKE_NONCOPYABLE(DecodingTask);
public:

[James Robinson]

Reasoning about threading makes my head hurt.  Ken pointed out that the callback refcounts are only manipulated from the main thread, so that seems fine, but I'm still a bit concerned about the fact that we initialize the refcount on DecoderData::m_audioData from the decoder thread and then decrement it from the main thread - is that safe?

[David Levin]

(In reply to comment #11)
> Reasoning about threading makes my head hurt.  Ken pointed out that the callback refcounts are only manipulated from the main thread, so that seems fine, but I'm still a bit concerned about the fact that we initialize the refcount on DecoderData::m_audioData from the decoder thread and then decrement it from the main thread - is that safe?

Should be safe because the ref count is done before the callOnMainThread and if callOnMainThread doesn't do a memory barrier, then there would be lots of trouble.

(Also there is no ref count on the AudioBuffer in m_audioData other than m_audioData itself unless the decode function is doing something very tricky.)

[James Robinson]

OK cool, then this looks fine to me as well.

[Chris Rogers]

(In reply to comment #12)
> (Also there is no ref count on the AudioBuffer in m_audioData other than m_audioData itself unless the decode function is doing something very tricky.)

Nothing tricky there.  AudioBuffer::createFromAudioFileData() returns a normal PassRefPtr<AudioBuffer>

[Kenneth Russell]

Comment on attachment 97669 [details]
Patch

Per the discussion above I'm r+'ing this again.

[Chris Rogers]

Committed r89478: <http://trac.webkit.org/changeset/89478>