Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777903005:
Add WebThread in AudioDestination to support AudioWorkletThread (Closed)
DescriptionAdd WebThread in AudioDestination to support AudioWorkletThread
This CL to introduce a WebThread to run WebAudio graph rendering.
FYI: with the box 2d demo on my local machine (MacPro), the release
build of this patch was able to render 50 cubes concurrently before
glitch, which is similar to the current stable build that runs only on the
audio device thread. No significant performance regression is observed.
Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsryaQ/edit#
BUG=706133
Review-Url: https://codereview.chromium.org/2777903005
Cr-Commit-Position: refs/heads/master@{#467819}
Committed: https://chromium.googlesource.com/chromium/src/+/1492075fce7add79a11e13a3c40d55261f4ae89d
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressing feedback #
Total comments: 8
Patch Set 3 : Added thread checking #Patch Set 4 : Rebased after Blink renaming #Patch Set 5 : Refactoring WIP (please ignore) #
Total comments: 8
Patch Set 6 : Refactoring AudioDestination for thread safety #
Total comments: 10
Patch Set 7 : Remove callback between Destination and FIFO #
Total comments: 20
Patch Set 8 : Cleaning up #Patch Set 9 : Issue with unit test #Patch Set 10 : Added Unit Test for multi-thread support #
Total comments: 15
Patch Set 11 : Parameterized Unit Test #
Total comments: 22
Patch Set 12 : Addressed feedback #
Total comments: 3
Patch Set 13 : Clean up after l-g-t-m #Messages
Total messages: 68 (29 generated)
Description was changed from ========== Add WebThread in AudioDestination to support AudioWorkletThread [DO NOT SUBMIT] This is a POC CL to introduce WebThread for WebAudio rendering. With the box 2d demo, the release build of this patch was able to create 35~40 cubes, which is similar to the current stable build (without the dual thread pipe line). PTAL at the high level design. If this looks acceptable, then we need to discuss how to optimize this approach. Eventually, the WebThread belongs to AudioDestination will be passed to AudioWorkletGlobalScope to render the AudioNodes in the associated AudioContext. BUG=706133 ========== to ========== Add WebThread in AudioDestination to support AudioWorkletThread [DO NOT SUBMIT] PTAL at the high level design. This is a POC CL to introduce WebThread for WebAudio rendering. With the box 2d demo, the release build of this patch was able to create 35~40 cubes, which is similar to the current stable build (without the WebThread pipe line). If this looks acceptable, then we need to discuss how to optimize this approach. Eventually, the WebThread belongs to AudioDestination will be passed to AudioWorkletGlobalScope to render the AudioNodes in the associated AudioContext. Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... BUG=706133 ==========
hongchan@chromium.org changed reviewers: + haraken@chromium.org, nhiroki@chromium.org, olka@chromium.org, rtoy@chromium.org
This is a proof-of-concept. PTAL at the high level approach.
Looks good to me. I'll defer to WebAudio/Arch experts. Added some early comments about thread safety. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // quantum, RQ) thus FIFO is needed to handle the general case. Can you add comments about thread safety of this class? Which thread can access getters like framesAvailable()? https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:69: Mutex m_lock; What does this lock protect?
The approach looks good. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:127: m_fifo->pull(m_outputBus.get(), numberOfFrames); Is it okay to pull the result before requestRenderOnWebThread has completed? https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:69: Mutex m_lock; On 2017/03/29 01:54:05, nhiroki wrote: > What does this lock protect? Same question from me.
The CL look fine performance-wise. I see some issues with muti-threading and suggest a bit of redesign for the FIFO. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:125: delayTimestamp, priorFramesSkipped)); See the comment below: I would redesign the fifo to calculate how much data needs to be renderer and to post the request to render - all this in the m_fifo->pull() https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:140: m_outputPositionReceivedTimestamp = base::TimeTicks::Now(); Have you considered encapsulating all these Web-thread only members in some class? Otherwise, it's hard to make sure that they are never access from a wrong thread. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:145: size_t framesToRender = numberOfFrames > m_fifo->framesAvailable() framesAvailable() is not thread safe. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:171: const PushPullFIFOStateForTest PushPullFIFO::getStateForTest() const { Not thread safe any more https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: // PushPullFIFO class is an intermediate audio sample storage between I understand that this is POC, so it's a side note (which actually deserves a TODO in unit tests): we'll need multy-threading unit tests. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:59: size_t framesAvailable() const { return m_framesAvailable; } This is a non thread-safe method. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:62: AudioBus* bus() const { return m_fifoBus.get(); } Providing direct access to a memory buffer which is accessed from multiple threads is dangerous even for testing. If we can't avoid having it, we need to name it ForTesting, and it should be guarded with the same mutex as other access to the bus.
https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // quantum, RQ) thus FIFO is needed to handle the general case. On 2017/03/29 01:54:05, nhiroki wrote: > Can you add comments about thread safety of this class? Which thread can access > getters like framesAvailable()? Yes, please. And add appropriate CHECKs in each of the routines to verify that only the right threads are allowed.
nhiroki@ Please advise how I can check the thread inside of method. Both threads are not the main thread, so we can't use isMainThread() here. olka@ If you think a complete redesign of FIFO is necessary, I think we can continue to discuss it on the design doc. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:125: delayTimestamp, priorFramesSkipped)); On 2017/03/29 09:05:11, o1ka wrote: > See the comment below: I would redesign the fifo to calculate how much data > needs to be renderer and to post the request to render - all this in the > m_fifo->pull() Hmm. This is a rather big change and different from what we discussed in the design doc. Could you clarify your proposal? How can m_fifo_pull() have all this when "this" is a part of AudioDestination? https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:127: m_fifo->pull(m_outputBus.get(), numberOfFrames); On 2017/03/29 07:56:24, haraken wrote: > > Is it okay to pull the result before requestRenderOnWebThread has completed? Yes. Then the result of pulling is going to be silent and it's a valid operation. If we enforce the order of rendering procedure, then the lock should be applied at the level of AudioDestination. In order to avoid it, I put the lock in the FIFO to minimize the scope of lock. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:140: m_outputPositionReceivedTimestamp = base::TimeTicks::Now(); On 2017/03/29 09:05:11, o1ka wrote: > Have you considered encapsulating all these Web-thread only members in some > class? Otherwise, it's hard to make sure that they are never access from a wrong > thread. I tried to have a clear separation between render() and requestRenderOnWebThread(). All the timestamp stuffs are being accessed only in this method. Do you think this is not good enough? https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:145: size_t framesToRender = numberOfFrames > m_fifo->framesAvailable() On 2017/03/29 09:05:11, o1ka wrote: > framesAvailable() is not thread safe. Based on our conversation, I thought this was okay. Of course I can put a lock around the getter. https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:171: const PushPullFIFOStateForTest PushPullFIFO::getStateForTest() const { On 2017/03/29 09:05:11, o1ka wrote: > Not thread safe any more Added a lock. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: // PushPullFIFO class is an intermediate audio sample storage between On 2017/03/29 09:05:11, o1ka wrote: > I understand that this is POC, so it's a side note (which actually deserves a > TODO in unit tests): we'll need multy-threading unit tests. Of course. This CL will eventually have a unit test. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // quantum, RQ) thus FIFO is needed to handle the general case. On 2017/03/29 16:13:13, Raymond Toy wrote: > On 2017/03/29 01:54:05, nhiroki wrote: > > Can you add comments about thread safety of this class? Which thread can > access > > getters like framesAvailable()? > > Yes, please. And add appropriate CHECKs in each of the routines to verify that > only the right threads are allowed. nhiroki@ Can you suggest a way to check which thread I am on here? Both are not the main thread so we can't use isMainThread() to differentiate. The webaudio rendering thread is WebThread and also we have the audio device thread. Not sure how to check within the method. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:59: size_t framesAvailable() const { return m_framesAvailable; } On 2017/03/29 09:05:11, o1ka wrote: > This is a non thread-safe method. Added a lock. Done. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:62: AudioBus* bus() const { return m_fifoBus.get(); } On 2017/03/29 09:05:11, o1ka wrote: > Providing direct access to a memory buffer which is accessed from multiple > threads is dangerous even for testing. > If we can't avoid having it, we need to name it ForTesting, and it should be > guarded with the same mutex as other access to the bus. Done. https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:69: Mutex m_lock; On 2017/03/29 07:56:24, haraken wrote: > On 2017/03/29 01:54:05, nhiroki wrote: > > What does this lock protect? > > Same question from me. See push()/pull() methods. This lock protects: m_fifoBus m_indexWrite m_indexRead m_framesAvailable
On 2017/03/29 19:39:59, hongchan wrote: > olka@ > If you think a complete redesign of FIFO is necessary, I think we can continue > to discuss it on the design doc. It's not a complete redesign :) I would call it "refactoring": we are just rearranging responcibilities between two classes. Most of the work will be in unit tests, but there will be a lot of work there anyways. And I'm not sure if using a design doc here is effective - we are discussing the source code. > > https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:125: > delayTimestamp, priorFramesSkipped)); > On 2017/03/29 09:05:11, o1ka wrote: > > See the comment below: I would redesign the fifo to calculate how much data > > needs to be renderer and to post the request to render - all this in the > > m_fifo->pull() > > Hmm. This is a rather big change and different from what we discussed in the > design doc. Could you clarify your proposal? It may sound scary, but it's a quick change in terms of the main code. More changes are required to unit tests. (That's why I suggested to tackle multi-threading in the previous CL for the FIFO) See below: FIFO can be responsible for calculation of how much needs to be rendered. > > How can m_fifo_pull() have all this when "this" is a part of AudioDestination? Why can't we move all "audio-thread-only" members to the FIFO? > https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:140: > m_outputPositionReceivedTimestamp = base::TimeTicks::Now(); > On 2017/03/29 09:05:11, o1ka wrote: > > Have you considered encapsulating all these Web-thread only members in some > > class? Otherwise, it's hard to make sure that they are never access from a > wrong > > thread. > > I tried to have a clear separation between render() and > requestRenderOnWebThread(). All the timestamp stuffs are being accessed only in > this method. Do you think this is not good enough? The restrictions like this should be enforced/verified by the code; if it's something we can verify only by manual code inspection - it's not enough, because a later change to the code may lead to hard-to-debug problems. That is, we want our code to be robust to future modifications. See my comment above on how I propose to address it. > > https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:145: size_t > framesToRender = numberOfFrames > m_fifo->framesAvailable() > On 2017/03/29 09:05:11, o1ka wrote: > > framesAvailable() is not thread safe. > > Based on our conversation, I thought this was okay. Of course I can put a lock > around the getter. As I mentioned above, it's better to get rid of it. I don't have access to the design doc history now, but I remember we discussed that pseudo-code there was a representation of an idea, not the real code. That's why I feel design doc is not very effective here when we are discussing 2 classes and reasoning on the source code level. It may be more efficient to discuss this face to face - maybe book a meeting?
https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // quantum, RQ) thus FIFO is needed to handle the general case. On 2017/03/29 19:39:58, hongchan wrote: > On 2017/03/29 16:13:13, Raymond Toy wrote: > > On 2017/03/29 01:54:05, nhiroki wrote: > > > Can you add comments about thread safety of this class? Which thread can > > access > > > getters like framesAvailable()? > > > > Yes, please. And add appropriate CHECKs in each of the routines to verify > that > > only the right threads are allowed. > > nhiroki@ > > Can you suggest a way to check which thread I am on here? Both are not the main > thread so we can't use isMainThread() to differentiate. The webaudio rendering > thread is WebThread and also we have the audio device thread. Not sure how to > check within the method. Hmm... I cannot think of a good idea. If this class has pointers to threads, we could use WebThread::isCurrentThread(). Or, we could keep a returned value of wtf::currentThread() on the first call and then compare it on following calls. (It'd be nice if we could use base/threading/thread_checker.h in blink...)
(forgot to publish comments) https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: return m_framesAvailable; This is correct but unfortunate to have a lock here, as we discussed - we want to minimize locking. That's why I propose to refactor the code and to get rid of this method. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:77: // This lock protects all members in this class from being accessed by two This is technically not quite correct: see length()/m_fifoLength (which is const so no need to protect) https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:79: mutable Mutex m_threadMutex; I'm not sure if this rename is useful. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:84: RefPtr<AudioBus> m_fifoBus; Should it be const?
PS3 adds the thread checking. Starting from the next patch set, I will address on the refactoring/thread safety issue with olka@. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: return m_framesAvailable; On 2017/03/30 13:17:05, o1ka wrote: > This is correct but unfortunate to have a lock here, as we discussed - we want > to minimize locking. That's why I propose to refactor the code and to get rid of > this method. Acknowledged. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:77: // This lock protects all members in this class from being accessed by two On 2017/03/30 13:17:05, o1ka wrote: > This is technically not quite correct: see length()/m_fifoLength (which is const > so no need to protect) Done. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:79: mutable Mutex m_threadMutex; On 2017/03/30 13:17:05, o1ka wrote: > I'm not sure if this rename is useful. Done. https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:84: RefPtr<AudioBus> m_fifoBus; On 2017/03/30 13:17:05, o1ka wrote: > Should it be const? We can, but currently the constructor allocates the bus after checking parameters like requested length and max FIFO length.
Patchset #4 (id:60001) has been deleted
On 2017/03/30 18:36:01, hongchan wrote: > PS3 adds the thread checking. > > Starting from the next patch set, I will address on the refactoring/thread > safety issue with olka@. > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: return > m_framesAvailable; > On 2017/03/30 13:17:05, o1ka wrote: > > This is correct but unfortunate to have a lock here, as we discussed - we want > > to minimize locking. That's why I propose to refactor the code and to get rid > of > > this method. > > Acknowledged. > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:77: // This lock > protects all members in this class from being accessed by two > On 2017/03/30 13:17:05, o1ka wrote: > > This is technically not quite correct: see length()/m_fifoLength (which is > const > > so no need to protect) > > Done. > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:79: mutable Mutex > m_threadMutex; > On 2017/03/30 13:17:05, o1ka wrote: > > I'm not sure if this rename is useful. > > Done. > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:84: RefPtr<AudioBus> > m_fifoBus; > On 2017/03/30 13:17:05, o1ka wrote: > > Should it be const? > > We can, but currently the constructor allocates the bus after checking > parameters like requested length and max FIFO length. olka@ After our meeting last week, I've spent some time in refactoring PushPullFIFO and AudioDestination. Here's my opinion: 1. The biggest motivation of refactoring is to hide framesAvailable() method out of AudioDestination. 2. However, hiding the calculation of "frames to render" inside of FIFO actually changes the structure of PushPullFIFO and AudioDestination. - FIFO needs to have AudioCallbackIO and calculate outputTime stamp. It also needs to know the hardware callback size. - The call stack will change: DeviceThread pulls -> AudioDestination requests FIFO data -> FIFO requests WebAudio render -> WebAudio renders. - This is basically reverting a change we made to improve the structure of FIFO/Destination. 3. framesAvailable() only exists in two places. If FIFO can have a method "GetFramesToRender()" then we can hide frames_available_ with a single lock. Actually I want to take the path 3) and then focus on writing a set of unit tests for it. WDYT?
Patchset #4 (id:80001) has been deleted
On 2017/04/12 19:07:44, hongchan wrote: > On 2017/03/30 18:36:01, hongchan wrote: > > PS3 adds the thread checking. > > > > Starting from the next patch set, I will address on the refactoring/thread > > safety issue with olka@. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: return > > m_framesAvailable; > > On 2017/03/30 13:17:05, o1ka wrote: > > > This is correct but unfortunate to have a lock here, as we discussed - we > want > > > to minimize locking. That's why I propose to refactor the code and to get > rid > > of > > > this method. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:77: // This lock > > protects all members in this class from being accessed by two > > On 2017/03/30 13:17:05, o1ka wrote: > > > This is technically not quite correct: see length()/m_fifoLength (which is > > const > > > so no need to protect) > > > > Done. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:79: mutable Mutex > > m_threadMutex; > > On 2017/03/30 13:17:05, o1ka wrote: > > > I'm not sure if this rename is useful. > > > > Done. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:84: RefPtr<AudioBus> > > m_fifoBus; > > On 2017/03/30 13:17:05, o1ka wrote: > > > Should it be const? > > > > We can, but currently the constructor allocates the bus after checking > > parameters like requested length and max FIFO length. > > olka@ > > After our meeting last week, I've spent some time in refactoring PushPullFIFO > and AudioDestination. Here's my opinion: > > 1. The biggest motivation of refactoring is to hide framesAvailable() method out > of AudioDestination. > 2. However, hiding the calculation of "frames to render" inside of FIFO actually > changes the structure of PushPullFIFO and AudioDestination. > - FIFO needs to have AudioCallbackIO and calculate outputTime stamp. It also > needs to know the hardware callback size. > - The call stack will change: DeviceThread pulls -> AudioDestination requests > FIFO data -> FIFO requests WebAudio render -> WebAudio renders. > - This is basically reverting a change we made to improve the structure of > FIFO/Destination. > 3. framesAvailable() only exists in two places. If FIFO can have a method > "GetFramesToRender()" then we can hide frames_available_ with a single lock. > > Actually I want to take the path 3) and then focus on writing a set of unit > tests for it. > > WDYT? PS4: Replacing FramesAvailable() with GetFramesToRender(). PS5: Moving output position, web audio render callback, frame calculation to FIFO.FillRequestedFrames(). I ran a stress test (Box2D webaudio example) against the release build of these two PS and got the same result (~50 cubes before glitch starts).
On 2017/04/12 19:07:44, hongchan wrote: > On 2017/03/30 18:36:01, hongchan wrote: > > PS3 adds the thread checking. > > > > Starting from the next patch set, I will address on the refactoring/thread > > safety issue with olka@. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: return > > m_framesAvailable; > > On 2017/03/30 13:17:05, o1ka wrote: > > > This is correct but unfortunate to have a lock here, as we discussed - we > want > > > to minimize locking. That's why I propose to refactor the code and to get > rid > > of > > > this method. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:77: // This lock > > protects all members in this class from being accessed by two > > On 2017/03/30 13:17:05, o1ka wrote: > > > This is technically not quite correct: see length()/m_fifoLength (which is > > const > > > so no need to protect) > > > > Done. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:79: mutable Mutex > > m_threadMutex; > > On 2017/03/30 13:17:05, o1ka wrote: > > > I'm not sure if this rename is useful. > > > > Done. > > > > > https://codereview.chromium.org/2777903005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/PushPullFIFO.h:84: RefPtr<AudioBus> > > m_fifoBus; > > On 2017/03/30 13:17:05, o1ka wrote: > > > Should it be const? > > > > We can, but currently the constructor allocates the bus after checking > > parameters like requested length and max FIFO length. > > olka@ > > After our meeting last week, I've spent some time in refactoring PushPullFIFO > and AudioDestination. Here's my opinion: > > 1. The biggest motivation of refactoring is to hide framesAvailable() method out > of AudioDestination. > 2. However, hiding the calculation of "frames to render" inside of FIFO actually > changes the structure of PushPullFIFO and AudioDestination. > - FIFO needs to have AudioCallbackIO and calculate outputTime stamp. It also > needs to know the hardware callback size. > - The call stack will change: DeviceThread pulls -> AudioDestination requests > FIFO data -> FIFO requests WebAudio render -> WebAudio renders. > - This is basically reverting a change we made to improve the structure of > FIFO/Destination. > 3. framesAvailable() only exists in two places. If FIFO can have a method > "GetFramesToRender()" then we can hide frames_available_ with a single lock. > > Actually I want to take the path 3) and then focus on writing a set of unit > tests for it. > > WDYT? > olka@ > > After our meeting last week, I've spent some time in refactoring PushPullFIFO > and AudioDestination. Here's my opinion: > > 1. The biggest motivation of refactoring is to hide framesAvailable() method out > of AudioDestination. That was one of the issues. My concerns are described in the previous review. The code was not using c++ best practices regarding threading and separation of concerns. > 2. However, hiding the calculation of "frames to render" inside of FIFO actually > changes the structure of PushPullFIFO and AudioDestination. > - FIFO needs to have AudioCallbackIO and calculate outputTime stamp. It also > needs to know the hardware callback size. Why? It can be done by AudioDestination. I was talking about general C++ callback, not AudioCallbackIO. > - The call stack will change: DeviceThread pulls -> AudioDestination requests > FIFO data -> FIFO requests WebAudio render -> WebAudio renders. > - This is basically reverting a change we made to improve the structure of > FIFO/Destination. I’m not sure what you mean here, I’m confuzed. > 3. framesAvailable() only exists in two places. If FIFO can have a method > "GetFramesToRender()" then we can hide frames_available_ with a single lock. > > Actually I want to take the path 3) and then focus on writing a set of unit > tests for it. > > WDYT? I’m confused with both your comments and the new patchset. Which solution does it represent? My proposal was: 1. AudioDestintion::Render(callback_size, ...) calls FIFO::Pull(callback_size, PullRequestCallback). 2. FIFO::Pull(callback_size) serves the request as it used to (fills with zeros if there are not enough frames) and if needed calls PushRequestCallback, passing |required_frames| into it. 3. This callback is bound to some AudioDestination::RequestRender(), which posts AudioDestination::RequestRenderOnWebThread() to the web thread, passing |required_frames| and the rest of info to it. Data like |delay|, |delay_timestamp|, |prior_frames_skipped|, |prior_frames_skipped| may be bound to RequestRender() at step 1 and passed to RequestRenderOnWebThread() here. 4. AudioDestination::RequestRenderOnWebThread() calculates position, pulls the graph to get |required_frames| frames and pushes them into FIFO.
https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:118: if (!fifo_ || fifo_->length() < number_of_frames) Are you keeping silent returns? https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:155: // delay; It's really inconvenient to review patches with commented out parts like this. Could you please load a clean patch next time? https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:83: size_t priorFramesSkipped); Should these params also be renamed with a new style? https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: : rendering_thread_id_(rendering_thread_id), This is too much information for a FIFO. Also, we may potentially have situations when rendering thread changes. The less classes know about a specific thread, the better.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Description was changed from ========== Add WebThread in AudioDestination to support AudioWorkletThread [DO NOT SUBMIT] PTAL at the high level design. This is a POC CL to introduce WebThread for WebAudio rendering. With the box 2d demo, the release build of this patch was able to create 35~40 cubes, which is similar to the current stable build (without the WebThread pipe line). If this looks acceptable, then we need to discuss how to optimize this approach. Eventually, the WebThread belongs to AudioDestination will be passed to AudioWorkletGlobalScope to render the AudioNodes in the associated AudioContext. Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... BUG=706133 ========== to ========== Add WebThread in AudioDestination to support AudioWorkletThread This CL to introduce a WebThread to run WebAudio graph rendering. FYI: with the box 2d demo on my local machine (MacPro), the release build of this patch was able to render 50 cubes concurrently before glitch, which is similar to the current stable build that runs only on the audio device thread. No significant performance regression is observed. Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... BUG=706133 ==========
Please ignore PS5. PS6 introduces a major change in AudioDestination: - Based on the suggestion from olka@, I minimized the exposure of FIFO in AudioDestination. Thanks to this change now we have only two locks in the call chain - Push() and Pull(). - FIFO is still thread-agnostic. It does not care where it's pushed or pulled, and I believe this is better in terms of the abstraction. AudioDestination checks the thread before it does anything with FIFO. If the PS6 looks good, I can focus on the multi-thread unit testing against FIFO with help from olka@. https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:118: if (!fifo_ || fifo_->length() < number_of_frames) On 2017/04/13 08:36:53, o1ka OOO until April 18th wrote: > Are you keeping silent returns? This inexplicable case happens only on Android. If this check fails, what else can we do? Until we figure out why I think this is the best we can do. I rather not fix this here. https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:155: // delay; On 2017/04/13 08:36:53, o1ka OOO until April 18th wrote: > It's really inconvenient to review patches with commented out parts like this. > Could you please load a clean patch next time? I am terribly sorry about this mistake. :( https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:83: size_t priorFramesSkipped); On 2017/04/13 08:36:53, o1ka OOO until April 18th wrote: > Should these params also be renamed with a new style? Done. https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: : rendering_thread_id_(rendering_thread_id), On 2017/04/13 08:36:53, o1ka OOO until April 18th wrote: > This is too much information for a FIFO. > Also, we may potentially have situations when rendering thread changes. The less > classes know about a specific thread, the better. Done. https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: Bind(&AudioDestination::RequestRender, WTF::Unretained(this), @nhiroki @haraken Please advise this binding looks problematic. https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:70: push_request_render_callback); @nhiroki @haraken Let me know if this is a weird thing to do in Blink.
https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: Bind(&AudioDestination::RequestRender, WTF::Unretained(this), On 2017/04/14 16:31:48, hongchan wrote: > @nhiroki @haraken Please advise this binding looks problematic. I'm just curious but what are you concerned about?
https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: Bind(&AudioDestination::RequestRender, WTF::Unretained(this), On 2017/04/14 19:01:43, haraken wrote: > On 2017/04/14 16:31:48, hongchan wrote: > > @nhiroki @haraken Please advise this binding looks problematic. > > I'm just curious but what are you concerned about? I am not really familiar with Bind and Unretained/WrapPersistent friends. If you think there is a better way than what I do here. Please let me know.
https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: Bind(&AudioDestination::RequestRender, WTF::Unretained(this), On 2017/04/14 20:46:29, hongchan wrote: > On 2017/04/14 19:01:43, haraken wrote: > > On 2017/04/14 16:31:48, hongchan wrote: > > > @nhiroki @haraken Please advise this binding looks problematic. > > > > I'm just curious but what are you concerned about? > > I am not really familiar with Bind and Unretained/WrapPersistent friends. If you > think there is a better way than what I do here. Please let me know. This Bind/Unretained usage would be correct as follows, but I'm not really sure if we need to use this callback pattern here (see my comment in PushPullFIFO.cpp) 1) AudioDestination is not a GC-managed object, so we don't have to use WrapPersistent() family (see the API guide[1] for more details). 2) IIUC, it's guaranteed that |this| is valid when the callback is called, so we can use WTF::Unretained(). 3) The callback is called on the same thread (ie., AudioDeviceThread), so we can use WTF::Bind() instead of WTF::CrossThreadBind()[2]. [1] https://chromium.googlesource.com/chromium/src/+/193127beb64ef62f7e199b2c05a1... [2] https://chromium.googlesource.com/chromium/src/+/193127beb64ef62f7e199b2c05a1... https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:97: // Silently return to avoid crash. Generally, I'd recommend to run the callback on all return paths (w/ some status code) so that the caller can have a chance to release resources. https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:183: (*push_request_render_callback)(frames_requested, frames_to_render); This callback is called within the mutex section (see line 120). That could be risky, for example, some callback may wrongly call PushPullFIFO::Pull() again. If |frames_requested| is never modified in this function and |frames_to_render| is the only real return value, how about just returning |frames_to_render| instead of calling a given callback?
https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: Bind(&AudioDestination::RequestRender, WTF::Unretained(this), On 2017/04/17 03:27:30, nhiroki wrote: > On 2017/04/14 20:46:29, hongchan wrote: > > On 2017/04/14 19:01:43, haraken wrote: > > > On 2017/04/14 16:31:48, hongchan wrote: > > > > @nhiroki @haraken Please advise this binding looks problematic. > > > > > > I'm just curious but what are you concerned about? > > > > I am not really familiar with Bind and Unretained/WrapPersistent friends. If > you > > think there is a better way than what I do here. Please let me know. > > This Bind/Unretained usage would be correct as follows, but I'm not really sure > if we need to use this callback pattern here (see my comment in > PushPullFIFO.cpp) > > 1) AudioDestination is not a GC-managed object, so we don't have to use > WrapPersistent() family (see the API guide[1] for more details). > 2) IIUC, it's guaranteed that |this| is valid when the callback is called, so we > can use WTF::Unretained(). > 3) The callback is called on the same thread (ie., AudioDeviceThread), so we can > use WTF::Bind() instead of WTF::CrossThreadBind()[2]. > > [1] > https://chromium.googlesource.com/chromium/src/+/193127beb64ef62f7e199b2c05a1... > [2] > https://chromium.googlesource.com/chromium/src/+/193127beb64ef62f7e199b2c05a1... Thank you for the clarification! https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:97: // Silently return to avoid crash. On 2017/04/17 03:27:30, nhiroki wrote: > Generally, I'd recommend to run the callback on all return paths (w/ some status > code) so that the caller can have a chance to release resources. A good point. I missed that. https://codereview.chromium.org/2777903005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:183: (*push_request_render_callback)(frames_requested, frames_to_render); On 2017/04/17 03:27:30, nhiroki wrote: > This callback is called within the mutex section (see line 120). That could be > risky, for example, some callback may wrongly call PushPullFIFO::Pull() again. > > If |frames_requested| is never modified in this function and |frames_to_render| > is the only real return value, how about just returning |frames_to_render| > instead of calling a given callback? Actually that'd be cleaner than what I have here. I would like to know how olka@ think about this.
Patchset #8 (id:220001) has been deleted
locking/threading/FIFO sgtm. A couple of questions: 1) Is the intention here to switch all WebAudio rendering to asynchronous, not matter if a worklet node is present or not? 2) Should we make sure that Web thread priority is not lower than audio rendering thread one, to decrease a probability of glitches? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:144: output_position_.position = output_position; Shouldn't |output_position_| be just a local variable here? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:146: output_position_received_timestamp_ = base::TimeTicks::Now(); Same for |output_position_received_timestamp_|? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:103: unsigned number_of_output_channels_; Make it const? (I know it's an old code - we can make it better :) ) https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:108: AudioIOCallback& callback_; Side comment: Is it WebKit style to use references for not-owned members with guaranteed lifetime? Chromium style would be to receive (both in AudioDestination::Create() and the constructor) and store a pointer, not a reference. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:121: std::unique_ptr<WebThread> rendering_thread_; Could you group data members by thread they are accessed on, and add comments about it: |render_bus_|, |frames_elapsed_| and |callback_| are accessed on |rendering_thread_| only; |output_bus_| is accessed on the device thread only? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:130: size_t HardwareBufferSize(); Chromium style is first methods, then members. Is WebKit different? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: // audio device. Better to put this comment into the method description in the header. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:67: size_t Pull(AudioBus* output_bus, size_t frames_requested); Could you add a description of the return value? https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:95: // |index_write_|. I've seen it done this way: a mutex member is placed above the variables it protects, and the comment says "protects the variables below". Might be convenient here. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:96: mutable Mutex lock_; No need for mutable any more?
Thanks olka@! > 1) Is the intention here to switch all WebAudio rendering to asynchronous, not > matter if a worklet node is present or not? It is a complete switch. We don't want a complication in the main pipeline. Given that user can invoke AudioWorklet.import() at any moment, changing the thread in the middle of rendering does not sound appropriate. > 2) Should we make sure that Web thread priority is not lower than audio > rendering thread one, to decrease a probability of glitches? This is also not recommended by the common practice of user-exposed JS APIs. Once developers realize they can take advantage of the higher priority thread from the design, the exploitation will ensue. With said that, I am still looking for a way not to lose the rendering stability. For the smoke testing, do you have any specific setting in your mind? My plan is to create two threads and to pull/push the FIFO with random settings. I am looking at the example you suggested: https://cs.chromium.org/chromium/src/content/renderer/media/audio_renderer_si... https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:144: output_position_.position = output_position; On 2017/04/18 14:14:57, o1ka wrote: > Shouldn't |output_position_| be just a local variable here? Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:146: output_position_received_timestamp_ = base::TimeTicks::Now(); On 2017/04/18 14:14:57, o1ka wrote: > Same for |output_position_received_timestamp_|? Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:103: unsigned number_of_output_channels_; On 2017/04/18 14:14:57, o1ka wrote: > Make it const? (I know it's an old code - we can make it better :) ) Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:108: AudioIOCallback& callback_; On 2017/04/18 14:14:57, o1ka wrote: > Side comment: Is it WebKit style to use references for not-owned members with > guaranteed lifetime? Chromium style would be to receive (both in > AudioDestination::Create() and the constructor) and store a pointer, not a > reference. I haven't checked why it was written like this. If we have to change this, I prefer to do that in a different CL after further investigation. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:121: std::unique_ptr<WebThread> rendering_thread_; On 2017/04/18 14:14:57, o1ka wrote: > Could you group data members by thread they are accessed on, and add comments > about it: > |render_bus_|, |frames_elapsed_| and |callback_| are accessed on > |rendering_thread_| only; > |output_bus_| is accessed on the device thread only? Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:130: size_t HardwareBufferSize(); On 2017/04/18 14:14:57, o1ka wrote: > Chromium style is first methods, then members. Is WebKit different? Not that I know of. Eventually we're merging to Chromium style, so I will take your suggestion here. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:173: // audio device. On 2017/04/18 14:14:57, o1ka wrote: > Better to put this comment into the method description in the header. I moved the comment about the return value to the header. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:67: size_t Pull(AudioBus* output_bus, size_t frames_requested); On 2017/04/18 14:14:57, o1ka wrote: > Could you add a description of the return value? Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:95: // |index_write_|. On 2017/04/18 14:14:57, o1ka wrote: > I've seen it done this way: a mutex member is placed above the variables it > protects, and the comment says "protects the variables below". Might be > convenient here. Done. https://codereview.chromium.org/2777903005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:96: mutable Mutex lock_; On 2017/04/18 14:14:57, o1ka wrote: > No need for mutable any more? Done.
nhiroki@
Could you help me with the unit test?
I am getting the following result from the new smoke test boilerplate code.
---
[ RUN ] PushPullFIFOMultithreadTest.SmokeTest
[33531:775:0418/125044.365457:221094466662132:INFO:PushPullFIFOTest.cpp(372)]
PushPullFIFOMultithreadTest::Setup
../../third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:377: Failure
Expected: (device_thread_.get()) != (nullptr), actual: NULL vs 8-byte object
<00-00 00-00 00-00 00-00>
../../third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:378: Failure
Expected: (rendering_thread_.get()) != (nullptr), actual: NULL vs 8-byte object
<00-00 00-00 00-00 00-00>
[33531:775:0418/125044.365677:221094466872064:INFO:PushPullFIFOTest.cpp(384)]
PushPullFIFOMultithreadTest::TearDown
[ FAILED ] PushPullFIFOMultithreadTest.SmokeTest (0 ms)
[----------] 1 test from PushPullFIFOMultithreadTest (0 ms total)
---
In short, Platform::Current()->CreateThread("foo") always returns nullptr and I
don't know why.
I did some digging to find out other unit tests using CreateThread() and they
are doing nothing special.
WDYT?
On Tue, Apr 18, 2017 at 11:16 AM, <hongchan@chromium.org> wrote: > Thanks olka@! > > > 1) Is the intention here to switch all WebAudio rendering to > asynchronous, not > > matter if a worklet node is present or not? > > It is a complete switch. We don't want a complication in the main pipeline. > Given that user can invoke AudioWorklet.import() at any moment, changing > the thread in the middle of rendering does not sound appropriate. > Yeah, let's find out how this works in practice with just plain WebAudio without worklets. If bad things happen, let's solve them then, not before. > > https://codereview.chromium.org/2777903005/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Tue, Apr 18, 2017 at 11:16 AM, <hongchan@chromium.org> wrote: > Thanks olka@! > > > 1) Is the intention here to switch all WebAudio rendering to > asynchronous, not > > matter if a worklet node is present or not? > > It is a complete switch. We don't want a complication in the main pipeline. > Given that user can invoke AudioWorklet.import() at any moment, changing > the thread in the middle of rendering does not sound appropriate. > Yeah, let's find out how this works in practice with just plain WebAudio without worklets. If bad things happen, let's solve them then, not before. > > https://codereview.chromium.org/2777903005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/18 19:55:39, hongchan wrote:
> nhiroki@
>
> Could you help me with the unit test?
> I am getting the following result from the new smoke test boilerplate code.
>
> ---
> [ RUN ] PushPullFIFOMultithreadTest.SmokeTest
> [33531:775:0418/125044.365457:221094466662132:INFO:PushPullFIFOTest.cpp(372)]
> PushPullFIFOMultithreadTest::Setup
> ../../third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:377:
Failure
> Expected: (device_thread_.get()) != (nullptr), actual: NULL vs 8-byte object
> <00-00 00-00 00-00 00-00>
> ../../third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:378:
Failure
> Expected: (rendering_thread_.get()) != (nullptr), actual: NULL vs 8-byte
object
> <00-00 00-00 00-00 00-00>
> [33531:775:0418/125044.365677:221094466872064:INFO:PushPullFIFOTest.cpp(384)]
> PushPullFIFOMultithreadTest::TearDown
> [ FAILED ] PushPullFIFOMultithreadTest.SmokeTest (0 ms)
> [----------] 1 test from PushPullFIFOMultithreadTest (0 ms total)
> ---
>
> In short, Platform::Current()->CreateThread("foo") always returns nullptr and
I
> don't know why.
> I did some digging to find out other unit tests using CreateThread() and they
> are doing nothing special.
>
> WDYT?
Apparently, we need to add this test into "webkit_unit_tests" binary instead of
"blink_platform_unittests" to use the Platform object. See comments in BUILD.gn:
https://chromium.googlesource.com/chromium/src/+/af23bf8abe775d204d61672d8e2a...
o1ka@ rtoy@ PTAL at the new unit test: PushPullFIFOSmokeTest.cpp. If this makes sense to you, I will generalize/randomize the test even further. haraken@ PTAL platform/BUILD.gn as OWNER. I had to touch it to run the new test properly. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BUILD.gn:2166: "audio/PushPullFIFOSmokeTest.cpp", To be able to spawn a thread from Platform::Current(), this change is necessary.
The threading implementation LGTM, although I want to have oika@ confirm it. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BUILD.gn:2166: "audio/PushPullFIFOSmokeTest.cpp", On 2017/04/20 18:01:37, hongchan wrote: > To be able to spawn a thread from Platform::Current(), this change is necessary. Would you help me understand the problem? platform/audio/ should be able to use Platform::Current(). What's an issue of moving it to audio/BUILD.gn?
https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/BUILD.gn:2166: "audio/PushPullFIFOSmokeTest.cpp", On 2017/04/20 20:20:51, haraken wrote: > On 2017/04/20 18:01:37, hongchan wrote: > > To be able to spawn a thread from Platform::Current(), this change is > necessary. > > Would you help me understand the problem? platform/audio/ should be able to use > Platform::Current(). What's an issue of moving it to audio/BUILD.gn? With the blink_platform_unittests, Platform::Current()->CreateThread() returns nullptr. Thus all the thread operations makes the test crash.
On 2017/04/20 20:35:09, hongchan wrote: > https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/BUILD.gn (right): > > https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/BUILD.gn:2166: > "audio/PushPullFIFOSmokeTest.cpp", > On 2017/04/20 20:20:51, haraken wrote: > > On 2017/04/20 18:01:37, hongchan wrote: > > > To be able to spawn a thread from Platform::Current(), this change is > > necessary. > > > > Would you help me understand the problem? platform/audio/ should be able to > use > > Platform::Current(). What's an issue of moving it to audio/BUILD.gn? > > With the blink_platform_unittests, Platform::Current()->CreateThread() returns > nullptr. Thus all the thread operations makes the test crash. ah, makes sense.
https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:72: + (static_cast<double>(std::rand()) / RAND_MAX) * jitter_range_ms_; Maybe use std::uniform_real_distribution to generate the random numbers instead of std::rand()/RAND_MAX? Also, interval_with_jitter >= interval_ms. So interval_ms is the lower bound? If so, maybe rename interval_ms to reflect that? https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:73: elapesd_ms_ += interval_with_jitter; elapesd_ms should probably be elapsed_ms https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:80: interval_with_jitter); The documentation for PostDelayedTask says the delay could be ignored. Don't know if this really ignored for our platforms, but would it matter if it is? https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:139: const double pull_interval_ms = pull_buffer_size / sample_rate * 1000; pull_buffer_size / sample_rate * 1000 isn't exactly representable as a double. Will this matter? https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:140: const double pull_jitter_range_ms = 5; Why 5? Please comment on valid values and reason for 5. Here and below for push_jitter_range_ms. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:146: const double test_duration_ms = 1000; Is this long enough? I think this means that there will only be about 375 render quanta and 46 HW pulls.
@o1ka PTAL at the parameterized test cases in PS10. If you think we should add more test cases please let me know. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:72: + (static_cast<double>(std::rand()) / RAND_MAX) * jitter_range_ms_; On 2017/04/21 16:26:57, Raymond Toy wrote: > Maybe use std::uniform_real_distribution to generate the random numbers instead > of std::rand()/RAND_MAX? > > Also, interval_with_jitter >= interval_ms. So interval_ms is the lower bound? > If so, maybe rename interval_ms to reflect that? Hmm, I think the variable names quite accurate and intuitive. Can you suggest better names for them? https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:73: elapesd_ms_ += interval_with_jitter; On 2017/04/21 16:26:57, Raymond Toy wrote: > elapesd_ms should probably be elapsed_ms Done. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:80: interval_with_jitter); On 2017/04/21 16:26:57, Raymond Toy wrote: > The documentation for PostDelayedTask says the delay could be ignored. Don't > know if this really ignored for our platforms, but would it matter if it is? Not sure if it would matters for anything, but this is only way to schedule a repeated task. Also the testing environment is quite isolated unlike the actual application, so I think this is okay. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:139: const double pull_interval_ms = pull_buffer_size / sample_rate * 1000; On 2017/04/21 16:26:57, Raymond Toy wrote: > pull_buffer_size / sample_rate * 1000 isn't exactly representable as a double. > Will this matter? As we discussed offline, this does not really matter. Small errors will be ignored by the task scheduler anyway. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:140: const double pull_jitter_range_ms = 5; On 2017/04/21 16:26:57, Raymond Toy wrote: > Why 5? Please comment on valid values and reason for 5. Here and below for > push_jitter_range_ms. Multiple test cases with various parameters were added. https://codereview.chromium.org/2777903005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:146: const double test_duration_ms = 1000; On 2017/04/21 16:26:57, Raymond Toy wrote: > Is this long enough? I think this means that there will only be about 375 render > quanta and 46 HW pulls. Multiple test cases with various parameters were added.
I ran some benchmark to measure the overhead caused by the lock and the thread hop. https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... I also want to remind you all that this CL is a prerequisite for the AudioWorklet project. Without this change, I cannot work on JS API changes.
LGTM from threading pov https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: index_write_(0) { Optional: How about initializing these variables in the header like |overflow_count_|? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:176: const PushPullFIFOStateForTest PushPullFIFO::GetStateForTest() const { Should we protect this by the lock, too? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:59: // Pulling |framesRequested| by the audio device thread and returns the actual s/Pulling/Pulls/
Oh it turned out I reviewed it yesterday but forgot to publish comments. Sorry for the delay! The code is in a really good shape. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:74: AudioBus* GetFIFOBusForTest() const { return fifo_bus_.Get(); } We should fix this TODO before landing the CL. WDYT of inheriting from PushPullFIFO in PushPullFIFOTest.cpp and implementing the method there? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:92: size_t index_write_; Initialize everything with zeroes here? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:32: // TODO(hongchan): move this hack into Test class. Fix TODOs? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:68: int counter_ = 0; Could you refactor the code to make all the members private? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:90: double jitter_range_ms_; Add comments for these members? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:183: // Test case 1 (Windows): 480 Pull, 128 Push. Moderate Jitter. Since WebAudio has introduced user-specified buffer sizes, should the tests cover both 10 ms and 20 ms pulls? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:203: {44100, 2, 8192, 1000, 5768, 120, 128, 12} Consider running at least one test for a longer time, like 10 seconds? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:22: Add a comment that tests here do not test multi-threading logic?
https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: index_write_(0) { On 2017/04/25 00:35:51, nhiroki wrote: > Optional: How about initializing these variables in the header like > |overflow_count_|? Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:176: const PushPullFIFOStateForTest PushPullFIFO::GetStateForTest() const { On 2017/04/25 00:35:51, nhiroki wrote: > Should we protect this by the lock, too? This method is only for test, and to use the lock here I have to change the lock to mutable. I rather refactor the unit test to fix this in a different CL. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:59: // Pulling |framesRequested| by the audio device thread and returns the actual On 2017/04/25 00:35:51, nhiroki wrote: > s/Pulling/Pulls/ Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:74: AudioBus* GetFIFOBusForTest() const { return fifo_bus_.Get(); } On 2017/04/25 08:23:22, o1ka wrote: > We should fix this TODO before landing the CL. > WDYT of inheriting from PushPullFIFO in PushPullFIFOTest.cpp and implementing > the method there? I have to think about moving |bus| to the protected section. That makes all derived classes can access directly to the bus. The name clearly suggests this method cannot be used in the actual application. I also think that the refactoring for this can be done in a separate CL. I rather want to refactor the test so it doesn't have to call this at all, and remove it from the class completely. WDYT? https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:92: size_t index_write_; On 2017/04/25 08:23:22, o1ka wrote: > Initialize everything with zeroes here? Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:32: // TODO(hongchan): move this hack into Test class. On 2017/04/25 08:23:22, o1ka wrote: > Fix TODOs? It seems to be impossible at the moment. Other Blink unit tests running on multi-thread use the same hack like this. (registering start/stop functions in global scope) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Virt... I spent quite a while to fix this but couldn't find another way. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:68: int counter_ = 0; On 2017/04/25 08:23:22, o1ka wrote: > Could you refactor the code to make all the members private? Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:90: double jitter_range_ms_; On 2017/04/25 08:23:22, o1ka wrote: > Add comments for these members? Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:183: // Test case 1 (Windows): 480 Pull, 128 Push. Moderate Jitter. On 2017/04/25 08:23:22, o1ka wrote: > Since WebAudio has introduced user-specified buffer sizes, should the tests > cover both 10 ms and 20 ms pulls? 10ms = 480. (in 48000Hz) I can do 960 as well if that's what you mean. By the way, you can see why I picked these numbers here: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:203: {44100, 2, 8192, 1000, 5768, 120, 128, 12} On 2017/04/25 08:23:22, o1ka wrote: > Consider running at least one test for a longer time, like 10 seconds? Good idea. Done. https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2777903005/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:22: On 2017/04/25 08:23:22, o1ka wrote: > Add a comment that tests here do not test multi-threading logic? Done.
lgtm https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:226: {48000, 2, 8192, 10000, 256, 0, 128, 1} 44100 / 441 might be a better example since pull won't be a multiple of push.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with typo nit. Be sure to add a test for 44.1 kHz. https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:59: // Pulls |framesRequested| by the audio device thread and returns the actual Typo: |framesRequested| -> |frames_requested| https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp (right): https://codereview.chromium.org/2777903005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOSmokeTest.cpp:226: {48000, 2, 8192, 10000, 256, 0, 128, 1} On 2017/04/26 12:32:57, o1ka wrote: > 44100 / 441 might be a better example since pull won't be a multiple of push. I agree, that's a test we really need.
Patchset #13 (id:340001) has been deleted
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nhiroki@chromium.org, olka@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2777903005/#ps360001 (title: "Clean up after l-g-t-m")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1493332354343290,
"parent_rev": "a0d005f4f1d364bb312aeb25cbbea0d82a980a26", "commit_rev":
"1492075fce7add79a11e13a3c40d55261f4ae89d"}
Message was sent while issue was closed.
Description was changed from ========== Add WebThread in AudioDestination to support AudioWorkletThread This CL to introduce a WebThread to run WebAudio graph rendering. FYI: with the box 2d demo on my local machine (MacPro), the release build of this patch was able to render 50 cubes concurrently before glitch, which is similar to the current stable build that runs only on the audio device thread. No significant performance regression is observed. Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... BUG=706133 ========== to ========== Add WebThread in AudioDestination to support AudioWorkletThread This CL to introduce a WebThread to run WebAudio graph rendering. FYI: with the box 2d demo on my local machine (MacPro), the release build of this patch was able to render 50 cubes concurrently before glitch, which is similar to the current stable build that runs only on the audio device thread. No significant performance regression is observed. Design doc: https://docs.google.com/document/d/1WnqZHcA2TQjS1aKvzx2nnJ2y2tWK1Xp_0BhFGpsry... BUG=706133 Review-Url: https://codereview.chromium.org/2777903005 Cr-Commit-Position: refs/heads/master@{#467819} Committed: https://chromium.googlesource.com/chromium/src/+/1492075fce7add79a11e13a3c40d... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:360001) as https://chromium.googlesource.com/chromium/src/+/1492075fce7add79a11e13a3c40d... |
