Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1405413004:
Implement suspend() and resume() for OfflineAudioContext (Closed)
DescriptionImplement suspend() and resume() for OfflineAudioContext
NOTE: This is the second attempt of https://crrev.com/1140723003/
Adds suspend() and resume() feature in OfflineAudioContext to support the
synchronous graph manipulation with the render block precision (k-rate) in the
non-realtime audio rendering.
The benefit of being able to suspend/resume the context with the render block
precision is:
1) The audio graph can be modified in a time-accurate way, independent of the
hardware. Without this, setTimeout, completion events, or state change events
are needed to manipulate the graph, and the results depend on when the events
are fired and on how fast the hardware is.
2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which
already supports suspend/resume. (There are minor difference required by the
difference between offline and online contexts.)
This feature also can be used in Blink layout tests to verify the behavior of
audio rendering. With this feature in the implementation, several flaky web
audio layout tests can be fixed.
http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface
https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885
BUG=497933, 545686
TEST=
webaudio/offlineaudiocontext-suspend-resume-basic.html
webaudio/offlineaudiocontext-suspend-resume-eventhandler.html
webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html
webaudio/offlineaudiocontext-suspend-resume-promise.html
webaudio/offlineaudiocontext-suspend-resume-sequence.html
Committed: https://crrev.com/cdc0e15efab0e82a6e95deaae11a394535ea82de
Cr-Commit-Position: refs/heads/master@{#361367}
Patch Set 1 #Patch Set 2 : #
Total comments: 69
Patch Set 3 : Addressing feedback and reverting back audiocontext-suspend-resume.html #Patch Set 4 : Clarifying error messages in layout tests #
Total comments: 56
Patch Set 5 : Introducing OfflineAutoLocker #Patch Set 6 : Addressing feedback #
Total comments: 24
Patch Set 7 : Addressing feedback #
Total comments: 4
Patch Set 8 : Addressing feedback #
Total comments: 4
Patch Set 9 : L-G-T-M webaudio part #
Total comments: 2
Patch Set 10 : Fixing resuming behavior according to TPAC decision #
Total comments: 6
Patch Set 11 : Removing redundant definitions of context.suspend() #
Total comments: 4
Patch Set 12 : Addressed feedback from yhirano #
Total comments: 24
Patch Set 13 : Addressing feedback from haraken (1) #Patch Set 14 : Added comment for m_schduledSuspends #Patch Set 15 : UseCounter.h + global-interface-listing #Patch Set 16 : Updating UseCounter.h after L-G-T-M #Patch Set 17 : Updating UseCounter.h after L-G-T-M #Messages
Total messages: 75 (25 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
This CL is for the preview. I've ran a couple of critical trybots and they look okay so far. The TSAN bot failed, but the failure seems to be irrelevant to the change. If you think this CL is good enough for the review from the Oilpan devs, we can start the actual review with more people involved.
Some initial comments. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html:12: description("Test suspend/resume for an AudioContext"); Why does audiocontext-suspend-resume.html need to be modified? Nothing you're doing should affect this right? Except maybe you need a test for context.suspend(<time>). https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html:58: 'interface', Don't need to list all of the tests anymore if we're running them all. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:50: var suspendTime2 = (renderQuantum + renderQuantum * 0.5) / sampleRate; Is this clearer to write 1.5*renderQuantum? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:110: m_didInitializeContextGraphMutex = true; Why is this initialized to true here and false in line 106? (Not part of your CL. I just noticed this.) https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:261: void initialize(); Where did initialize() go? Is it no longer needed? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:317: void handleStoppableSourceNodes(); Where is this now? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:139: return ScriptPromise(); Although not reachable, maybe return a rejected promise. Just in case. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioNode.h:244: // This raw pointer is safe because this is cleared for all of live Where is this code now? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:66: ASSERT(isAudioThread()); What should we do in a release build if we accidentally do call this from the audio thread? It seems pretty bad if this is called from the audio thread. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h:115: // CANNOT be used in the real-time audio context. CANNOT -> MUST NOT. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:179: size_t frame = when * sampleRate(); No check for negative value of when? That should be illegal, right? Oh, it's below. Then I would move this code down past the check. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:180: frame = frame - (frame % destinationHandler().renderQuantumLength()); Maybe "frame -= ..." is clearer than "frame = frame - ..." https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:247: "cannot resume a context that is not suspended")); Maybe say what state we're actually in. Just for extra info since we know what state we're in for free. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:276: return ScriptPromise(); Although this isn't reachable, maybe we should return a rejected promise. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:310: // This is offline rendering, so it is safe to lock. I would expand on this to say why it's safe to cause pauses. (Is that covered somewhere else?) https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:324: // This is offline rendering, so it is safe to lock. Expand on this comment, like above. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:58: // CANNOT be called on OfflineAudioContext. What does "called on OfflineAudioContext" mean here? You're really saying suspend without a time value is not valid for offline contexts? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:94: SuspendMap m_scheduledSuspends; Say something that this can only be read or written by the main thread? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:100: // state when the context is suspended. Not for this CL, but maybe it would be good in the spec to allow a new state, say, 'Initialized', for offline contexts. I guess online context can also start in 'Initialized' but transition right away to 'Running'. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState, ImplementedAs=suspendContext] Promise<void> suspend(double suspendTime); Maybe add metrics for suspend/resume on offline contexts? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:133: size_t OfflineAudioDestinationHandler::renderQuantumLength() const Maybe renderQuantumFrames is a better name to indicate this is the number of frames. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:189: if (m_shouldSuspend) { I think it would be clearer if offlineRender returned a value for m_shouldSuspend (without actually modifying it in offlineRender). https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:203: m_framesToProcess -= framesAvailableToCopy; Is it possible for wrap around to happen? Maybe assert m_framesToProcess >= framesAvailableToCopy. If this failed, m_framesToProcess becomes some huge number since it's a size_t. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:207: if (m_framesToProcess <= 0) { Since m_framesToProcess is size_t, less than 0 isn't possible. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:210: return; return not really necessary here. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:298: releaseStore(&m_currentSampleFrame, newSampleFrame); Not for this CL, but we should probably add a setter and getter for m_currentSampleFrame. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:72: bool m_startedRendering; Document what this is for. (You already mentioned above somewhere, but document here since this is the definition.) Document the following variables too. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:66: // Destination's internal methods for start/pause/continue/finish rendering. Maybe document what these do. Hard to tell from the name the difference between startOfflineRendering and doOfflineRendering. Also can't tell what finishOfflineRendering does.
Description was changed from ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html ========== to ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ TODO: 1. Edit core/frame/UseCounter.h accordingly. 2. Add UMA metric in OfflineAudioContext.idl. Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html ==========
1. I reverted back to the original test: audiocontext-suspend-resume.html 2. Needs feedback on OAD::offlineRender() returning a boolean value instead of changing the m_shouldSuspend internally. 3. For the metric part, I will add them after passing the first round of the review. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html:12: description("Test suspend/resume for an AudioContext"); On 2015/10/16 23:32:35, Raymond Toy wrote: > Why does audiocontext-suspend-resume.html need to be modified? Nothing you're > doing should affect this right? Except maybe you need a test for > context.suspend(<time>). Okay, I only proposed the stopgap solution for this test by reverting back to the original test and fixing the part causing errors. Now we have different interface for AC and OAC, so we need to reconsider this test as whole. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiocontext-suspend-resume.html:58: 'interface', On 2015/10/16 23:32:35, Raymond Toy wrote: > Don't need to list all of the tests anymore if we're running them all. This will be reverted back to the original version as mentioned at the head. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:50: var suspendTime2 = (renderQuantum + renderQuantum * 0.5) / sampleRate; On 2015/10/16 23:32:35, Raymond Toy wrote: > Is this clearer to write 1.5*renderQuantum? Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:110: m_didInitializeContextGraphMutex = true; On 2015/10/16 23:32:36, Raymond Toy wrote: > Why is this initialized to true here and false in line 106? > > (Not part of your CL. I just noticed this.) It has been like this from the beginning. Honestly I couldn't figure out why, but I assumed that the mutex is false before the actual execution of constructor and the timing might be important? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:261: void initialize(); On 2015/10/16 23:32:36, Raymond Toy wrote: > Where did initialize() go? Is it no longer needed? 1. initialize() has been moved to 'protected' section, since it is used when the child object (OfflineAudioContext) finishing the constructor. 2. I believe this is okay since we already have uninitialized() method in the protected section. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:317: void handleStoppableSourceNodes(); On 2015/10/16 23:32:36, Raymond Toy wrote: > Where is this now? See the protected section as well. In this patch, this method is called by handlePreOfflineRenderTasks() which is a part of OfflineAudioContext. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:139: return ScriptPromise(); On 2015/10/16 23:32:36, Raymond Toy wrote: > Although not reachable, maybe return a rejected promise. Just in case. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioNode.h:244: // This raw pointer is safe because this is cleared for all of live On 2015/10/16 23:32:36, Raymond Toy wrote: > Where is this code now? It moved up to the protected section because OAD has a another version of the method. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:66: ASSERT(isAudioThread()); On 2015/10/16 23:32:36, Raymond Toy wrote: > What should we do in a release build if we accidentally do call this from the > audio thread? It seems pretty bad if this is called from the audio thread. This is why I completely separated the render method for the offline audio context. So there is no possibility this method gets called by the realtime context. However, I can agree with your concern. We can always use tryLock() if the current thread is not audio thread. WDYT? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h:115: // CANNOT be used in the real-time audio context. On 2015/10/16 23:32:36, Raymond Toy wrote: > CANNOT -> MUST NOT. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:179: size_t frame = when * sampleRate(); On 2015/10/16 23:32:36, Raymond Toy wrote: > No check for negative value of when? That should be illegal, right? > > Oh, it's below. Then I would move this code down past the check. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:180: frame = frame - (frame % destinationHandler().renderQuantumLength()); On 2015/10/16 23:32:36, Raymond Toy wrote: > Maybe "frame -= ..." is clearer than "frame = frame - ..." Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:276: return ScriptPromise(); On 2015/10/16 23:32:36, Raymond Toy wrote: > Although this isn't reachable, maybe we should return a rejected promise. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:310: // This is offline rendering, so it is safe to lock. On 2015/10/16 23:32:36, Raymond Toy wrote: > I would expand on this to say why it's safe to cause pauses. (Is that covered > somewhere else?) Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:324: // This is offline rendering, so it is safe to lock. On 2015/10/16 23:32:36, Raymond Toy wrote: > Expand on this comment, like above. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:58: // CANNOT be called on OfflineAudioContext. On 2015/10/16 23:32:36, Raymond Toy wrote: > What does "called on OfflineAudioContext" mean here? You're really saying > suspend without a time value is not valid for offline contexts? Yes, the IDL will raise the exception if the time argument is not provided. I think it is a part of what we have on the spec? The time argument is not optional or nullable. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:94: SuspendMap m_scheduledSuspends; On 2015/10/16 23:32:36, Raymond Toy wrote: > Say something that this can only be read or written by the main thread? Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:100: // state when the context is suspended. On 2015/10/16 23:32:36, Raymond Toy wrote: > Not for this CL, but maybe it would be good in the spec to allow a new state, > say, 'Initialized', for offline contexts. I guess online context can also start > in 'Initialized' but transition right away to 'Running'. I agree with the idea, but I suggest a different name for the initial state. Here the life of AC/OAC in my mind: created > running > suspended > running > closed https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState, ImplementedAs=suspendContext] Promise<void> suspend(double suspendTime); On 2015/10/16 23:32:36, Raymond Toy wrote: > Maybe add metrics for suspend/resume on offline contexts? It will be the next step of this CL. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:133: size_t OfflineAudioDestinationHandler::renderQuantumLength() const On 2015/10/16 23:32:36, Raymond Toy wrote: > Maybe renderQuantumFrames is a better name to indicate this is the number of > frames. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:189: if (m_shouldSuspend) { On 2015/10/16 23:32:36, Raymond Toy wrote: > I think it would be clearer if offlineRender returned a value for > m_shouldSuspend (without actually modifying it in offlineRender). I can certainly do that and I have thought about it. Then we have return true when: 1) dest is not initialized, 2) the number of input is below than 1 and 3) the rendering is successful. So the method returns true and it means three different things. Not sure if this is the right design. Surely the current status is not pretty, but I think this is a bearable compromise. Perhaps we can rename the method like "renderAndSuspendIfNecessary()' and change it to return true when it needs to suspend and returns false for all other cases? WDYT? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:203: m_framesToProcess -= framesAvailableToCopy; On 2015/10/16 23:32:36, Raymond Toy wrote: > Is it possible for wrap around to happen? Maybe assert m_framesToProcess >= > framesAvailableToCopy. If this failed, m_framesToProcess becomes some huge > number since it's a size_t. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:207: if (m_framesToProcess <= 0) { On 2015/10/16 23:32:36, Raymond Toy wrote: > Since m_framesToProcess is size_t, less than 0 isn't possible. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:210: return; On 2015/10/16 23:32:36, Raymond Toy wrote: > return not really necessary here. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:298: releaseStore(&m_currentSampleFrame, newSampleFrame); On 2015/10/16 23:32:36, Raymond Toy wrote: > Not for this CL, but we should probably add a setter and getter for > m_currentSampleFrame. You mean having the override = operator with the atomic access? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:66: // Destination's internal methods for start/pause/continue/finish rendering. On 2015/10/16 23:32:37, Raymond Toy wrote: > Maybe document what these do. Hard to tell from the name the difference between > startOfflineRendering and doOfflineRendering. Also can't tell what > finishOfflineRendering does. Done.
https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:9: PASS Calling multiple suspends at the same rendering quantum rejected correctly (with InvalidStateError: cannot schedule more than one suspend at frame 128 (0.00435374 seconds)). Might be a little more informative if this test consisted of several PASS messages. The first can be something like PASS suspend(<time1>) succeeded PASS suspend(<time2>) correctly rejected (with...) (Actual messages may vary to make it clearer.) https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:6: PASS context.currentTime is equal to 0. Is it possible to get more descriptive output. It's easy to see that we're passing, but it's really hard to tell why that the actual times are correct. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation-expected.txt:6: PASS The first suspend time is equal to 1. Is it possible to have more verbose output so that we can see that the PASS messages actually make sense? Maybe add more PASS/FAIL messages to show what's happening? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:66: ASSERT(isAudioThread()); On 2015/10/19 at 20:08:12, hoch wrote: > On 2015/10/16 23:32:36, Raymond Toy wrote: > > What should we do in a release build if we accidentally do call this from the > > audio thread? It seems pretty bad if this is called from the audio thread. > > This is why I completely separated the render method for the offline audio context. So there is no possibility this method gets called by the realtime context. However, I can agree with your concern. > > We can always use tryLock() if the current thread is not audio thread. WDYT? Maybe RELEASE_ASSERT or RELEASE_ASSERT_WITH_MESSAGE? https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:58: // CANNOT be called on OfflineAudioContext. On 2015/10/19 at 20:08:12, hoch wrote: > On 2015/10/16 23:32:36, Raymond Toy wrote: > > What does "called on OfflineAudioContext" mean here? You're really saying > > suspend without a time value is not valid for offline contexts? > > Yes, the IDL will raise the exception if the time argument is not provided. I think it is a part of what we have on the spec? The time argument is not optional or nullable. Maybe change "called on Offline..." with "called for an ..." or "called from an ..." https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:298: releaseStore(&m_currentSampleFrame, newSampleFrame); On 2015/10/19 at 20:08:12, hoch wrote: > On 2015/10/16 23:32:36, Raymond Toy wrote: > > Not for this CL, but we should probably add a setter and getter for > > m_currentSampleFrame. > > You mean having the override = operator with the atomic access? I think the chromium/blink style is currentSampleFrame() to get m_currentSampleFrame and setCurrentSampleFrame(new) to set it.
We agreed to leave audiocontext-suspend-resume-basic.html test and make some changes to pass the testing. I will eventually remove this test in a separate CL. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:9: PASS Calling multiple suspends at the same rendering quantum rejected correctly (with InvalidStateError: cannot schedule more than one suspend at frame 128 (0.00435374 seconds)). On 2015/10/19 at 20:27:27, Raymond Toy wrote: > Might be a little more informative if this test consisted of several PASS messages. The first can be something like > > PASS suspend(<time1>) succeeded > PASS suspend(<time2>) correctly rejected (with...) > > (Actual messages may vary to make it clearer.) Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:6: PASS context.currentTime is equal to 0. On 2015/10/19 at 20:27:27, Raymond Toy wrote: > Is it possible to get more descriptive output. It's easy to see that we're passing, but it's really hard to tell why that the actual times are correct. Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation-expected.txt (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation-expected.txt:6: PASS The first suspend time is equal to 1. On 2015/10/19 at 20:27:27, Raymond Toy wrote: > Is it possible to have more verbose output so that we can see that the PASS messages actually make sense? Maybe add more PASS/FAIL messages to show what's happening? Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:66: ASSERT(isAudioThread()); On 2015/10/19 at 20:27:27, Raymond Toy wrote: > On 2015/10/19 at 20:08:12, hoch wrote: > > On 2015/10/16 23:32:36, Raymond Toy wrote: > > > What should we do in a release build if we accidentally do call this from the > > > audio thread? It seems pretty bad if this is called from the audio thread. > > > > This is why I completely separated the render method for the offline audio context. So there is no possibility this method gets called by the realtime context. However, I can agree with your concern. > > > > We can always use tryLock() if the current thread is not audio thread. WDYT? > > Maybe RELEASE_ASSERT or RELEASE_ASSERT_WITH_MESSAGE? Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:247: "cannot resume a context that is not suspended")); On 2015/10/16 at 23:32:36, Raymond Toy wrote: > Maybe say what state we're actually in. Just for extra info since we know what state we're in for free. Okay, I will reverse the order of the checks. So if your context has not been started, resuming will be rejected with the proper error message. Otherwise it means user is trying to resume without suspending it previously. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:58: // CANNOT be called on OfflineAudioContext. On 2015/10/19 at 20:27:27, Raymond Toy wrote: > On 2015/10/19 at 20:08:12, hoch wrote: > > On 2015/10/16 23:32:36, Raymond Toy wrote: > > > What does "called on OfflineAudioContext" mean here? You're really saying > > > suspend without a time value is not valid for offline contexts? > > > > Yes, the IDL will raise the exception if the time argument is not provided. I think it is a part of what we have on the spec? The time argument is not optional or nullable. > > Maybe change "called on Offline..." with "called for an ..." or "called from an ..." Done. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:189: if (m_shouldSuspend) { On 2015/10/19 at 20:08:12, hoch wrote: > On 2015/10/16 23:32:36, Raymond Toy wrote: > > I think it would be clearer if offlineRender returned a value for > > m_shouldSuspend (without actually modifying it in offlineRender). > > I can certainly do that and I have thought about it. > Then we have return true when: 1) dest is not initialized, 2) the number of input is below than 1 and 3) the rendering is successful. > > So the method returns true and it means three different things. Not sure if this is the right design. Surely the current status is not pretty, but I think this is a bearable compromise. > > Perhaps we can rename the method like "renderAndSuspendIfNecessary()' and change it to return true when it needs to suspend and returns false for all other cases? WDYT? Done per the first comment on this. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:298: releaseStore(&m_currentSampleFrame, newSampleFrame); On 2015/10/19 at 20:27:27, Raymond Toy wrote: > On 2015/10/19 at 20:08:12, hoch wrote: > > On 2015/10/16 23:32:36, Raymond Toy wrote: > > > Not for this CL, but we should probably add a setter and getter for > > > m_currentSampleFrame. > > > > You mean having the override = operator with the atomic access? > > I think the chromium/blink style is currentSampleFrame() to get m_currentSampleFrame and setCurrentSampleFrame(new) to set it. Acknowledged. https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (left): https://codereview.chromium.org/1405413004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:72: bool m_startedRendering; On 2015/10/16 at 23:32:37, Raymond Toy wrote: > Document what this is for. (You already mentioned above somewhere, but document here since this is the definition.) > > Document the following variables too. Done.
In the previous patch, checking m_scheduledSuspends was not protected by the graph lock. The new OfflineAutoLocker is introduced to provide the lock mechanism over the local scope, and it will protect the shouldSuspend() until the scope is finished. Perhaps this CL is ready for the oilpan reviwers?
https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:15: PASS Calling resume on context without any previous suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). The description doesn't seem to match the InvalidStateError message. What is really supposed to be tested here? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:60: var suspendTime2 = renderQuantum * 1.5 / sampleRate; 1.5 * renderQuantum seems more typical and obvious. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:80: context.suspend(renderDuration * 0.5).then(done); Comment and code don't quite match. It's 0.5 sec only if renderDuration is 1. Update comment. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:89: Should('Resuming at 0 second (before the actual suspend happens)', context.resume()).beRejected(); Is there a potential race here? You've started rendering with a suspend at time 0.5? What if the rendering is super fast and there's delay between startRendering and resume(). then the resume could happen after suspend. Odds are very low, but if there is a race, please make a note of it. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:106: 'suspend-invalid-argument', I wouldn't list the individual tasks. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:26: // in to the render quantum boundary. "fall in to the" -> "will fall on the" https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:25: var constantBuffer = createConstantBuffer(context, 128, 1.0); Since you loop, why not just create a buffer of length 1? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:76: var subarray0 = data.subarray(0, suspendIndex1); slice or subarray? Which is better? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt:6: PASS The actual resolution order of the suspend 5 (0.00 sec.) is equal to 0. I think it would be really helpful if you printed out PASS when each suspend is called. Then you can see the actual order of the calls to suspend, and then the messages here and below make obvious sense that they're suspended in time order. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:68: RELEASE_ASSERT(isAudioThread()); Maybe use RELEASE_ASSERT_WITH_MESSAGE so a simple message is printed before we crash. Then we get some useful information even if the stacktrace is broken for some reason. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:106: initialize(); What happens if m_renderTarget is actually nullptr because the buffer couldn't be created? Note that there is a CL in progress that is trying to make DOM arrays throw an exception if they can't be created. Not sure if we need to do that in this CL. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:145: if (contextState() != AudioContextState::Suspended) { Maybe combine this with the case in 127 and use state() to produce the correct message with the actual state? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:167: return ScriptPromise::rejectWithDOMException(scriptState, Does the style checks enforce this? If not, don't reindent. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:247: "cannot resume a context that has not started")); Maybe say "resume an offline context", just to make it more obvious? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:97: // main thread and accessed by the audio thread with the graph lock. Add comment on what the key is and the value in the hash table. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:54: m_renderBus = AudioBus::create(renderTarget->numberOfChannels(), renderQuantumSize); What if AudioBus::create fails to allocate space? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:101: ASSERT(m_renderThread); I would reverse these so that ASSERT(m_renderTarget) is closer to the if. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:110: m_isRenderingStarted = true; Does it matter if m_isRenderingStarted is set to true after posting the task? Does startOfflineRendering every check m_isRenderingStarted? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:133: size_t OfflineAudioDestinationHandler::renderQuantumFrames() const Should this be inline? It's so small. Or maybe make renderQuantumSize a class static constant? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:163: m_framesToProcess = m_renderTarget->length(); Shouldn't this be set when the offline context is created where the m_renderTarget is created with the actual output buffer? And I don't understand how this comment is relevant to this line of code. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:185: // change |m_shouldSuspend| internally according to the scheduled This comment about changing m_shouldSuspend internally is now wrong, right? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:209: if (m_framesToProcess == 0) { I hate the style, but you're not supposed to compare against 0. Just if (!m_framesToProcess) https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:210: ASSERT(m_framesToProcess == 0); How can this assert ever fail? Delete it. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:259: if (!context()->isDestinationInitialized()) { For an offline context, is it actually possible for the destination not to be initialized here? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:265: // will change |m_shouldSuspend| flag if there is a suspend scheduled at the Comment is wrong? m_shouldSuspend isn't modified. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:85: // The offline version of render() method. virtual/override were not used How is virtual/override relevant? Could another class also have an equivalent to checkSuspendsAndRender?
Almost there! https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:90: // reaches to 0.5 second even before the resume here. Nit: "0.5 second" is wrong. Do we really need this? If yes, maybe schedule the suspend at the latest possible time to minimize the possibility of the race. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:50: testPassed('Scheduling a suspend #' + index + ' at ' + suspendTime + ' second(s).'); Nit: Reads better in the output if you just use "Scheduling suspend #". https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:73: } As discussed, we should just move all of this into the OfflineAutoLocker. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:108: exceptionState.throwDOMException(InvalidAccessError, I think the CL was using RangeError for arrays that can't be allocated. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:109: "failed to create a target buffer for offline audio rendering."); Include the the requested number of frames and number of channels in the message. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:245: AutoLocker locker(this); AutoLocker or OfflineAutoLocker? Add a comment to explain which. It's not immediately obvious to me. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:267: // TODO(hongchan): there is a conflict in the spec regarding resuming a If you have the crbug number, add that too. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:337: OfflineAutoLocker locker(this); I only seem to be able to see OfflineAudioLocker being used in handlePre and post offline tasks. Shouldn't there be a call somewhere on the main thread? Otherwise, I don't understand what really needs to be locked. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:79: using OfflineAutoLocker = DeferredTaskHandler::OfflineAutoLocker; What is the style on using "using"? https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:137: // } You forgot to remove this. :-) https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:187: return; Would it make more sense if checkSuspendsAndRender actually called suspendOfflineRendering itself? https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:90: bool checkSuspendsAndRender(AudioBus* sourceBus, AudioBus* destinationBus, size_t numberOfFrames); Not sure, but maybe renderIfPossible or renderIfNotSuspended is a better name?
Note that the comments are a bit mixed up because of going back and forth between new UI and deprecated UI. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:15: PASS Calling resume on context without any previous suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). On 2015/10/21 at 18:22:45, Raymond Toy wrote: > The description doesn't seem to match the InvalidStateError message. What is really supposed to be tested here? It tries to resume a context that has not started. Throwing InvalidStateError per the current spec. http://webaudio.github.io/web-audio-api/#methods-1 https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:60: var suspendTime2 = renderQuantum * 1.5 / sampleRate; On 2015/10/21 at 18:22:45, Raymond Toy wrote: > 1.5 * renderQuantum seems more typical and obvious. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:80: context.suspend(renderDuration * 0.5).then(done); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Comment and code don't quite match. It's 0.5 sec only if renderDuration is 1. Update comment. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:89: Should('Resuming at 0 second (before the actual suspend happens)', context.resume()).beRejected(); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Is there a potential race here? You've started rendering with a suspend at time 0.5? What if the rendering is super fast and there's delay between startRendering and resume(). then the resume could happen after suspend. > > Odds are very low, but if there is a race, please make a note of it. Ack. I will make a note about it. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:106: 'suspend-invalid-argument', On 2015/10/21 at 18:22:45, Raymond Toy wrote: > I wouldn't list the individual tasks. To be honest, I prefer to keep individual tasks, since it is convenient enable/disable each task by commenting in/out. It is really helpful when pinpointing a failing test. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:26: // in to the render quantum boundary. On 2015/10/21 at 18:22:45, Raymond Toy wrote: > "fall in to the" -> "will fall on the" Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:25: var constantBuffer = createConstantBuffer(context, 128, 1.0); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Since you loop, why not just create a buffer of length 1? Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:76: var subarray0 = data.subarray(0, suspendIndex1); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > slice or subarray? Which is better? Array.slice() creates a new array. Array.subarray() gives you the ranged reference. So I prefer to use subarray here. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt:6: PASS The actual resolution order of the suspend 5 (0.00 sec.) is equal to 0. On 2015/10/21 at 18:22:45, Raymond Toy wrote: > I think it would be really helpful if you printed out PASS when each suspend is called. Then you can see the actual order of the calls to suspend, and then the messages here and below make obvious sense that they're suspended in time order. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:68: RELEASE_ASSERT(isAudioThread()); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Maybe use RELEASE_ASSERT_WITH_MESSAGE so a simple message is printed before we crash. Then we get some useful information even if the stacktrace is broken for some reason. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:106: initialize(); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > What happens if m_renderTarget is actually nullptr because the buffer couldn't be created? > Note that there is a CL in progress that is trying to make DOM arrays throw an exception if they can't be created. Not sure if we need to do that in this CL. That means the OAC object cannot be created, we should throw an exception for that. It was not in the original code, but I agree that we need an check for that. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:145: if (contextState() != AudioContextState::Suspended) { On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Maybe combine this with the case in 127 and use state() to produce the correct message with the actual state? Done. Also moved to this check and the ASSERT for m_isRenderingStarted. PS: I have tried your suggestion, but I had a crash in offlineaudiocontext-detached-no-crash.html. Please see https://code.google.com/p/chromium/issues/detail?id=435867. isContextClosed() has several meanings and it cannot be replaced with (state() != 'closed'). So I will leave this as it is and will tackle this with another CL. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:167: return ScriptPromise::rejectWithDOMException(scriptState, On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Does the style checks enforce this? If not, don't reindent. Acknowledged. I made it consistent with other parts. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:247: "cannot resume a context that has not started")); On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Maybe say "resume an offline context", just to make it more obvious? Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:97: // main thread and accessed by the audio thread with the graph lock. On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Add comment on what the key is and the value in the hash table. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:54: m_renderBus = AudioBus::create(renderTarget->numberOfChannels(), renderQuantumSize); On 2015/10/21 at 18:22:46, Raymond Toy wrote: > What if AudioBus::create fails to allocate space? That would be an exception. Also without a valid m_renderBus, the rendering cannot be started. Still we are better off throwing an exception for this case, and I think that should be another CL. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:101: ASSERT(m_renderThread); On 2015/10/21 at 18:22:46, Raymond Toy wrote: > I would reverse these so that ASSERT(m_renderTarget) is closer to the if. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:110: m_isRenderingStarted = true; On 2015/10/21 at 18:22:46, Raymond Toy wrote: > Does it matter if m_isRenderingStarted is set to true after posting the task? Does startOfflineRendering every check m_isRenderingStarted? m_isRenderingStart can only be read/modified by startRendering() - which is invoked by OAC.startRendering(). The rendering loop does not check m_isRenderingStarted, so I think this is safe. However, I can move it up before posting a task. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:133: size_t OfflineAudioDestinationHandler::renderQuantumFrames() const On 2015/10/21 at 18:22:45, Raymond Toy wrote: > Should this be inline? It's so small. Or maybe make renderQuantumSize a class static constant? Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:163: m_framesToProcess = m_renderTarget->length(); On 2015/10/21 at 18:22:46, Raymond Toy wrote: > Shouldn't this be set when the offline context is created where the m_renderTarget is created with the actual output buffer? > > And I don't understand how this comment is relevant to this line of code. Done, and I mistakenly kept the old comment. I will remove it. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:185: // change |m_shouldSuspend| internally according to the scheduled On 2015/10/21 at 18:22:46, Raymond Toy wrote: > This comment about changing m_shouldSuspend internally is now wrong, right? Oops. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:209: if (m_framesToProcess == 0) { On 2015/10/21 at 18:22:46, Raymond Toy wrote: > I hate the style, but you're not supposed to compare against 0. Just if (!m_framesToProcess) Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:210: ASSERT(m_framesToProcess == 0); On 2015/10/21 at 18:22:46, Raymond Toy wrote: > How can this assert ever fail? Delete it. This was suggested by haraken@ before, but I think I can remove this. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:259: if (!context()->isDestinationInitialized()) { On 2015/10/21 at 18:22:45, Raymond Toy wrote: > For an offline context, is it actually possible for the destination not to be initialized here? This method is just a ported version of render() method from AudioDestinatioNode. So I try not to change the code if the part is not relevant to suspend/resume. However, offline audio context does not have to wait for the audio device, we might be safe without this check. I will investigate this further. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:265: // will change |m_shouldSuspend| flag if there is a suspend scheduled at the On 2015/10/21 at 18:22:46, Raymond Toy wrote: > Comment is wrong? m_shouldSuspend isn't modified. Oops. Done. https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:85: // The offline version of render() method. virtual/override were not used On 2015/10/21 at 18:22:46, Raymond Toy wrote: > How is virtual/override relevant? Could another class also have an equivalent to checkSuspendsAndRender? I thought about overriding render() method - but I have decided to create a new method with a different name because this call needs to be faster and we might need something else in the method later. (i.e. offline-specific optimization) I will change the comment accordingly. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:50: testPassed('Scheduling a suspend #' + index + ' at ' + suspendTime + ' second(s).'); On 2015/10/21 at 23:14:18, Raymond Toy wrote: > Nit: Reads better in the output if you just use "Scheduling suspend #". Done. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:108: exceptionState.throwDOMException(InvalidAccessError, On 2015/10/21 at 23:14:18, Raymond Toy wrote: > I think the CL was using RangeError for arrays that can't be allocated. Done. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:109: "failed to create a target buffer for offline audio rendering."); On 2015/10/21 at 23:14:18, Raymond Toy wrote: > Include the the requested number of frames and number of channels in the message. Done. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:245: AutoLocker locker(this); On 2015/10/21 at 23:14:18, Raymond Toy wrote: > AutoLocker or OfflineAutoLocker? Add a comment to explain which. It's not immediately obvious to me. This gets called in the main thread. It needs to be AutoLocker. We are better off changing the name to 'MainThreadAutoLocker' or something like that, but that should be another CL. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:337: OfflineAutoLocker locker(this); On 2015/10/21 at 23:14:18, Raymond Toy wrote: > I only seem to be able to see OfflineAudioLocker being used in handlePre and post offline tasks. Shouldn't there be a call somewhere on the main thread? Otherwise, I don't understand what really needs to be locked. OfflineAutoLocker can be called from the AudioThread. We cannot use AutoLocker when it's called from the audio thread because it is designed to be called only from the main thread. Note that both of them does the exactly same thing (locking m_contextGraphMutex) but they are originated from different threads. I suggest we should change the names of AutoLocker/OfflineAutoLocker as whole, but that should be another CL. Probably MainThreadAutoLocker/AudioThreadAutoLocker might be more explicit and clearer. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:79: using OfflineAutoLocker = DeferredTaskHandler::OfflineAutoLocker; On 2015/10/21 at 23:14:18, Raymond Toy wrote: > What is the style on using "using"? I simply followed the example of 'AutoLocker'. I could find the example of 'using' in C+11 guide. This conforms the style. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:137: // } On 2015/10/21 at 23:14:18, Raymond Toy wrote: > You forgot to remove this. :-) Oops. Sorry. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:187: return; On 2015/10/21 at 23:14:18, Raymond Toy wrote: > Would it make more sense if checkSuspendsAndRender actually called suspendOfflineRendering itself? Works fine with me. It makes more sense if we change the name to 'renderIfNonSuspended'. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:90: bool checkSuspendsAndRender(AudioBus* sourceBus, AudioBus* destinationBus, size_t numberOfFrames); On 2015/10/21 at 23:14:18, Raymond Toy wrote: > Not sure, but maybe renderIfPossible or renderIfNotSuspended is a better name? Changing this to renderIfNotSuspended().
https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:15: PASS Calling resume on context without any previous suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). On 2015/10/22 at 18:23:48, hoch wrote: > On 2015/10/21 at 18:22:45, Raymond Toy wrote: > > The description doesn't seem to match the InvalidStateError message. What is really supposed to be tested here? > > It tries to resume a context that has not started. Throwing InvalidStateError per the current spec. > > http://webaudio.github.io/web-audio-api/#methods-1 The description is "resume on context without previous suspend". The error message is "cannot resume a context that has not been started". The context has been started, right? Perhaps the error message should include the current state. That might make it clearer why you can't resume. Basically, you can only resume if the state is "suspended", right? https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:106: 'suspend-invalid-argument', On 2015/10/22 at 18:23:48, hoch wrote: > On 2015/10/21 at 18:22:45, Raymond Toy wrote: > > I wouldn't list the individual tasks. > > To be honest, I prefer to keep individual tasks, since it is convenient enable/disable each task by commenting in/out. It is really helpful when pinpointing a failing test. Ok. It goes both ways: If I add a new test, it won't get run until I remember to add it. Which I always forget to do. :-) https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:79: using OfflineAutoLocker = DeferredTaskHandler::OfflineAutoLocker; On 2015/10/22 at 18:23:49, hoch wrote: > On 2015/10/21 at 23:14:18, Raymond Toy wrote: > > What is the style on using "using"? > > I simply followed the example of 'AutoLocker'. I could find the example of 'using' in C+11 guide. This conforms the style. Follow this then: https://www.chromium.org/blink/coding-style#TOC-using-Statements
https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:68: // rather than accidentally blocking the audio thread. This comment kind of doesn't make sense. You say it's safe to lock from the audio thread, but there's a RELEASE_ASSERT. This can't really both be true. I would fix the comment and remove the "safe to lock" stuff. https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:113: } I think the message you're producing is: "failed to create a target buffer for offline rendering nc, nf)" That seems wrong.
https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:15: PASS Calling resume on context without any previous suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). On 2015/10/22 18:41:44, Raymond Toy wrote: > On 2015/10/22 at 18:23:48, hoch wrote: > > On 2015/10/21 at 18:22:45, Raymond Toy wrote: > > > The description doesn't seem to match the InvalidStateError message. What > is really supposed to be tested here? > > > > It tries to resume a context that has not started. Throwing InvalidStateError > per the current spec. > > > > http://webaudio.github.io/web-audio-api/#methods-1 > > The description is "resume on context without previous suspend". The error > message is "cannot resume a context that has not been started". The context has > been started, right? No. This is a separate test from others. It create a new OfflineAudioContext and resume it right away. So when this check runs, the context is not started, thus the message is correct. > > Perhaps the error message should include the current state. That might make it > clearer why you can't resume. Basically, you can only resume if the state is > "suspended", right? Printing out the current state is not really helpful either because the context is initially 'suspended.' I am changing the description and the comment for this test task, so it makes sense. A nice catch! https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:106: 'suspend-invalid-argument', On 2015/10/22 18:41:44, Raymond Toy wrote: > On 2015/10/22 at 18:23:48, hoch wrote: > > On 2015/10/21 at 18:22:45, Raymond Toy wrote: > > > I wouldn't list the individual tasks. > > > > To be honest, I prefer to keep individual tasks, since it is convenient > enable/disable each task by commenting in/out. It is really helpful when > pinpointing a failing test. > > Ok. It goes both ways: If I add a new test, it won't get run until I remember > to add it. Which I always forget to do. :-) Acknowledged. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:79: using OfflineAutoLocker = DeferredTaskHandler::OfflineAutoLocker; On 2015/10/22 18:41:44, Raymond Toy wrote: > On 2015/10/22 at 18:23:49, hoch wrote: > > On 2015/10/21 at 23:14:18, Raymond Toy wrote: > > > What is the style on using "using"? > > > > I simply followed the example of 'AutoLocker'. I could find the example of > 'using' in C+11 guide. This conforms the style. > > Follow this then: > https://www.chromium.org/blink/coding-style#TOC-using-Statements Done. https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:68: // rather than accidentally blocking the audio thread. On 2015/10/22 18:49:47, Raymond Toy wrote: > This comment kind of doesn't make sense. You say it's safe to lock from the > audio thread, but there's a RELEASE_ASSERT. This can't really both be true. I > would fix the comment and remove the "safe to lock" stuff. Done. https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:113: } On 2015/10/22 18:49:47, Raymond Toy wrote: > I think the message you're producing is: > > "failed to create a target buffer for offline rendering nc, nf)" > > That seems wrong. Oops. Sorry.
lgtm with nits. https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:70: "DeferredTaskHandler::offlineLock() should be called within the audio thread."); Nit: "should be called" -> "must be called" And "the audio thread" -> "offline audio thread". Just to be explicit. https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:112: ")")); Nit: If you're going to do this, I would propose this: "failed to create OfflineAudioContext(nc, nf, rate)" What you have has little context on what the numbers mean. Instead of this, you can explicitly say something like "offline rendering of nc channels of nf frames"
Thanks for your patience. I will loop Oilpan people in from now on. https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:90: // reaches to 0.5 second even before the resume here. On 2015/10/21 23:14:18, Raymond Toy wrote: > Nit: "0.5 second" is wrong. Do we really need this? If yes, maybe schedule > the suspend at the latest possible time to minimize the possibility of the race. Done. https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:70: "DeferredTaskHandler::offlineLock() should be called within the audio thread."); On 2015/10/22 20:27:17, Raymond Toy wrote: > Nit: "should be called" -> "must be called" > And "the audio thread" -> "offline audio thread". > > Just to be explicit. Done. https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:112: ")")); On 2015/10/22 20:27:17, Raymond Toy wrote: > Nit: If you're going to do this, I would propose this: > > "failed to create OfflineAudioContext(nc, nf, rate)" > > What you have has little context on what the numbers mean. Instead of this, you > can explicitly say something like "offline rendering of nc channels of nf > frames" Done. I took your first solution.
hongchan@chromium.org changed reviewers: + haraken@chromium.org, tkent@chromium.org
https://codereview.chromium.org/1405413004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:107: // it reaches to the suspend even before the resume here. Nit: "it reaches to the suspend" -> "it reaches the suspend"
Description was changed from ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ TODO: 1. Edit core/frame/UseCounter.h accordingly. 2. Add UMA metric in OfflineAudioContext.idl. Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html ========== to ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ TODO: 1. Edit core/frame/UseCounter.h accordingly. 2. Add UMA metric in OfflineAudioContext.idl. Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933, 545686 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html ==========
haraken@, tkent@ PTAL at the oilpan related parts. Here are the overview documentation (the design is slightly changed since you reviewed last time): https://docs.google.com/document/d/1VU4BZHcDPo4NymTBmnLo8GnIRpMAOsSgFh9CasgjL... In short, the current design exchanges the frame time stamp (size_t) instead of throwing an object back and forth between OfflineAudioContext and OfflineAudioDestinationHandler. As you already know, the OfflineAudioDestinatioHandler is not covered by the oilpan. https://codereview.chromium.org/1405413004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:107: // it reaches to the suspend even before the resume here. On 2015/10/22 22:11:00, Raymond Toy wrote: > Nit: "it reaches to the suspend" -> "it reaches the suspend" Done.
hongchan@chromium.org changed reviewers: + yhirano@chromium.org
hongchan@chromium.org changed reviewers: + hiroshige@chromium.org
yhirano@ This CL contains JS methods return Promise. Could you please take a look? hiroshige@ Last time you reviewed the oilpan part of the previous attempt of this CL. Could you take a look this one as well?
https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:172: virtual ScriptPromise suspendContext(ScriptState*, double) = 0; Why do we need to define this function here? Isn't just defining it in OfflineAudioContext enough? https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:139: return ScriptPromise::rejectWithDOMException( RELEASE_ASSERT always takes effect, so L139-L142 does not run. Just returning ScriptPromise() may be enough. https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:301: RELEASE_ASSERT always takes effect, so L139-L142 does not run. Just returning ScriptPromise() may be enough.
https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:172: virtual ScriptPromise suspendContext(ScriptState*, double) = 0; On 2015/11/06 08:13:29, yhirano wrote: > Why do we need to define this function here? Isn't just defining it in > OfflineAudioContext enough? Done. https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:139: return ScriptPromise::rejectWithDOMException( On 2015/11/06 08:13:29, yhirano wrote: > RELEASE_ASSERT always takes effect, so L139-L142 does not run. Just returning > ScriptPromise() may be enough. Since we don't need to define this at all here, I am going to remove this method as whole. https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:301: On 2015/11/06 08:13:29, yhirano wrote: > RELEASE_ASSERT always takes effect, so L139-L142 does not run. Just returning > ScriptPromise() may be enough. Done.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Promise usage lgtm https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:21: // reject the promise. or less than or equal to the current time? https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:43: context.resume(); return needed?
Promise usage lgtm https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:21: // reject the promise. or less than or equal to the current time? https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:43: context.resume(); return needed?
Promise usage lgtm
https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:21: // reject the promise. On 2015/11/11 16:10:55, yhirano wrote: > or less than or equal to the current time? Here we are not testing 'less than or equal to' case. I added the missing case to the comment. https://codereview.chromium.org/1405413004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:43: context.resume(); On 2015/11/11 16:10:55, yhirano wrote: > return needed? No, Should().beResolved() is expecting the resolution of 'context.suspend()'. Returning context.resume() is not necessary here.
hongchan@chromium.org changed reviewers: - hiroshige@chromium.org
hakaren@ This feature will be tremendously helpful in resolving a couple of flaky tests in Web Audio API, so could you take a look?
Mostly looks good. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioNode.h:236: AbstractAudioContext* m_context; Why do we need to move m_context to a protected member? As mentioned in the comment, m_context must be accessed via context(). https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:241: Nit: Remove the empty line. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:117: m_destinationNode = OfflineAudioDestinationNode::create(this, m_renderTarget.get()); if (renderTarget) { m_destinationNode = ... } else { exceptionState.throwRangeError(...) } https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:258: AutoLocker locker(this); Don't you need to acquire the lock before looking up m_scheduledSuspends.contains(frame) at line 248? https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:292: Can we add ASSERT(contextState() == AudioContextState::Suspended)? https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:294: // "Running." and calling startRendering(). Note that resuming is possible "Running". https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:377: AutoLocker locker(this); I guess we need to acquire the lock before calling m_scheduledSuspends.contains(). https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:389: if (m_scheduledSuspends.contains(currentSampleFrame())) Don't we need to acquire OfflineGraphAutoLocker locker(this)? I think all operations on m_scheduledSuspends should be protected by a lock. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:30: Nit: Remove the empty line. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:98: // The map is consist of key-value pairs of: The map consists of
https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioNode.h:236: AbstractAudioContext* m_context; On 2015/11/17 08:28:10, haraken wrote: > > Why do we need to move m_context to a protected member? As mentioned in the > comment, m_context must be accessed via context(). 1. OfflineAudioDestinationNode (inherited from AudioNode) needs to access m_context, 2. Because its override method context() returns OfflineAudioContext* instead of AbstractAudioContext*. (See line 110) If you have a better way to structure this, I can certainly change this. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:241: On 2015/11/17 08:28:10, haraken wrote: > > Nit: Remove the empty line. Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:117: m_destinationNode = OfflineAudioDestinationNode::create(this, m_renderTarget.get()); On 2015/11/17 08:28:10, haraken wrote: > > if (renderTarget) { > m_destinationNode = ... > } else { > exceptionState.throwRangeError(...) > } Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:258: AutoLocker locker(this); On 2015/11/17 08:28:10, haraken wrote: > > Don't you need to acquire the lock before looking up > m_scheduledSuspends.contains(frame) at line 248? Yes, you are correct. Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:292: On 2015/11/17 08:28:10, haraken wrote: > > Can we add ASSERT(contextState() == AudioContextState::Suspended)? Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:294: // "Running." and calling startRendering(). Note that resuming is possible On 2015/11/17 08:28:10, haraken wrote: > > "Running". Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:377: AutoLocker locker(this); On 2015/11/17 08:28:10, haraken wrote: > > I guess we need to acquire the lock before calling > m_scheduledSuspends.contains(). Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:389: if (m_scheduledSuspends.contains(currentSampleFrame())) On 2015/11/17 08:28:10, haraken wrote: > > Don't we need to acquire OfflineGraphAutoLocker locker(this)? > > I think all operations on m_scheduledSuspends should be protected by a lock. This method only gets called when the lock is in place. See line 339. I know this is not the most elegant design, so perhaps I can add some comments here that it needs a lock to before accessing m_scheduledSuspends. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:30: On 2015/11/17 08:28:10, haraken wrote: > > Nit: Remove the empty line. Done. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:98: // The map is consist of key-value pairs of: On 2015/11/17 08:28:10, haraken wrote: > > The map consists of Oops. Done.
https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioNode.h:236: AbstractAudioContext* m_context; On 2015/11/17 18:11:44, hoch wrote: > On 2015/11/17 08:28:10, haraken wrote: > > > > Why do we need to move m_context to a protected member? As mentioned in the > > comment, m_context must be accessed via context(). > > 1. OfflineAudioDestinationNode (inherited from AudioNode) needs to access > m_context, > 2. Because its override method context() returns OfflineAudioContext* instead of > AbstractAudioContext*. (See line 110) > > If you have a better way to structure this, I can certainly change this. - Rename OfflineAudioDestinationNode::context() to OfflineAudioDestinationNode::offlineAudioContext(). - Keep AudioNode::context() non-virtual. - OfflineAudioDestinationNode::offlineAudioContext() calls static_cast<OfflineAudioContext*>(context()). ?
https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioNode.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioNode.h:236: AbstractAudioContext* m_context; On 2015/11/18 00:20:26, haraken wrote: > On 2015/11/17 18:11:44, hoch wrote: > > On 2015/11/17 08:28:10, haraken wrote: > > > > > > Why do we need to move m_context to a protected member? As mentioned in the > > > comment, m_context must be accessed via context(). > > > > 1. OfflineAudioDestinationNode (inherited from AudioNode) needs to access > > m_context, > > 2. Because its override method context() returns OfflineAudioContext* instead > of > > AbstractAudioContext*. (See line 110) > > > > If you have a better way to structure this, I can certainly change this. > > - Rename OfflineAudioDestinationNode::context() to > OfflineAudioDestinationNode::offlineAudioContext(). > > - Keep AudioNode::context() non-virtual. > > - OfflineAudioDestinationNode::offlineAudioContext() calls > static_cast<OfflineAudioContext*>(context()). > > ? Yes, it is obvious thing to do - but 'context()' is used at so many places in OfflineAudioDestinationNode. So I did not want to replace all the instances of 'context()' with something new, mainly to keep the render process same. I do not have a strong opinion on this, so I can fix it if you prefer to have this change.
https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:103: SuspendMap m_scheduledSuspends; Since this needs to be locked (as haraken pointed out), can you add a comment here that it must be protected by the context lock(?)
On 2015/11/18 17:23:48, hoch wrote: > https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AudioNode.h (right): > > https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioNode.h:236: > AbstractAudioContext* m_context; > On 2015/11/18 00:20:26, haraken wrote: > > On 2015/11/17 18:11:44, hoch wrote: > > > On 2015/11/17 08:28:10, haraken wrote: > > > > > > > > Why do we need to move m_context to a protected member? As mentioned in > the > > > > comment, m_context must be accessed via context(). > > > > > > 1. OfflineAudioDestinationNode (inherited from AudioNode) needs to access > > > m_context, > > > 2. Because its override method context() returns OfflineAudioContext* > instead > > of > > > AbstractAudioContext*. (See line 110) > > > > > > If you have a better way to structure this, I can certainly change this. > > > > - Rename OfflineAudioDestinationNode::context() to > > OfflineAudioDestinationNode::offlineAudioContext(). > > > > - Keep AudioNode::context() non-virtual. > > > > - OfflineAudioDestinationNode::offlineAudioContext() calls > > static_cast<OfflineAudioContext*>(context()). > > > > ? > > Yes, it is obvious thing to do - but 'context()' is used at so many places in > OfflineAudioDestinationNode. So I did not want to replace all the instances of > 'context()' with something new, mainly to keep the render process same. > > I do not have a strong opinion on this, so I can fix it if you prefer to have > this change. Fair enough :) Thanks for the clarification. LGTM.
tkent@ PTAL. This contains API changes in WebAudio. The Oilpan part is reviewed by haraken@, and the promise part is reviewed by yhirano@. https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1405413004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.h:103: SuspendMap m_scheduledSuspends; On 2015/11/18 21:40:41, Raymond Toy wrote: > Since this needs to be locked (as haraken pointed out), can you add a comment > here that it must be protected by the context lock(?) Done.
Web-exposed change LGTM. Intent to ship thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qvjMs5umAAY
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Although the TSAN test failed, the failure seems to be irrelevant to this CL. I'll be waiting 2 more days and will commit the CL.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org, yhirano@chromium.org, haraken@chromium.org, tkent@chromium.org Link to the patchset: https://codereview.chromium.org/1405413004/#ps300001 (title: "Updating UseCounter.h after L-G-T-M")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, haraken@chromium.org, yhirano@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/1405413004/#ps320001 (title: "Updating UseCounter.h after L-G-T-M")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405413004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405413004/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/cdc0e15efab0e82a6e95deaae11a394535ea82de Cr-Commit-Position: refs/heads/master@{#361367}
Message was sent while issue was closed.
Description was changed from ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ TODO: 1. Edit core/frame/UseCounter.h accordingly. 2. Add UMA metric in OfflineAudioContext.idl. Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933, 545686 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html Committed: https://crrev.com/cdc0e15efab0e82a6e95deaae11a394535ea82de Cr-Commit-Position: refs/heads/master@{#361367} ========== to ========== Implement suspend() and resume() for OfflineAudioContext NOTE: This is the second attempt of https://crrev.com/1140723003/ Adds suspend() and resume() feature in OfflineAudioContext to support the synchronous graph manipulation with the render block precision (k-rate) in the non-realtime audio rendering. The benefit of being able to suspend/resume the context with the render block precision is: 1) The audio graph can be modified in a time-accurate way, independent of the hardware. Without this, setTimeout, completion events, or state change events are needed to manipulate the graph, and the results depend on when the events are fired and on how fast the hardware is. 2) Makes an OfflineAudioContext more symmetrical to the AudioContext, which already supports suspend/resume. (There are minor difference required by the difference between offline and online contexts.) This feature also can be used in Blink layout tests to verify the behavior of audio rendering. With this feature in the implementation, several flaky web audio layout tests can be fixed. http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface https://github.com/WebAudio/web-audio-api/issues/302#issuecomment-106101885 BUG=497933, 545686 TEST= webaudio/offlineaudiocontext-suspend-resume-basic.html webaudio/offlineaudiocontext-suspend-resume-eventhandler.html webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html webaudio/offlineaudiocontext-suspend-resume-promise.html webaudio/offlineaudiocontext-suspend-resume-sequence.html Committed: https://crrev.com/cdc0e15efab0e82a6e95deaae11a394535ea82de Cr-Commit-Position: refs/heads/master@{#361367} ========== |
