[Webkit-unassigned] [Bug 57676] Add support for offline audio rendering to AudioContext API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57676





--- Comment #5 from Chris Rogers <crogers at google.com>  2011-04-04 12:42:30 PST ---
(From update of attachment 87936)
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'

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.