Implement suspend() and resume() for OfflineAudioContext

Source/modules/webaudio/OfflineAudioContext.cpp

Index: Source/modules/webaudio/OfflineAudioContext.cpp
diff --git a/Source/modules/webaudio/OfflineAudioContext.cpp b/Source/modules/webaudio/OfflineAudioContext.cpp
index d3488419e947e373d50f13cb105be0fb0604d178..5d1fa65e08a886569cad6aaecbeae56e6d927606 100644
--- a/Source/modules/webaudio/OfflineAudioContext.cpp
+++ b/Source/modules/webaudio/OfflineAudioContext.cpp
@@ -31,7 +31,11 @@
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
+#include "modules/webaudio/OfflineAudioCompletionEvent.h"
+#include "modules/webaudio/OfflineAudioDestinationNode.h"
+#include "platform/ThreadSafeFunctional.h"
 #include "platform/audio/AudioUtilities.h"
+#include "public/platform/Platform.h"
 
 namespace blink {
 
@@ -92,15 +96,105 @@ OfflineAudioContext* OfflineAudioContext::create(ExecutionContext* context, unsi
 
 OfflineAudioContext::OfflineAudioContext(Document* document, unsigned numberOfChannels, size_t numberOfFrames, float sampleRate)
     : AudioContext(document, numberOfChannels, numberOfFrames, sampleRate)
+    , m_isRenderingStarted(false)
 {
+    m_totalRenderFrames = numberOfFrames;

[Raymond Toy, 2015/05/28 16:37:35]

Why not move this to the initializer part?

[hongchan, 2015/06/09 20:49:59]

Done.

 }
 
 OfflineAudioContext::~OfflineAudioContext()
 {
 }
 
+// FIXME: What should be done to trace members in OfflineAudioContext?
+// DEFINE_TRACE(OfflineAudioContext)
+// {
+//     visitor->trace(m_scheduledSuspends);
+//     visitor->trace(m_completeResolver);
+// }
+
+OfflineAudioContext::ScheduledSuspendContainer::ScheduledSuspendContainer(
+    size_t when, PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)

[Raymond Toy, 2015/05/28 16:37:35]

I think frame is better than when.

[hongchan, 2015/06/09 20:49:59]

Done.

+    : m_when(when)
+    , m_resolver(resolver)
+{
+}
+
+OfflineAudioContext::ScheduledSuspendContainer::~ScheduledSuspendContainer()
+{
+}
+
+DEFINE_TRACE(OfflineAudioContext::ScheduledSuspendContainer)
+{
+    visitor->trace(m_resolver);
+}
+
+PassOwnPtr<OfflineAudioContext::ScheduledSuspendContainer>
+OfflineAudioContext::ScheduledSuspendContainer::create(
+    size_t when, PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver)
+{
+    return adoptPtr(new ScheduledSuspendContainer(when, resolver));
+}
+
+bool OfflineAudioContext::ScheduledSuspendContainer::shouldSuspendAt(size_t when) const
+{
+    return m_when == when;
+}
+
+bool OfflineAudioContext::shouldSuspendNow()
+{
+    ASSERT(!isMainThread());
+
+    // Suspend if necessary and resolve the associated promise.
+    size_t now = currentSampleFrame();
+    for (unsigned index = 0; index < m_scheduledSuspends.size(); ++index) {
+        if (m_scheduledSuspends.at(index)->shouldSuspendAt(now)) {
+            // Resolve the scheduled suspend on the main thread. (async)
+            Platform::current()->mainThread()->postTask(FROM_HERE,
+                threadSafeBind(
+                    &OfflineAudioContext::resolveSuspendOnMainThread,
+                    this,
+                    index));

[Raymond Toy, 2015/05/28 16:37:36]

I think we have a problem here.  If the main thread is moving items around in
m_scheduledSuspends, this loop is going to behave badly.

Maybe the solution is for the main thread to deliver the promise, and just mark
the element as having been handled. (By setting the time to negative or an
additional boolean to say so.)

Then the post render task can remove the handled suspends.

[hongchan, 2015/06/09 20:49:59]

Done. However, the post render task happens every render quantum and we don't
want that here. We should resolve the pending suspend promise only when the
context is actually suspended.

+            return true;
+        }
+    }
+
+    return false;
+}
+
+void OfflineAudioContext::fireCompletionEvent()
+{
+    ASSERT(isMainThread());
+    if (!isMainThread())
+        return;
+
+    // We set the state to closed here so that the oncomplete event handler sees
+    // that the context has been closed.
+    setContextState(Closed);
+
+    AudioBuffer* renderedBuffer = m_renderTarget.get();
+
+    ASSERT(renderedBuffer);
+    if (!renderedBuffer)
+        return;
+
+    // Avoid firing the event if the document has already gone away.
+    if (executionContext()) {
+        // Call the offline rendering completion event listener and resolve the
+        // promise too.
+        dispatchEvent(OfflineAudioCompletionEvent::create(renderedBuffer));
+        m_completeResolver->resolve(renderedBuffer);
+    }
+}
+
 ScriptPromise OfflineAudioContext::startOfflineRendering(ScriptState* scriptState)
 {
+    ASSERT(isMainThread());
+
+    // FIXME: This check causes crash on oac-detached-no-crash.html
+    // ASSERT(m_destinationNode);

[Raymond Toy, 2015/05/28 16:37:36]

We'll have to fix this before landing this CL.

[hongchan, 2015/06/09 20:49:59]

Acknowledged.

+
+    AutoLocker locker(this);
+
     // Calling close() on an OfflineAudioContext is not supported/allowed,
     // but it might well have been stopped by its execution context.
     if (isContextClosed()) {
@@ -111,7 +205,7 @@ ScriptPromise OfflineAudioContext::startOfflineRendering(ScriptState* scriptStat
                 "cannot call startRendering on an OfflineAudioContext in a stopped state."));
     }
 
-    if (m_offlineResolver) {
+    if (m_completeResolver) {
         // Can't call startRendering more than once.  Return a rejected promise now.
         return ScriptPromise::rejectWithDOMException(
             scriptState,
@@ -120,9 +214,112 @@ ScriptPromise OfflineAudioContext::startOfflineRendering(ScriptState* scriptStat
                 "cannot call startRendering more than once"));
     }
 
-    m_offlineResolver = ScriptPromiseResolver::create(scriptState);
-    startRendering();
-    return m_offlineResolver->promise();
+    m_completeResolver = ScriptPromiseResolver::create(scriptState);
+
+    if (contextState() == AudioContextState::Suspended) {
+        // If the context state is valid (Suspended), star rendering and return

[Raymond Toy, 2015/05/28 16:37:35]

I think the code is easier to understand if you handle the invalid state  here
and returned.

"star" -> "start"

[hongchan, 2015/06/09 20:49:59]

Done.

+        // the promise.
+        destination()->audioDestinationHandler().startRendering();
+        m_isRenderingStarted = true;
+        setContextState(Running);
+        return m_completeResolver->promise();
+    }
+
+    // If the context is not in a valid state, reject promise.
+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(
+            InvalidStateError,
+            "invalid context state"));

[Raymond Toy, 2015/05/28 16:37:35]

Use better error message saying why this is invalid.

[hongchan, 2015/06/09 20:49:59]

Done.

+}
+
+ScriptPromise OfflineAudioContext::suspendOfflineRendering(ScriptState* scriptState, double when)
+{
+    ASSERT(isMainThread());
+
+    AutoLocker locker(this);
+
+    // Quantize the suspend time based on the rendering block boundary.
+    unsigned quantizedFrame = destination()->audioDestinationHandler().quantizeTime(when);
+
+    if (isValidToScheduleAt(quantizedFrame)) {
+        RefPtrWillBeRawPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+        ScriptPromise promise = resolver->promise();
+        m_scheduledSuspends.append(ScheduledSuspendContainer::create(quantizedFrame, resolver));
+
+        return promise;
+    }
+
+    // If suspendTime is already passed, reject the promise and throw an exception.
+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(
+            InvalidStateError,
+            "cannot schedule a suspend in the past, beyond the total render duration or at the duplicate position."));

[Raymond Toy, 2015/05/28 16:37:36]

Can we provide more information here?

[hongchan, 2015/06/09 20:49:59]

I broke this down into multiple checks and rejections.

+}
+
+ScriptPromise OfflineAudioContext::resumeOfflineRendering(ScriptState* scriptState)
+{
+    ASSERT(isMainThread());
+
+    AutoLocker locker(this);
+
+    // If the context is suspended, resume rendering by calling startRendering()
+    // and set the state to "Running." Note that resuming is possible only after
+    // the rendering started.
+    if (contextState() == AudioContextState::Suspended && m_isRenderingStarted) {

[Raymond Toy, 2015/05/28 16:37:35]

I think it's easier to understand if you reverse the logic and have the if
handle the bad case.

[hongchan, 2015/06/09 20:49:59]

Done.

+        destination()->audioDestinationHandler().startRendering();
+        setContextState(Running);
+
+        RefPtrWillBeRawPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+        ScriptPromise promise = resolver->promise();
+
+        // Resolve the promise immediately.
+        resolver->resolve();
+
+        return promise;
+    }
+
+    // If the rendering is already running or not started, reject the promise.
+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(
+            InvalidStateError,
+            "cannot resume the context is already running or has not started"));

