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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 4 11:53:36 PDT 2011


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





--- Comment #4 from Kenneth Russell <kbr at google.com>  2011-04-04 11:53:37 PST ---
(From update of attachment 87936)
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.

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



More information about the webkit-unassigned mailing list