Bug 57676 – Add support for offline audio rendering to AudioContext API

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add support for offline audio rendering to AudioContext API

[Chris Rogers]

Created attachment 87936 [details]
Patch

[WebKit Review Bot]

Attachment 87936 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/EventTarget.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

This adds a new constructor to AudioContext for offline rendering into an AudioBuffer.  Offline rendering will be an important part of the audio layout test system.  It will also have some other interesting applications.

This is an early version of the patch for initial review.  It's still incomplete, since a few new files have been added but not all of the necessary makefiles have yet been updated.

A few notes:

* I've re-factored AudioDestinationNode to now be a base class, with the existing default implementation (talking to audio hardware) being hoisted up into DefaultAudioDestinationNode.  There's also a new sub-class OfflineAudioDestinationNode where most of the interesting new offline stuff happens.

* I've currently kept the AudioContext implementation for both default (talking to audio hardware) and offline processing in the same AudioContext class.  An alternative would be to also subclass AudioContext in a similar way to AudioDestinationNode (as explained above).

* JSAudioContextCustom.cpp also needs similar changes to V8AudioContextCustom.cpp but I haven't done that yet.

* This has been minimally tested and works with a simple test case.  It's pretty remarkable to see how much faster than real-time the offline context can render.  I think the audio layout tests should mostly be able to run very quickly.

* Here's part of a simple test case I used which shows what this looks like in JavaScript:


function playNote(context, time) {
    var source = context.createBufferSource();

    source.buffer = hihatShort;
    source.connect(context.destination);

    source.noteOn(time);
}

// Gets called when resource loading has finished.
function startOfflineProcessing() {
    offlineContext = new webkitAudioContext(2, 2 * 44100, 44100);

    offlineContext.oncomplete = function(event) {
      var source = context.createBufferSource();
      source.buffer = event.renderedBuffer;
      source.connect(context.destination);
      source.noteOn(0);
    };

    playNote(offlineContext, 0.0);
    playNote(offlineContext, 0.5);
    playNote(offlineContext, 0.75);
    playNote(offlineContext, 1.25);
    playNote(offlineContext, 1.5);
    playNote(offlineContext, 1.6);
    playNote(offlineContext, 1.8);

    offlineContext.startRendering();    
}

function init() {    
    context = new webkitAudioContext();
    loadHihat("sounds/drum-samples/hihat-short.wav");
}

[Kenneth Russell]

Comment on attachment 87936 [details]
Patch

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

This looks good overall. There's a minor issue around initialization of one of the members of OfflineAudioDestinationNode, and a couple of formatting issues. I'm not touching the review bit at the moment; let me know if you want me to r+ this and fix any issues upon landing.

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:57
> +        // Construct for default AudioContext which talks to audio hardware.

Typo: Construct -> Constructor?

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:60
> +        // Construct for offline (render-target) AudioContext which renders into an AudioBuffer.

Same typo?

> Source/WebCore/webaudio/AudioContext.cpp:103
> +AudioContext::AudioContext(Document* document, unsigned numberOfChannels, size_t numberOfFrames, double sampleRate)

I think it's worth commenting above this function that this constructor is used for offline rendering, and above the old constructor, that it is used for rendering to the actual audio hardware.

> Source/WebCore/webaudio/AudioContext.cpp:104
> +        : ActiveDOMObject(document, this)

The indentation looks off here. It should match that of the constructor above.

> Source/WebCore/webaudio/AudioContext.h:193
> +    virtual EventTargetData* ensureEventTargetData()  { return &m_eventTargetData; }

Extra space after ().

> Source/WebCore/webaudio/OfflineAudioDestinationNode.cpp:83
> +        m_startedRendering = true;
> +        m_renderThread = createThread(OfflineAudioDestinationNode::renderEntry, this, "offline renderer");

Due to this state transition you'll only be able to run each OfflineAudioDestinationNode to completion once. If you reset m_renderThread to 0 when renderEntry returned and determined whether to spawn the thread based on whether m_renderThread was 0, you could make them reusable.

> Source/WebCore/webaudio/OfflineAudioDestinationNode.h:66
> +    ThreadIdentifier m_renderThread;

