Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1140723003:
Implement suspend() and resume() for OfflineAudioContext (Closed)
DescriptionNOTE: This CL is replaced by https://codereview.chromium.org/1405413004.
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.
# Editor's draft spec PR:
http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface
# Web Audio API issue tracker entry:
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
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199129
Patch Set 1 : Initial Design #
Total comments: 19
Patch Set 2 : Promise Resolution #
Total comments: 8
Patch Set 3 : Using while() instead of recursion #Patch Set 4 : Initial review + layout tests #
Total comments: 89
Patch Set 5 : Restructured the resolution of suspend promise #
Total comments: 1
Patch Set 6 : Ready for Review #
Total comments: 60
Patch Set 7 : Added UseCounter #
Total comments: 29
Patch Set 8 : Added a layout test for synchronous graph manipulation #
Total comments: 28
Patch Set 9 : Fixed a LO test and minor nits #
Total comments: 40
Patch Set 10 : Making Oilpan-compatible changes #
Total comments: 8
Patch Set 11 : Moving the suspend list manipulation to the render thread (WIP) #
Total comments: 7
Patch Set 12 : Resolving the render thread issue (WIP) #
Total comments: 3
Patch Set 13 : Revering oilpan-compatible changes + HashMap #
Total comments: 35
Patch Set 14 : Ready for Review (1) #
Total comments: 2
Patch Set 15 : Ready for Review (2) #
Total comments: 22
Patch Set 16 : Ready for Review (3): refactor and clean up #Patch Set 17 : Bring to ToT #
Total comments: 20
Patch Set 18 : Removing ThreadSafeRefCounted from ScheduledSuspendContainer #
Total comments: 3
Patch Set 19 : Allowing cross-thread access for ThreadSafeBind() #Patch Set 20 : Adapting CL to AbstractAudioContext #
Total comments: 31
Patch Set 21 : Fixing nits #
Total comments: 4
Patch Set 22 : Fixing nits (2) #Patch Set 23 : Adding missing pre/post render tasks + Fixing minor nits #
Total comments: 2
Patch Set 24 : Fixed minor nits after L-G-T-M #Patch Set 25 : Added RELEASE_ASSERT_NOT_REACHED() for non-reachable code paths #
Total comments: 2
Patch Set 26 : Introduced ThreadSafeRefCounted again #Patch Set 27 : Using raw pointer again with the manual reference management. #
Total comments: 15
Patch Set 28 : Adding RELEASE_ASSERT() when resolving/resjecting suspends. #Patch Set 29 : Bring ToT #Messages
Total messages: 138 (31 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
No rush - PTAL if you're interested.
Just a quick preliminary review. Looks good so far. We'll have to look over the corner cases and weird usages. Are you going to add support for suspend returning a promise with the rendered data from the last resume point? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:740: } Why? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.h (left): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.h:262: Member<AudioDestinationNode> m_destinationNode; Try not to move things around. It makes it hard to figure out what changed and this particular one looks like m_destinationNode was removed, but it was just moved above. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioContext.cpp:99: , m_suspendTime(-0.0) Why -0.0? This probably won't do what you think it does. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioContext.h:52: WillBeHeapVector<RefPtrWillBeMember<ScriptPromiseResolver>> m_offlineResumeResolvers; As discussed earlier, the online context is going to merge the suspend resume resolvers onto one list/vector so that they happen in order. Does that matter here? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:40: const size_t kRenderQuantumSize = 128; Don't rename. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:194: if (context()) How can the context go away? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.h (left): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.h:72: bool m_startedRendering; Changed the name? Why? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.h:83: size_t m_framesToProcess; Why is m_framesProcessed unsigned but m_framesToProcess size_t?
In the initial patch does not consider the partial render buffer. I have to think about creating new event (like completion event) with the buffer and the playback time. The main purpose of this first patch is to show and discuss the overall design. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:740: } On 2015/05/13 17:16:08, Raymond Toy wrote: > Why? Added some line breaks to make the code read better. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.h (left): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.h:262: Member<AudioDestinationNode> m_destinationNode; On 2015/05/13 17:16:08, Raymond Toy wrote: > Try not to move things around. It makes it hard to figure out what changed and > this particular one looks like m_destinationNode was removed, but it was just > moved above. Yes, this relocation was not necessary. However, there are things need to be moved because of the order of initializers. Some methods and variables should be accessed by OAC so they need to be in the protected state (from the private). This is just work-in-progress artifacts. Will be cleaned up. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioContext.cpp:99: , m_suspendTime(-0.0) On 2015/05/13 17:16:08, Raymond Toy wrote: > Why -0.0? This probably won't do what you think it does. What would be the proper 'null' value for this case? Actually it really doesn't matter since we have another flag for the validity of suspend time. Just want to ask your opinion. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioContext.h:52: WillBeHeapVector<RefPtrWillBeMember<ScriptPromiseResolver>> m_offlineResumeResolvers; On 2015/05/13 17:16:08, Raymond Toy wrote: > As discussed earlier, the online context is going to merge the suspend resume > resolvers onto one list/vector so that they happen in order. Does that matter > here? Yes. That makes sense. These are actually not used in this patch at all - they are just placeholders. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:40: const size_t kRenderQuantumSize = 128; On 2015/05/13 17:16:08, Raymond Toy wrote: > Don't rename. renderQuantumSize does look like a local variable. Adding 'k' at the head seems to be reasonable and helpful. Is there any other way to express this variable is a constant for this class? https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:194: if (context()) On 2015/05/13 17:16:08, Raymond Toy wrote: > How can the context go away? I simply used the code below. I should think about corner cases for this. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.h:83: size_t m_framesToProcess; On 2015/05/13 17:16:08, Raymond Toy wrote: > Why is m_framesProcessed unsigned but m_framesToProcess size_t? I just followed the previous implementation. Should I change?
Patchset #2 (id:20001) has been deleted
PS2 is crashing audiocontext-suspend-resume.html and I am working on it. rtoy@ suggested to programmtically compute the expected suspend time data rather than using the numbers from the console.
Perhaps there should also be a short test that promises and events are handled properly when both are used. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:40: const size_t kRenderQuantumSize = 128; On 2015/05/13 17:30:53, hoch wrote: > On 2015/05/13 17:16:08, Raymond Toy wrote: > > Don't rename. > > renderQuantumSize does look like a local variable. Adding 'k' at the head seems > to be reasonable and helpful. Is there any other way to express this variable is > a constant for this class? Blink style guide doesn't seem to say, but I think the chromium style is start with k. This isn't really relevant to this CL. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.h:83: size_t m_framesToProcess; On 2015/05/13 17:30:53, hoch wrote: > On 2015/05/13 17:16:08, Raymond Toy wrote: > > Why is m_framesProcessed unsigned but m_framesToProcess size_t? > > I just followed the previous implementation. Should I change? IIUC, m_framesProcessed counts up from 0 to the length of the offline context. If so, I think it should be the same type as m_framesToProcess which seems to count down from the the offline context length.
Just some random comments. We'll need to fix this eventually, but not right away. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:107: : m_when(when) Shouldn't this quantize the time to the rendering quantum start time? https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:228: m_resumeResolvers.append(resolver); Not sure, but I think you can resolve the promise here immediately. It's not like the online context where we want to wait for the HW to restart. We're already restarted as soon as startRendering() is called, right? Allows us to get rid of the vector. Or is it really needed in some situation? https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.h:57: bool shouldBeSuspendedNow(double currentTime) const; Add comment on what this is supposed to do. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.h:70: // Unlike the design of real-time audio context, we keep suspend/resume This comment is wrong now since we're not using just one. Online context only has a list for resume. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:107: // TO FIX: is there any corner case for this? I think blink style is FIXME.
Note that this PS made layout tests flaky. It fails 3~4 out of 10 iterations - maybe it might have been that way in PS1 since I have never ran the layout test multiple iteration. Will fix the issue with PS4. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:40: const size_t kRenderQuantumSize = 128; On 2015/05/19 21:46:40, Raymond Toy wrote: > On 2015/05/13 17:30:53, hoch wrote: > > On 2015/05/13 17:16:08, Raymond Toy wrote: > > > Don't rename. > > > > renderQuantumSize does look like a local variable. Adding 'k' at the head > seems > > to be reasonable and helpful. Is there any other way to express this variable > is > > a constant for this class? > > Blink style guide doesn't seem to say, but I think the chromium style is start > with k. > > This isn't really relevant to this CL. Acknowledged. Will change in PS4. https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/1/Source/modules/webaudio/Off... Source/modules/webaudio/OfflineAudioDestinationNode.h:83: size_t m_framesToProcess; On 2015/05/19 21:46:40, Raymond Toy wrote: > On 2015/05/13 17:30:53, hoch wrote: > > On 2015/05/13 17:16:08, Raymond Toy wrote: > > > Why is m_framesProcessed unsigned but m_framesToProcess size_t? > > > > I just followed the previous implementation. Should I change? > > IIUC, m_framesProcessed counts up from 0 to the length of the offline context. > If so, I think it should be the same type as m_framesToProcess which seems to > count down from the the offline context length. Done. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.cpp:228: m_resumeResolvers.append(resolver); On 2015/05/19 22:04:45, Raymond Toy wrote: > Not sure, but I think you can resolve the promise here immediately. It's not > like the online context where we want to wait for the HW to restart. We're > already restarted as soon as startRendering() is called, right? > > Allows us to get rid of the vector. > > Or is it really needed in some situation? I agree. This was just to make the structure symmetric. Will try the new idea in PS4. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioContext.h:57: bool shouldBeSuspendedNow(double currentTime) const; On 2015/05/19 22:04:45, Raymond Toy wrote: > Add comment on what this is supposed to do. Done. https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/40001/Source/modules/webaudio... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:107: // TO FIX: is there any corner case for this? On 2015/05/19 22:04:46, Raymond Toy wrote: > I think blink style is FIXME. Done.
PTAL - no rush.
https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt:17: PASS offlineContext.suspend() on a closed context rejected: InvalidStateError: cannot schedule a suspend in the past, beyond the total render duration or at the duplicate position. It would be nice if three different messages were sent with some information about suspend time, the current time (or render length). If we do, we need to think about whether the quantized time is used or the requested time. (Probably both?) https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:54: // OfflineAudioContext rejects the promise. This comment seems wrong now. suspend() is allowed for an offline context. That also implies that the code in lines 57-67 is probably wrong. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:77: // OfflineAudioContext rejects the promise. Since resume() is allowed now for an offline context, this comment seems wrong, and the code in lines 84-93 are probably wrong. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:110: offlineContext.suspend(1.0).then( Why 1.0 and not durationInSeconds? This seems to be a corner case too. If you want to suspend at time 1 and the duration is 1 sec, what should happen? I think maybe suspend should be handled first. When resumed, the context is then closed. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:44: context.suspend(1.0); Doesn't this produce an error because the suspend time is the same as the render length? https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:47: .beRejected().then(done); Should there also be a test for calling resume without a preceding suspend()? https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:7: PASS context.currentTime is equal to 0.09868480725623582. Might be nice to include the render quantum index to we can see this is incrementing in nice way. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:18: var scheduledSuspendTime = 0.1; I think you should compute scheduledSuspendTime from the sample rate and render quantum so that it definitely does not fall on a render boundary. (In case some one changes sampleRate to a different value.) https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:34: // When context.currentTime 0.1 > renderDuration, the promise should "currentTime 0.1"? https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:36: scheduledSuspendTime = context.currentTime + 0.1; This delta of 0.1 should also be computed so that it does (or does not) cause the suspend time to be on a rendering boundary. Or perhaps it doesn't matter? https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:48: context.startRendering(); If you really want to use oncomplete instead of a promise for offline context, say why. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:18: var scheduledSuspendTime = 0.1; Same comments here as for the suspend-resume-event test. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/r... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/r... LayoutTests/webaudio/resources/audio-testing.js:644: this._testFailed('resolved correctly. (with ' + err + ')'); You mean incorrectly? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:725: } This change isn't really necessary, is it? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:945: // This is only for the real-time context. Add assert for !isOfflineContext()? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:968: bool AudioContext::shouldSuspendNow() Can't this be a method just on an OfflineAudioContext? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:974: void AudioContext::fireCompletionEvent() Could this be moved to the OfflineAudioContext class? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (left): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:300: // Graph locking. Preserve these comments with the variable that you moved. Also line 308. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:227: virtual bool shouldSuspendNow(); Isn't this only used by an offline context? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:250: Member<AudioDestinationNode> m_destinationNode; I think an offline context should use destination() to get this, so this member variable doesn't need to be protected. Maybe we should also add isOfflineContext() method and renderTarget() method? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: virtual size_t quantizeTime(double) const { return 0; } I think this is too vague. Choose a more descriptive name. Maybe quantizeTimeToRenderingQuantum? Add some comment on what this really does. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:101: m_totalRenderFrames = numberOfFrames; Why not move this to the initializer part? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:116: size_t when, PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) I think frame is better than when. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:156: index)); I think we have a problem here. If the main thread is moving items around in m_scheduledSuspends, this loop is going to behave badly. Maybe the solution is for the main thread to deliver the promise, and just mark the element as having been handled. (By setting the time to negative or an additional boolean to say so.) Then the post render task can remove the handled suspends. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:194: // ASSERT(m_destinationNode); We'll have to fix this before landing this CL. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:220: // If the context state is valid (Suspended), star rendering and return I think the code is easier to understand if you handle the invalid state here and returned. "star" -> "start" https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:233: "invalid context state")); Use better error message saying why this is invalid. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:258: "cannot schedule a suspend in the past, beyond the total render duration or at the duplicate position.")); Can we provide more information here? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:270: if (contextState() == AudioContextState::Suspended && m_isRenderingStarted) { I think it's easier to understand if you reverse the logic and have the if handle the bad case. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:288: "cannot resume the context is already running or has not started")); Can we have a more specific message? We know exactly why this is wrong, so say exactly why. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:295: // FIXME: This process (removing suspend from the list) should be Why does this need fixing? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:63: static PassOwnPtr<ScheduledSuspendContainer> create(size_t, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); I think adding an argument variable for size_t would be useful. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:67: bool shouldSuspendAt(size_t) const; Add parameter name for size_t. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:72: ScheduledSuspendContainer(size_t, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); Add parameter name for size_t. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:74: // Specified suspend time in samples and associated promise resolver. This comment looks like it applies to m_when, which doesn't make sense. Simplify and move the last part to a new comment for m_resolver. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:75: size_t m_when; "when" is used for time (in sec) everywhere else. Maybe rename this to mention frame or rendering quantum or something to show that it's not in sec. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:80: // changes the state of context and then remove the promise from the list. "remove" -> "removes" https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:84: bool isValidToScheduleAt(size_t); Add parameter name. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState,ImplementedAs=suspendOfflineRendering] Promise<void> suspend(double suspendTime); Do we want to add a use counter for offline suspend/resume? That might be nice to know. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:114: size_t whenAsFrame = when * sampleRate(); Do we need an ASSERT(when >= 0)? This is going to work badly if when is negative. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:155: // resolves the suspend on the main thread if necessary. I think more description is needed here on how this works. From a quick glance, the loop ends and we're done rendering. It's not immediately obvious that we require the user to call resume to continue processing. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (left): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:72: bool m_startedRendering; Don't need this anymore? Or was this renamed to m_isRenderingStarted? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:57: // This should NOT be called within OfflineAudioContext. Explain why.
PTAL. No rush. PS6 is an intermediate patch to demonstrate the stability: - Promise rejection now have more descriptive message. - Fixed the problem in managing the resolution of a suspend promise. One important note is that context() in OfflineAudioDestination will return the AudioContext, not the OfflineAudioContext. So I had to create several public methods in AudioContext which are only implemented in OfflineAudioContext. If you have a better idea on this problem, please let me know! https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt:17: PASS offlineContext.suspend() on a closed context rejected: InvalidStateError: cannot schedule a suspend in the past, beyond the total render duration or at the duplicate position. On 2015/05/28 16:37:34, Raymond Toy wrote: > It would be nice if three different messages were sent with some information > about suspend time, the current time (or render length). > > If we do, we need to think about whether the quantized time is used or the > requested time. (Probably both?) Good idea. I will try to be more specific on these error messages. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:54: // OfflineAudioContext rejects the promise. On 2015/05/28 16:37:34, Raymond Toy wrote: > This comment seems wrong now. suspend() is allowed for an offline context. That > also implies that the code in lines 57-67 is probably wrong. Actually I am not quite sure if this whole test is appropriate. Now OAC has suspend/resume, so this whole test might be rewritten for AudioContext - and I am not sure how. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:44: context.suspend(1.0); On 2015/05/28 16:37:34, Raymond Toy wrote: > Doesn't this produce an error because the suspend time is the same as the render > length? Oops. This is a mistake. This should be removed. One more point - it should not throw the error since it will be earlier than 1.0 second when it is quantized. When the quantized time is equal to the render duration, it will reject the promise. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:47: .beRejected().then(done); On 2015/05/28 16:37:34, Raymond Toy wrote: > Should there also be a test for calling resume without a preceding suspend()? Removing the suspend above. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:7: PASS context.currentTime is equal to 0.09868480725623582. On 2015/05/28 16:37:34, Raymond Toy wrote: > Might be nice to include the render quantum index to we can see this is > incrementing in nice way. Should we check if those indices match with the computed ones? I think it is a bit excessive. Also not sure how to print out additional information as a result while not doing pass/fail check. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:18: var scheduledSuspendTime = 0.1; On 2015/05/28 16:37:34, Raymond Toy wrote: > I think you should compute scheduledSuspendTime from the sample rate and render > quantum so that it definitely does not fall on a render boundary. (In case some > one changes sampleRate to a different value.) It should not matter, I believe. It should do what it is supposed to do whether the suspend time falls into the boundary or somewhere else. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:34: // When context.currentTime 0.1 > renderDuration, the promise should On 2015/05/28 16:37:34, Raymond Toy wrote: > "currentTime 0.1"? Oops. This should read |context.currentTime + 0.1 > renderDuration|. Will fix it. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:36: scheduledSuspendTime = context.currentTime + 0.1; On 2015/05/28 16:37:34, Raymond Toy wrote: > This delta of 0.1 should also be computed so that it does (or does not) cause > the suspend time to be on a rendering boundary. Or perhaps it doesn't matter? I don't think it matters as the comment above. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:48: context.startRendering(); On 2015/05/28 16:37:34, Raymond Toy wrote: > If you really want to use oncomplete instead of a promise for offline context, > say why. This test is about testing all event handlers are working properly. The CL changed the code position for oncomplete event handler and I just wanted to show that everything works as intended. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:18: var scheduledSuspendTime = 0.1; On 2015/05/28 16:37:34, Raymond Toy wrote: > Same comments here as for the suspend-resume-event test. Acknowledged. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/r... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/r... LayoutTests/webaudio/resources/audio-testing.js:644: this._testFailed('resolved correctly. (with ' + err + ')'); On 2015/05/28 16:37:35, Raymond Toy wrote: > You mean incorrectly? Oops. Will fix it. https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/r... LayoutTests/webaudio/resources/audio-testing.js:656: this._testFailed('resolved correctly.'); Will fix this too. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:725: } On 2015/05/28 16:37:35, Raymond Toy wrote: > This change isn't really necessary, is it? No, I am reverting this in PS6. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:945: // This is only for the real-time context. On 2015/05/28 16:37:35, Raymond Toy wrote: > Add assert for !isOfflineContext()? Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:968: bool AudioContext::shouldSuspendNow() On 2015/05/28 16:37:35, Raymond Toy wrote: > Can't this be a method just on an OfflineAudioContext? The problem is OAC is a very thin wrapper for AC. So calling context() inside of OfflineAudioDestintion always goes to AudioContext first. That's why I made this as public/virtual so I can override in OfflineAudioContext. Does it make sense? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:974: void AudioContext::fireCompletionEvent() On 2015/05/28 16:37:35, Raymond Toy wrote: > Could this be moved to the OfflineAudioContext class? The same reason above. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (left): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:300: // Graph locking. On 2015/05/28 16:37:35, Raymond Toy wrote: > Preserve these comments with the variable that you moved. Also line 308. Will do! https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:227: virtual bool shouldSuspendNow(); On 2015/05/28 16:37:35, Raymond Toy wrote: > Isn't this only used by an offline context? This is used by OfflineAudioDestinatioNode. That's why I made it public. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:250: Member<AudioDestinationNode> m_destinationNode; On 2015/05/28 16:37:35, Raymond Toy wrote: > I think an offline context should use destination() to get this, so this member > variable doesn't need to be protected. Maybe we should also add > isOfflineContext() method and renderTarget() method? Seems like a good idea. Will look into it. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: virtual size_t quantizeTime(double) const { return 0; } On 2015/05/28 16:37:35, Raymond Toy wrote: > I think this is too vague. Choose a more descriptive name. Maybe > quantizeTimeToRenderingQuantum? Add some comment on what this really does. Yes, I agree. Will fix it. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:101: m_totalRenderFrames = numberOfFrames; On 2015/05/28 16:37:35, Raymond Toy wrote: > Why not move this to the initializer part? Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:116: size_t when, PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver) On 2015/05/28 16:37:35, Raymond Toy wrote: > I think frame is better than when. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:156: index)); On 2015/05/28 16:37:36, Raymond Toy wrote: > I think we have a problem here. If the main thread is moving items around in > m_scheduledSuspends, this loop is going to behave badly. > > Maybe the solution is for the main thread to deliver the promise, and just mark > the element as having been handled. (By setting the time to negative or an > additional boolean to say so.) > > Then the post render task can remove the handled suspends. Done. However, the post render task happens every render quantum and we don't want that here. We should resolve the pending suspend promise only when the context is actually suspended. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:194: // ASSERT(m_destinationNode); On 2015/05/28 16:37:36, Raymond Toy wrote: > We'll have to fix this before landing this CL. Acknowledged. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:220: // If the context state is valid (Suspended), star rendering and return On 2015/05/28 16:37:35, Raymond Toy wrote: > I think the code is easier to understand if you handle the invalid state here > and returned. > > "star" -> "start" Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:233: "invalid context state")); On 2015/05/28 16:37:35, Raymond Toy wrote: > Use better error message saying why this is invalid. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:258: "cannot schedule a suspend in the past, beyond the total render duration or at the duplicate position.")); On 2015/05/28 16:37:36, Raymond Toy wrote: > Can we provide more information here? I broke this down into multiple checks and rejections. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:270: if (contextState() == AudioContextState::Suspended && m_isRenderingStarted) { On 2015/05/28 16:37:35, Raymond Toy wrote: > I think it's easier to understand if you reverse the logic and have the if > handle the bad case. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:288: "cannot resume the context is already running or has not started")); On 2015/05/28 16:37:36, Raymond Toy wrote: > Can we have a more specific message? We know exactly why this is wrong, so say > exactly why. I separate this into two checks and rejections as well. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:295: // FIXME: This process (removing suspend from the list) should be On 2015/05/28 16:37:35, Raymond Toy wrote: > Why does this need fixing? First, the location of this comment is wrong, so I moved to the line 306. Also I restructured the promise resolution so we don't have the issue discussed above. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:63: static PassOwnPtr<ScheduledSuspendContainer> create(size_t, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); On 2015/05/28 16:37:36, Raymond Toy wrote: > I think adding an argument variable for size_t would be useful. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:67: bool shouldSuspendAt(size_t) const; On 2015/05/28 16:37:36, Raymond Toy wrote: > Add parameter name for size_t. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:72: ScheduledSuspendContainer(size_t, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); On 2015/05/28 16:37:36, Raymond Toy wrote: > Add parameter name for size_t. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:74: // Specified suspend time in samples and associated promise resolver. On 2015/05/28 16:37:36, Raymond Toy wrote: > This comment looks like it applies to m_when, which doesn't make sense. > Simplify and move the last part to a new comment for m_resolver. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:75: size_t m_when; On 2015/05/28 16:37:36, Raymond Toy wrote: > "when" is used for time (in sec) everywhere else. Maybe rename this to mention > frame or rendering quantum or something to show that it's not in sec. I used m_whenAsFrame once, but thought it was too long. Will think about it. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:80: // changes the state of context and then remove the promise from the list. On 2015/05/28 16:37:36, Raymond Toy wrote: > "remove" -> "removes" Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:84: bool isValidToScheduleAt(size_t); On 2015/05/28 16:37:36, Raymond Toy wrote: > Add parameter name. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState,ImplementedAs=suspendOfflineRendering] Promise<void> suspend(double suspendTime); On 2015/05/28 16:37:36, Raymond Toy wrote: > Do we want to add a use counter for offline suspend/resume? That might be nice > to know. Should it be a separate CL? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:114: size_t whenAsFrame = when * sampleRate(); On 2015/05/28 16:37:36, Raymond Toy wrote: > Do we need an ASSERT(when >= 0)? This is going to work badly if when is > negative. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:155: // resolves the suspend on the main thread if necessary. On 2015/05/28 16:37:36, Raymond Toy wrote: > I think more description is needed here on how this works. From a quick glance, > the loop ends and we're done rendering. It's not immediately obvious that we > require the user to call resume to continue processing. Done. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (left): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:72: bool m_startedRendering; On 2015/05/28 16:37:37, Raymond Toy wrote: > Don't need this anymore? Or was this renamed to m_isRenderingStarted? This is removed. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:57: // This should NOT be called within OfflineAudioContext. On 2015/05/28 16:37:37, Raymond Toy wrote: > Explain why. Done.
https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:44: context.suspend(1.0); On 2015/06/09 20:49:58, hoch wrote: > On 2015/05/28 16:37:34, Raymond Toy wrote: > > Doesn't this produce an error because the suspend time is the same as the > render > > length? > > Oops. This is a mistake. This should be removed. > > One more point - it should not throw the error since it will be earlier than 1.0 > second when it is quantized. When the quantized time is equal to the render > duration, it will reject the promise. Oh, yeah, 1.0 isn't the start of a rendering quantum. This is rather messy. I wonder if things might be easier to understand of the sample rate were a multiple of 128? Then whole seconds are on exact boundaries, as are other fractional seconds. But not all. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:968: bool AudioContext::shouldSuspendNow() On 2015/06/09 20:49:58, hoch wrote: > On 2015/05/28 16:37:35, Raymond Toy wrote: > > Can't this be a method just on an OfflineAudioContext? > > The problem is OAC is a very thin wrapper for AC. So calling context() inside of > OfflineAudioDestintion always goes to AudioContext first. That's why I made this > as public/virtual so I can override in OfflineAudioContext. Does it make sense? Why not make context() be virtual? I don't know which would be better. https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:227: virtual bool shouldSuspendNow(); On 2015/06/09 20:49:59, hoch wrote: > On 2015/05/28 16:37:35, Raymond Toy wrote: > > Isn't this only used by an offline context? > > This is used by OfflineAudioDestinatioNode. That's why I made it public. Then why not make it a private method in OfflineAudioDestinationNode? https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:84: bool isValidToScheduleAt(size_t); On 2015/06/09 20:50:00, hoch wrote: > On 2015/05/28 16:37:36, Raymond Toy wrote: > > Add parameter name. > > Done. Where did isValidToScheduleAt go to? https://codereview.chromium.org/1140723003/diff/120001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/120001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:56: shouldBeType("p1", "Promise"); Oh, this test was always wrong?!?
Go ahead and add the use counter now.
On 2015/06/09 22:34:59, Raymond Toy wrote: > https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... > File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): > > https://codereview.chromium.org/1140723003/diff/100001/LayoutTests/webaudio/o... > LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:44: > context.suspend(1.0); > On 2015/06/09 20:49:58, hoch wrote: > > On 2015/05/28 16:37:34, Raymond Toy wrote: > > > Doesn't this produce an error because the suspend time is the same as the > > render > > > length? > > > > Oops. This is a mistake. This should be removed. > > > > One more point - it should not throw the error since it will be earlier than > 1.0 > > second when it is quantized. When the quantized time is equal to the render > > duration, it will reject the promise. > > Oh, yeah, 1.0 isn't the start of a rendering quantum. This is rather messy. I > wonder if things might be easier to understand of the sample rate were a > multiple of 128? Then whole seconds are on exact boundaries, as are other > fractional seconds. But not all. I think it should not matter no matter when I scheduled the suspend. We compute the quantized time correctly and everything falls into the place so there is no failure here. Or making it easier to understand is more important? I am not sure. > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > File Source/modules/webaudio/AudioContext.cpp (right): > > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > Source/modules/webaudio/AudioContext.cpp:968: bool > AudioContext::shouldSuspendNow() > On 2015/06/09 20:49:58, hoch wrote: > > On 2015/05/28 16:37:35, Raymond Toy wrote: > > > Can't this be a method just on an OfflineAudioContext? > > > > The problem is OAC is a very thin wrapper for AC. So calling context() inside > of > > OfflineAudioDestintion always goes to AudioContext first. That's why I made > this > > as public/virtual so I can override in OfflineAudioContext. Does it make > sense? > > Why not make context() be virtual? I don't know which would be better. Hmm. That might work. I will try that in the next patch set. Also we can have a discussion on the design of this whole thing (OAC - AC - OAD). > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > File Source/modules/webaudio/AudioContext.h (right): > > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > Source/modules/webaudio/AudioContext.h:227: virtual bool shouldSuspendNow(); > On 2015/06/09 20:49:59, hoch wrote: > > On 2015/05/28 16:37:35, Raymond Toy wrote: > > > Isn't this only used by an offline context? > > > > This is used by OfflineAudioDestinatioNode. That's why I made it public. > > Then why not make it a private method in OfflineAudioDestinationNode? Managing suspends should be a part of OfflineAudioContext. I would like to keep the boundary intact and logical as much as possible. If the 'virtual' on context() works, this might be cleaned up nicely. > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > File Source/modules/webaudio/OfflineAudioContext.h (right): > > https://codereview.chromium.org/1140723003/diff/100001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.h:84: bool > isValidToScheduleAt(size_t); > On 2015/06/09 20:50:00, hoch wrote: > > On 2015/05/28 16:37:36, Raymond Toy wrote: > > > Add parameter name. > > > > Done. > > Where did isValidToScheduleAt go to? Now it is broken down to separate checks and added to resumeOfflineRendering(). I think it is more verbose, but better. > https://codereview.chromium.org/1140723003/diff/120001/LayoutTests/webaudio/a... > File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): > > https://codereview.chromium.org/1140723003/diff/120001/LayoutTests/webaudio/a... > LayoutTests/webaudio/audiocontext-suspend-resume.html:56: shouldBeType("p1", > "Promise"); > Oh, this test was always wrong?!? I spent a very little time to check up on this test, because this test needs to be rewritten or restructured.
Patchset #7 (id:140001) has been deleted
Although the proper decoupling between AC and OAC is needed, I believe this CL is ready for the review. On local machine, now it passes all the layout test without any flakiness even with the option of '--iteration=5'. I believe fixing the pre/post render task locking issue helped greatly. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:108: // FIXME: What should be done to trace members in OfflineAudioContext? Oilpan team's opinion is needed here. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState,ImplementedAs=suspendOfflineRendering] Promise<void> suspend(double suspendTime); Should the use counter be a part of this CL as well?
https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.idl (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.idl:35: [CallWith=ScriptState,ImplementedAs=suspendOfflineRendering] Promise<void> suspend(double suspendTime); On 2015/06/12 17:51:44, hoch wrote: > Should the use counter be a part of this CL as well? I would do it now.
https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:45: // }).notThrow(); Remove the commented-out code. And update comment to say why we can't test resume. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:7: PASS Calling multiple suspends at the same rendering quantum rejected correctly (with InvalidStateError: cannot schedule a suspend at the duplicate time). Can you add the actual time here in the error message? https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:14: PASS context.suspend(scheduledSuspendTime) rejected correctly (with InvalidStateError: cannot schedule a suspend beyond the total render duration). Can the error message include the actual suspend time? And maybe duration? Can't remember what we decided on if we schedule a suspend at the end of the rendering duration. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:16: var sampleRate = renderQuantum * 100; Explain why the sampleRate is 12800 Hz. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:19: var suspendInterval = 0.25; Why 0.25? https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:30: // should be rejected and throw an exception. And throw exception? I think the comment is wrong. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:36: context.suspend(scheduledSuspendTime); Should this handle the promise in case it was rejected for some reason? I also think the code would be easier to understand if you did something like context.suspend(scheduledSuspendTime).then(function () {}, function (e) { if (scheduledSuspendTime >= renderDuration) { // print out appropriate message } else { // something } }); Not sure if this makes sense for the test or not. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:16: var sampleRate = renderQuantum * 100; Pretty much the same comments for this file as for the eventhandler test. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt:6: PASS Sorted suspend times array is identical to the array [0,0.25,0.5,0.75,1,1.25,1.75]. I think it might be nice to see PASS message from each suspension. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:19: // Hard-coded arbitrary suspend times. Add more comment. Presumably, they should be in a random order. Also, any reason why these are on a nice rendering quantum boundary? If so, add comment on why you want that. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/r... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/r... LayoutTests/webaudio/resources/audio-testing.js:639: // "PASS My promise resolved correctly." We should do this everywhere, but I think we should include a FAIL example in the comments too. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:851: if (!isOfflineContext() && tryLock()) { Add comment on why we have this test now for isOfflineContext()! https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:864: // If this is Offline Audio Context, simply lock the graph and do the tasks. I think this is wrong. If we're running online, and tryLock() fails, we do all the stuff below. We didn't do that before for online contexts. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:881: if (!isOfflineContext() && tryLock()) { Same comment as for handlePreRenderTasks(). And for lines 896ff below. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // FIXME: Refacotring needed. This method is only used by OfflineAudioContext. Typo: Refacotring. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:48: ASSERT(!isMainThread()); According to the comment, this is for a non-real-time audio thread. Shouldn't we assert for that? And test for it in line 49? https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:162: // that duplicate entries in the suspend list are prohibited so it returns Comment on who prohibits duplicate entries. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:218: // AutoLocker locker(this); Fix these. Does the crash still happen? https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:261: // AutoLocker locker(this); Remove this. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:281: "cannot schedule a suspend in the past")); Is there a test for this? https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:303: "cannot schedule a suspend at the duplicate time")); Include the time in the message. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:326: "cannot resume the context that is not suspended")); the -> a https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:342: setContextState(Running); Should the state be set for calling startRendering()? I think it should. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:77: // Mark the suspend as 'pending'. Describe better what pending means. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:157: // the render loop again from where it suspended. "it suspended" -> "it was suspended" https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:177: if (m_framesToProcess <= 0) { Isn't there a compiler warning here since m_framesToProcess is unsigned? https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:195: // Perform post-render tasks once more. Why do you think this is necessary? What processing do you think still needs to e done? And if you do, shouldn't this be done before calling notifyComplete? https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:61: // rounding it down to the nearest block boundary. "block" -> "rendering quantum"
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:45: // }).notThrow(); On 2015/06/12 21:11:36, Raymond Toy wrote: > Remove the commented-out code. And update comment to say why we can't test > resume. Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:7: PASS Calling multiple suspends at the same rendering quantum rejected correctly (with InvalidStateError: cannot schedule a suspend at the duplicate time). On 2015/06/12 21:11:36, Raymond Toy wrote: > Can you add the actual time here in the error message? Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:14: PASS context.suspend(scheduledSuspendTime) rejected correctly (with InvalidStateError: cannot schedule a suspend beyond the total render duration). On 2015/06/12 21:11:36, Raymond Toy wrote: > Can the error message include the actual suspend time? And maybe duration? > > Can't remember what we decided on if we schedule a suspend at the end of the > rendering duration. We agreed on: ASSERT(suspensionTime < renderDuration) Otherwise, it will reject the promise. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler-expected.txt:14: PASS context.suspend(scheduledSuspendTime) rejected correctly (with InvalidStateError: cannot schedule a suspend beyond the total render duration). On 2015/06/12 21:11:36, Raymond Toy wrote: > Can the error message include the actual suspend time? And maybe duration? > > Can't remember what we decided on if we schedule a suspend at the end of the > rendering duration. Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:30: // should be rejected and throw an exception. On 2015/06/12 21:11:36, Raymond Toy wrote: > And throw exception? I think the comment is wrong. Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:36: context.suspend(scheduledSuspendTime); On 2015/06/12 21:11:36, Raymond Toy wrote: > Should this handle the promise in case it was rejected for some reason? > > I also think the code would be easier to understand if you did something like > > context.suspend(scheduledSuspendTime).then(function () {}, > function (e) { > if (scheduledSuspendTime >= renderDuration) { > // print out appropriate message > } else { > // something > } > }); > > Not sure if this makes sense for the test or not. This test should be about the event handler. I think I will remove this part. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:16: var sampleRate = renderQuantum * 100; On 2015/06/12 21:11:36, Raymond Toy wrote: > Pretty much the same comments for this file as for the eventhandler test. Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence-expected.txt:6: PASS Sorted suspend times array is identical to the array [0,0.25,0.5,0.75,1,1.25,1.75]. On 2015/06/12 21:11:36, Raymond Toy wrote: > I think it might be nice to see PASS message from each suspension. Done. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:19: // Hard-coded arbitrary suspend times. On 2015/06/12 21:11:36, Raymond Toy wrote: > Add more comment. Presumably, they should be in a random order. > > Also, any reason why these are on a nice rendering quantum boundary? If so, add > comment on why you want that. I am a bit confused. Isn't this what we agreed on? Surely I can change it so they are randomly generated in runtime, but I would like to confirm. https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/r... File LayoutTests/webaudio/resources/audio-testing.js (right): https://codereview.chromium.org/1140723003/diff/160001/LayoutTests/webaudio/r... LayoutTests/webaudio/resources/audio-testing.js:639: // "PASS My promise resolved correctly." On 2015/06/12 21:11:36, Raymond Toy wrote: > We should do this everywhere, but I think we should include a FAIL example in > the comments too. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:851: if (!isOfflineContext() && tryLock()) { On 2015/06/12 21:11:36, Raymond Toy wrote: > Add comment on why we have this test now for isOfflineContext()! Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:864: // If this is Offline Audio Context, simply lock the graph and do the tasks. On 2015/06/12 21:11:36, Raymond Toy wrote: > I think this is wrong. If we're running online, and tryLock() fails, we do all > the stuff below. We didn't do that before for online contexts. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:881: if (!isOfflineContext() && tryLock()) { On 2015/06/12 21:11:36, Raymond Toy wrote: > Same comment as for handlePreRenderTasks(). And for lines 896ff below. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // FIXME: Refacotring needed. This method is only used by OfflineAudioContext. On 2015/06/12 21:11:36, Raymond Toy wrote: > Typo: Refacotring. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:48: ASSERT(!isMainThread()); On 2015/06/12 21:11:36, Raymond Toy wrote: > According to the comment, this is for a non-real-time audio thread. Shouldn't > we assert for that? And test for it in line 49? We can't use |isOfflineContext()| here. DTHandler does not have any clue on what type of context it is in. Please let me know if you have a better idea. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:162: // that duplicate entries in the suspend list are prohibited so it returns On 2015/06/12 21:11:37, Raymond Toy wrote: > Comment on who prohibits duplicate entries. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:218: // AutoLocker locker(this); On 2015/06/12 21:11:37, Raymond Toy wrote: > Fix these. Does the crash still happen? Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:261: // AutoLocker locker(this); On 2015/06/12 21:11:37, Raymond Toy wrote: > Remove this. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:281: "cannot schedule a suspend in the past")); On 2015/06/12 21:11:37, Raymond Toy wrote: > Is there a test for this? I will add one in |oac-suspend-resume-basic.html| https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:303: "cannot schedule a suspend at the duplicate time")); On 2015/06/12 21:11:37, Raymond Toy wrote: > Include the time in the message. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:326: "cannot resume the context that is not suspended")); On 2015/06/12 21:11:36, Raymond Toy wrote: > the -> a Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:342: setContextState(Running); On 2015/06/12 21:11:37, Raymond Toy wrote: > Should the state be set for calling startRendering()? I think it should. Can you elaborate? I can swap the order of the state setter and starting the renderer, but I am not sure if that is what you're asking. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:77: // Mark the suspend as 'pending'. On 2015/06/12 21:11:37, Raymond Toy wrote: > Describe better what pending means. Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:157: // the render loop again from where it suspended. On 2015/06/12 21:11:37, Raymond Toy wrote: > "it suspended" -> "it was suspended" Done. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:177: if (m_framesToProcess <= 0) { On 2015/06/12 21:11:37, Raymond Toy wrote: > Isn't there a compiler warning here since m_framesToProcess is unsigned? No. I haven't seen any warning. Also m_framesToProcess is size_t (which I believe it is identical to unsigned) https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:195: // Perform post-render tasks once more. On 2015/06/12 21:11:37, Raymond Toy wrote: > Why do you think this is necessary? What processing do you think still needs to > e done? And if you do, shouldn't this be done before calling notifyComplete? I just followed how it was structured before. I will swap the order, but I am not sure if this should be removed completely. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.h:61: // rounding it down to the nearest block boundary. On 2015/06/12 21:11:37, Raymond Toy wrote: > "block" -> "rendering quantum" Done.
hongchan@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:342: setContextState(Running); On 2015/06/15 18:40:46, hoch wrote: > On 2015/06/12 21:11:37, Raymond Toy wrote: > > Should the state be set for calling startRendering()? I think it should. > > Can you elaborate? I can swap the order of the state setter and starting the > renderer, but I am not sure if that is what you're asking. Pretty much. I think setting the Running state before calling startRendering makes sure we can never detect a non-Running state after rendering has started. https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/160001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:195: // Perform post-render tasks once more. On 2015/06/15 18:40:46, hoch wrote: > On 2015/06/12 21:11:37, Raymond Toy wrote: > > Why do you think this is necessary? What processing do you think still needs > to > > e done? And if you do, shouldn't this be done before calling notifyComplete? > > I just followed how it was structured before. I will swap the order, but I am > not sure if this should be removed completely. As I understand it, the offline render loop now calls post-render always, so this shouldn't be needed anymore because it's redundant, right? Nothing should happen. (That wasn't the case before were we weren't calling post-render in the render loop. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:20: // Task: Calling suspend with no argument or the negative time should "or the negative" -> "or negative" https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:22: audit.defineTask('suspend-no-argument', function (done) { Task name doesn't match the code anymore. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:35: context.suspend(context.currentTime - 0.1)).beRejected(); Did you forget to update the expected results? I couldn't find any thing mentioning suspending in the past. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:17: // The sample rate is multiple of the rendering quantum, so suspension You mean the suspension times in this test. Say that his is just a convenience and isn't necessary. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise-expected.txt (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise-expected.txt:14: PASS context.suspend(scheduledSuspendTime) rejected correctly (with InvalidStateError: cannot schedule a suspend at 2 seconds which is greater than or equal to the total render duration of 2). "duration of 2" -> "duration of 2 sec". https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:25: // in a random order in runtime. I wonder if it might be useful to have some randomly generated (Math.random) times. This is ok, as long as if the test fails we can print out the actual values used. Up to you. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:28: // in the render quantum boundary for the easier and intuitive "for the easier" -> "for easier" "intuitive" -> "more intuitive" https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:44: // which is pushed into |actualSuspendTimes| should be in the ascending There's no |actualSuspendTimes| variable here. I'd just say that the suspends must happen in ascending order. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:47: Should('The suspend time (' + currentSuspendIndex + ')', context.currentTime) Is printing currentSuspendIndex useful? I was actually expecting this in the output to be in a different order instead of sequential from 0. https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... Source/core/frame/UseCounter.h:739: OfflineAudioContextStartRendering = 835, What will this tell us? Will this essentially be the same as the number of offline contexts created - the number of times someone didn't call startRendering()? If so, do we care? https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:849: // If this is Offline Audio Context, simply lock the graph and do the tasks. You should probably mention why it's ok to wait for the lock. https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:859: } Previous version had this as part of the trylock below at line 863. Why this change? Does it not work? https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:217: // ASSERT(m_destinationNode); Does this still crash? If not, remove these lines. https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:279: " which is in the past")); Perhaps this is more helpful in debugging if we also included the current time in the message. https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:292: String::number(m_totalRenderFrames / sampleRate()))); I wonder if this would be clearer if we used frame numbers instead of sec. Plus the message is a little inconsistent with the condition of the if in line 285.
I've changed error messages to include more details on the timing information. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:20: // Task: Calling suspend with no argument or the negative time should On 2015/06/15 20:42:13, Raymond Toy wrote: > "or the negative" -> "or negative" Done. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:22: audit.defineTask('suspend-no-argument', function (done) { On 2015/06/15 20:42:13, Raymond Toy wrote: > Task name doesn't match the code anymore. Done. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:35: context.suspend(context.currentTime - 0.1)).beRejected(); On 2015/06/15 20:42:13, Raymond Toy wrote: > Did you forget to update the expected results? I couldn't find any thing > mentioning suspending in the past. Hmm. It is in there: PASS context.suspend(-1.0) rejected correctly (with InvalidStateError: negative suspend time is not allowed). https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:17: // The sample rate is multiple of the rendering quantum, so suspension On 2015/06/15 20:42:13, Raymond Toy wrote: > You mean the suspension times in this test. Say that his is just a convenience > and isn't necessary. Done. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise-expected.txt (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise-expected.txt:14: PASS context.suspend(scheduledSuspendTime) rejected correctly (with InvalidStateError: cannot schedule a suspend at 2 seconds which is greater than or equal to the total render duration of 2). On 2015/06/15 20:42:13, Raymond Toy wrote: > "duration of 2" -> "duration of 2 sec". Done. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:28: // in the render quantum boundary for the easier and intuitive On 2015/06/15 20:42:13, Raymond Toy wrote: > "for the easier" -> "for easier" > "intuitive" -> "more intuitive" Oops. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:44: // which is pushed into |actualSuspendTimes| should be in the ascending On 2015/06/15 20:42:13, Raymond Toy wrote: > There's no |actualSuspendTimes| variable here. I'd just say that the suspends > must happen in ascending order. Oops again. https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:47: Should('The suspend time (' + currentSuspendIndex + ')', context.currentTime) On 2015/06/15 20:42:13, Raymond Toy wrote: > Is printing currentSuspendIndex useful? I was actually expecting this in the > output to be in a different order instead of sequential from 0. I will think about this a bit more. https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... Source/core/frame/UseCounter.h:739: OfflineAudioContextStartRendering = 835, On 2015/06/15 20:42:13, Raymond Toy wrote: > What will this tell us? Will this essentially be the same as the number of > offline contexts created - the number of times someone didn't call > startRendering()? If so, do we care? In either cases, I would like to keep this in the list. Just to make everything accountable. https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:279: " which is in the past")); On 2015/06/15 20:42:13, Raymond Toy wrote: > Perhaps this is more helpful in debugging if we also included the current time > in the message. Done. https://codereview.chromium.org/1140723003/diff/200001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:292: String::number(m_totalRenderFrames / sampleRate()))); On 2015/06/15 20:42:13, Raymond Toy wrote: > I wonder if this would be clearer if we used frame numbers instead of sec. Plus > the message is a little inconsistent with the condition of the if in line 285. Done.
https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:35: context.suspend(context.currentTime - 0.1)).beRejected(); On 2015/06/15 21:44:20, hoch wrote: > On 2015/06/15 20:42:13, Raymond Toy wrote: > > Did you forget to update the expected results? I couldn't find any thing > > mentioning suspending in the past. > > Hmm. It is in there: > > PASS context.suspend(-1.0) rejected correctly (with InvalidStateError: negative > suspend time is not > allowed). Isn't that from line 26? I was expecting some output that said 'Scheduling a suspend in the past...', because that's what line 34 says. https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1140723003/diff/200001/Source/core/frame/UseC... Source/core/frame/UseCounter.h:739: OfflineAudioContextStartRendering = 835, On 2015/06/15 21:44:20, hoch wrote: > On 2015/06/15 20:42:13, Raymond Toy wrote: > > What will this tell us? Will this essentially be the same as the number of > > offline contexts created - the number of times someone didn't call > > startRendering()? If so, do we care? > > In either cases, I would like to keep this in the list. Just to make everything > accountable. Ok.
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/200001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:35: context.suspend(context.currentTime - 0.1)).beRejected(); On 2015/06/15 22:03:17, Raymond Toy wrote: > On 2015/06/15 21:44:20, hoch wrote: > > On 2015/06/15 20:42:13, Raymond Toy wrote: > > > Did you forget to update the expected results? I couldn't find any thing > > > mentioning suspending in the past. > > > > Hmm. It is in there: > > > > PASS context.suspend(-1.0) rejected correctly (with InvalidStateError: > negative > > suspend time is not > > allowed). > > Isn't that from line 26? I was expecting some output that said > > 'Scheduling a suspend in the past...', because that's what line 34 says. It was a stupid mistake of mine. I forgot to add the task at the bottom. My apology.
hongchan@chromium.org changed reviewers: + yhirano@chromium.org
haraken@ Could you take a look at Oilpan part in AudioContext and OfflineAudioContext? I've made some changes, and I believe the trace in OfflineAudioContext is not right. yhirano@ These new methods return a promise. Could you take a look at them?
Can you add an additional test with graph manipulation? Similar to the example you did that uncovered the issue with locking? I would suggest a constant buffer that is connected to the destination at a certain time and then disconnected. The test would verify the connection and disconnection happened at the correct times.
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
PTAL. Just added oac-suspend-resume-graph-manipulation.html It worked! Can't wait to resolve all the flaky tests!
https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:27: constantSource.buffer = dcOffsetBuffer; dcOffsetBuffer? How did this work? https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:60: // Split the rendered buffer into 3 segments: [0.0 ~ 1.0], [1.0 ~ 2.0] What does the ~ mean? Did you mean -?
https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:27: constantSource.buffer = dcOffsetBuffer; On 2015/06/16 17:12:23, Raymond Toy wrote: > dcOffsetBuffer? How did this work? I missed the variable name change. Fixing it. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:60: // Split the rendered buffer into 3 segments: [0.0 ~ 1.0], [1.0 ~ 2.0] On 2015/06/16 17:12:23, Raymond Toy wrote: > What does the ~ mean? Did you mean -? I replace this with the mathematical expression for inclusion/exclusion.
https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt:11: PASS Calling context.resume() after close() rejected correctly (with InvalidAccessError: cannot resume a closed AudioContext). We should probably make the error messages in line 10 and 11 more consistent. (Not in this CL, though.) https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:38: // Shell because it requires the physical audio device to run. This comment seems irrelevant to the following code. If you want to keep it, add another comment briefly describing the test in line 40-43. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:7: PASS context.suspend(-1.0) rejected correctly (with InvalidStateError: negative suspend time is not allowed). Would be nice to print out the requested suspend time in the error message. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:10: PASS Resuming before suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). This error message doesn't quite fit with the description. What is it that we are trying to test here? https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:46: Should('The suspend time (' + currentSuspendIndex + ')', context.currentTime) I don't think printing currentSuspendIndex is useful since the output is in sequential order. Can you print something better like suspendTimes[i] for the suspension that is happening? Then it's pretty obvious that the suspends are happening in the correct order. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:886: deferredTaskHandler().requestToDeleteHandlersOnMainThread(); It would be nice if the common code for the offline and online context were actually common. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:47: // This allows the regular lock in non-real-time audio thread. I think this can only be used with an online context. Is there any way we can check or assert that? If not, add comment saying this can only be used for an offline context. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:49: if (isMainThread()) { This is confusing. You ASSERT that it's not the main thread and then test that it is. I'd invert the if condition if possible. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:161: // Suspend if necessary and mark the associated promise as pending. Note Add note that pending promises will get resolved later https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:362: // Resolve any pending suspend and remove it from the list. Since we should never have more than one suspend at any time, and this gets called after every rendering quantum (if we have suspensions), we should only remove one item at most from the list, right? If so, should we have an assert for that? https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:191: context()->handlePostRenderTasks(); Is this necessary? Is there something that could have happened between the last call to render() and this that needs cleanup? It probably doesn't matter, but I'm curious to know if you see some case where something still needs to be handled.
Patchset #12 (id:330001) has been deleted
Patchset #12 (id:350001) has been deleted
Patchset #11 (id:310001) has been deleted
https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume-expected.txt:11: PASS Calling context.resume() after close() rejected correctly (with InvalidAccessError: cannot resume a closed AudioContext). On 2015/06/16 17:57:09, Raymond Toy wrote: > We should probably make the error messages in line 10 and 11 more consistent. > (Not in this CL, though.) Acknowledged. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... File LayoutTests/webaudio/audiocontext-suspend-resume.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/a... LayoutTests/webaudio/audiocontext-suspend-resume.html:38: // Shell because it requires the physical audio device to run. On 2015/06/16 17:57:09, Raymond Toy wrote: > This comment seems irrelevant to the following code. If you want to keep it, > add another comment briefly describing the test in line 40-43. Done. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:7: PASS context.suspend(-1.0) rejected correctly (with InvalidStateError: negative suspend time is not allowed). On 2015/06/16 17:57:09, Raymond Toy wrote: > Would be nice to print out the requested suspend time in the error message. Done. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:10: PASS Resuming before suspend rejected correctly (with InvalidStateError: cannot resume a context that has not started). On 2015/06/16 17:57:09, Raymond Toy wrote: > This error message doesn't quite fit with the description. What is it that we > are trying to test here? Good catch. I need to think about how this test will work. https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/300001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:46: Should('The suspend time (' + currentSuspendIndex + ')', context.currentTime) On 2015/06/16 17:57:09, Raymond Toy wrote: > I don't think printing currentSuspendIndex is useful since the output is in > sequential order. Can you print something better like suspendTimes[i] for the > suspension that is happening? Then it's pretty obvious that the suspends are > happening in the correct order. Acknowledged. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:886: deferredTaskHandler().requestToDeleteHandlersOnMainThread(); On 2015/06/16 17:57:09, Raymond Toy wrote: > It would be nice if the common code for the offline and online context were > actually common. Will refactor this. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:47: // This allows the regular lock in non-real-time audio thread. On 2015/06/16 17:57:09, Raymond Toy wrote: > I think this can only be used with an online context. Is there any way we can > check or assert that? If not, add comment saying this can only be used for an > offline context. No. This lock is only for offline audio context. My comment about the 'non-real-time' audio thread seems to be clear enough. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:49: if (isMainThread()) { On 2015/06/16 17:57:09, Raymond Toy wrote: > This is confusing. You ASSERT that it's not the main thread and then test that > it is. I'd invert the if condition if possible. I though we discussed this: for debug build, the assertion will fail if this gets called from the main thread. However, if that happens in the release build, we can't crash the browser. So we just do the best thing we can do - tryLock(). I just followed the logic of tryLock and inverted it. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:161: // Suspend if necessary and mark the associated promise as pending. Note On 2015/06/16 17:57:09, Raymond Toy wrote: > Add note that pending promises will get resolved later Done. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:362: // Resolve any pending suspend and remove it from the list. On 2015/06/16 17:57:09, Raymond Toy wrote: > Since we should never have more than one suspend at any time, and this gets > called after every rendering quantum (if we have suspensions), we should only > remove one item at most from the list, right? If so, should we have an assert > for that? As a safety net I loop through the whole list here, but you are correct. Adding an assertion seems like a good idea. https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:191: context()->handlePostRenderTasks(); On 2015/06/16 17:57:09, Raymond Toy wrote: > Is this necessary? Is there something that could have happened between the last > call to render() and this that needs cleanup? It probably doesn't matter, but > I'm curious to know if you see some case where something still needs to be > handled. I've thought about it and I think we should remove this. In the previous implementation, handling post render task depended on the tryLock() mechanism. It means there was a possibility of missing the chance to run the task. However, now we forced the OAC to execute the post render task and thus we don't need this anymore.
https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:47: // This allows the regular lock in non-real-time audio thread. On 2015/06/16 21:31:21, hoch wrote: > On 2015/06/16 17:57:09, Raymond Toy wrote: > > I think this can only be used with an online context. Is there any way we can > > check or assert that? If not, add comment saying this can only be used for an > > offline context. > > No. This lock is only for offline audio context. My comment about the > 'non-real-time' audio thread seems to be clear enough. Typo on my part. I meant offline. I think since it's called offline, replacing "non-real-time" to "offline" makes it clearer. The thread name itself says "offline", not "non-real-time" https://codereview.chromium.org/1140723003/diff/300001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:49: if (isMainThread()) { On 2015/06/16 21:31:20, hoch wrote: > On 2015/06/16 17:57:09, Raymond Toy wrote: > > This is confusing. You ASSERT that it's not the main thread and then test > that > > it is. I'd invert the if condition if possible. > > I though we discussed this: for debug build, the assertion will fail if this > gets called from the main thread. However, if that happens in the release build, > we can't crash the browser. So we just do the best thing we can do - tryLock(). > > I just followed the logic of tryLock and inverted it. As we discussed offline, this is ok. But now I wonder if the main thread needs to do a tryLock. The main thread is allowed to block, so why not just grab the lock anyway?
https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:236: m_completeResolver = ScriptPromiseResolver::create(scriptState); Shouldn't this statement be placed after the L239-L245 block? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:256: ASSERT(isMainThread()); if (state == closed) { return Promise.reject(...) } is needed? How about when it is already suspended?
Sorry about the review delay... https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:239: // FIXME: Refactoring needed. These are OfflineAudioContext-specific tasks. FIXME => TODO https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:243: Member<AudioBuffer> renderTarget() const { return m_renderTarget; } Member<AudioBuffer> => AudioBuffer* m_renderTarget => m_renderTarget.get() https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // FIXME: Refactoring needed. This method is only used by OfflineAudioContext. FIXME => TODO https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:51: m_contextGraphMutex.tryLock(); Two questions: - This can fail. Is it OK? - Why does the main thread need to use tryLock()? The main thread is not a realtime thread. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:108: // FIXME: What should be done to trace members in OfflineAudioContext? This trace method looks correct. You can just trace all members. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:191: if (!isMainThread()) Basically it is not a good idea to add both ASSERT(x()) and if (!x()). If !x() shouldn't happen, we should only have ASSERT(x()). The same comment for other places in this CL. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:357: AutoLocker locker(this); Why do you need this lock? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:362: // FIXME: is removing elements efficient? What if there are 10K suspends? FIXME => TODO https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:65: : public NoBaseWillBeGarbageCollected<ScheduledSuspendContainer> { There would be no reason not to use an oilpan by default. NoBaseWillBeGarbageCollected => GarbageCollected. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:67: static PassOwnPtr<ScheduledSuspendContainer> create(size_t suspendFrame, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); PassOwnPtr<ScheduledSuspendContainer> => ScheduledSuspendContainer* https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:96: // Resolve pending suspend promises and removes it from the list. it => them https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: WillBeHeapVector<OwnPtrWillBeMember<ScheduledSuspendContainer>> m_scheduledSuspends; HeapVector<Member<ScheduledSuspendContainer>> https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: WillBeHeapVector<OwnPtrWillBeMember<ScheduledSuspendContainer>> m_scheduledSuspends; Wouldn't it be better to use a HashSet instead of a Vector? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:121: Can we add ASSERT(context()->isOfflineContext()) ? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:150: Ditto. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:177: if (m_framesToProcess <= 0) { Shall we add ASSERT(m_framesToProcess == 0)? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:189: Add ASSERT(context->isOfflineContext()).
https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:318: AutoLocker locker(this); destination()->audioDestinationHandler().startRendering() is run while this lock is held, right? Won't this prevent any thing on the main thread from happening until startRendering() returns? If so, this is probably not what we want.
My local compilation setting does not have 'enable_oilpan=1' so all the oil pan changes did not work well. (I can't compile.) Also I am looking into using HashSet rather than HeapVector in the next patch set. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:239: // FIXME: Refactoring needed. These are OfflineAudioContext-specific tasks. On 2015/06/17 06:46:08, haraken wrote: > > FIXME => TODO Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.h:243: Member<AudioBuffer> renderTarget() const { return m_renderTarget; } On 2015/06/17 06:46:08, haraken wrote: > > Member<AudioBuffer> => AudioBuffer* > > m_renderTarget => m_renderTarget.get() Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // FIXME: Refactoring needed. This method is only used by OfflineAudioContext. On 2015/06/17 06:46:08, haraken wrote: > > FIXME => TODO Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:51: m_contextGraphMutex.tryLock(); On 2015/06/17 06:46:08, haraken wrote: > > Two questions: > > - This can fail. Is it OK? > - Why does the main thread need to use tryLock()? The main thread is not a > realtime thread. For this pattern, I would like to ask your opinion. I just want to be able to lock the graph in the offline audio rendering. What would be the most simple/elegant? Just lock() it? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:108: // FIXME: What should be done to trace members in OfflineAudioContext? On 2015/06/17 06:46:08, haraken wrote: > > This trace method looks correct. You can just trace all members. It produces error during the compilation. Probably it is because the oilpan option was not enabled. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:191: if (!isMainThread()) On 2015/06/17 06:46:08, haraken wrote: > > Basically it is not a good idea to add both ASSERT(x()) and if (!x()). If !x() > shouldn't happen, we should only have ASSERT(x()). The same comment for other > places in this CL. > Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:236: m_completeResolver = ScriptPromiseResolver::create(scriptState); On 2015/06/17 01:07:50, yhirano wrote: > Shouldn't this statement be placed after the L239-L245 block? Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:256: ASSERT(isMainThread()); On 2015/06/17 01:07:50, yhirano wrote: > if (state == closed) { return Promise.reject(...) } is needed? How about when it > is already suspended? 1) OfflineAudioContext does not have an explicit close() method. I think it is not possible to be a 'closed' state unless it reaches the end of the rendering. 2) Calling suspend(t2) when the context is suspended by suspend(t1) is a legitimate pattern. However, we check the time argument of suspend() call so we can avoid duplicates in the scheduled suspension. In other words, we allow to call suspend(t2) when (t1 != t2). I can share a mini design doc for the usage pattern if that's helpful to you. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:318: AutoLocker locker(this); On 2015/06/17 15:56:10, Raymond Toy wrote: > destination()->audioDestinationHandler().startRendering() is run while this lock > is held, right? Won't this prevent any thing on the main thread from happening > until startRendering() returns? If so, this is probably not what we want. Yes. I agree. However, it didn't matter because startRendering() actually jumps to the other thread. So the lock will be released when this scope is over. Will remove this. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:357: AutoLocker locker(this); On 2015/06/17 06:46:09, haraken wrote: > > Why do you need this lock? When we resolve the suspend promise, the script can change the graph. So I wanted to make sure the graph is not touched by the other thread while being manipulated. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:362: // FIXME: is removing elements efficient? What if there are 10K suspends? On 2015/06/17 06:46:08, haraken wrote: > > FIXME => TODO Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:65: : public NoBaseWillBeGarbageCollected<ScheduledSuspendContainer> { On 2015/06/17 06:46:09, haraken wrote: > > There would be no reason not to use an oilpan by default. > > NoBaseWillBeGarbageCollected => GarbageCollected. I couldn't change this because I am getting a compilation error. I don't use enable_oilpan=1 option for GYP_DEFINE, should I enable it before I make this change? https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:67: static PassOwnPtr<ScheduledSuspendContainer> create(size_t suspendFrame, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); On 2015/06/17 06:46:09, haraken wrote: > > PassOwnPtr<ScheduledSuspendContainer> => ScheduledSuspendContainer* For the same reason above, it breaks the compilation. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:96: // Resolve pending suspend promises and removes it from the list. On 2015/06/17 06:46:09, haraken wrote: > > it => them Oops. Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: WillBeHeapVector<OwnPtrWillBeMember<ScheduledSuspendContainer>> m_scheduledSuspends; On 2015/06/17 06:46:09, haraken wrote: > > HeapVector<Member<ScheduledSuspendContainer>> I agree. Will look into it. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:121: On 2015/06/17 06:46:09, haraken wrote: > > Can we add ASSERT(context()->isOfflineContext()) ? Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:177: if (m_framesToProcess <= 0) { On 2015/06/17 06:46:09, haraken wrote: > > Shall we add ASSERT(m_framesToProcess == 0)? Done. https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioDestinationNode.cpp:189: On 2015/06/17 06:46:09, haraken wrote: > > Add ASSERT(context->isOfflineContext()). Done.
https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:65: : public NoBaseWillBeGarbageCollected<ScheduledSuspendContainer> { On 2015/06/17 20:10:39, hoch wrote: > On 2015/06/17 06:46:09, haraken wrote: > > > > There would be no reason not to use an oilpan by default. > > > > NoBaseWillBeGarbageCollected => GarbageCollected. > > I couldn't change this because I am getting a compilation error. I don't use > enable_oilpan=1 option for GYP_DEFINE, should I enable it before I make this > change? Just to clarify: You need to pass both oilpan bulids and non-oilpan builds.
@haraken PTAL patch set 13. Thanks for your help!
https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/370001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:256: ASSERT(isMainThread()); On 2015/06/17 20:10:39, hoch wrote: > On 2015/06/17 01:07:50, yhirano wrote: > > if (state == closed) { return Promise.reject(...) } is needed? How about when > it > > is already suspended? > > 1) OfflineAudioContext does not have an explicit close() method. I think it is > not possible to be a 'closed' state unless it reaches the end of the rendering. > > 2) Calling suspend(t2) when the context is suspended by suspend(t1) is a > legitimate pattern. However, we check the time argument of suspend() call so we > can avoid duplicates in the scheduled suspension. In other words, we allow to > call suspend(t2) when (t1 != t2). > > I can share a mini design doc for the usage pattern if that's helpful to you. I see, thanks! https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:114: bool OfflineAudioContext::shouldSuspendNow() Is m_scheduledSuspends protected by lock? I'm not 100% sure, but this function is called by OfflineAudioDestinationHandler::runOfflineRendering which is called by OfflineAudioDestinationHandler::startOfflineRendering which is not locked, right? https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:139: if (m_scheduledSuspends.size() > 0) { ditto https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:48: bool shouldSuspendNow(); +override ditto for below
This is the error that I am getting with PS13:
../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:108:1:
error: [blink-gc] Base class 'AudioContext' of derived class
'OfflineAudioContext' requires tracing.
DEFINE_TRACE(OfflineAudioContext)
^
../../third_party/WebKit/Source/platform/heap/Visitor.h:106:5: note: expanded
from macro 'DEFINE_TRACE'
ALWAYS_INLINE void T::traceImpl(VisitorDispatcher visitor)
^
../../third_party/WebKit/Source/wtf/Compiler.h:71:23: note: expanded from macro
'ALWAYS_INLINE'
#define ALWAYS_INLINE inline
On 2015/06/18 17:27:33, hoch wrote: > This is the error that I am getting with PS13: > > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:108:1: > error: [blink-gc] Base class 'AudioContext' of derived class > 'OfflineAudioContext' requires tracing. > DEFINE_TRACE(OfflineAudioContext) > ^ > ../../third_party/WebKit/Source/platform/heap/Visitor.h:106:5: note: expanded > from macro 'DEFINE_TRACE' > ALWAYS_INLINE void T::traceImpl(VisitorDispatcher visitor) > ^ > ../../third_party/WebKit/Source/wtf/Compiler.h:71:23: note: expanded from macro > 'ALWAYS_INLINE' > #define ALWAYS_INLINE inline You need to call AudioContext::trace(visitor) in the trace method.
@haraken Thanks. Now it compiles correctly. https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:114: bool OfflineAudioContext::shouldSuspendNow() On 2015/06/18 13:40:52, yhirano wrote: > Is m_scheduledSuspends protected by lock? > I'm not 100% sure, but this function is called by > > OfflineAudioDestinationHandler::runOfflineRendering which is called by > OfflineAudioDestinationHandler::startOfflineRendering which is not locked, > right? I understand your concern, but adding a lock in here will make the render loop slow. Would it be possible to make m_scheduledSuspends.add()/remove() atomic process instead? https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:139: if (m_scheduledSuspends.size() > 0) { On 2015/06/18 13:40:52, yhirano wrote: > ditto Acknowledged. https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:48: bool shouldSuspendNow(); On 2015/06/18 13:40:52, yhirano wrote: > +override > ditto for below Done.
https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:114: bool OfflineAudioContext::shouldSuspendNow() On 2015/06/18 18:22:31, hoch wrote: > On 2015/06/18 13:40:52, yhirano wrote: > > Is m_scheduledSuspends protected by lock? > > I'm not 100% sure, but this function is called by > > > > OfflineAudioDestinationHandler::runOfflineRendering which is called by > > OfflineAudioDestinationHandler::startOfflineRendering which is not locked, > > right? > > I understand your concern, but adding a lock in here will make the render loop > slow. Would it be possible to make m_scheduledSuspends.add()/remove() atomic > process instead? No, I don't think so. The following functions touche m_scheduledSuspends. - trace (it should be no problem, but I'm not 100% sure) - shouldSuspendNow (render thread) - resolvePendingSuspendPromises (render thread) - suspendOfflineRendering (main thread) - resolvePendingSuspendPromisesOnMainThread (main thread) It seems shouldSuspendNow uses m_scheduledSuspends in the render thread because it returns a boolean variable. I propose the following: - m_scheduledSuspends is manipulated on the render thread. - The duplication check in suspendOfflineRendering is moved to the render thread. The method does no longer manipulate the list, just post a task to the render thread and a ScheduledSuspendContainer is attached to the task. - The task runs on the render thread and checks if there is duplication. If there is, it posts a rejection task to the main thread (with the container). Otherwise, it adds the container to the list. - resolvePendingSuspendPromises scans the list on the render thread. When a pending container is found, it removes the container from the list. Post a task to the main thread to do the remaining job.
Patchset #14 (id:430001) has been deleted
@yhirano PTAL patch set 14. https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/410001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:114: bool OfflineAudioContext::shouldSuspendNow() On 2015/06/19 02:31:59, yhirano wrote: > On 2015/06/18 18:22:31, hoch wrote: > > On 2015/06/18 13:40:52, yhirano wrote: > > > Is m_scheduledSuspends protected by lock? > > > I'm not 100% sure, but this function is called by > > > > > > OfflineAudioDestinationHandler::runOfflineRendering which is called by > > > OfflineAudioDestinationHandler::startOfflineRendering which is not locked, > > > right? > > > > I understand your concern, but adding a lock in here will make the render loop > > slow. Would it be possible to make m_scheduledSuspends.add()/remove() atomic > > process instead? > > No, I don't think so. > The following functions touche m_scheduledSuspends. > > - trace (it should be no problem, but I'm not 100% sure) > - shouldSuspendNow (render thread) > - resolvePendingSuspendPromises (render thread) > - suspendOfflineRendering (main thread) > - resolvePendingSuspendPromisesOnMainThread (main thread) > > It seems shouldSuspendNow uses m_scheduledSuspends in the render thread because > it returns a boolean variable. I propose the following: > > - m_scheduledSuspends is manipulated on the render thread. > - The duplication check in suspendOfflineRendering is moved to the render > thread. The method does no longer manipulate the list, just post a task to the > render thread and a ScheduledSuspendContainer is attached to the task. > - The task runs on the render thread and checks if there is duplication. If > there is, it posts a rejection task to the main thread (with the container). > Otherwise, it adds the container to the list. > - resolvePendingSuspendPromises scans the list on the render thread. When a > pending container is found, it removes the container from the list. Post a task > to the main thread to do the remaining job. @yhirano Although I agree with your idea of clear separation, not sure it is worth the effort. We already have this sort of patterns in AudioContext. (manipulating one vector from different places/threads) However, I wanted to try out your suggestion but I wasn't able to compile the build. (since I am not particularly good at the multi-thread patterns in Chromium) So I thought you could help me with this. PTAL OfflineAudioContext::suspendOfflineRendering and OfflineAudioContext::validateSuspend. https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:225: threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, resolver)); This doesn't compile with the following error message: --- ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:171:60: error: implicit instantiation of undefined template 'blink::CrossThreadCopierBase<false, false, false, WTF::RefPtr<blink::ScriptPromiseResolver> >' template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, ^ ../../third_party/WebKit/Source/platform/ThreadSafeFunctional.h:63:9: note: in instantiation of template class 'blink::CrossThreadCopier<WTF::RefPtr<blink::ScriptPromiseResolver> >' requested here CrossThreadCopier<P3>::copy(parameter3)); ^ ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:225:9: note: in instantiation of function template specialization 'blink::threadSafeBind<, void (blink::OfflineAudioContext::*)(double, WTF::PassRefPtr<blink::ScriptPromiseResolver>), blink::OfflineAudioContext *, double, WTF::RefPtr<blink::ScriptPromiseResolver> >' requested here threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, resolver)); ^ ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:64:116: note: template is declared here template<bool isConvertibleToInteger, bool isThreadSafeRefCounted, bool isGarbageCollected, typename T> struct CrossThreadCopierBase; ^ 1 error generated. --- https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:227: return promise; @yhirano We return the promise here, but the rejection by invalid parameter will be done inside of validateSuspend() on the render thread. Is this okay? https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:312: void OfflineAudioContext::resolvePendingSuspendPromisesOnMainThread() @yhirano This method needs to be rewritten. I will work on it once the problem above is resolved.
https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:225: threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, resolver)); On 2015/06/19 22:15:40, hoch wrote: > This doesn't compile with the following error message: > > --- > > ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:171:60: error: > implicit instantiation of undefined template > 'blink::CrossThreadCopierBase<false, false, false, > WTF::RefPtr<blink::ScriptPromiseResolver> >' > template<typename T> struct CrossThreadCopier : public > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > ^ > ../../third_party/WebKit/Source/platform/ThreadSafeFunctional.h:63:9: note: in > instantiation of template class > 'blink::CrossThreadCopier<WTF::RefPtr<blink::ScriptPromiseResolver> >' requested > here > CrossThreadCopier<P3>::copy(parameter3)); > ^ > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:225:9: > note: in instantiation of function template specialization > 'blink::threadSafeBind<, void (blink::OfflineAudioContext::*)(double, > WTF::PassRefPtr<blink::ScriptPromiseResolver>), blink::OfflineAudioContext *, > double, WTF::RefPtr<blink::ScriptPromiseResolver> >' requested here > threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, > resolver)); > ^ > ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:64:116: note: > template is declared here > template<bool isConvertibleToInteger, bool isThreadSafeRefCounted, bool > isGarbageCollected, typename T> struct CrossThreadCopierBase; > > ^ > 1 error generated. > > --- You cannot pass a ScriptPromiseResolver to another thread because it is a RefCounted, not a ThreadSafeRefCounted at least for now. I meant to pass a ScheduledSuspendContainer. https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:227: return promise; On 2015/06/19 22:15:39, hoch wrote: > @yhirano > > We return the promise here, but the rejection by invalid parameter will be done > inside of validateSuspend() on the render thread. > > Is this okay? No, resolution / rejection must be done on the original thread.
@yhirano PTAL at PS15. When the duplication check by OfflineAudioContext on the render thread touches m_scheduledSuspends, the tab crashes silently. If this is something difficult to fix, I think the CL has more fundamental structure problem. If you have a better idea on the overall design, please let me know. https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:225: threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, resolver)); On 2015/06/22 02:26:19, yhirano wrote: > On 2015/06/19 22:15:40, hoch wrote: > > This doesn't compile with the following error message: > > > > --- > > > > ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:171:60: error: > > implicit instantiation of undefined template > > 'blink::CrossThreadCopierBase<false, false, false, > > WTF::RefPtr<blink::ScriptPromiseResolver> >' > > template<typename T> struct CrossThreadCopier : public > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > ^ > > ../../third_party/WebKit/Source/platform/ThreadSafeFunctional.h:63:9: note: in > > instantiation of template class > > 'blink::CrossThreadCopier<WTF::RefPtr<blink::ScriptPromiseResolver> >' > requested > > here > > CrossThreadCopier<P3>::copy(parameter3)); > > ^ > > > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:225:9: > > note: in instantiation of function template specialization > > 'blink::threadSafeBind<, void (blink::OfflineAudioContext::*)(double, > > WTF::PassRefPtr<blink::ScriptPromiseResolver>), blink::OfflineAudioContext *, > > double, WTF::RefPtr<blink::ScriptPromiseResolver> >' requested here > > threadSafeBind(&OfflineAudioContext::validateSuspend, this, when, > > resolver)); > > ^ > > ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:64:116: note: > > template is declared here > > template<bool isConvertibleToInteger, bool isThreadSafeRefCounted, bool > > isGarbageCollected, typename T> struct CrossThreadCopierBase; > > > > > ^ > > 1 error generated. > > > > --- > > You cannot pass a ScriptPromiseResolver to another thread because it is a > RefCounted, not a ThreadSafeRefCounted at least for now. I meant to pass a > ScheduledSuspendContainer. Done. So, is ScheduledSuspendContainer a ThreadSafeRefCounted? If so, how can you tell? https://codereview.chromium.org/1140723003/diff/450001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:227: return promise; On 2015/06/22 02:26:19, yhirano wrote: > On 2015/06/19 22:15:39, hoch wrote: > > @yhirano > > > > We return the promise here, but the rejection by invalid parameter will be > done > > inside of validateSuspend() on the render thread. > > > > Is this okay? > > No, resolution / rejection must be done on the original thread. Okay. I moved the resolution/rejection to the main thread. https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:313: m_scheduledSuspends.append(suspendContainer); This actually crashes the tab (no error message in the console with the debug build). I only can see up to "adding suspend..." and it does not proceed after that. Not sure how to prevent the crash. Any thought?
https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:313: m_scheduledSuspends.append(suspendContainer); On 2015/06/22 18:23:59, hoch wrote: > This actually crashes the tab (no error message in the console with the debug > build). I only can see up to "adding suspend..." and it does not proceed after > that. Not sure how to prevent the crash. > > Any thought? I didn't notice that the render thread doesn't support Oilpan. As a result, we cannot use oilpan-related objects (GarbageCollected, HeapVector, etc.) in the render thread. That means: - ScheduledSuspendContainer should be ThreadSafeRefCounted, - m_scheduledSuspends should be Vector<RefPtr<ScheduledSuspendContainer>>, if you want to access them in the render thread. haraken@, please correct me if I'm wrong. I'm sorry for the inconvenience.
https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:313: m_scheduledSuspends.append(suspendContainer); On 2015/06/23 02:33:42, yhirano wrote: > On 2015/06/22 18:23:59, hoch wrote: > > This actually crashes the tab (no error message in the console with the debug > > build). I only can see up to "adding suspend..." and it does not proceed after > > that. Not sure how to prevent the crash. > > > > Any thought? > > I didn't notice that the render thread doesn't support Oilpan. As a result, we > cannot use oilpan-related objects (GarbageCollected, HeapVector, etc.) in the > render thread. > > That means: > - ScheduledSuspendContainer should be ThreadSafeRefCounted, > - m_scheduledSuspends should be Vector<RefPtr<ScheduledSuspendContainer>>, > if you want to access them in the render thread. > > haraken@, please correct me if I'm wrong. > > > I'm sorry for the inconvenience. > yhirano@ haraken@ I guess we can decide what we want here, but probably the best thing for the future is to make the render thread oilpan-compatible. Or as you suggested, we can just leave the render thread alone and revert the shared components back to the old reference counting mechanism. WDYT?
On 2015/06/23 17:57:38, hoch wrote: > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:313: > m_scheduledSuspends.append(suspendContainer); > On 2015/06/23 02:33:42, yhirano wrote: > > On 2015/06/22 18:23:59, hoch wrote: > > > This actually crashes the tab (no error message in the console with the > debug > > > build). I only can see up to "adding suspend..." and it does not proceed > after > > > that. Not sure how to prevent the crash. > > > > > > Any thought? > > > > I didn't notice that the render thread doesn't support Oilpan. As a result, we > > cannot use oilpan-related objects (GarbageCollected, HeapVector, etc.) in the > > render thread. > > > > That means: > > - ScheduledSuspendContainer should be ThreadSafeRefCounted, > > - m_scheduledSuspends should be Vector<RefPtr<ScheduledSuspendContainer>>, > > if you want to access them in the render thread. > > > > haraken@, please correct me if I'm wrong. > > > > > > I'm sorry for the inconvenience. > > > > yhirano@ haraken@ > > I guess we can decide what we want here, but probably the best thing for the > future is to make the render thread oilpan-compatible. > > Or as you suggested, we can just leave the render thread alone and revert the > shared components back to the old reference counting mechanism. > > WDYT? My limited understanding is that you can't have the audio thread use oilpan because oilpan can cause 10 ms (or much more) pauses. That would destroy the audio thread.
On 2015/06/23 20:34:12, Raymond Toy wrote: > On 2015/06/23 17:57:38, hoch wrote: > > > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > > > > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > > Source/modules/webaudio/OfflineAudioContext.cpp:313: > > m_scheduledSuspends.append(suspendContainer); > > On 2015/06/23 02:33:42, yhirano wrote: > > > On 2015/06/22 18:23:59, hoch wrote: > > > > This actually crashes the tab (no error message in the console with the > > debug > > > > build). I only can see up to "adding suspend..." and it does not proceed > > after > > > > that. Not sure how to prevent the crash. > > > > > > > > Any thought? > > > > > > I didn't notice that the render thread doesn't support Oilpan. As a result, > we > > > cannot use oilpan-related objects (GarbageCollected, HeapVector, etc.) in > the > > > render thread. > > > > > > That means: > > > - ScheduledSuspendContainer should be ThreadSafeRefCounted, > > > - m_scheduledSuspends should be Vector<RefPtr<ScheduledSuspendContainer>>, > > > if you want to access them in the render thread. > > > > > > haraken@, please correct me if I'm wrong. > > > > > > > > > I'm sorry for the inconvenience. > > > > > > > yhirano@ haraken@ > > > > I guess we can decide what we want here, but probably the best thing for the > > future is to make the render thread oilpan-compatible. > > > > Or as you suggested, we can just leave the render thread alone and revert the > > shared components back to the old reference counting mechanism. > > > > WDYT? > > My limited understanding is that you can't have the audio thread use oilpan > because oilpan can cause 10 ms (or much more) pauses. > That would destroy the audio thread. I am fine with not having oilpan for the offline audio render thread. However, the offline audio render thread is not a real-time audio thread and I thought having oilpan might be nice. If there is any performance issue that can be caused by oilpan in the offline render thread, please let me know. Either works fine with me, I just want to choose the best design for the future.
On 2015/06/23 20:40:27, hoch wrote: > On 2015/06/23 20:34:12, Raymond Toy wrote: > > On 2015/06/23 17:57:38, hoch wrote: > > > > > > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > > > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > > > > > > > > https://codereview.chromium.org/1140723003/diff/470001/Source/modules/webaudi... > > > Source/modules/webaudio/OfflineAudioContext.cpp:313: > > > m_scheduledSuspends.append(suspendContainer); > > > On 2015/06/23 02:33:42, yhirano wrote: > > > > On 2015/06/22 18:23:59, hoch wrote: > > > > > This actually crashes the tab (no error message in the console with the > > > debug > > > > > build). I only can see up to "adding suspend..." and it does not proceed > > > after > > > > > that. Not sure how to prevent the crash. > > > > > > > > > > Any thought? > > > > > > > > I didn't notice that the render thread doesn't support Oilpan. As a > result, > > we > > > > cannot use oilpan-related objects (GarbageCollected, HeapVector, etc.) in > > the > > > > render thread. > > > > > > > > That means: > > > > - ScheduledSuspendContainer should be ThreadSafeRefCounted, > > > > - m_scheduledSuspends should be > Vector<RefPtr<ScheduledSuspendContainer>>, > > > > if you want to access them in the render thread. > > > > > > > > haraken@, please correct me if I'm wrong. > > > > > > > > > > > > I'm sorry for the inconvenience. > > > > > > > > > > yhirano@ haraken@ > > > > > > I guess we can decide what we want here, but probably the best thing for the > > > future is to make the render thread oilpan-compatible. > > > > > > Or as you suggested, we can just leave the render thread alone and revert > the > > > shared components back to the old reference counting mechanism. > > > > > > WDYT? > > > > My limited understanding is that you can't have the audio thread use oilpan > > because oilpan can cause 10 ms (or much more) pauses. > > That would destroy the audio thread. > > I am fine with not having oilpan for the offline audio render thread. However, > the offline audio render thread is not a real-time audio thread and I thought > having oilpan might be nice. If there is any performance issue that can be > caused by oilpan in the offline render thread, please let me know. > > Either works fine with me, I just want to choose the best design for the future. Our long-term plan is to remove all cross-thread pointers from the Blink code base and completely isolate Oilpan's heaps per thread. Then, even if the main thread allocates a lot of things, it just affects a GC of the main thread. Other threads don't need to get involved in the GC. In that world, there would be no concern about having Oilpan in the audio thread and the offline audio render thread. That said, this is not going to happen anytime soon, so I'd recommend to take a short-term solution for the current problem.
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
haraken@ yhirano@ PTAL at PS16. Based on your suggestions, I brought the non-oilpan code back and it worked. Then I replaced the vector with HashMap and it reduced many lines of code. (removing loops, retrieval and dupe checks) However, now I have a compilation error with trace() for the hashmap and not sure how I can solve this. I can revert it back to the vector version, but wanted to ask your opinion first. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:113: visitor->trace(m_scheduledSuspends); This gives me a compile error: ./../third_party/WebKit/Source/wtf/HashTable.h:1340:41: error: no matching member function for call to 'trace' Allocator::template trace<VisitorDispatcher, ValueType, Traits>(visitor, *element); ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../third_party/WebKit/Source/wtf/HashMap.h:157:56: note: in instantiation of function template specialization 'WTF::HashTable<unsigned long, WTF::KeyValuePair<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer> >, WTF::KeyValuePairKeyExtractor, WTF::IntHash<unsigned long>, WTF::HashMapValueTraits<WTF::HashTraits<unsigned long>, WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> > >, WTF::HashTraits<unsigned long>, WTF::DefaultAllocator>::trace<blink::Visitor *>' requested here void trace(VisitorDispatcher visitor) { m_impl.trace(visitor); } ^ ../../third_party/WebKit/Source/platform/heap/TraceTraits.h:164:32: note: in instantiation of function template specialization 'WTF::HashMap<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, WTF::DefaultAllocator>::trace<blink::Visitor *>' requested here static_cast<T*>(self)->trace(visitor); ^ ../../third_party/WebKit/Source/platform/heap/Visitor.h:222:24: note: in instantiation of member function 'blink::TraceTrait<WTF::HashMap<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, WTF::DefaultAllocator> >::trace' requested here TraceTrait<T>::trace(Derived::fromHelper(this), &const_cast<T&>(t)); ^ ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:113:14: note: in instantiation of function template specialization 'blink::VisitorHelper<blink::Visitor>::trace<WTF::HashMap<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, WTF::DefaultAllocator> >' requested here visitor->trace(m_scheduledSuspends); ^ ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:111:1: note: in instantiation of function template specialization 'blink::OfflineAudioContext::traceImpl<blink::Visitor *>' requested here DEFINE_TRACE(OfflineAudioContext) ^ ../../third_party/WebKit/Source/platform/heap/Visitor.h:103:39: note: expanded from macro 'DEFINE_TRACE' void T::trace(Visitor* visitor) { traceImpl(visitor); } \ ^ ../../third_party/WebKit/Source/wtf/DefaultAllocator.h:170:17: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'Traits' static void trace(...) Not sure what this is about. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:315: m_scheduledSuspends.set(suspendContainer->suspendFrame(), adoptedContainer); This might be the problematic part. Since the |suspendContainer| is a raw pointer, I converted it back to RefPtr before adding it to the hashmap. Please let me know if this does not look good. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:37: using SuspendContainerMap = HashMap<size_t, RefPtr<ScheduledSuspendContainer>>; I changed the vector to HashMap. I didn't use HeapHashMap since we decided to ditch the oilpan on this variable. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:78: SuspendContainerMap m_scheduledSuspends; This is HashMap of <size_t suspendFrame, RefPtr<SchduledSuspendContainer>>.
https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:113: visitor->trace(m_scheduledSuspends); On 2015/06/24 21:14:12, hoch wrote: > This gives me a compile error: > > ./../third_party/WebKit/Source/wtf/HashTable.h:1340:41: error: no matching > member function for call to 'trace' > Allocator::template trace<VisitorDispatcher, ValueType, > Traits>(visitor, *element); > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../third_party/WebKit/Source/wtf/HashMap.h:157:56: note: in instantiation of > function template specialization 'WTF::HashTable<unsigned long, > WTF::KeyValuePair<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer> > >, WTF::KeyValuePairKeyExtractor, WTF::IntHash<unsigned long>, > WTF::HashMapValueTraits<WTF::HashTraits<unsigned long>, > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> > >, > WTF::HashTraits<unsigned long>, WTF::DefaultAllocator>::trace<blink::Visitor *>' > requested here > void trace(VisitorDispatcher visitor) { m_impl.trace(visitor); } > ^ > ../../third_party/WebKit/Source/platform/heap/TraceTraits.h:164:32: note: in > instantiation of function template specialization 'WTF::HashMap<unsigned long, > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > WTF::HashTraits<unsigned long>, > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > WTF::DefaultAllocator>::trace<blink::Visitor *>' requested here > static_cast<T*>(self)->trace(visitor); > ^ > ../../third_party/WebKit/Source/platform/heap/Visitor.h:222:24: note: in > instantiation of member function 'blink::TraceTrait<WTF::HashMap<unsigned long, > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > WTF::HashTraits<unsigned long>, > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > WTF::DefaultAllocator> >::trace' requested here > TraceTrait<T>::trace(Derived::fromHelper(this), &const_cast<T&>(t)); > ^ > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:113:14: > note: in instantiation of function template specialization > 'blink::VisitorHelper<blink::Visitor>::trace<WTF::HashMap<unsigned long, > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > WTF::HashTraits<unsigned long>, > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > WTF::DefaultAllocator> >' requested here > visitor->trace(m_scheduledSuspends); > ^ > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:111:1: > note: in instantiation of function template specialization > 'blink::OfflineAudioContext::traceImpl<blink::Visitor *>' requested here > DEFINE_TRACE(OfflineAudioContext) > ^ > ../../third_party/WebKit/Source/platform/heap/Visitor.h:103:39: note: expanded > from macro 'DEFINE_TRACE' > void T::trace(Visitor* visitor) { traceImpl(visitor); } \ > ^ > ../../third_party/WebKit/Source/wtf/DefaultAllocator.h:170:17: note: candidate > template ignored: invalid explicitly-specified argument for template parameter > 'Traits' > static void trace(...) > > Not sure what this is about. m_scheduledSuspends is not a HashMap of Oilpan. You shouldn't trace it.
https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:147: RefPtrWillBeRawPtr<ScheduledSuspendContainer> suspendContainer = it->value; This should be just RefPtr. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:149: threadSafeBind(&OfflineAudioContext::resolvePendingSuspendPromisesOnMainThread, this, suspendContainer)); suspendContainer.release() https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:150: m_scheduledSuspends.remove(nowFrame); Please move this statement to L148. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:150: m_scheduledSuspends.remove(nowFrame); [optional] remove(it) may be more straightforward? https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:256: ScheduledSuspendContainer* suspendContainer = ScheduledSuspendContainer::create(when, quantizedFrame, resolver); RefPtr<ScheduledSuspendContainer> suspendContainer = ...; https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:298: void OfflineAudioContext::checkDuplicateSuspend(ScheduledSuspendContainer* suspendContainer) PassRefPtr<ScheduledSuspendContainer> https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:314: RefPtr<ScheduledSuspendContainer> adoptedContainer = adoptRef(suspendContainer); You should not use adoptRef here. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:318: void OfflineAudioContext::rejectSuspendPromiseOnMainThread(ScheduledSuspendContainer* suspendContainer) PassRefPtr<ScheduledSuspendContainer> https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:328: void OfflineAudioContext::resolvePendingSuspendPromisesOnMainThread(ScheduledSuspendContainer* suspendContainer) PassRefPtr<ScheduledSuspendContainer> https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:346: { ASSERT(isMainThread); https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:350: { ASSERT(isMainThread); https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:353: DEFINE_TRACE(ScheduledSuspendContainer) No trace is needed. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: static ScheduledSuspendContainer* create(double suspendTime, size_t suspendFrame, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); static PassRefPtr<ScheduledSuspendContainer> create(...) {...} https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:102: DECLARE_TRACE(); No TRACE is needed. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:130: RefPtrWillBeMember<ScriptPromiseResolver> m_resolver; RefPtrWillBePersistent
yhirano@, haraken@ - Thanks so much for your help! Now the CL is ready for the review and final edits. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:113: visitor->trace(m_scheduledSuspends); On 2015/06/24 at 23:25:57, haraken wrote: > On 2015/06/24 21:14:12, hoch wrote: > > This gives me a compile error: > > > > ./../third_party/WebKit/Source/wtf/HashTable.h:1340:41: error: no matching > > member function for call to 'trace' > > Allocator::template trace<VisitorDispatcher, ValueType, > > Traits>(visitor, *element); > > > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../third_party/WebKit/Source/wtf/HashMap.h:157:56: note: in instantiation of > > function template specialization 'WTF::HashTable<unsigned long, > > WTF::KeyValuePair<unsigned long, WTF::RefPtr<blink::ScheduledSuspendContainer> > > >, WTF::KeyValuePairKeyExtractor, WTF::IntHash<unsigned long>, > > WTF::HashMapValueTraits<WTF::HashTraits<unsigned long>, > > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> > >, > > WTF::HashTraits<unsigned long>, WTF::DefaultAllocator>::trace<blink::Visitor *>' > > requested here > > void trace(VisitorDispatcher visitor) { m_impl.trace(visitor); } > > ^ > > ../../third_party/WebKit/Source/platform/heap/TraceTraits.h:164:32: note: in > > instantiation of function template specialization 'WTF::HashMap<unsigned long, > > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > > WTF::HashTraits<unsigned long>, > > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > > WTF::DefaultAllocator>::trace<blink::Visitor *>' requested here > > static_cast<T*>(self)->trace(visitor); > > ^ > > ../../third_party/WebKit/Source/platform/heap/Visitor.h:222:24: note: in > > instantiation of member function 'blink::TraceTrait<WTF::HashMap<unsigned long, > > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > > WTF::HashTraits<unsigned long>, > > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > > WTF::DefaultAllocator> >::trace' requested here > > TraceTrait<T>::trace(Derived::fromHelper(this), &const_cast<T&>(t)); > > ^ > > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:113:14: > > note: in instantiation of function template specialization > > 'blink::VisitorHelper<blink::Visitor>::trace<WTF::HashMap<unsigned long, > > WTF::RefPtr<blink::ScheduledSuspendContainer>, WTF::IntHash<unsigned long>, > > WTF::HashTraits<unsigned long>, > > WTF::HashTraits<WTF::RefPtr<blink::ScheduledSuspendContainer> >, > > WTF::DefaultAllocator> >' requested here > > visitor->trace(m_scheduledSuspends); > > ^ > > ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:111:1: > > note: in instantiation of function template specialization > > 'blink::OfflineAudioContext::traceImpl<blink::Visitor *>' requested here > > DEFINE_TRACE(OfflineAudioContext) > > ^ > > ../../third_party/WebKit/Source/platform/heap/Visitor.h:103:39: note: expanded > > from macro 'DEFINE_TRACE' > > void T::trace(Visitor* visitor) { traceImpl(visitor); } \ > > ^ > > ../../third_party/WebKit/Source/wtf/DefaultAllocator.h:170:17: note: candidate > > template ignored: invalid explicitly-specified argument for template parameter > > 'Traits' > > static void trace(...) > > > > Not sure what this is about. > > m_scheduledSuspends is not a HashMap of Oilpan. You shouldn't trace it. Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:147: RefPtrWillBeRawPtr<ScheduledSuspendContainer> suspendContainer = it->value; On 2015/06/25 at 04:00:53, yhirano wrote: > This should be just RefPtr. Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:149: threadSafeBind(&OfflineAudioContext::resolvePendingSuspendPromisesOnMainThread, this, suspendContainer)); On 2015/06/25 at 04:00:53, yhirano wrote: > suspendContainer.release() Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:150: m_scheduledSuspends.remove(nowFrame); On 2015/06/25 at 04:00:52, yhirano wrote: > Please move this statement to L148. Done and done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:256: ScheduledSuspendContainer* suspendContainer = ScheduledSuspendContainer::create(when, quantizedFrame, resolver); On 2015/06/25 at 04:00:52, yhirano wrote: > RefPtr<ScheduledSuspendContainer> suspendContainer = ...; Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:298: void OfflineAudioContext::checkDuplicateSuspend(ScheduledSuspendContainer* suspendContainer) On 2015/06/25 at 04:00:53, yhirano wrote: > PassRefPtr<ScheduledSuspendContainer> Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:314: RefPtr<ScheduledSuspendContainer> adoptedContainer = adoptRef(suspendContainer); On 2015/06/25 at 04:00:53, yhirano wrote: > You should not use adoptRef here. Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:318: void OfflineAudioContext::rejectSuspendPromiseOnMainThread(ScheduledSuspendContainer* suspendContainer) On 2015/06/25 at 04:00:52, yhirano wrote: > PassRefPtr<ScheduledSuspendContainer> Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:328: void OfflineAudioContext::resolvePendingSuspendPromisesOnMainThread(ScheduledSuspendContainer* suspendContainer) On 2015/06/25 at 04:00:53, yhirano wrote: > PassRefPtr<ScheduledSuspendContainer> Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:346: { On 2015/06/25 at 04:00:53, yhirano wrote: > ASSERT(isMainThread); Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:350: { On 2015/06/25 at 04:00:53, yhirano wrote: > ASSERT(isMainThread); Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:353: DEFINE_TRACE(ScheduledSuspendContainer) On 2015/06/25 at 04:00:53, yhirano wrote: > No trace is needed. Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: static ScheduledSuspendContainer* create(double suspendTime, size_t suspendFrame, PassRefPtrWillBeRawPtr<ScriptPromiseResolver>); On 2015/06/25 at 04:00:53, yhirano wrote: > static PassRefPtr<ScheduledSuspendContainer> create(...) {...} Done. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:102: DECLARE_TRACE(); On 2015/06/25 at 04:00:53, yhirano wrote: > No TRACE is needed. Removed. https://codereview.chromium.org/1140723003/diff/490001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:130: RefPtrWillBeMember<ScriptPromiseResolver> m_resolver; On 2015/06/25 at 04:00:53, yhirano wrote: > RefPtrWillBePersistent Done.
https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); Can you tell me why this lock is needed here? There are two statements in this function and the latter needn't lock. Does setContextState need locking? https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:343: return new ScheduledSuspendContainer(suspendTime, suspendFrame, resolver); +adoptRef
On 2015/06/26 at 02:29:10, yhirano wrote: > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); > Can you tell me why this lock is needed here? There are two statements in this function and the latter needn't lock. Does setContextState need locking? I was following the pattern from AudioContext::resolvePromisesForResumeOnMainThread(). Correct me if I am wrong here - the audio graph might change when resolving the promise (the scriptState usually manipulates the audio graph). Thus the main thread should be the graph owner and perhaps I should add an assertion for that. > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:343: return new ScheduledSuspendContainer(suspendTime, suspendFrame, resolver); > +adoptRef Will be done in the next patch set. Now I understand logically why adoptRef is needed here, but it is surprising to see the current patch compiles and runs okay without it. Is this normal?
LGTM On 2015/06/26 04:10:29, hoch wrote: > On 2015/06/26 at 02:29:10, yhirano wrote: > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); > > Can you tell me why this lock is needed here? There are two statements in this > function and the latter needn't lock. Does setContextState need locking? > > I was following the pattern from > AudioContext::resolvePromisesForResumeOnMainThread(). Correct me if I am wrong > here - the audio graph might change when resolving the promise (the scriptState > usually manipulates the audio graph). Thus the main thread should be the graph > owner and perhaps I should add an assertion for that. > The lock seems to be there in order to protect m_resumeResolvers. I'm not sure if setContextState should be protected. Anyway, I don't understand setContextState restriction, so I'm happy to defer to rtoy@ on this point. > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > Source/modules/webaudio/OfflineAudioContext.cpp:343: return new > ScheduledSuspendContainer(suspendTime, suspendFrame, resolver); > > +adoptRef > > Will be done in the next patch set. > > Now I understand logically why adoptRef is needed here, but it is surprising to > see the current patch compiles and runs okay without it. Is this normal? I think "adoptionIsRequired" assertion runs on assertion-enabled builds. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/06/26 at 06:47:21, yhirano wrote: > LGTM > > On 2015/06/26 04:10:29, hoch wrote: > > On 2015/06/26 at 02:29:10, yhirano wrote: > > > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > > > > > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > > Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); > > > Can you tell me why this lock is needed here? There are two statements in this > > function and the latter needn't lock. Does setContextState need locking? > > > > I was following the pattern from > > AudioContext::resolvePromisesForResumeOnMainThread(). Correct me if I am wrong > > here - the audio graph might change when resolving the promise (the scriptState > > usually manipulates the audio graph). Thus the main thread should be the graph > > owner and perhaps I should add an assertion for that. > > > The lock seems to be there in order to protect m_resumeResolvers. I'm not sure if setContextState should be protected. > Anyway, I don't understand setContextState restriction, so I'm happy to defer to rtoy@ on this point. rtoy@ is on vacation right now. If you think this lock needs to be reviewed thoroughly, we might have to wait for next 2 weeks. It does not seem to be posing a serious hit on the performance of the offline context, so I would like to land this change for now. > > > > > https://codereview.chromium.org/1140723003/diff/510001/Source/modules/webaudi... > > > Source/modules/webaudio/OfflineAudioContext.cpp:343: return new > > ScheduledSuspendContainer(suspendTime, suspendFrame, resolver); > > > +adoptRef > > > > Will be done in the next patch set. > > > > Now I understand logically why adoptRef is needed here, but it is surprising to > > see the current patch compiles and runs okay without it. Is this normal? > I think "adoptionIsRequired" assertion runs on assertion-enabled builds. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I see. However, I've always tested and verified with the debug build, which I believe it is assertion-enabled. Added adoptRef anyway.
@yhirano Thanks for your patience in reviewing the oil-pan part. @haraken PTAL at the last PS18. rtoy@ is currently on vacation and the most critical mechanism of this CL has been being reviewed by you and yhirano@. So I would appreciate your one final look before I land this patch.
Given the complexity of the CL, I might want to wait for rtoy@ to get back. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:867: deferredTaskHandler().handleDeferredTasks(); Don't we need to call resolvePromisesForResume()? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.cpp:100: ASSERT(context()->isOfflineContext()); ASSERT_NOT_REACHED() would be better. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // TODO: Refactoring needed. This method is only used by OfflineAudioContext. Add your name to TODO. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:163: m_completeResolver->resolve(renderedBuffer); If executionContext() is already gone, is it guaranteed that the resolver has been already rejected? Otherwise, the resolver will leak memory. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:202: m_isRenderingStarted = true; Shall we add ASSERT(!m_isRenderingStarted)? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:212: ASSERT(destination()->audioDestinationHandler().offlineRenderThread()); How is it guaranteed that the offline render thread still exists? Isn't it possible that the render thread has already finished? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:228: if (quantizedFrame < currentSampleFrame()) { There is a delay between this check (conducted by the main thread) and the timing where the suspend container gets added to the map by the render thread. Isn't it possible that the render thread ends up with adding a stale suspend container to the map? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); I didn't follow all the discussions about the lock, but do we really need this lock? What is this lock protecting? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:348: if (m_suspendFrame != whenFrame) Just to confirm: This is not m_suspendFrame < whenFrame, but m_suspendFrame != whenFrame? https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:94: // TODO: This class is |ThreadSafeRefCounted| because it needs to be accessed by Add your name to TODO. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:102: PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver() { return m_resolver; } I guess this should be ScriptPromiseResolver*, because it is not passing an ownership of the ScriptPromiseResolver.
haraken@ I've changed |checkDuplicateSuspend()| to |validateSuspendOnTheMainThread()|. Now the duplicate check and the timing check happens in the render thread. So there should be no timing issue when adding a suspend to the map. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:867: deferredTaskHandler().handleDeferredTasks(); On 2015/06/30 at 07:41:50, haraken wrote: > Don't we need to call resolvePromisesForResume()? No. resolvePromisesForResume() here is for promises created from the real-time audio context. Since the mechanism of suspend/resume in the real-time audio context is quite different from the offline one, so we keep things separated. This routine only applies to the offline context, thus I can skip the unnecessary part. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.cpp:100: ASSERT(context()->isOfflineContext()); On 2015/06/30 at 07:41:50, haraken wrote: > ASSERT_NOT_REACHED() would be better. Done. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/AudioDestinationNode.h (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/AudioDestinationNode.h:59: // TODO: Refactoring needed. This method is only used by OfflineAudioContext. On 2015/06/30 at 07:41:50, haraken wrote: > Add your name to TODO. Done. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:163: m_completeResolver->resolve(renderedBuffer); On 2015/06/30 at 07:41:51, haraken wrote: > If executionContext() is already gone, is it guaranteed that the resolver has been already rejected? Otherwise, the resolver will leak memory. I agree. The resolver should be rejected when the execution context is gone. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:202: m_isRenderingStarted = true; On 2015/06/30 at 07:41:51, haraken wrote: > Shall we add ASSERT(!m_isRenderingStarted)? Done. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:212: ASSERT(destination()->audioDestinationHandler().offlineRenderThread()); On 2015/06/30 at 07:41:51, haraken wrote: > How is it guaranteed that the offline render thread still exists? Isn't it possible that the render thread has already finished? Good point. I need to check if the render thread does exist here. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:228: if (quantizedFrame < currentSampleFrame()) { On 2015/06/30 at 07:41:51, haraken wrote: > There is a delay between this check (conducted by the main thread) and the timing where the suspend container gets added to the map by the render thread. Isn't it possible that the render thread ends up with adding a stale suspend container to the map? Moving this timing check into 'checkDuplicateSuspend()' and it posts the rejection to the main thread if necessary. I believe that will solve the delay issue. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:320: AutoLocker locker(this); On 2015/06/30 at 07:41:51, haraken wrote: > I didn't follow all the discussions about the lock, but do we really need this lock? What is this lock protecting? I was following the pattern from AudioContext::resolvePromisesForResumeOnMainThread() and yhirano@ wanted to ask rtoy@'s opinion on this. However, I am happy to remove the lock if it is redundant here. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:348: if (m_suspendFrame != whenFrame) On 2015/06/30 at 07:41:50, haraken wrote: > Just to confirm: This is not m_suspendFrame < whenFrame, but m_suspendFrame != whenFrame? Yes, I wanted to be specific about this. If we allow |m_suspendFrame < whenFrame|, the suspension will be performed even after it misses the exact moment. This is not the real-time audio rendering, so having the higher level of accuracy is important. Also all the frame numbers are quantized by the render quantum size, so there should be no room for errors. Actually ASSERT(whenFrame <= m_suspendFrame) might be nice to add here. Any scheduled suspension should be resolved and removed at its exact timing. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:94: // TODO: This class is |ThreadSafeRefCounted| because it needs to be accessed by On 2015/06/30 at 07:41:51, haraken wrote: > Add your name to TODO. Done. https://codereview.chromium.org/1140723003/diff/530001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:102: PassRefPtrWillBeRawPtr<ScriptPromiseResolver> resolver() { return m_resolver; } On 2015/06/30 at 07:41:51, haraken wrote: > I guess this should be ScriptPromiseResolver*, because it is not passing an ownership of the ScriptPromiseResolver. I made this change from the suggestion from yhirano@. I tried to change it to the raw pointer (and all the related parts), but it does not compile any more so I reverted it back. I will keep PassRefPtrWillBeRawPtr for the time being but am open to other suggestions.
PTAL at PS20. I refactored and cleaned up |ScheduledSuspendContainer| a bit. Please note that try bot failures are not relevant with this CL.
haraken@ Could you take a look at PS22 to confirm the oilpan or GC related part? I would love to land this CL as soon as rtoy@ comes back next week. FYI, the feature in this CL will resolve a bunch of flaky layout tests for web audio.
haraken@chromium.org changed reviewers: + tkent@chromium.org
Here is the final round of comments! tkent-san for API owner review. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:867: deferredTaskHandler().handleDeferredTasks(); Maybe worth having a comment on why we can skip resolvePromisesForResume() here. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:122: if (m_scheduledSuspends.isEmpty() || !m_scheduledSuspends.contains(currentSampleFrame())) You won't need the m_scheduledSuspends.isEmpty() check. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:135: if (m_scheduledSuspends.isEmpty() || it == m_scheduledSuspends.end()) You won't need the m_scheduledSuspends.isEmpty() check. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:166: m_completeResolver->reject(); Do we want to specify some DOM exception when rejecting the promise? https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:220: // The render thread does not exist; reject the promise. Just to confirm: If the render thread exists but does not yet get started, the created promise shouldn't be rejected, right? https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:367: bool ScheduledSuspendContainer::shouldSuspendAtFrame(size_t whenFrame) const Who calls this? It seems this method is unused. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:378: { Add ASSERT(!isMainThread()). https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:38: using SuspendContainerMap = HashMap<size_t, RefPtr<ScheduledSuspendContainer>, DefaultHash<size_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<size_t>>; Actually this ScheduledSuspendContainer doesn't need to be a RefPtr, because it must be guaranteed that the main thread keeps the ScheduledSuspendContainer alive while the render thread uses the ScheduledSuspendContainer. Maybe it is clearer to just use a raw pointer with a good comment. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:86: bool m_isRenderingStarted; I'm not sure but would it be better to introduce a new state like NotStarted? https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:98: class ScheduledSuspendContainer : public ThreadSafeRefCounted<ScheduledSuspendContainer> { As commented above, the render thread can keep ScheduledSuspendContainer with a raw pointer. Then this doesn't need to be ThreadSafeRefCounted. (It seems a bit strange to make a class that needs to be created and destructed in the same thread ThreadSafeRefCounted.)
haraken@ I removed the ref-counting mechanism (ThreadSafeRefCounted) from |ScheduledSuspendContainer|, but am getting some errors with throwing the raw pointer with threadSafeBind() method. PTAL at PS23. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:867: deferredTaskHandler().handleDeferredTasks(); On 2015/07/08 at 04:05:41, haraken wrote: > Maybe worth having a comment on why we can skip resolvePromisesForResume() here. Done. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:122: if (m_scheduledSuspends.isEmpty() || !m_scheduledSuspends.contains(currentSampleFrame())) On 2015/07/08 at 04:05:41, haraken wrote: > You won't need the m_scheduledSuspends.isEmpty() check. Done. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:135: if (m_scheduledSuspends.isEmpty() || it == m_scheduledSuspends.end()) On 2015/07/08 at 04:05:41, haraken wrote: > You won't need the m_scheduledSuspends.isEmpty() check. Done. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:166: m_completeResolver->reject(); On 2015/07/08 at 04:05:41, haraken wrote: > Do we want to specify some DOM exception when rejecting the promise? I am not completely sure about the meaning of throwing a DOM exception when you do not have a document. However, this is a very simple change so I might add few lines while we're at it. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:220: // The render thread does not exist; reject the promise. On 2015/07/08 at 04:05:41, haraken wrote: > Just to confirm: If the render thread exists but does not yet get started, the created promise shouldn't be rejected, right? Good question. The answer is no. "If the render thread exists but does not yet get started" - that is exactly how you should use this feature. At least one suspend MUST be scheduled before the rendering is started to ensure the accurate suspension. Otherwise, the timing of suspension can be guaranteed at the specified time because the offline rendering is much faster than real-time in general. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:367: bool ScheduledSuspendContainer::shouldSuspendAtFrame(size_t whenFrame) const On 2015/07/08 at 04:05:41, haraken wrote: > Who calls this? It seems this method is unused. Oops. This should have been removed with the introduction of HashMap. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:378: { On 2015/07/08 at 04:05:41, haraken wrote: > Add ASSERT(!isMainThread()). Done. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:38: using SuspendContainerMap = HashMap<size_t, RefPtr<ScheduledSuspendContainer>, DefaultHash<size_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<size_t>>; On 2015/07/08 04:05:41, haraken wrote: > > Actually this ScheduledSuspendContainer doesn't need to be a RefPtr, because it > must be guaranteed that the main thread keeps the ScheduledSuspendContainer > alive while the render thread uses the ScheduledSuspendContainer. > > Maybe it is clearer to just use a raw pointer with a good comment. Done. However, I am having a cross-thread issue in |resolveSuspendContainerOnMainThread()| method. Please have a look at the method in the implementation file. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:86: bool m_isRenderingStarted; On 2015/07/08 at 04:05:41, haraken wrote: > I'm not sure but would it be better to introduce a new state like NotStarted? rtoy@ and I thought about having a 'created' state, but then we need to change the Web Audio API spec first. I agree with the idea, but I would like to discuss it with the Audio WG and change the spec before I introduce a new state. https://codereview.chromium.org/1140723003/diff/610001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:98: class ScheduledSuspendContainer : public ThreadSafeRefCounted<ScheduledSuspendContainer> { On 2015/07/08 at 04:05:42, haraken wrote: > As commented above, the render thread can keep ScheduledSuspendContainer with a raw pointer. Then this doesn't need to be ThreadSafeRefCounted. > > (It seems a bit strange to make a class that needs to be created and destructed in the same thread ThreadSafeRefCounted.) Unfortunately, |resolvePendingSuspendPromises()| creates a local pointer in the render thread to pass it to the main thread, and that violates the |threadSafeBind()| method. Do you have a suggestion to avoid this error here? https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:141: threadSafeBind(&OfflineAudioContext::resolveSuspendContainerOnMainThread, this, suspendContainer)); haraken@ Now |suspendContainer| being created on the render thread and passed to the main thread for promise resolution. Calling the resolve method should be thread-safe with |threadSafeBind()| whereas the |suspendContainer| is not. I am trying to find a clean/elegant way of solving this without bringing too much RefPtr stuff into here. Do you have any suggestion? FYI, This is the error I am getting from |resolvePendingSuspendPromises()|: ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:171:60: error: implicit instantiation of undefined template 'blink::CrossThreadCopierBase<false, false, false, blink::ScheduledSuspendContainer *>' template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, ^ ../../third_party/WebKit/Source/platform/ThreadSafeFunctional.h:52:9: note: in instantiation of template class 'blink::CrossThreadCopier<blink::ScheduledSuspendContainer *>' requested here CrossThreadCopier<P2>::copy(parameter2)); ^ ../../third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:141:9: note: in instantiation of function template specialization 'blink::threadSafeBind<, void (blink::OfflineAudioContext::*)(blink::ScheduledSuspendContainer *), blink::OfflineAudioContext *, blink::ScheduledSuspendContainer *>' requested here threadSafeBind(&OfflineAudioContext::resolveSuspendContainerOnMainThread, this, suspendContainer)); ^ ../../third_party/WebKit/Source/platform/CrossThreadCopier.h:64:116: note: template is declared here template<bool isConvertibleToInteger, bool isThreadSafeRefCounted, bool isGarbageCollected, typename T> struct CrossThreadCopierBase; ^ 1 error generated.
haraken@chromium.org changed reviewers: + hiroshige@chromium.org
+hiroshige for the threadSafeBind issue.
https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:141: threadSafeBind(&OfflineAudioContext::resolveSuspendContainerOnMainThread, this, suspendContainer)); IIUC, - |suspendContainer| is created/managed/destructed on the main thread, and - |suspendContainer| is guaranteed to be alive until the task is executed by code somewhere else and thus it is thread-safe to pass the raw pointer to the main thread. If so, passing |AllowCrossThreadAccess(suspendContainer)| to threadSafeBind() will resolve the compiler error. (Functionally |suspendContainer| is just passed as the raw pointer as-is. AllowCrossThreadAccess() just tells threadSafeBind() to allow passing the pointer.) Please add a brief comment describing why passing the raw pointer is thread-safe.
hiroshige@: Thanks for your help on ThreadSafeBind() issue. haraken@: PTAL at PS24. https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/630001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:141: threadSafeBind(&OfflineAudioContext::resolveSuspendContainerOnMainThread, this, suspendContainer)); On 2015/07/09 05:20:15, OOO wrote: > IIUC, > - |suspendContainer| is created/managed/destructed on the main thread, and > - |suspendContainer| is guaranteed to be alive until the task is executed by > code somewhere else > and thus it is thread-safe to pass the raw pointer to the main thread. > > If so, passing |AllowCrossThreadAccess(suspendContainer)| to threadSafeBind() > will resolve the compiler error. > (Functionally |suspendContainer| is just passed as the raw pointer as-is. > AllowCrossThreadAccess() just tells threadSafeBind() to allow passing the > pointer.) > > Please add a brief comment describing why passing the raw pointer is > thread-safe. Done.
Patchset #25 (id:670001) has been deleted
Patchset #25 (id:690001) has been deleted
Patchset #25 (id:710001) has been deleted
Patchset #25 (id:730001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #10 (id:390001) has been deleted
Patchset #16 (id:550001) has been deleted
Patchset #17 (id:590001) has been deleted
Patchset #20 (id:750001) has been deleted
Patchset #20 (id:770001) has been deleted
Patchset #20 (id:790001) has been deleted
rtoy@, tkent@ PTAL at PS20. It includes the newly introduced AbstractAudioContext class while keeping the underlying design of OfflineAudioContext. Please note that this CL also touches UseCounter.h.
https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:8: PASS Scheduling a suspend in the past rejected correctly (with InvalidStateError: cannot schedule a suspend at frame 17536 (0.399229 seconds) because it is earlier than the current frame of 22016). Might be nice to have the current frame also include the time, like you did for the suspend time. I was initialized confused by the suspend time of 0.399229 sec because I was expecting something much closer to 0.4 sec, because the suspend time was 0.5 sec (which gets quantized to 22016/44100 sec). https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:33: context.suspend(0.5).then(function () { Should there be a test that this suspend actually happened at time 0.5? https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:70: Should('Resuming before suspend', context.resume()).beRejected(); Isn't there a race here? What if GC happens between context.resume() and startRendering()? Then resume() gets called after suspend(). https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:41: // the out-of-bound error, but it will reject the promise and "but it" -> "it" https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:11: description('Test synchronous graph manipulation with OfflineAudioContext.suspend() and OfflineAudioContext.suspend().'); One of these suspend()'s should be resume(). https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:53: }); Would it be nicer to write startRendering() like this: startRendering().then(function () { ... }) .then(finishJSTest); https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:71: }); Would it be nicer to write this as startRendering().then(verifyResult).then(finishJSTest); https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/AbstractAudioContext.h:164: // different method signature. Maybe comment that the first can only be used for online and the second only for offline? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:100: return promise; Is there a bug here? I don't think uninitialize() will resolve m_closeResolver, so where does it get resolved now? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to What do you mean by "allows the regular lock"? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:41: // because it must be guaranteed that the main thread keeps it alive while the What does "must be guaranteed that the main thread keeps it alive..." mean? Is it because a RefPtr would allow the main thread to delete it while the audio thread still wants it? Would the solution to this be to call ref() on it on creation so that there's another reference to it and deref it when the audio thread is done with it? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:110: // needs to be created and destructed in the same thread. "same thread" as what? And why does it need to be created and destroyed in the same thread?
https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:8: PASS Scheduling a suspend in the past rejected correctly (with InvalidStateError: cannot schedule a suspend at frame 17536 (0.399229 seconds) because it is earlier than the current frame of 22016). On 2015/07/15 20:59:13, Raymond Toy wrote: > Might be nice to have the current frame also include the time, like you did for > the suspend time. I was initialized confused by the suspend time of 0.399229 > sec because I was expecting something much closer to 0.4 sec, because the > suspend time was 0.5 sec (which gets quantized to 22016/44100 sec). The frame 17536 is a quantized frame number, but 0.399229 second is the non-quantized/original suspension time. It is because the current time after the suspension is not exact 0.5; it is 0.4992290249433107. (= 128/44100 * (344/2)) so taking 0.1 from it would be ~0.399229. https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:33: context.suspend(0.5).then(function () { On 2015/07/15 20:59:13, Raymond Toy wrote: > Should there be a test that this suspend actually happened at time 0.5? Isn't that considered as 'duplicate' time entry testing? https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic.html:70: Should('Resuming before suspend', context.resume()).beRejected(); On 2015/07/15 20:59:13, Raymond Toy wrote: > Isn't there a race here? What if GC happens between context.resume() and > startRendering()? Then resume() gets called after suspend(). Would chaining all these into a sequence of promises work? https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-eventhandler.html:41: // the out-of-bound error, but it will reject the promise and On 2015/07/15 20:59:13, Raymond Toy wrote: > "but it" -> "it" Done. https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-graph-manipulation.html:11: description('Test synchronous graph manipulation with OfflineAudioContext.suspend() and OfflineAudioContext.suspend().'); On 2015/07/15 20:59:13, Raymond Toy wrote: > One of these suspend()'s should be resume(). Oops. Done. https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-promise.html:53: }); On 2015/07/15 20:59:13, Raymond Toy wrote: > Would it be nicer to write startRendering() like this: > > startRendering().then(function () { ... }) > .then(finishJSTest); > Done. https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-sequence.html:71: }); On 2015/07/15 20:59:13, Raymond Toy wrote: > Would it be nicer to write this as > > startRendering().then(verifyResult).then(finishJSTest); Done. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/AbstractAudioContext.h:164: // different method signature. On 2015/07/15 20:59:13, Raymond Toy wrote: > Maybe comment that the first can only be used for online and the second only for > offline? Done. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/AudioContext.cpp:100: return promise; On 2015/07/15 20:59:14, Raymond Toy wrote: > Is there a bug here? I don't think uninitialize() will resolve m_closeResolver, > so where does it get resolved now? I blindly merged the ToT and change the location. I will look into the issue, but that should be another CL. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to On 2015/07/15 20:59:14, Raymond Toy wrote: > What do you mean by "allows the regular lock"? What I meant was this lock is not a 'tryLock()'. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:41: // because it must be guaranteed that the main thread keeps it alive while the On 2015/07/15 20:59:14, Raymond Toy wrote: > What does "must be guaranteed that the main thread keeps it alive..." mean? Is > it because a RefPtr would allow the main thread to delete it while the audio > thread still wants it? Would the solution to this be to call ref() on it on > creation so that there's another reference to it and deref it when the audio > thread is done with it? Conceptually, I believe it is cleaner to separate the role here; the main thread is the provider and the render thread is the user of SuspendContainer object. Based on the review from haraken@ and hiroshige@, I believe this is safe in terms of the cross-thread operation. If you have a better idea/design, I am open to that as well. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:110: // needs to be created and destructed in the same thread. On 2015/07/15 20:59:14, Raymond Toy wrote: > "same thread" as what? And why does it need to be created and destroyed in the > same thread? This object is created and destroyed in the main thread. This design is suggested by haraken@; if we can constrain the object to be created/destroyed in the same thread, we do not need |ThreadSafeRefCounted|. I think it is cleaner that way and also easier to apply oilpan in the future.
web-exposed API change LGTM https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... File Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... Source/modules/webaudio/AbstractAudioContext.h:252: // RefPtrWillBeMember<ScriptPromiseResolver> m_offlineResolver; Remove this line. https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to Such comment should be in the header.
rtoy@: PTAL. Fixing nits based on tkent@ comments after L-G-T-M. https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... File Source/modules/webaudio/AbstractAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... Source/modules/webaudio/AbstractAudioContext.h:252: // RefPtrWillBeMember<ScriptPromiseResolver> m_offlineResolver; On 2015/07/16 00:33:43, tkent wrote: > Remove this line. Done. https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/830001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to On 2015/07/16 00:33:43, tkent wrote: > Such comment should be in the header. Done.
https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... File LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt (right): https://codereview.chromium.org/1140723003/diff/810001/LayoutTests/webaudio/o... LayoutTests/webaudio/offlineaudiocontext-suspend-resume-basic-expected.txt:8: PASS Scheduling a suspend in the past rejected correctly (with InvalidStateError: cannot schedule a suspend at frame 17536 (0.399229 seconds) because it is earlier than the current frame of 22016). On 2015/07/15 23:24:22, hoch wrote: > On 2015/07/15 20:59:13, Raymond Toy wrote: > > Might be nice to have the current frame also include the time, like you did > for > > the suspend time. I was initialized confused by the suspend time of 0.399229 > > sec because I was expecting something much closer to 0.4 sec, because the > > suspend time was 0.5 sec (which gets quantized to 22016/44100 sec). > > The frame 17536 is a quantized frame number, but 0.399229 second is the > non-quantized/original suspension time. > > It is because the current time after the suspension is not exact 0.5; it is > 0.4992290249433107. (= 128/44100 * (344/2)) so taking 0.1 from it would be > ~0.399229. Yes, that's why I think it might be nice to print out the time for the current frame (22016) which would make it obvious why .399229 happens. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:110: // needs to be created and destructed in the same thread. On 2015/07/15 23:24:22, hoch wrote: > On 2015/07/15 20:59:14, Raymond Toy wrote: > > "same thread" as what? And why does it need to be created and destroyed in > the > > same thread? > > This object is created and destroyed in the main thread. > > This design is suggested by haraken@; if we can constrain the object to be > created/destroyed in the same thread, we do not need |ThreadSafeRefCounted|. I > think it is cleaner that way and also easier to apply oilpan in the future. Just change the comment from "same thread" to "main thread". It wasn't clear whether it was audio thread, offline thread, main thread or some possible mix of all as long as the creation/destruction was in the same thread.
https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to On 2015/07/15 23:24:22, hoch wrote: > On 2015/07/15 20:59:14, Raymond Toy wrote: > > What do you mean by "allows the regular lock"? > > What I meant was this lock is not a 'tryLock()'. Maybe it would be clearer if forceLock were named offlineContextLock or something? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:48: ASSERT(!isMainThread()); Can we also assert that this is not the audio thread (online context)? Or assert that this is the offline context thread? https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:41: // because it must be guaranteed that the main thread keeps it alive while the On 2015/07/15 23:24:22, hoch wrote: > On 2015/07/15 20:59:14, Raymond Toy wrote: > > What does "must be guaranteed that the main thread keeps it alive..." mean? > Is > > it because a RefPtr would allow the main thread to delete it while the audio > > thread still wants it? Would the solution to this be to call ref() on it on > > creation so that there's another reference to it and deref it when the audio > > thread is done with it? > > Conceptually, I believe it is cleaner to separate the role here; the main thread > is the provider and the render thread is the user of SuspendContainer object. > Based on the review from haraken@ and hiroshige@, I believe this is safe in > terms of the cross-thread operation. > > If you have a better idea/design, I am open to that as well. Acknowledged.
https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.cpp (right): https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:46: // This allows the regular lock in offline audio rendering. It is okay to On 2015/07/16 16:52:04, Raymond Toy wrote: > On 2015/07/15 23:24:22, hoch wrote: > > On 2015/07/15 20:59:14, Raymond Toy wrote: > > > What do you mean by "allows the regular lock"? > > > > What I meant was this lock is not a 'tryLock()'. > > Maybe it would be clearer if forceLock were named offlineContextLock or > something? Done. https://codereview.chromium.org/1140723003/diff/810001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.cpp:48: ASSERT(!isMainThread()); On 2015/07/16 16:52:04, Raymond Toy wrote: > Can we also assert that this is not the audio thread (online context)? Or > assert that this is the offline context thread? This is not possible. The offline render thread is the audio thread; calling render() sets the offline render thread to the audio thread for every render quantum. Also DeferredTaskHandler does not have a clue about the type of context. So we cannot query like |isOfflineContext()| here. I think keeping them separate is better and that's why I was hesitant to include 'offline' in the method name.
lgtm with nit. https://codereview.chromium.org/1140723003/diff/870001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.h (right): https://codereview.chromium.org/1140723003/diff/870001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.h:113: // lock the offline audio render thread because it is not real-time thread. Nit: Add comment this must only be used for offline contexts.
Thank you all for the review! https://codereview.chromium.org/1140723003/diff/870001/Source/modules/webaudi... File Source/modules/webaudio/DeferredTaskHandler.h (right): https://codereview.chromium.org/1140723003/diff/870001/Source/modules/webaudi... Source/modules/webaudio/DeferredTaskHandler.h:113: // lock the offline audio render thread because it is not real-time thread. On 2015/07/17 15:44:03, Raymond Toy wrote: > Nit: Add comment this must only be used for offline contexts. Done.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, tkent@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/1140723003/#ps910001 (title: "Added RELEASE_ASSERT_NOT_REACHED() for non-reachable code paths")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140723003/910001
Message was sent while issue was closed.
Committed patchset #25 (id:910001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199129
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:910001) has been created in https://codereview.chromium.org/1237383004/ by hongchan@chromium.org. The reason for reverting is: Leak detected. (numberOfLiveActiveDOMObjects is non-zero).
haraken@ PTAL at the latest PS. I had to revert all the raw pointer approach since it caused the leak. The leaking log: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea... So I reverted the raw pointer class to ThreadSafeRefCounted and it does not leak any more. (verified locally with content_shell --enable-leak-detection) WDYT?
On 2015/07/20 20:34:11, hoch wrote: > haraken@ > > PTAL at the latest PS. I had to revert all the raw pointer approach since it > caused the leak. > > The leaking log: > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea... > > So I reverted the raw pointer class to ThreadSafeRefCounted and it does not leak > any more. (verified locally with content_shell --enable-leak-detection) > > WDYT? Why does the raw pointer leak whereas ThreadSafeRefCounted doesn't leak?
I've been looking at it but wasn't able to find the cause in the raw-pointer version. I will keep investigating, but also want to push this change to ToT so we can resolve a couple of flaky layout tests in Web Audio API. On Mon, Jul 20, 2015 at 4:19 PM <haraken@chromium.org> wrote: > On 2015/07/20 20:34:11, hoch wrote: > > haraken@ > > > PTAL at the latest PS. I had to revert all the raw pointer approach since > > it > > caused the leak. > > > The leaking log: > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea... > > > So I reverted the raw pointer class to ThreadSafeRefCounted and it does > > not > leak > > any more. (verified locally with content_shell --enable-leak-detection) > > > WDYT? > > Why does the raw pointer leak whereas ThreadSafeRefCounted doesn't leak? > > > https://codereview.chromium.org/1140723003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/07/20 23:39:48, blink-reviews wrote: > I've been looking at it but wasn't able to find the cause in the > raw-pointer version. I will keep investigating, but also want to push this > change to ToT so we can resolve a couple of flaky layout tests in Web Audio > API. Perhaps my understanding and comments about the raw pointers to ScheduledSuspendContainer were wrong. I thought |suspendContainer| is owned by someone on the main thread and thus posted tasks do not have to hold its ownership, but my new understandings are: > ScheduledSuspendContainer* suspendContainer = ScheduledSuspendContainer::create(when, quantizedFrame, resolver); // [1] > ... > destinationHandler().offlineRenderThread()->postTask(FROM_HERE, > threadSafeBind(&OfflineAudioContext::validateSuspendContainerOnRenderThread, this, AllowCrossThreadAccess(suspendContainer))); // [2] - |suspendContainer| is created in [1]. - The ownership of |suspendContainer| is moved to the posted task in [2]. - In validateSuspendContainerOnRenderThread(), |suspendContainer| is added to |m_scheduledSuspends| (so the ownership of |suspendContainer| is moved to |m_scheduledSuspends|), or reposted to rejectSuspendContainerOnMainThread(). - In rejectSuspendContainerOnMainThread(), |suspendContainer| must be destructed. In resolvePendingSuspendPromises(): > ScheduledSuspendContainer* suspendContainer = it->value; > m_scheduledSuspends.remove(it); > Platform::current()->mainThread()->postTask(FROM_HERE, > threadSafeBind(&OfflineAudioContext::resolveSuspendContainerOnMainThread, this, AllowCrossThreadAccess(suspendContainer))); - The ownership of |suspendContainer| is moved from |m_scheduledSuspends| to the posted task. - In resolveSuspendContainerOnMainThread(), |suspendContainer| must be destructed. If my understandings above are correct, then ScheduledSuspendContainer leaks because: - If the posted task is destructed before executed (e.g. when the thread terminates), |suspendContainer| is not destructed, because the posted task contains a raw pointer and does not own it. - In rejectSuspendContainerOnMainThread(), |suspendContainer| is not destructed. - In resolveSuspendContainerOnMainThread(), |suspendContainer| is not destructed. And using PassOwnPtr<ScheduledSuspendContainer> instead of ScheduledSuspendContainer* + AllowCrossThreadAccess() is a solution. Managing ScheduledSuspendContainer by OwnPtr<ScheduledSuspendContainer> throughout of the CL will be good for make the ownership of ScheduledSuspendContainer explicit. For example: > using SuspendContainerMap = HashMap<size_t, OwnPtr<ScheduledSuspendContainer>, ...>; > PassOwnPtr<ScheduledSuspendContainer> ScheduledSuspendContainer::create(...); > void validateSuspendContainerOnRenderThread(PassOwnPtr<ScheduledSuspendContainer>); > OwnPtr<ScheduledSuspendContainer> suspendContainer = ScheduledSuspendContainer::create(when, quantizedFrame, resolver); > destinationHandler().offlineRenderThread()->postTask(FROM_HERE, > threadSafeBind(&OfflineAudioContext::validateSuspendContainerOnRenderThread, this, suspendContainer.release())); (and also do similar for related methods)
https://codereview.chromium.org/1140723003/diff/910001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/910001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:277: threadSafeBind(&OfflineAudioContext::validateSuspendContainerOnRenderThread, this, AllowCrossThreadAccess(suspendContainer))); My solution of using OwnPtr doesn't ensure ScheduledSuspendContainer to be destructed on the main thread. If the offline render thread dies after this postTask() and before the task is executed, then |suspendContainer| will be destructed on the offline render thread. A possible fix would be: The main thread maintains the ownership of ScheduledSuspendContainer, and the offline render thread and posted tasks do not take the ownership. - Add a member with type of "set of ScheduledSuspendContainer*" [1] to OfflineAudioContext, which is maintained on the main thread and takes the ownership of ScheduledSuspendContainer. - Pass ScheduledSuspendContainer* or WeakPtr<ScheduledSuspendContainer> to the offline render thread here via threadSafeBind() + postTask(). AllowCrossThreadAccess() would be needed to pass the raw pointers or WeakPtrs. - The offline render thread does not dereference the pointers. setErrorMessageForRejection() can be removed by moving its parameters to rejectSuspendContainerOnMainThread(), suspendContainer->suspendFrame() and suspendContainer->suspendTime() can be removed by adding them as parameters of validateSuspendContainerOnRenderThread(). - In rejectSuspendContainerOnMainThread() and resolveSuspendContainerOnMainThread(), remove |suspendedCountainer| from the set [1] and destruct |*suspendedCountainer|. - In ~OfflineAudioContext(), delete |p| for each |p| in the set [1]. Using WeakPtr<ScheduledSuspendContainer> is thread-safe here because WeakPtr is dereferenced and invalidated on the main thread only (the offline render thread does/should not dereference WeakPtrs). Using raw pointers might be also feasible because ScheduledSuspendContainer objects are guaranteed to be alive until resolve/rejectSuspendContainerOnMainThread() is called or ~OfflineAudioContext() is called. (this is also not so clear/simple, the invariants are somehow hard to infer/ensure, though...any better ideas?)
hiroshige@ and rtoy@ PTAL at PS27. Now it became meaningless to keep |ScheduledSuspendContainer| class because all the necessary information is being passed between threads as parameters. So we do not need to keep the time and the frame info inside of the container. Here are two options: 1) Remove |ScheduledSuspendContainer| class and directly handle ScriptPromiseResolver with the associated frame information. 2) Keep the container class for the future. It might be handy when we need to put additional information for the scheduled suspend. FYI, I've confirmed PS27 locally with `--enable-leak-detection` option. It does not leak as well. WDYT? https://codereview.chromium.org/1140723003/diff/910001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/910001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:277: threadSafeBind(&OfflineAudioContext::validateSuspendContainerOnRenderThread, this, AllowCrossThreadAccess(suspendContainer))); On 2015/07/21 10:50:48, hiroshige wrote: > My solution of using OwnPtr doesn't ensure ScheduledSuspendContainer to be > destructed on the main thread. > If the offline render thread dies after this postTask() and before the task is > executed, then |suspendContainer| will be destructed on the offline render > thread. > > A possible fix would be: > The main thread maintains the ownership of ScheduledSuspendContainer, and the > offline render thread and posted tasks do not take the ownership. > - Add a member with type of "set of ScheduledSuspendContainer*" [1] to > OfflineAudioContext, which is maintained on the main thread and takes the > ownership of ScheduledSuspendContainer. > - Pass ScheduledSuspendContainer* or WeakPtr<ScheduledSuspendContainer> to the > offline render thread here via threadSafeBind() + postTask(). > AllowCrossThreadAccess() would be needed to pass the raw pointers or WeakPtrs. > - The offline render thread does not dereference the pointers. > setErrorMessageForRejection() can be removed by moving its parameters to > rejectSuspendContainerOnMainThread(), suspendContainer->suspendFrame() and > suspendContainer->suspendTime() can be removed by adding them as parameters of > validateSuspendContainerOnRenderThread(). > - In rejectSuspendContainerOnMainThread() and > resolveSuspendContainerOnMainThread(), remove |suspendedCountainer| from the set > [1] and destruct |*suspendedCountainer|. > - In ~OfflineAudioContext(), delete |p| for each |p| in the set [1]. > > Using WeakPtr<ScheduledSuspendContainer> is thread-safe here because WeakPtr is > dereferenced and invalidated on the main thread only (the offline render thread > does/should not dereference WeakPtrs). > Using raw pointers might be also feasible because ScheduledSuspendContainer > objects are guaranteed to be alive until > resolve/rejectSuspendContainerOnMainThread() is called or ~OfflineAudioContext() > is called. > > (this is also not so clear/simple, the invariants are somehow hard to > infer/ensure, though...any better ideas?) Done. I accepted your idea and created PS27.
https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:154: // be alive until the task is finished somewhere else. How about: Passing a raw pointer |suspendContainer| is safe here because it is created, destructed and owned (by |m_suspendContainers|) on the main thread. Also it is guaranteed to be alive until: - the raw pointer is passed back to the main thread and resolveSuspendContainerOnMainThread()/rejectSuspendContainerOnMainThread() is called on the main thread, or - OfflineAudioContext is destructed. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:372: errorMessage.isolatedCopy())); .isolatedCopy() is not needed because it is called automatically inside threadSafeBind(). https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:384: Please add RELEASE_ASSERT before dereferencing |suspendContainer| that |suspendContainer| is in |m_suspendContainers| before remove(). (If |suspendContainer| is not in the set, then |*suspendContainer| is probably already deleted and calling relectPromise() will be use-after-free, so RELEASE_ASSERT). https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:398: ditto. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:98: // raw pointers. Please add a comment that this field is accessed only on the main thread. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: HashSet<ScheduledSuspendContainer*> m_suspendContainers; nit optional: the names of |m_suspendedContainers| and |m_scheduledSuspends| is somehow similar and possibly confusing. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:101: // This map is to check the timing of scheduled suspends. Please add a comment that this field is accessed only on the offline render thread and its values (i.e. ScheduledSuspendContainer*) must not be dereferenced.
https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:291: threadSafeBind(&OfflineAudioContext::validateSuspendContainerOnRenderThread, this, IIUC, 1. OfflineAudioContext is GarbageCollected. 2. offlineRenderThread() is not a Oilpan-attached thread. From 1., threadSafeBind() passes a reference of OfflineAudioContext as CrossThreadPersistent<OfflineAudioContext> to offlineRenderThread() (See PointerParamStorageTraits<T*, true> in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). But holding/manipulating CrossThreadPersistent on a non-Oilpan thread (i.e. the offline render thread) might not be safe? haraken@, any comments?
hiroshige@ PTAL at PS28. Basically I accepted your suggestion and will wait for the review from haraken@. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:154: // be alive until the task is finished somewhere else. On 2015/07/22 17:32:12, hiroshige wrote: > How about: > Passing a raw pointer |suspendContainer| is safe here because it is > created, destructed and owned (by |m_suspendContainers|) on the main thread. > Also it is guaranteed to be alive until: > - the raw pointer is passed back to the main thread and > resolveSuspendContainerOnMainThread()/rejectSuspendContainerOnMainThread() is > called on the main thread, or > - OfflineAudioContext is destructed. Done. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:372: errorMessage.isolatedCopy())); On 2015/07/22 17:32:12, hiroshige wrote: > .isolatedCopy() is not needed because it is called automatically inside > threadSafeBind(). Done. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:384: On 2015/07/22 17:32:12, hiroshige wrote: > Please add RELEASE_ASSERT before dereferencing |suspendContainer| that > |suspendContainer| is in |m_suspendContainers| before remove(). > (If |suspendContainer| is not in the set, then |*suspendContainer| is probably > already deleted and calling relectPromise() will be use-after-free, so > RELEASE_ASSERT). Done. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.cpp:398: On 2015/07/22 17:32:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... File Source/modules/webaudio/OfflineAudioContext.h (right): https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:98: // raw pointers. On 2015/07/22 17:32:12, hiroshige wrote: > Please add a comment that this field is accessed only on the main thread. Done. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:99: HashSet<ScheduledSuspendContainer*> m_suspendContainers; On 2015/07/22 17:32:12, hiroshige wrote: > nit optional: the names of |m_suspendedContainers| and |m_scheduledSuspends| is > somehow similar and possibly confusing. Acknowledged. https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... Source/modules/webaudio/OfflineAudioContext.h:101: // This map is to check the timing of scheduled suspends. On 2015/07/22 17:32:12, hiroshige wrote: > Please add a comment that this field is accessed only on the offline render > thread and its values (i.e. ScheduledSuspendContainer*) must not be > dereferenced. Done.
On 2015/07/22 20:36:35, hoch wrote: > hiroshige@ > > PTAL at PS28. Basically I accepted your suggestion and will wait for the review > from haraken@. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > File Source/modules/webaudio/OfflineAudioContext.cpp (right): > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:154: // be alive until the task > is finished somewhere else. > On 2015/07/22 17:32:12, hiroshige wrote: > > How about: > > Passing a raw pointer |suspendContainer| is safe here because it is > > created, destructed and owned (by |m_suspendContainers|) on the main thread. > > Also it is guaranteed to be alive until: > > - the raw pointer is passed back to the main thread and > > resolveSuspendContainerOnMainThread()/rejectSuspendContainerOnMainThread() is > > called on the main thread, or > > - OfflineAudioContext is destructed. > > Done. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:372: > errorMessage.isolatedCopy())); > On 2015/07/22 17:32:12, hiroshige wrote: > > .isolatedCopy() is not needed because it is called automatically inside > > threadSafeBind(). > > Done. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:384: > On 2015/07/22 17:32:12, hiroshige wrote: > > Please add RELEASE_ASSERT before dereferencing |suspendContainer| that > > |suspendContainer| is in |m_suspendContainers| before remove(). > > (If |suspendContainer| is not in the set, then |*suspendContainer| is probably > > already deleted and calling relectPromise() will be use-after-free, so > > RELEASE_ASSERT). > > Done. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.cpp:398: > On 2015/07/22 17:32:12, hiroshige wrote: > > ditto. > > Done. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > File Source/modules/webaudio/OfflineAudioContext.h (right): > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.h:98: // raw pointers. > On 2015/07/22 17:32:12, hiroshige wrote: > > Please add a comment that this field is accessed only on the main thread. > > Done. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.h:99: > HashSet<ScheduledSuspendContainer*> m_suspendContainers; > On 2015/07/22 17:32:12, hiroshige wrote: > > nit optional: the names of |m_suspendedContainers| and |m_scheduledSuspends| > is > > somehow similar and possibly confusing. > > Acknowledged. > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > Source/modules/webaudio/OfflineAudioContext.h:101: // This map is to check the > timing of scheduled suspends. > On 2015/07/22 17:32:12, hiroshige wrote: > > Please add a comment that this field is accessed only on the offline render > > thread and its values (i.e. ScheduledSuspendContainer*) must not be > > dereferenced. > > Done. Also it is required to file a bug with a repro case when RELEASE_ASSERT() is used. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... However, I am not sure about the actual scenario that RELEASE_ASSERT() can fails, especially from the code written in JavaScript.
haraken@ hiroshige@: PTAL at PS29. There are two issues: 1) Based on the hiroshige@'s suggestion, I've added RELEASE_ASSERT() when the raw pointer is destructed. it is required to file a bug with a repro case when RELEASE_ASSERT() is used. However, I am not sure about the actual scenario that RELEASE_ASSERT() can fails, especially from the code written in JavaScript. 2) I think we still need to address the question hiroshige@ brought up in the last PS: (perhaps haraken@ can gave us the confirmation on this) https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... "holding/manipulating CrossThreadPersistent on a non-Oilpan thread (i.e. theoffline render thread) might not be safe?"
On 2015/07/27 18:41:26, hoch wrote: > haraken@ hiroshige@: PTAL at PS29. > > There are two issues: > > 1) Based on the hiroshige@'s suggestion, I've added RELEASE_ASSERT() when the > raw pointer is destructed. it is required to file a bug with a repro case when > RELEASE_ASSERT() is used. However, I am not sure about the actual scenario that > RELEASE_ASSERT() can fails, especially from the code written in JavaScript. > > 2) I think we still need to address the question hiroshige@ brought up in the > last PS: (perhaps haraken@ can gave us the confirmation on this) > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > > "holding/manipulating CrossThreadPersistent on a non-Oilpan thread (i.e. > theoffline render thread) might not be safe?" That is not safe. When accessing a CrossThreadPersistent from a non-oilpan thread, it means that the non-oilpan thread is accessing an on-heap object. At that point, oilpan threads may be performing a stop-the-world garbage collection. That is problematic. CrossThreadPersistent can be created and destructed in different threads, which don't need to be oilpan threads. However, only oilpan threads are allowed to use the CrossThreadPersistent. (I'm sorry about the weird programming rule -- I'm planning to propose a substantial refactoring to make the threading model of Oilpan clearer.)
haraken@ I gather that this is your suggestion for the ThreadSafeBind() issue here: > CrossThreadPersistent can be created and destructed in different threads, > which don't need to be oilpan threads. However, only oilpan threads are > allowed to use the CrossThreadPersistent. First off, CrossThreadPersistent in ThreadSafeBind() here is OfflineAudioContext. I am not sure how this can be created/destructed in different thread rather than the main thread. So this is a bit confusing to me. Also note that posting a task goes both ways 1) main thread (oilpan) => render thread (non-oilpan) : createSuspend() 2) render thread (non-oilpan) => main thread (oilpan) : reject/resolveSuspendContainerOnMainThread() So we just saw that 1) is not safe. How about 2)? If both ways are unsafe, I think we have a structural problem that needs a complete do-over. At some point, rtoy@ suggested that we can use a simple lock mechanism for the cross-thread operation. Should I consider it instead of the current approach? Thanks. On Mon, Jul 27, 2015 at 12:34 PM <haraken@chromium.org> wrote: > On 2015/07/27 18:41:26, hoch wrote: > > haraken@ hiroshige@: PTAL at PS29. > > > There are two issues: > > > 1) Based on the hiroshige@'s suggestion, I've added RELEASE_ASSERT() > when > > the > > raw pointer is destructed. it is required to file a bug with a repro case > > when > > RELEASE_ASSERT() is used. However, I am not sure about the actual > scenario > that > > RELEASE_ASSERT() can fails, especially from the code written in > > JavaScript. > > > 2) I think we still need to address the question hiroshige@ brought up > in > > the > > last PS: (perhaps haraken@ can gave us the confirmation on this) > > > > https://codereview.chromium.org/1140723003/diff/950001/Source/modules/webaudi... > > > "holding/manipulating CrossThreadPersistent on a non-Oilpan thread (i.e. > > theoffline render thread) might not be safe?" > > That is not safe. When accessing a CrossThreadPersistent from a non-oilpan > thread, it means that the non-oilpan thread is accessing an on-heap object. > At > that point, oilpan threads may be performing a stop-the-world garbage > collection. That is problematic. > > CrossThreadPersistent can be created and destructed in different threads, > which > don't need to be oilpan threads. However, only oilpan threads are allowed > to use > the CrossThreadPersistent. > > (I'm sorry about the weird programming rule -- I'm planning to propose a > substantial refactoring to make the threading model of Oilpan clearer.) > > > https://codereview.chromium.org/1140723003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/07/27 20:14:02, blink-reviews wrote: > haraken@ > > I gather that this is your suggestion for the ThreadSafeBind() issue here: > > > CrossThreadPersistent can be created and destructed in different threads, > > which don't need to be oilpan threads. However, only oilpan threads are > > allowed to use the CrossThreadPersistent. > > First off, CrossThreadPersistent in ThreadSafeBind() here is > OfflineAudioContext. > I am not sure how this can be created/destructed in different thread rather > than > the main thread. So this is a bit confusing to me. > > Also note that posting a task goes both ways > 1) main thread (oilpan) => render thread (non-oilpan) : createSuspend() > 2) render thread (non-oilpan) => main thread (oilpan) : > reject/resolveSuspendContainerOnMainThread() > > So we just saw that 1) is not safe. How about 2)? > > If both ways are unsafe, I think we have a structural problem that needs a > complete do-over. At some point, rtoy@ suggested that we can use a > simple lock mechanism for the cross-thread operation. Should I consider > it instead of the current approach? Just to be clear, the suggestion was to make the main thread and the offline context thread synchronized and therefore proceed in lock-step on each rendering quantum. We can do this because there is no issue with glitching on the offline thread. This could mean switching threads every 3 ms (or less) which is probably a pretty bad idea. (This could be optimized so that we don't, but fundamentally if suspends are scheduled often enough, 3 ms thread hops will happen. But that can happen now with this implementation anyway.)
On 2015/07/27 18:41:26, hoch wrote: > 1) Based on the hiroshige@'s suggestion, I've added RELEASE_ASSERT() when the > raw pointer is destructed. it is required to file a bug with a repro case when > RELEASE_ASSERT() is used. However, I am not sure about the actual scenario that > RELEASE_ASSERT() can fails, especially from the code written in JavaScript. I'm also not sure when the RELEASE_ASSERT() can fail. If RELEASE_ASSERT() should fail, then it would tell us that/when the RELEASE_ASSERT() can fail, and it would avoid use-after-free (by immediately crashing). I suspect this can fail e.g. if there is a threading bug that we couldn't catch. The object ownership management of SuspendedContainer and its threading here are complex and thus I expect more chances of bugs than normal RefPtr or GC-based object management. > it is required to file a bug with a repro case when RELEASE_ASSERT() is used. I think this means we must file a bug using the security template, when we found runtime crashes at RELEASE_ASSERT(), not when we commit code using RELEASE_ASSERT().
On 2015/07/27 20:14:02, blink-reviews wrote: > haraken@ > > I gather that this is your suggestion for the ThreadSafeBind() issue here: > > > CrossThreadPersistent can be created and destructed in different threads, > > which don't need to be oilpan threads. However, only oilpan threads are > > allowed to use the CrossThreadPersistent. > > First off, CrossThreadPersistent in ThreadSafeBind() here is > OfflineAudioContext. > I am not sure how this can be created/destructed in different thread rather > than > the main thread. So this is a bit confusing to me. > > Also note that posting a task goes both ways > 1) main thread (oilpan) => render thread (non-oilpan) : createSuspend() > 2) render thread (non-oilpan) => main thread (oilpan) : > reject/resolveSuspendContainerOnMainThread() > > So we just saw that 1) is not safe. How about 2)? > > If both ways are unsafe, I think we have a structural problem that needs a > complete do-over. At some point, rtoy@ suggested that we can use a > simple lock mechanism for the cross-thread operation. Should I consider > it instead of the current approach? I'm confused. It is safe to keep a CrossThreadPersistent from an non-oilpan thread to keep the on-heap object alive. The only restriction is that you cannot use the CrossThreadPersistent (i.e., you cannot use m_persistent->...) from an non-oilpan thread. Is the offline context thread doing that?
On 2015/07/28 01:43:09, hiroshige wrote: > On 2015/07/27 18:41:26, hoch wrote: > > 1) Based on the hiroshige@'s suggestion, I've added RELEASE_ASSERT() when the > > raw pointer is destructed. it is required to file a bug with a repro case when > > RELEASE_ASSERT() is used. However, I am not sure about the actual scenario > that > > RELEASE_ASSERT() can fails, especially from the code written in JavaScript. > I'm also not sure when the RELEASE_ASSERT() can fail. > If RELEASE_ASSERT() should fail, then it would tell us that/when the > RELEASE_ASSERT() can fail, and it would avoid use-after-free (by immediately > crashing). > > I suspect this can fail e.g. if there is a threading bug that we couldn't catch. > The object ownership management of SuspendedContainer and its threading here are > complex and thus I expect more chances of bugs than normal RefPtr or GC-based > object management. > > > it is required to file a bug with a repro case when RELEASE_ASSERT() is used. > I think this means we must file a bug using the security template, when we found > runtime crashes at RELEASE_ASSERT(), not when we commit code using > RELEASE_ASSERT(). Thanks. Understood.
> The only restriction is that you cannot use the CrossThreadPersistent (i.e., > you cannot use m_persistent->...) from an non-oilpan thread. There is no explicit instance of 'm_persistent->' in the render thread code. We have three instances of ThreadSafeBind(): (where M is the main thread, R is the render thread) 1) suspendContext(M) => validateSuspendContainerOnRenderThread(R) 2) validateSuspendContainerOnRenderThread(R) => rejectSuspendContainerOnMainThread(M) 3) resolvingPendingSuspendPromises(R) => resolveSuspendContainerOnMainThread(M) As we discussed, the problem is the instance 1) because the render thread is not covered by oilpan. Note that validateSuspendContainerOnRenderThread() accesses few members of OfflineAudioContext(); currentSampleFrame(), currentTime() and m_scheduledSuspendContainer. I am not sure if this violates the restriction you mentioned. Even if ThreadSafeBind() passes CrossThreadPersistent<OfflineAudioContext> as an argument, but I believe it is not being used in the code. Please correct me if I am wrong.
On 2015/07/28 16:39:52, hoch wrote: > > The only restriction is that you cannot use the CrossThreadPersistent (i.e., > > you cannot use m_persistent->...) from an non-oilpan thread. > > There is no explicit instance of 'm_persistent->' in the render thread code. > > We have three instances of ThreadSafeBind(): > (where M is the main thread, R is the render thread) > > 1) suspendContext(M) => validateSuspendContainerOnRenderThread(R) > 2) validateSuspendContainerOnRenderThread(R) => > rejectSuspendContainerOnMainThread(M) > 3) resolvingPendingSuspendPromises(R) => resolveSuspendContainerOnMainThread(M) > > As we discussed, the problem is the instance 1) because the render thread is > not covered by oilpan. > > Note that validateSuspendContainerOnRenderThread() accesses few members of > OfflineAudioContext(); currentSampleFrame(), currentTime() and > m_scheduledSuspendContainer. I am not sure if this violates the restriction you > mentioned. Even if ThreadSafeBind() passes > CrossThreadPersistent<OfflineAudioContext> as an argument, but I believe it is > not being used in the code. > > Please correct me if I am wrong. Sorry, I'm getting lost :) Would you help me remember what our current problem is?
> Sorry, I'm getting lost :) Would you help me remember what our current problem > is? No, thank you for reviewing this CL. I am still learning how the oilpan and the non-oilpan part can interact. The core issue here is that OfflineAudioContext uses two threads and one of them are non-oilpan thread (offline audio render thread). 1. When the user schedules a suspend, the suspend object goes into two storages: m_scheduledSuspendContainers and m_suspendContainers. The former is to keep a list of scheduled suspends so we can actually suspend the context at the specified time on the render thread. The latter is to hold the ownership of SuspendContainer object on the main thread and to manage the lifetime of references manually. So we can guarantee the reference of SuspendContainer is created and destructed only on the main thread. (See PS27) 2. Therefore there is a transaction between the main thread and the render thread when we schedule a suspend. In order to pass the arguments between two threads ThreadSafeBind() is used. The question raised by hiroshige@ (PS 27) was: - OfflineAudioContext is oilpan-attached whereas the render thread is not. - ThreadSafeBind() passes CrossThreadPersistent<OfflineAudioContext> the to non-oilpan render thread. - Holding/manipulating CrossThreadPersistent on a non-oilpan thread might not be safe. 3. So your most recent comments confirmed it is not safe if the explicit access to m_persistent of CrossThreadPersistent happens on the non-oilpan thread. However, I am not entirely sure about the meaning of this. Even though m_persistent (CrossThreadPersistent<OfflineAudioContext>) is not directly accessed, validateSuspendContainerOnRenderThread() needs to access the member of OfflineAudioContext so the scheduled suspend can be stored in the map. So if we cannot access the member of OfflineAudioContext in the render thread, there is nothing we can do. I have to reconsider the approach from scratch. 4. So my follow up question was: if the code on the render thread does not explicitly access CrossThreadPersistent<OfflineAudioContext> but OfflineAudioContext is it safe?
Thanks for a lot of explanations. Now I understand the problem. On 2015/07/28 17:31:20, hoch wrote: > > Sorry, I'm getting lost :) Would you help me remember what our current problem > > is? > > No, thank you for reviewing this CL. I am still learning how the oilpan and the > non-oilpan part can interact. > > The core issue here is that OfflineAudioContext uses two threads and one of them > are non-oilpan thread (offline audio render thread). > > 1. > When the user schedules a suspend, the suspend object goes into two storages: > m_scheduledSuspendContainers and m_suspendContainers. The former is to keep a > list > of scheduled suspends so we can actually suspend the context at the specified > time on the render thread. The latter is to hold the ownership of > SuspendContainer object on the main thread and to manage the lifetime of > references manually. So we can guarantee the reference of SuspendContainer is > created and destructed only on the main thread. (See PS27) > > 2. > Therefore there is a transaction between the main thread and the render thread > when we schedule a suspend. In order to pass the arguments between two threads > ThreadSafeBind() is used. The question raised by hiroshige@ (PS 27) was: > > - OfflineAudioContext is oilpan-attached whereas the render thread is not. > - ThreadSafeBind() passes CrossThreadPersistent<OfflineAudioContext> the > to non-oilpan render thread. > - Holding/manipulating CrossThreadPersistent on a non-oilpan thread might not > be safe. > > 3. > So your most recent comments confirmed it is not safe if the explicit access > to m_persistent of CrossThreadPersistent happens on the non-oilpan thread. > However, I am not entirely sure about the meaning of this. > > Even though m_persistent (CrossThreadPersistent<OfflineAudioContext>) is not > directly accessed, validateSuspendContainerOnRenderThread() needs to access > the member of OfflineAudioContext so the scheduled suspend can be stored in the > map. So if we cannot access the member of OfflineAudioContext in the render > thread, there is nothing we can do. I have to reconsider the approach from > scratch. > > 4. > So my follow up question was: if the code on the render thread does not > explicitly access CrossThreadPersistent<OfflineAudioContext> but > OfflineAudioContext is it safe? This is unsafe either. You're not allowed to touch any on-heap object from a non-oilpan thread. I have a couple of questions: - Even without Oilpan, it looks problematic to allow the render thread to touch the AudioContext. How can we guarantee that the access to the AudioContext doesn't cause a threading race? - Blink's threading model is based on message passing. We don't want to share data (like AudioContext) between Blink threads. - Would it be possible to post a task from the render thread to the main thread to access the AudioContext members? - How is the normal render thread (not the offline render thread) working? I guess the normal render thread needs to touch the AudioContext as well.
> This is unsafe either. You're not allowed to touch any on-heap object from a > non-oilpan thread. 'Touch' here means 'read' or 'read/write'? > - Even without Oilpan, it looks problematic to allow the render thread to touch > the AudioContext. How can we guarantee that the access to the AudioContext > doesn't cause a threading race? Okay, but the overall structure here I followed the example from the realtime AudioContext. So I think we already have this kind of patterns in there as well, but I have to go through the code with your comments in mind. > - Blink's threading model is based on message passing. We don't want to share > data (like AudioContext) between Blink threads. Okay. I did not know the convention. However, we have been already doing that in AudioContext; ThreadSafeBind(&someFunc, this) - where 'this' is AudioContext. > - Would it be possible to post a task from the render thread to the main thread > to access the AudioContext members? Then will all this work? It seems like an only solution to me right now. > - How is the normal render thread (not the offline render thread) working? I > guess the normal render thread needs to touch the AudioContext as well. From what I see, the normal (real-time) render thread touches AudioContext through DeferredTaskHandler. It is non-oilpan;ThreadSafeRefCounted. If you have any suggestion, I would love to listen. Otherwise, I think I will have to re-approach this problem from a different angle. Possibly from scratch.
On 2015/07/28 22:49:06, hoch wrote: > > This is unsafe either. You're not allowed to touch any on-heap object from a > > non-oilpan thread. > > 'Touch' here means 'read' or 'read/write'? > > > - Even without Oilpan, it looks problematic to allow the render thread to > touch > > the AudioContext. How can we guarantee that the access to the AudioContext > > doesn't cause a threading race? > > Okay, but the overall structure here I followed the example from the realtime > AudioContext. So I think we already have this kind of patterns in there as well, > but I have to go through the code with your comments in mind. > > > - Blink's threading model is based on message passing. We don't want to share > > data (like AudioContext) between Blink threads. > > Okay. I did not know the convention. However, we have been already doing that in > AudioContext; ThreadSafeBind(&someFunc, this) - where 'this' is AudioContext. > > > - Would it be possible to post a task from the render thread to the main > thread > > to access the AudioContext members? > > Then will all this work? It seems like an only solution to me right now. > > > - How is the normal render thread (not the offline render thread) working? I > > guess the normal render thread needs to touch the AudioContext as well. > > From what I see, the normal (real-time) render thread touches AudioContext > through DeferredTaskHandler. It is non-oilpan;ThreadSafeRefCounted. This sounds like a reasonable design. - DeferredTaskHandler is ThreadSafeRefCounted - AudioContext is GarbageCollected. - The AudioContext is read/written only by the main thread (i.e., when the real-time render thread wants to read/write the AudioContext, it needs to ask the DeferredTaskHandler to do the work in the main thread). Before talking about oilpan, we must design OfflineAudioContext in a way in which it is read/written only by one thread. > > - Even without Oilpan, it looks problematic to allow the render thread to > touch > > the AudioContext. How can we guarantee that the access to the AudioContext > > doesn't cause a threading race? > > Okay, but the overall structure here I followed the example from the realtime > AudioContext. So I think we already have this kind of patterns in there as well, > but I have to go through the code with your comments in mind. But the real-time render thread uses AudioContext only via the DeferredTaskHandler and thus it's guaranteed that the AudioContext is read/written only by the main thread, right?
> This sounds like a reasonable design. > > - DeferredTaskHandler is ThreadSafeRefCounted > - AudioContext is GarbageCollected. > - The AudioContext is read/written only by the main thread (i.e., when the > real-time render thread wants to read/write the AudioContext, it needs to ask > the DeferredTaskHandler to do the work in the main thread). > > Before talking about oilpan, we must design OfflineAudioContext in a way in > which it is read/written only by one thread. > > But the real-time render thread uses AudioContext only via the > DeferredTaskHandler and thus it's guaranteed that the AudioContext is > read/written only by the main thread, right? Yes, I should have thought about the design more carefully. Since DeferredTaskHandler is a part of AbstractAudioContext, so OfflineAudioContext can also benefit from it. It seems like a high level change from the current status of this CL, so I might have to start a new CL from scratch. Thanks for your review.
On 2015/07/29 15:21:54, hoch wrote: > > This sounds like a reasonable design. > > > > - DeferredTaskHandler is ThreadSafeRefCounted > > - AudioContext is GarbageCollected. > > - The AudioContext is read/written only by the main thread (i.e., when the > > real-time render thread wants to read/write the AudioContext, it needs to ask > > the DeferredTaskHandler to do the work in the main thread). > > > > Before talking about oilpan, we must design OfflineAudioContext in a way in > > which it is read/written only by one thread. > > > > But the real-time render thread uses AudioContext only via the > > DeferredTaskHandler and thus it's guaranteed that the AudioContext is > > read/written only by the main thread, right? > > Yes, I should have thought about the design more carefully. Since > DeferredTaskHandler is a part of AbstractAudioContext, so OfflineAudioContext > can also benefit from it. > It seems like a high level change from the current status of this CL, so I might > have to start a new CL from scratch. > > Thanks for your review. Yeah, sorry about that -- I should have noticed that problem earlier. Even without oilpan, we need to design a class so that it is not shared between threads...
On 2015/07/29 15:23:43, haraken wrote: > On 2015/07/29 15:21:54, hoch wrote: > > > This sounds like a reasonable design. > > > > > > - DeferredTaskHandler is ThreadSafeRefCounted > > > - AudioContext is GarbageCollected. > > > - The AudioContext is read/written only by the main thread (i.e., when the > > > real-time render thread wants to read/write the AudioContext, it needs to > ask > > > the DeferredTaskHandler to do the work in the main thread). > > > > > > Before talking about oilpan, we must design OfflineAudioContext in a way in > > > which it is read/written only by one thread. > > > > > > But the real-time render thread uses AudioContext only via the > > > DeferredTaskHandler and thus it's guaranteed that the AudioContext is > > > read/written only by the main thread, right? > > > > Yes, I should have thought about the design more carefully. Since > > DeferredTaskHandler is a part of AbstractAudioContext, so OfflineAudioContext > > can also benefit from it. > > It seems like a high level change from the current status of this CL, so I > might > > have to start a new CL from scratch. > > > > Thanks for your review. > > Yeah, sorry about that -- I should have noticed that problem earlier. > > Even without oilpan, we need to design a class so that it is not shared between > threads... Well. Don't be! Now I have better understanding on threading in Web Audio API, so that is already a huge win for me.
Description was changed from ========== 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. # Editor's draft spec PR: http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface # Web Audio API issue tracker entry: 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 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199129 ========== to ========== NOTE: This CL is replaced by https://codereview.chromium.org/1405413004. 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. # Editor's draft spec PR: http://webaudio.github.io/web-audio-api/#the-offlineaudiocontext-interface # Web Audio API issue tracker entry: 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 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199129 ========== |