[Raymond Toy, 2015/05/28 16:37:36]

Can we have a more specific message?  We know exactly why this is wrong, so say
exactly why.

[hongchan, 2015/06/09 20:49:59]

I separate this into two checks and rejections as well.

+}
+
+void OfflineAudioContext::resolveSuspendOnMainThread(unsigned index)
+{
+    ASSERT(isMainThread());
+
+    // FIXME: This process (removing suspend from the list) should be

[Raymond Toy, 2015/05/28 16:37:35]

Why does this need fixing?

[hongchan, 2015/06/09 20:49:59]

First, the location of this comment is wrong, so I moved to the line 306. Also I
restructured the promise resolution so we don't have the issue discussed above.

+    // synchronous.
+
+    AutoLocker locker(this);
+
+    // Suspend the context first. This will fire onstatechange event.
+    setContextState(Suspended);
+
+    // Resolve the suspend and remove it from the list.
+    // FIXME: is removing an element efficient? What if there are 10K suspends?
+    m_scheduledSuspends.at(index)->resolver()->resolve();
+    m_scheduledSuspends.remove(index);
+}
+
+bool OfflineAudioContext::isValidToScheduleAt(size_t when)
+{
+    // The specified suspend time should be between the current frame and the
+    // total render frame.
+    bool isInValidRange = currentSampleFrame() < when && when < m_totalRenderFrames;
+
+    // Also there should be no duplicate in the existing scheduled suspends.
+    bool isDuplicate = false;
+    for (unsigned index = 0; index < m_scheduledSuspends.size(); ++index) {
+        if (m_scheduledSuspends.at(index)->shouldSuspendAt(when))
+            isDuplicate = true;
+    }
+
+    return isInValidRange && !isDuplicate;
 }
 
 } // namespace blink