Should probably be volatile for safety. Also, you should initialize this in the constructor. I think 0 is the safe uninitialized value on all platforms. See Source/WebCore/workers/WorkerThread.cpp and libxmlLoaderThread in Source/WebCore/dom/XMLDocumentParserLibxml2.cpp. You might want to clean up UndefinedThreadIdentifier in Source/WebCore/webaudio/AudioContext.cpp.

[Chris Rogers]

Comment on attachment 87936 [details]
Patch

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

>> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:57
>> +        // Construct for default AudioContext which talks to audio hardware.
> 
> Typo: Construct -> Constructor?

FIXED

>> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:60
>> +        // Construct for offline (render-target) AudioContext which renders into an AudioBuffer.
> 
> Same typo?

FIXED

>> Source/WebCore/dom/EventTarget.h:43
>> +    class AudioContext;
> 
> Code inside a namespace should not be indented.  [whitespace/indent] [4]

The file already has this incorrect indentation (I didn't add it).  Should I fix the whole file?

>> Source/WebCore/webaudio/AudioContext.cpp:103
>> +AudioContext::AudioContext(Document* document, unsigned numberOfChannels, size_t numberOfFrames, double sampleRate)
> 
> I think it's worth commenting above this function that this constructor is used for offline rendering, and above the old constructor, that it is used for rendering to the actual audio hardware.

DONE

>> Source/WebCore/webaudio/AudioContext.cpp:104
>> +        : ActiveDOMObject(document, this)
> 
> The indentation looks off here. It should match that of the constructor above.

FIXED

>> Source/WebCore/webaudio/AudioContext.h:193
>> +    virtual EventTargetData* ensureEventTargetData()  { return &m_eventTargetData; }
> 
> Extra space after ().

FIXED

>> Source/WebCore/webaudio/OfflineAudioDestinationNode.cpp:83
>> +        m_renderThread = createThread(OfflineAudioDestinationNode::renderEntry, this, "offline renderer");
> 
> Due to this state transition you'll only be able to run each OfflineAudioDestinationNode to completion once. If you reset m_renderThread to 0 when renderEntry returned and determined whether to spawn the thread based on whether m_renderThread was 0, you could make them reusable.

To make them re-usable there would be more work necessary than this because the entire AudioContext state (including all of its AudioNodes) would need to be reset.  It's non-trivial to do this, so for now at least I think it would be good to limit to the single usage case.  I can add a FIXME about throwing an exception if startRendering() is called a second time.

>> Source/WebCore/webaudio/OfflineAudioDestinationNode.h:66
>> +    ThreadIdentifier m_renderThread;
> 
> Should probably be volatile for safety. Also, you should initialize this in the constructor. I think 0 is the safe uninitialized value on all platforms. See Source/WebCore/workers/WorkerThread.cpp and libxmlLoaderThread in Source/WebCore/dom/XMLDocumentParserLibxml2.cpp. You might want to clean up UndefinedThreadIdentifier in Source/WebCore/webaudio/AudioContext.cpp.

Actually, at least on Mac OS X in my debugging I found that 0 is actually a thread value of a real thread and does not mean "no thread'

[Chris Rogers]

Created attachment 88104 [details]
Patch

[Chris Rogers]

Latest patch also adds the makefile stuff for the mac port

[Kenneth Russell]

Comment on attachment 88104 [details]
Patch

OK, the revision looks good to me.

[Chris Rogers]

Created attachment 88148 [details]
Patch

[WebKit Review Bot]

Attachment 88148 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/dom/EventTarget.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

Committed r82963: <http://trac.webkit.org/changeset/82963>

[Eric Seidel (no email)]

Comment on attachment 88148 [details]
Patch

Cleared review? from attachment 88148 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Karl Godard]

Is this implemented in windows, or just mac osx?

[Karl Godard]

nevermind, I think i was passing wrong parameters to the constructor. Looks like my revised test shows that this is indeed working in windows! 

Web Audio API is awesome. Keep up the great work!

[Chris Rogers]

(In reply to comment #14)
> nevermind, I think i was passing wrong parameters to the constructor. Looks like my revised test shows that this is indeed working in windows! 
> 
> Web Audio API is awesome. Keep up the great work!

Thanks Karl, please keep in mind that the AudioContext constructor currently available is *not* documented in the spec and will likely change.