Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549093009:
Introduce PushPullFIFO class and remove other FIFOs (Closed)
DescriptionThis CL is to reduce the complexity of current FIFO structure.
1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the
cyclic call structure between AudioDestination and AudioPullFIFO.
2. This refactoring will be helpful when the new WebThread for AudioWorklet
is implemented. (push and pull operation now can be separated.)
BUG=678678
Review-Url: https://codereview.chromium.org/2549093009
Cr-Original-Commit-Position: refs/heads/master@{#448784}
Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c329b559186a2e
Review-Url: https://codereview.chromium.org/2549093009
Cr-Commit-Position: refs/heads/master@{#449467}
Committed: https://chromium.googlesource.com/chromium/src/+/52db730e621a1cba97430be2a888feb9a962187a
Patch Set 1 : Initial patch #Patch Set 2 : Remove old FIFO classes #
Total comments: 50
Patch Set 3 : Renaming and clarification (1) #
Total comments: 25
Patch Set 4 : Renaming and clarification (2) #Patch Set 5 : Handling corner cases and added a unit test #
Total comments: 34
Patch Set 6 : Addressing feedback + Parameterized unit test #
Total comments: 13
Patch Set 7 : More comments #
Total comments: 18
Patch Set 8 : Introduced new streamline unit test #
Total comments: 49
Patch Set 9 : Added FIFO contract and more CHECKs #
Total comments: 49
Patch Set 10 : Comments fixed #Patch Set 11 : Simplified test #
Total comments: 8
Patch Set 12 : Refining unit tests #
Total comments: 24
Patch Set 13 : Comments and unit test #
Total comments: 7
Patch Set 14 : Addressing feedback and more unit tests #Patch Set 15 : Rebased and added CHECKs again after l-g-t-m #
Total comments: 1
Patch Set 16 : Death test comparison string dropped after l-g-t-m #Messages
Total messages: 105 (43 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implements PushPullFIFO class in platform/audio [DO NOT SUBMIT] ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) ==========
hongchan@chromium.org changed reviewers: + olka@chromium.org, rtoy@chromium.org
Description was changed from ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) ==========
PTAL. olka@ PushPullFIFO here does not have fancy features like AudioShifter, but at least it clearly separates push and pull operations for two different threads to access the FIFO. Currently this is a single thread operation, but we will have to use two threads (AudioDevice from media, WebThread for WebAudio rendering) to support AudioWorkletNode. Would love to have your feedback on this design.
Description was changed from ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 ==========
https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:104: if (destinationData.size() != m_numberOfOutputChannels || Should there be a DCHECK for this case? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:98: std::unique_ptr<PushPullFIFO> m_fifo; Please add comments on what m_renderBus and m_fifo are supposed to do. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: inputBusLength + m_numberOfFrames > m_length) { Need a check if number of channels for the bus matches the number of channels in the FIFO? What should happen if they don't match? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: } Can we add DCHECKs for these too? While it's nice to have NOTREACHED, when it fails, there are 4 conditions that could have caused the failure and we won't know what caused it. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:60: m_indexWrite = (m_indexWrite + inputBusLength) % m_length; It would be nice if m_length were a power of two. Should we make that a requirement? Or do you think we need to be more flexible? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:63: // Pull the data out of FIFO to |outputBus|. Specify what happens if outputBus is longer than framesToPull. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:89: memcpy(outputBusChannel, fifoChannel + m_indexWrite, You mean indexRead, not indexWrite? It would be great if we could have a test that would have caught this. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:34: RefPtr<AudioBus> m_dataBus; Maybe m_fifoDataBus? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_indexWrite; Please document what all of these mean. If m_length is the length of the FIFO, then maybe m_fifoSize is a better name?
Could you please clarify naming of variables? The CL is a bit tricky to read. Also, a good set of unit tests is needed for the new FIFO (I'm sure you would discover some issues with it.) https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: NOTREACHED(); Why not just DCHECK(inputBus)? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: inputBusLength + m_numberOfFrames > m_length) { Same comment about naming: m_length, inputBusLength and m_numberOfFrames look very confusing side by side (inputBusLength looks good by itself). https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:35: NOTREACHED(); How will we debug silent failures like this? Should we have a CHECK instead? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:39: for (unsigned i = 0; i < m_dataBus->numberOfChannels(); ++i) { Can we name it something like "m_fifoBus" to go along with inputBus? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:40: float* fifoChannel = m_dataBus->channel(i)->mutableData(); Can we name int fifoBusChannel to match inputBusChannel? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:50: memcpy(fifoChannel, inputBusChannel + remainder, What if you overwrite the beginning of data written above? What if you overwrite data m_indexRead is pointing to? How is it ensured that m_indexRead always makes sense after wrap-around? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:56: m_numberOfFrames += inputBusLength; What does |m_numberOfFrames| represent? Can it be more than FIFO length? What should it be in case of overflow/wrap around above? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:66: NOTREACHED(); Same question as above for all NOTREACHED(). https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:71: DCHECK(remainder >= 0)? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: void pull(AudioBus* outputBus, size_t framesToPull); How do you plan to ensure thread safety?
Thanks so much for the review. The new FIFO is largely based on AudioFIFO.h/cpp. However, as you all pointed out we need more thoughts on corner cases, especially this FIFO is supposed to support two threads. Please take a look at TODO() items in push/pull methods and let us discuss how to handle various corner cases. If you think creating a separate design doc is better, let me know! https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:104: if (destinationData.size() != m_numberOfOutputChannels || On 2017/01/06 22:22:03, Raymond Toy wrote: > Should there be a DCHECK for this case? It's there; line 101. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:98: std::unique_ptr<PushPullFIFO> m_fifo; On 2017/01/06 22:22:03, Raymond Toy wrote: > Please add comments on what m_renderBus and m_fifo are supposed to do. Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: NOTREACHED(); On 2017/01/09 14:09:03, o1ka wrote: > Why not just DCHECK(inputBus)? Done. The early return is for the release build. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: inputBusLength + m_numberOfFrames > m_length) { On 2017/01/06 22:22:03, Raymond Toy wrote: > Need a check if number of channels for the bus matches the number of channels in > the FIFO? What should happen if they don't match? @rtoy Hmm. My code is based on the current FIFO design, but maybe we need a fresh look on its heuristic. However, is it really possible? The size of AudioBus is fixed after the creation, which is based on the info from the audio device. I don't think that the audio device channel config can be changed on the fly. I think it's up to how our audio backend behaves. @o1ka Should I separate these checks into multiple assertions? What do you mean by "inputBusLength looking good by itself"? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:35: NOTREACHED(); On 2017/01/09 14:09:03, o1ka wrote: > How will we debug silent failures like this? > Should we have a CHECK instead? This is a more of WebAudio's practice from the old days. The original author preferred to have a silent failure rather than a crash. We can certainly change that if there's a better way to handle this kind of failure. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: } On 2017/01/06 22:22:03, Raymond Toy wrote: > Can we add DCHECKs for these too? While it's nice to have NOTREACHED, when it > fails, there are 4 conditions that could have caused the failure and we won't > know what caused it. I think these checks are being overly careful about the conditions. Here's my thoughts on 4 checks: 1) remainder > m_length: this is redundant because the remainder cannot be greater than m_length after the line 29, but size_t can go crazy when it wraps around. 2) m_indexWrite > m_length: this cannot happen after the line 97, but I think this check is to handle the size_t wrap-around that might happen. I can't think of such case though. 3) inputBusLength > m_length: is this needed? it depends on the behavior that we want from this FIFO. Even if the condition is met at the current iteration, the next iteration easily overflows and wraps around the FIFO if the consumer does not pull the frames out of it. 4) inputBusLength + m_numberOfFrames: almost same with 3). We can hope for the best, but the bad thing always can happen when the consumer does not behave nicely. I blindly used these checks from the old code, but we might have to reevaluate them again. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:39: for (unsigned i = 0; i < m_dataBus->numberOfChannels(); ++i) { On 2017/01/09 14:09:03, o1ka wrote: > Can we name it something like "m_fifoBus" to go along with inputBus? Done. Changed it to fifoDataBus. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:40: float* fifoChannel = m_dataBus->channel(i)->mutableData(); On 2017/01/09 14:09:03, o1ka wrote: > Can we name int fifoBusChannel to match inputBusChannel? Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:50: memcpy(fifoChannel, inputBusChannel + remainder, On 2017/01/09 14:09:03, o1ka wrote: > What if you overwrite the beginning of data written above? > What if you overwrite data m_indexRead is pointing to? > How is it ensured that m_indexRead always makes sense after wrap-around? Yes, this is something I wanted to discuss in this CL. > What if you overwrite the beginning of data written above? For this to happen, the incoming data must be bigger than m_fifoLength. I don't think this can be happen. > What if you overwrite data m_indexRead is pointing to? This means pulling has not happened even after the multiple pushing occurred. We can assume that the audio device is failing and stop the WebAudio rendering. > How is it ensured that m_indexRead always makes sense after wrap-around? The previous FIFO has the assumption of 'a single render call chain' in the same thread. So push/pull always happens sequentially/synchronously. Now we are about to have two threads to do the push/pull, so I need to think about this carefully. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:56: m_numberOfFrames += inputBusLength; On 2017/01/09 14:09:03, o1ka wrote: > What does |m_numberOfFrames| represent? Can it be more than FIFO length? What > should it be in case of overflow/wrap around above? It represents 'meaningful frames' that have not been played by the device. |framesToBePlayed| might be better? Please note that 'inputBusLength + m_numberOfFrames < m_length' check exists above. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:60: m_indexWrite = (m_indexWrite + inputBusLength) % m_length; On 2017/01/06 22:22:03, Raymond Toy wrote: > It would be nice if m_length were a power of two. Should we make that a > requirement? Or do you think we need to be more flexible? What's the benefit of the FIFO length being power of two? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:63: // Pull the data out of FIFO to |outputBus|. On 2017/01/06 22:22:03, Raymond Toy wrote: > Specify what happens if outputBus is longer than framesToPull. Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:66: NOTREACHED(); On 2017/01/09 14:09:03, o1ka wrote: > Same question as above for all NOTREACHED(). Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:71: On 2017/01/09 14:09:03, o1ka wrote: > DCHECK(remainder >= 0)? Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:89: memcpy(outputBusChannel, fifoChannel + m_indexWrite, On 2017/01/06 22:22:03, Raymond Toy wrote: > You mean indexRead, not indexWrite? It would be great if we could have a test > that would have caught this. Done. I am planning for a unit test for the FIFO. That should be a part of this CL. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: void pull(AudioBus* outputBus, size_t framesToPull); On 2017/01/09 14:09:03, o1ka wrote: > How do you plan to ensure thread safety? I believe this FIFO should give the higher priority to the pulling operation so we can ensure the exclusive access to the audio device callback. I added a lock in ::pull() method. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:34: RefPtr<AudioBus> m_dataBus; On 2017/01/06 22:22:03, Raymond Toy wrote: > Maybe m_fifoDataBus? Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_indexWrite; On 2017/01/06 22:22:03, Raymond Toy wrote: > Please document what all of these mean. If m_length is the length of the FIFO, > then maybe m_fifoSize is a better name? Done. AudioBus uses the term 'length'. The FIFO is based on AudioBus, so I wanted to make it consistent. Perhaps you can say the overall "size" is numberOfChannel * length.
https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:104: if (destinationData.size() != m_numberOfOutputChannels || On 2017/01/09 21:53:15, hongchan wrote: > On 2017/01/06 22:22:03, Raymond Toy wrote: > > Should there be a DCHECK for this case? > > It's there; line 101. Oops. Sorry, I missed that. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: } On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/06 22:22:03, Raymond Toy wrote: > > Can we add DCHECKs for these too? While it's nice to have NOTREACHED, when it > > fails, there are 4 conditions that could have caused the failure and we won't > > know what caused it. > > I think these checks are being overly careful about the conditions. Here's my > thoughts on 4 checks: > > 1) remainder > m_length: this is redundant because the remainder cannot be > greater than m_length after the line 29, but size_t can go crazy when it wraps > around. > > 2) m_indexWrite > m_length: this cannot happen after the line 97, but I think > this check is to handle the size_t wrap-around that might happen. I can't think > of such case though. > > 3) inputBusLength > m_length: is this needed? it depends on the behavior that we > want from this FIFO. Even if the condition is met at the current iteration, the > next iteration easily overflows and wraps around the FIFO if the consumer does > not pull the frames out of it. > > 4) inputBusLength + m_numberOfFrames: almost same with 3). We can hope for the > best, but the bad thing always can happen when the consumer does not behave > nicely. > > I blindly used these checks from the old code, but we might have to reevaluate > them again. Ok. I'll leave it up to you to decide which checks to remove and which to keep. But if we're going to keep some, a DCHECK for the individual check would be really nice for debugging. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:60: m_indexWrite = (m_indexWrite + inputBusLength) % m_length; On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/06 22:22:03, Raymond Toy wrote: > > It would be nice if m_length were a power of two. Should we make that a > > requirement? Or do you think we need to be more flexible? > > What's the benefit of the FIFO length being power of two? Modulo would just be a mask instead of a division. But I guess it doesn't really matter here. We only do it once per call. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:71: On 2017/01/09 21:53:15, hongchan wrote: > On 2017/01/09 14:09:03, o1ka wrote: > > DCHECK(remainder >= 0)? > > Done. That seems wrong. remainder is a size_t so it's always >= 0. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:119: size_t framesToPull = numberOfFrames - m_fifo->framesInFIFO(); Although this isn't caused by this CL, what happens if numberOfFrames < framesInFIFO()? Should there at least be a DCHECK for this? https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:99: // To push the rendered result from WebAudio graph into the FIFO. I'm sorry. "To pass" and "To push" don't really help me understand what m_outputBus and m_renderBus hold. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:32: // Returns the number of valid frames in FIFO. This comment is probably better placed as a description of m_framesInFIFO. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:40: size_t m_framesInFIFO; I can guess the difference between m_fifoLength and m_framesInFIFO, we should probably make it clear with comments. fifoLength is the size of the FIFO, and framesInFIFO is how many frames of data are actually in the FIFO. Presumably fifoLength is also measured in frames?
The subject says "deprecate other FIFOs". All other FIFOs are gone, right, so maybe say "remove other FIFOs"?
Since FIFO interface very minimal, you can have unit tests right now. A single-threaded version which covers all the indexing corner cases would save quite a lot of code review time. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: NOTREACHED(); On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/09 14:09:03, o1ka wrote: > > Why not just DCHECK(inputBus)? > > Done. The early return is for the release build. See my other comment about early returns https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: inputBusLength + m_numberOfFrames > m_length) { On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/06 22:22:03, Raymond Toy wrote: > > Need a check if number of channels for the bus matches the number of channels > in > > the FIFO? What should happen if they don't match? > > @rtoy > > Hmm. My code is based on the current FIFO design, but maybe we need a fresh look > on its heuristic. However, is it really possible? The size of AudioBus is fixed > after the creation, which is based on the info from the audio device. I don't > think that the audio device channel config can be changed on the fly. > > I think it's up to how our audio backend behaves. > > @o1ka > > Should I separate these checks into multiple assertions? What do you mean by > "inputBusLength looking good by itself"? Sorry for being unclear, my comment was about variable names. Yes, it's better to have separate DCHECKs https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:35: NOTREACHED(); On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/09 14:09:03, o1ka wrote: > > How will we debug silent failures like this? > > Should we have a CHECK instead? > > This is a more of WebAudio's practice from the old days. The original author > preferred to have a silent failure rather than a crash. We can certainly change > that if there's a better way to handle this kind of failure. See my other comment on this. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:50: memcpy(fifoChannel, inputBusChannel + remainder, On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/09 14:09:03, o1ka wrote: > > What if you overwrite the beginning of data written above? > > What if you overwrite data m_indexRead is pointing to? > > How is it ensured that m_indexRead always makes sense after wrap-around? > > Yes, this is something I wanted to discuss in this CL. > > > What if you overwrite the beginning of data written above? > > For this to happen, the incoming data must be bigger than m_fifoLength. I don't > think this can be happen. > You added a check for that, right? > > What if you overwrite data m_indexRead is pointing to? > > This means pulling has not happened even after the multiple pushing occurred. We > can assume that the audio device is failing and stop the WebAudio rendering. > It is possible that pulling has not happen - we call it "audio glitch". Have you considered rendering with glitches? > > How is it ensured that m_indexRead always makes sense after wrap-around? > > The previous FIFO has the assumption of 'a single render call chain' in the same > thread. So push/pull always happens sequentially/synchronously. Now we are about > to have two threads to do the push/pull, so I need to think about this > carefully. Right. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:56: m_numberOfFrames += inputBusLength; On 2017/01/09 21:53:16, hongchan wrote: > On 2017/01/09 14:09:03, o1ka wrote: > > What does |m_numberOfFrames| represent? Can it be more than FIFO length? What > > should it be in case of overflow/wrap around above? > > It represents 'meaningful frames' that have not been played by the device. > |framesToBePlayed| might be better? Please note that 'inputBusLength + > m_numberOfFrames < m_length' check exists above. |framesAvailable| maybe? https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:71: On 2017/01/09 22:46:02, Raymond Toy wrote: > On 2017/01/09 21:53:15, hongchan wrote: > > On 2017/01/09 14:09:03, o1ka wrote: > > > DCHECK(remainder >= 0)? > > > > Done. > > That seems wrong. remainder is a size_t so it's always >= 0. Right, we should have CHECKs before subtraction instead. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:25: // - What if the write index wraps around and passes the read index again? This is the most interesting part. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:26: // - What if the pulling is invoked while the pushing is in progress? Grab the lock :) https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:33: return; Why these silent returns are less preferable than crashes: If we have a crash, it will be reported and we can debug it and fix it. If we return silently, there will be "no audio" bugs files those impossible to debug, because there is no any data. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:35: size_t remainder = m_fifoLength - m_indexWrite; Here and in other places where you work with memory indexes: It should be CHECK(m_fifoLengthm_indexWrite <= m_fifoLength), not DCHECK: otherwise we can have size_t overflow and will be writing into random memory - it's a security issue. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:64: // silence. This is not true as of now. You have a DCHECK on l.73. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:68: // - What should the lock be placed? Not sure what it means. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:74: DCHECK_LE(m_indexRead, m_fifoLength); See my other comment about memory indexes: these should be CHECKs https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:81: MutexLocker locker(m_fifoDataBusLock); I'm not sure how "priority" is addressed here. However, you took the lock in pull(), and did not take it in push(). This is not quite how locks work. "locks are advisory locks, where each thread cooperates by acquiring the lock before accessing the corresponding data", see [https://en.wikipedia.org/wiki/Lock_(computer_science)] https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:33: size_t framesInFIFO() const { return m_framesInFIFO; } This one can be inline. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; const?
https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:33: return; On 2017/01/10 10:07:19, o1ka wrote: > Why these silent returns are less preferable than crashes: > If we have a crash, it will be reported and we can debug it and fix it. If we > return silently, there will be "no audio" bugs files those impossible to debug, > because there is no any data. This is how most of WebAudio works, from the original author. This is one aspect of the code that I really hate, exactly for this reason. If something truly bad happens (out-of-bounds access, invariants aren't), I think we should just crash instead of continuing silently. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; Also, do all of these really need to be size_t? I think int would be a perfectly fine type. This is another nit I have with the current code base: size_t everywhere, even for things we know will never be even close to 64-bits long. And it's also been a source of bugs when we take the difference of two size_t thing and save it in a size_t object and suddenly get a huge number because we didn't realize the difference could be negative.
https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:33: size_t framesInFIFO() const { return m_framesInFIFO; } On 2017/01/10 10:07:20, o1ka wrote: > This one can be inline. Ignore this comment, I thought I was in cpp :) https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; On 2017/01/10 16:18:15, Raymond Toy wrote: > Also, do all of these really need to be size_t? I think int would be a > perfectly fine type. > > This is another nit I have with the current code base: size_t everywhere, even > for things we know will never be even close to 64-bits long. And it's also been > a source of bugs when we take the difference of two size_t thing and save it in > a size_t object and suddenly get a huge number because we didn't realize the > difference could be negative. Agree. And as I mentioned somewhere we really need to crash (even in release build) if memory indexes are wrong.
Thanks very much for your review so far. I think now we have settled with variable names and meaning. In the following patch sets, I am going to work on handling corner cases and a unit test to verify them. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: inputBusLength + m_numberOfFrames > m_length) { On 2017/01/10 10:07:19, o1ka wrote: > On 2017/01/09 21:53:16, hongchan wrote: > > On 2017/01/06 22:22:03, Raymond Toy wrote: > > > Need a check if number of channels for the bus matches the number of > channels > > in > > > the FIFO? What should happen if they don't match? > > > > @rtoy > > > > Hmm. My code is based on the current FIFO design, but maybe we need a fresh > look > > on its heuristic. However, is it really possible? The size of AudioBus is > fixed > > after the creation, which is based on the info from the audio device. I don't > > think that the audio device channel config can be changed on the fly. > > > > I think it's up to how our audio backend behaves. > > > > @o1ka > > > > Should I separate these checks into multiple assertions? What do you mean by > > "inputBusLength looking good by itself"? > Sorry for being unclear, my comment was about variable names. > Yes, it's better to have separate DCHECKs Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: } On 2017/01/09 22:46:02, Raymond Toy wrote: > On 2017/01/09 21:53:16, hongchan wrote: > > On 2017/01/06 22:22:03, Raymond Toy wrote: > > > Can we add DCHECKs for these too? While it's nice to have NOTREACHED, when > it > > > fails, there are 4 conditions that could have caused the failure and we > won't > > > know what caused it. > > > > I think these checks are being overly careful about the conditions. Here's my > > thoughts on 4 checks: > > > > 1) remainder > m_length: this is redundant because the remainder cannot be > > greater than m_length after the line 29, but size_t can go crazy when it wraps > > around. > > > > 2) m_indexWrite > m_length: this cannot happen after the line 97, but I think > > this check is to handle the size_t wrap-around that might happen. I can't > think > > of such case though. > > > > 3) inputBusLength > m_length: is this needed? it depends on the behavior that > we > > want from this FIFO. Even if the condition is met at the current iteration, > the > > next iteration easily overflows and wraps around the FIFO if the consumer does > > not pull the frames out of it. > > > > 4) inputBusLength + m_numberOfFrames: almost same with 3). We can hope for the > > best, but the bad thing always can happen when the consumer does not behave > > nicely. > > > > I blindly used these checks from the old code, but we might have to reevaluate > > them again. > > Ok. I'll leave it up to you to decide which checks to remove and which to keep. > But if we're going to keep some, a DCHECK for the individual check would be > really nice for debugging. Done. https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:56: m_numberOfFrames += inputBusLength; On 2017/01/10 10:07:19, o1ka wrote: > On 2017/01/09 21:53:16, hongchan wrote: > > On 2017/01/09 14:09:03, o1ka wrote: > > > What does |m_numberOfFrames| represent? Can it be more than FIFO length? > What > > > should it be in case of overflow/wrap around above? > > > > It represents 'meaningful frames' that have not been played by the device. > > |framesToBePlayed| might be better? Please note that 'inputBusLength + > > m_numberOfFrames < m_length' check exists above. > > |framesAvailable| maybe? Done. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:119: size_t framesToPull = numberOfFrames - m_fifo->framesInFIFO(); On 2017/01/09 22:46:02, Raymond Toy wrote: > Although this isn't caused by this CL, what happens if numberOfFrames < > framesInFIFO()? Should there at least be a DCHECK for this? Yes, this is actually unsafe. framesToPull can be a huge number. Glad we caught this here. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:99: // To push the rendered result from WebAudio graph into the FIFO. On 2017/01/09 22:46:02, Raymond Toy wrote: > I'm sorry. "To pass" and "To push" don't really help me understand what > m_outputBus and m_renderBus hold. They are pointers. WebAudio renderer fill up the m_renderBus to push the data into FIFO while the audio device callback uses m_outputBus to pull out the data out of FIFO. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:32: // Returns the number of valid frames in FIFO. On 2017/01/09 22:46:02, Raymond Toy wrote: > This comment is probably better placed as a description of m_framesInFIFO. Done. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; On 2017/01/10 16:18:15, Raymond Toy wrote: > Also, do all of these really need to be size_t? I think int would be a > perfectly fine type. > > This is another nit I have with the current code base: size_t everywhere, even > for things we know will never be even close to 64-bits long. And it's also been > a source of bugs when we take the difference of two size_t thing and save it in > a size_t object and suddenly get a huge number because we didn't realize the > difference could be negative. Are you suggesting we change our convention starting from this CL? I am happy to do that. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; On 2017/01/10 10:07:20, o1ka wrote: > const? Done. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:39: size_t m_fifoLength; On 2017/01/10 16:41:39, o1ka wrote: > On 2017/01/10 16:18:15, Raymond Toy wrote: > > Also, do all of these really need to be size_t? I think int would be a > > perfectly fine type. > > > > This is another nit I have with the current code base: size_t everywhere, even > > for things we know will never be even close to 64-bits long. And it's also > been > > a source of bugs when we take the difference of two size_t thing and save it > in > > a size_t object and suddenly get a huge number because we didn't realize the > > difference could be negative. > > Agree. And as I mentioned somewhere we really need to crash (even in release > build) if memory indexes are wrong. I'll investigate corner cases in the next patch set with a unit test. https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:40: size_t m_framesInFIFO; On 2017/01/09 22:46:02, Raymond Toy wrote: > I can guess the difference between m_fifoLength and m_framesInFIFO, we should > probably make it clear with comments. fifoLength is the size of the FIFO, and > framesInFIFO is how many frames of data are actually in the FIFO. Presumably > fifoLength is also measured in frames? Done. Yes, that's correct.
PTAL. In this patch set, I have done: - Converted size_t to int - Added DCHECK(), SECURITY_DCHECK() to handle corner cases - Now pulling an empty FIFO is a valid operation; provide silence when frames are short. - A unit test
I'm a bit concerned about the underflow not being an error. I think previously we supported double buffering. Not great, but not horrible for latency. Now, with underflow not being an error, we can potentially be much worse than double buffering and there's no way to know. Would it not make more sense to set up the fifo for double buffering, and initializing the read and write indices to do that and the causing an error if we should ever underflow? (Well, I guess the audio will glitch, but we won't really know why.) https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:120: DCHECK_LE(m_fifo->framesAvailable(), static_cast<int>(numberOfFrames)); This is an unsafe cast because numberOfFrames is shortened from size_t to int (on 64-bit machines). It's safer to cast framesAvailable() to size_t. (Well, assuming framesAvailable is always non-negative.) https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:88: // and the consumer will get a block of silence. This is a good comment, but I think you really need to describe the behavior of the fifo in the header as a general description of the class. Include info on how the pointers work and what happens if they cross. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:106: memcpy(outputBusChannel + remainder, fifoBusChannel, Should we add DCHECKs here to make sure that we don't write past the end of the destination or read past the end of the source? (Other parts of webaudio seem to do that.) Same comment for other uses of memcpy and memset. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: const size_t kCallbackBufferSize = 1024; If you have to pick values for this, why not use "funny" values? Say 3 channels, fifo size that isn't a power of two, callback size like 3072 (or some other random Android value that we've seen.)
https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:28: int inputBusLength = static_cast<int>(inputBus->length()); const? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:29: int remainder = m_fifoLength - m_indexWrite; const? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: DCHECK(!fifoOverflow); DCHECK means further execution under this condition does not make sense. But l.61 assumes that fifoOverflow can be true. Remove DCHECK? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:69: void PushPullFIFO::pull(AudioBus* outputBus, size_t framesToPull) { Name it framesRequested? framesToPull, framesToFill and framesToProvide side by side are very confusing. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: int framesToProvide = static_cast<int>(framesToPull); both const? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: int framesToProvide = static_cast<int>(framesToPull); This is not safe cast. Switch interface to int? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:90: int silentFrames = std::max(0, framesToProvide - m_framesAvailable); both const? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:90: int silentFrames = std::max(0, framesToProvide - m_framesAvailable); Just framesToProvide-framesToFill? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:123: m_framesAvailable -= framesToFill; Something like DCHECK((m_indexRead + m_framesAvailable) % m_fifoLength == m_indexWrite) ? BTW - that's why I do not quite like to have all of three: one can be calculated from the other two. And if we forget to update one of 3, everything is broken. It may be safer to have getFramesAvailable() calculating the value from the two indexes. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // be non-blocking to ensure the glitch-less audio playback. What are the plans regarding thread safety? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: const size_t kCallbackBufferSize = 1024; On 2017/01/12 19:29:40, Raymond Toy wrote: > If you have to pick values for this, why not use "funny" values? Say 3 > channels, fifo size that isn't a power of two, callback size like 3072 (or some > other random Android value that we've seen.) I believe we need to have parametrized tests (TEST_P) and test different combinations of values. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:20: const size_t kRenderQuantum = 128; The values are multipliers of each other, so many corner cases are not covered. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:30: // Check if AudioBus is filled with a specific value within the given range. To make sure the indexes are always consistent and we read/write at the right places, we may want to fill with increments instead of uniform values. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:136: EXPECT_DEBUG_DEATH(fifo->push(inputBus.get()), "fifoOverflow"); But this should not be the case in real life, right? We should eb able to continue execution. So should it be in the code/tests as well.
o1ka@ Thanks so much for your review! It has been tremendously helpful. Summary of PS6: - DCHECK for over/underflow is removed. Instead, LOG(WARNING) is added. - The unit test uses the incremental vector as a test input, and verify the index and its value. - The unit test is now parameterized with TEST_P. - The unit test is not completed yet. If you think this is good enough to expand, I will add more corner cases. Also I will be OOO next week so the follow up might be slow. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:120: DCHECK_LE(m_fifo->framesAvailable(), static_cast<int>(numberOfFrames)); On 2017/01/12 19:29:40, Raymond Toy wrote: > This is an unsafe cast because numberOfFrames is shortened from size_t to int > (on 64-bit machines). It's safer to cast framesAvailable() to size_t. (Well, > assuming framesAvailable is always non-negative.) Oops. Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:28: int inputBusLength = static_cast<int>(inputBus->length()); On 2017/01/13 11:23:44, o1ka wrote: > const? Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:29: int remainder = m_fifoLength - m_indexWrite; On 2017/01/13 11:23:44, o1ka wrote: > const? Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:37: DCHECK(!fifoOverflow); On 2017/01/13 11:23:44, o1ka wrote: > DCHECK means further execution under this condition does not make sense. But > l.61 assumes that fifoOverflow can be true. > Remove DCHECK? I talked with rtoy@ about these conditions - what's the best practice to notify when this actually happens? We certainly want to know when and why in the debug build. LOG? LOG_IF? DVLOG? https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:69: void PushPullFIFO::pull(AudioBus* outputBus, size_t framesToPull) { On 2017/01/13 11:23:44, o1ka wrote: > Name it framesRequested? framesToPull, framesToFill and framesToProvide side by > side are very confusing. Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: int framesToProvide = static_cast<int>(framesToPull); On 2017/01/13 11:23:44, o1ka wrote: > both const? Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: int framesToProvide = static_cast<int>(framesToPull); On 2017/01/13 11:23:44, o1ka wrote: > This is not safe cast. Switch interface to int? I have thought about it, but WebAudio has been using size_t for frame numbers. So I wanted to keep size_t for public method signature. Also there is a SECURITY_CHECK() right below to catch when framesToProvide goes crazy. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:88: // and the consumer will get a block of silence. On 2017/01/12 19:29:40, Raymond Toy wrote: > This is a good comment, but I think you really need to describe the behavior of > the fifo in the header as a general description of the class. Include info on > how the pointers work and what happens if they cross. Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:90: int silentFrames = std::max(0, framesToProvide - m_framesAvailable); On 2017/01/13 11:23:44, o1ka wrote: > Just framesToProvide-framesToFill? Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:106: memcpy(outputBusChannel + remainder, fifoBusChannel, On 2017/01/12 19:29:40, Raymond Toy wrote: > Should we add DCHECKs here to make sure that we don't write past the end of the > destination or read past the end of the source? (Other parts of webaudio seem > to do that.) > > Same comment for other uses of memcpy and memset. I believe this is done by line 81 and 82. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:123: m_framesAvailable -= framesToFill; On 2017/01/13 11:23:44, o1ka wrote: > Something like DCHECK((m_indexRead + m_framesAvailable) % m_fifoLength == > m_indexWrite) ? > BTW - that's why I do not quite like to have all of three: one can be calculated > from the other two. And if we forget to update one of 3, everything is broken. > It may be safer to have getFramesAvailable() calculating the value from the two > indexes. It seems like a neat idea, but not sure how to make it always correct. To handle the case of index flipping, we still need something like m_framesAvailable. Right? How do you calculate it only with two indexes? For the time being, I minimized the usage of m_framesAvailable in the code. It is fetched in the beginning and updated at the end, not in the middle. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // be non-blocking to ensure the glitch-less audio playback. On 2017/01/13 11:23:44, o1ka wrote: > What are the plans regarding thread safety? Giving a higher priority to pulling operation is one thing I have in mind. WebAudio has this lock/trylock structure between the audio thread and the other thread. So I am thinking about applying the same principle. In short, when the data is being pulled the storage will be locked. The push operation always have to wait for the pull operation. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: const size_t kCallbackBufferSize = 1024; On 2017/01/13 11:23:44, o1ka wrote: > On 2017/01/12 19:29:40, Raymond Toy wrote: > > If you have to pick values for this, why not use "funny" values? Say 3 > > channels, fifo size that isn't a power of two, callback size like 3072 (or > some > > other random Android value that we've seen.) > > I believe we need to have parametrized tests (TEST_P) and test different > combinations of values. Please bear with me - this is my first attempt to write a blink unit test. WebAudio has been focused on the layout test, not the c++ unit test. I will try the TEST_P. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: const size_t kCallbackBufferSize = 1024; On 2017/01/12 19:29:40, Raymond Toy wrote: > If you have to pick values for this, why not use "funny" values? Say 3 > channels, fifo size that isn't a power of two, callback size like 3072 (or some > other random Android value that we've seen.) Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:30: // Check if AudioBus is filled with a specific value within the given range. On 2017/01/13 11:23:44, o1ka wrote: > To make sure the indexes are always consistent and we read/write at the right > places, we may want to fill with increments instead of uniform values. Done. https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:136: EXPECT_DEBUG_DEATH(fifo->push(inputBus.get()), "fifoOverflow"); On 2017/01/13 11:23:44, o1ka wrote: > But this should not be the case in real life, right? We should eb able to > continue execution. So should it be in the code/tests as well. Without DCHECK, how can the test runner detect such case?
Friendly ping. I would like to add more unit tests. Please take a look at the example I have (PushPullFIFOTest.cpp) and let me know what you think!
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { What does "IncrementalIndex" mean here? https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:105: // incremental index. "incremental index"? This is just basically a linear ramp? https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:112: // parameter. "test parameter" -> |framesToPull| argument. Or something because "test parameter" is too since there are many parameters to PushPullFIFOTestParam. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:124: } Should there be some checks on the values of the read/write indices? Or is testing the output good enough? Should there be multiple push/pulls tested too to verify that the indices are updated correctly? Need tests to verify wrap around too? Maybe that should be in a different test?
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { On 2017/01/23 17:52:25, Raymond Toy wrote: > What does "IncrementalIndex" mean here? [0, 1, 2, 3, 4, 5, 6, .... ] You can call it a ramp, but this is literally an array filled with indexes. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:105: // incremental index. On 2017/01/23 17:52:25, Raymond Toy wrote: > "incremental index"? This is just basically a linear ramp? See the comment above. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:112: // parameter. On 2017/01/23 17:52:25, Raymond Toy wrote: > "test parameter" -> |framesToPull| argument. Or something because "test > parameter" is too since there are many parameters to PushPullFIFOTestParam. Done. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:124: } On 2017/01/23 17:52:25, Raymond Toy wrote: > Should there be some checks on the values of the read/write indices? Or is > testing the output good enough? For that, I will have to add some methods to the class to extract the value. > Should there be multiple push/pulls tested too to verify that the indices are > updated correctly? Need tests to verify wrap around too? Maybe that should be > in a different test? Yes, that's the idea. If we like the pattern in this patchset then I will start expanding the unit test with different cases.
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { On 2017/01/23 19:37:36, hongchan wrote: > On 2017/01/23 17:52:25, Raymond Toy wrote: > > What does "IncrementalIndex" mean here? > > [0, 1, 2, 3, 4, 5, 6, .... ] > > You can call it a ramp, but this is literally an array filled with indexes. The name is not very informative because there's no context on what "Incremental" is or what "Index" you're referring to. At least fillBusWithLinearRamp kind of tells you whats in the bus. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:124: } On 2017/01/23 19:37:36, hongchan wrote: > On 2017/01/23 17:52:25, Raymond Toy wrote: > > Should there be some checks on the values of the read/write indices? Or is > > testing the output good enough? > > For that, I will have to add some methods to the class to extract the value. Oh. I was kind of expecting this to already be available since the Param already includes expectedReadIndex and expectedWriteIndex. So if you're not going to test the indices in this CL, I'd remove these (if that is what they really mean). If you are going to test, then we need the new methods.
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:124: } On 2017/01/23 21:53:53, Raymond Toy wrote: > On 2017/01/23 19:37:36, hongchan wrote: > > On 2017/01/23 17:52:25, Raymond Toy wrote: > > > Should there be some checks on the values of the read/write indices? Or is > > > testing the output good enough? > > > > For that, I will have to add some methods to the class to extract the value. > > Oh. I was kind of expecting this to already be available since the Param already > includes expectedReadIndex and expectedWriteIndex. So if you're not going to > test the indices in this CL, I'd remove these (if that is what they really > mean). If you are going to test, then we need the new methods. Based on quick offline discussion, the test looks fine. I am starting to expand with more test cases.
https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:120: DCHECK_LE(static_cast<size_t>(m_fifo->framesAvailable()), numberOfFrames); This does not guarantee that |framesToPull| fits into int. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:33: const int inputBusLength = static_cast<int>(inputBus->length()); unsafe https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:88: const int framesToProvide = static_cast<int>(framesRequested); unsafe https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: // Pushing audio data is done by the WebAudio render-backing thread. The Do you plan to first finalize all the index calculations and tests for them and then tackle multithreading? https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:47: const int m_fifoLength; You can use "= 0" initialization here. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:55: unsigned m_overflowCount; We should not use a mix of size_t, int and unsigned. Please consider https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... to decide on types, a well as base/numerics/safe_conversions.h base/numerics/safe_math.h base/numerics/saturated_arithmetic.h for safe arithmetic. (rtoy@, re: your initial suggestion to switch from size_t to int - wouldn't it be safe to stick with size_t and safe arithmetic?) https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:66: const size_t fifoSize; Could you provide comments on what each value means? https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:114: RefPtr<AudioBus> outputBus = This looks good. Another set of cases to test is long sequences of pulls and pushes with different patterns (m pushes in a row, n pulls in a row), starting from an empty fifo. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:118: // Verify the position of current indexes and their values. We also want to check the content of |outputBus|.
Sorry about massive (+ messy) changes since the previous patchset. Here is the summary: - A newly designed unit test: now the test parameters are completely configurable. The test runner can perform multiple push/pull and then inspect FIFO and the output bus. - int => size_t: I reverted all |int| to |size_t| based on Chromium's convention. - FIFO now keeps the record of overflow/underflow internally. The only thing I am missing is to check if SECURITY_CHECK() crashes the build correctly. However, that change is sort of orthogonal to what happens in this patchset so I will get to that later. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { On 2017/01/23 21:53:53, Raymond Toy wrote: > On 2017/01/23 19:37:36, hongchan wrote: > > On 2017/01/23 17:52:25, Raymond Toy wrote: > > > What does "IncrementalIndex" mean here? > > > > [0, 1, 2, 3, 4, 5, 6, .... ] > > > > You can call it a ramp, but this is literally an array filled with indexes. > > The name is not very informative because there's no context on what > "Incremental" is or what "Index" you're referring to. At least > fillBusWithLinearRamp kind of tells you whats in the bus. Done. https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:112: // parameter. On 2017/01/23 19:37:36, hongchan wrote: > On 2017/01/23 17:52:25, Raymond Toy wrote: > > "test parameter" -> |framesToPull| argument. Or something because "test > > parameter" is too since there are many parameters to PushPullFIFOTestParam. > > Done. This is sort of a combination of push and pull. So technically |the test parameter| is a correct term here. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:120: DCHECK_LE(static_cast<size_t>(m_fifo->framesAvailable()), numberOfFrames); On 2017/01/24 11:09:02, o1ka wrote: > This does not guarantee that |framesToPull| fits into int. Changing all ints to size_t. That will solve this issue. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:33: const int inputBusLength = static_cast<int>(inputBus->length()); On 2017/01/24 11:09:02, o1ka wrote: > unsafe Done. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:88: const int framesToProvide = static_cast<int>(framesRequested); On 2017/01/24 11:09:02, o1ka wrote: > unsafe Done. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: // Pushing audio data is done by the WebAudio render-backing thread. The On 2017/01/24 11:09:02, o1ka wrote: > Do you plan to first finalize all the index calculations and tests for them and > then tackle multithreading? Yes. That's the plan. The FIFO in this CL should be a drop-in replacement for the old FIFO, then I will introduce the multithreading. I would like to lay out some groundwork for it, but will not add any safety measure for multithreading in this CL. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:47: const int m_fifoLength; On 2017/01/24 11:09:02, o1ka wrote: > You can use "= 0" initialization here. Done. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:55: unsigned m_overflowCount; On 2017/01/24 11:09:02, o1ka wrote: > We should not use a mix of size_t, int and unsigned. > Please consider > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > to decide on types, a well as > base/numerics/safe_conversions.h > base/numerics/safe_math.h > base/numerics/saturated_arithmetic.h > for safe arithmetic. > > (rtoy@, re: your initial suggestion to switch from size_t to int - wouldn't it > be safe to stick with size_t and safe arithmetic?) This is a great resource. Thanks and we will follow this style. However, *flowCount variables are not a number of frames or an index. These are just internal counters for the under/overflow case. Should I still use size_t for this purpose? https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:66: const size_t fifoSize; On 2017/01/24 11:09:02, o1ka wrote: > Could you provide comments on what each value means? I have restructured the unit test. PTAL the new design. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:114: RefPtr<AudioBus> outputBus = On 2017/01/24 11:09:02, o1ka wrote: > This looks good. > Another set of cases to test is long sequences of pulls and pushes with > different patterns (m pushes in a row, n pulls in a row), starting from an empty > fifo. Done. https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:118: // Verify the position of current indexes and their values. On 2017/01/24 11:09:02, o1ka wrote: > We also want to check the content of |outputBus|. Done.
https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:105: return; As I said before, I feel strongly against silent failures and prefer CHECKs. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:148: framesToRender -= AudioUtilities::kRenderQuantumFrames; This will hit size_t underflow. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:70: m_framesAvailable += inputBusLength; m_framesAvailable cannot be more than m_fifoLength. But if you got to l.59, it will be. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:79: return; Can we avoid silent failures by using CHECK()? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:99: framesToFill * sizeof(*fifoBusChannel)); What if framesToFill * sizeof(*fifoBusChannel) overflows? Switch to safe math? (same in other places) https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:118: m_indexRead = (m_indexRead + framesToFill) % m_fifoLength; What is addition overflows? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:137: return {length(), numberOfChannels(), framesAvailable(), m_indexRead, Does this spacing comply with style guide? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:17: const unsigned numberOfChannels; is "unsigned" a part of WebKit style guide? Chromium usually uses int for these purposes. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:52: AudioBus* bus() { return m_fifoBus.get(); } const https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:70: unsigned m_overflowCount; just int? you count up to ~100 only https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:113: {335, 335, 0, 0, {}, {{0, 11}, {323, 334}}}}}; This is nice, but what I was suggesting was a bit different. Something like this: Fifo size: 12345 push: buffer size 11, frequency 121 pushes/100 cycles pull: buffer size 17, frequency 113 pulls/100 cycles test duration: 15000 cycles. Please make sure to use all primes for a portion of test cases, to catch various corner cases.
https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; Is this the right thing to do? If the FIFO doesn't have enough frames to fulfill the request for numberOfFrames, we do nothing. If this is the expected behavior, please add a comment on why this is right. Otherwise, generate an error or at least a LOG message https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:148: framesToRender -= AudioUtilities::kRenderQuantumFrames; On 2017/01/27 13:47:54, o1ka wrote: > This will hit size_t underflow. Yeah. And what's supposed to happen if it does underflow? Another reason why signed numbers are usually better than unsigned. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:61: LOG(WARNING) << "PushPullFIFO::overflow while pushing (" Grammar: "PushPullFIFO: overflow..." The message currently looks like its you're talking about the function overflow. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:63: << ", availalbeFrames=" << m_framesAvailable Typo. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:79: return; On 2017/01/27 13:47:54, o1ka wrote: > Can we avoid silent failures by using CHECK()? What is the intent here? Because of the checks below, outpuBus really needs to exist and have a size greater than framesRequested. I agree with olka. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:89: SECURITY_CHECK(m_indexRead < m_fifoLength); Move these to the beginning of the function. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:124: LOG(WARNING) << "PushPullFIFO::underflow while pulling (" Grammar: "PushPullFIFO: overflow while...". Looks like you're talking about the function underflow. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2016 or 2017? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:28: // render quantum; thus FIFO is needed. I think there have been quite a few comments on what the constraints are on this fifo. Can we had some comments here are the assumed contract? You mean WebAudio operates on 128 frames, so is that a contract on the push side of the fifo? Do you want this FIFO to be usable for "anything"? (I think not.) https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:20: // (e.g. [c + 0, c + 1, c + 2, c + 3, c + 4, ...]) Without looking at the code, what is c? How is frameCounter used? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:47: // Data type for FIFO actions {"PUSH", "PULL"} with the frame count. Put these comments with the variables below. |type| and |frames| are pretty meaningless with comments. Or better yet, choose better names. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:61: // Data type of AudioBus verification; an index and the associated value. This doesn't help me to understand what |index| and |value| mean. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:72: const size_t indexWrite; Document what these two really mean. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:93: // 1. Push 1024 frames first then pull 1024 frames. It would help if there were a contract/documentation on what this FIFO is intended to support. For example, will the audio thread ever push anything other than 128 frames? If not, we don't need to test anything other than this. (Except maybe to make sure we crash if something other than 128 is used.)
Patchset #9 (id:180001) has been deleted
I added some comments on the contract for FIFO: (RQ = WebAudio Render Quantum, 128 frames) - The maximum FIFO length is 32768 frames (256 RQs). - The push size is 128 frames (1 RQ), fixed. - The pull size can be flexible. - In case of overflow (FIFO full while push), the existing frames in FIFO will be overwritten and the read index will be forcibly moved to where the write index is located. - In case of underflow (FIFO empty while pull), the remaining space in the output bus will be filled with silence. Thus it will fulfill the request from the consumer without causing error, but with a glitch. o1ka@ I haven't changed the test as you suggested yet. If we all agree on the contract I will change the test structure. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:105: return; On 2017/01/27 13:47:54, o1ka wrote: > As I said before, I feel strongly against silent failures and prefer CHECKs. As we explained, it is a legacy pattern inWebAudio. I agree that it is a bad practice, I think it is equally bad for users to see random crash all of sudden. With that said, after the discussion with rtoy@ we are going to roll out the "early crash" policy starting from here. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; On 2017/01/27 17:39:43, Raymond Toy wrote: > Is this the right thing to do? If the FIFO doesn't have enough frames to > fulfill the request for numberOfFrames, we do nothing. No. That means we need to render more frames from the audio graph and fill the FIFO before the audio thread pulls. > If this is the expected > behavior, please add a comment on why this is right. Otherwise, generate an > error or at least a LOG message I believe the comment above is good enough. This is not a case of error or a incorrect behavior. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:148: framesToRender -= AudioUtilities::kRenderQuantumFrames; On 2017/01/27 17:39:43, Raymond Toy wrote: > On 2017/01/27 13:47:54, o1ka wrote: > > This will hit size_t underflow. > > Yeah. And what's supposed to happen if it does underflow? Another reason why > signed numbers are usually better than unsigned. I guess we have to be more careful and strict about the contract. I'll clamp the frame count here. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:61: LOG(WARNING) << "PushPullFIFO::overflow while pushing (" On 2017/01/27 17:39:44, Raymond Toy wrote: > Grammar: "PushPullFIFO: overflow..." The message currently looks like its > you're talking about the function overflow. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:63: << ", availalbeFrames=" << m_framesAvailable On 2017/01/27 17:39:43, Raymond Toy wrote: > Typo. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:70: m_framesAvailable += inputBusLength; On 2017/01/27 13:47:54, o1ka wrote: > m_framesAvailable cannot be more than m_fifoLength. > But if you got to l.59, it will be. You're right. I am reverting it with std::min(). https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:79: return; On 2017/01/27 17:39:44, Raymond Toy wrote: > On 2017/01/27 13:47:54, o1ka wrote: > > Can we avoid silent failures by using CHECK()? > > What is the intent here? Because of the checks below, outpuBus really needs to > exist and have a size greater than framesRequested. I agree with olka. Yes. I was simply following WebAudio's convention and will be happy to remove this pattern. I will use CHECK() and crash when it fails. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:89: SECURITY_CHECK(m_indexRead < m_fifoLength); On 2017/01/27 17:39:44, Raymond Toy wrote: > Move these to the beginning of the function. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:99: framesToFill * sizeof(*fifoBusChannel)); On 2017/01/27 13:47:54, o1ka wrote: > What if framesToFill * sizeof(*fifoBusChannel) overflows? > Switch to safe math? > (same in other places) With the newly introduced max FIFO length, the overflow cannot happen. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:118: m_indexRead = (m_indexRead + framesToFill) % m_fifoLength; On 2017/01/27 13:47:54, o1ka wrote: > What is addition overflows? With the newly introduced max FIFO length, the overflow cannot happen. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:124: LOG(WARNING) << "PushPullFIFO::underflow while pulling (" On 2017/01/27 17:39:44, Raymond Toy wrote: > Grammar: "PushPullFIFO: overflow while...". Looks like you're talking about the > function underflow. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:137: return {length(), numberOfChannels(), framesAvailable(), m_indexRead, On 2017/01/27 13:47:54, o1ka wrote: > Does this spacing comply with style guide? I checked twice; this is the result of |git cl format|. I really hate what it did, so I can indent this myself and ignore the warning. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/27 17:39:44, Raymond Toy wrote: > 2016 or 2017? A good question. I'll take 2017. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:17: const unsigned numberOfChannels; On 2017/01/27 13:47:54, o1ka wrote: > is "unsigned" a part of WebKit style guide? Chromium usually uses int for these > purposes. Yes, I have seen it in many files in WebKit. https://cs.chromium.org/search/?q=unsigned+file:%5Esrc/third_party/WebKit/Sou... WebAudio's convention is using |unsigned| for channel count. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:28: // render quantum; thus FIFO is needed. On 2017/01/27 17:39:44, Raymond Toy wrote: > I think there have been quite a few comments on what the constraints are on this > fifo. Can we had some comments here are the assumed contract? You mean > WebAudio operates on 128 frames, so is that a contract on the push side of the > fifo? Do you want this FIFO to be usable for "anything"? (I think not.) I agree. I will add few constraints in the comment. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:52: AudioBus* bus() { return m_fifoBus.get(); } On 2017/01/27 13:47:54, o1ka wrote: > const Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:70: unsigned m_overflowCount; On 2017/01/27 13:47:54, o1ka wrote: > just int? you count up to ~100 only I am using unsigned for the same reason explained above. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:20: // (e.g. [c + 0, c + 1, c + 2, c + 3, c + 4, ...]) On 2017/01/27 17:39:44, Raymond Toy wrote: > Without looking at the code, what is c? How is frameCounter used? Not sure how to rephrase this better. I think it is quiet clear - can you make some suggestions? https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:47: // Data type for FIFO actions {"PUSH", "PULL"} with the frame count. On 2017/01/27 17:39:44, Raymond Toy wrote: > Put these comments with the variables below. |type| and |frames| are pretty > meaningless with comments. Or better yet, choose better names. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:61: // Data type of AudioBus verification; an index and the associated value. On 2017/01/27 17:39:44, Raymond Toy wrote: > This doesn't help me to understand what |index| and |value| mean. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:72: const size_t indexWrite; On 2017/01/27 17:39:44, Raymond Toy wrote: > Document what these two really mean. Done. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:93: // 1. Push 1024 frames first then pull 1024 frames. On 2017/01/27 17:39:44, Raymond Toy wrote: > It would help if there were a contract/documentation on what this FIFO is > intended to support. For example, will the audio thread ever push anything > other than 128 frames? If not, we don't need to test anything other than this. > (Except maybe to make sure we crash if something other than 128 is used.) Added several comments in the FIFO header file. https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:113: {335, 335, 0, 0, {}, {{0, 11}, {323, 334}}}}}; On 2017/01/27 13:47:54, o1ka wrote: > This is nice, but what I was suggesting was a bit different. > Something like this: > Fifo size: 12345 > push: buffer size 11, frequency 121 pushes/100 cycles > pull: buffer size 17, frequency 113 pulls/100 cycles > test duration: 15000 cycles. > > Please make sure to use all primes for a portion of test cases, to catch various > corner cases. Before modifying the test structure, I think we all should agree with the premise/contract of the FIFO design. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; rtoy@ and I believe 32K is good enough based on the data we collected. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:37: // from the consumer without causing error, but with a glitch. Here's the contract of FIFO. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:73: RefPtr<AudioBus> m_fifoBus; The order is changed because of the change in initializers.
On 2017/01/27 22:39:15, hongchan wrote: > I added some comments on the contract for FIFO: > (RQ = WebAudio Render Quantum, 128 frames) > > - The maximum FIFO length is 32768 frames (256 RQs). Just took a look at the latest UMA numbers. There's some device out there that wants 46080. So 64K would be a better limit. > - The push size is 128 frames (1 RQ), fixed. > - The pull size can be flexible. > - In case of overflow (FIFO full while push), the existing frames in FIFO > will be overwritten and the read index will be forcibly moved to where the > write index is located. I assume this will mean that the next read will get the newly added samples. Is that right? Should we continue to read from the buffer, returning the old values? > - In case of underflow (FIFO empty while pull), the remaining space in the > output bus will be filled with silence. Thus it will fulfill the request > from the consumer without causing error, but with a glitch.
Unfortunately I'm under a heavy load now and don't have free cycles to iterate through each version of this CL. Feel free to ping me if you have any specific questions, otherwise I'll be happy to review it when all the safe memory offset math, FIFO logic, threading and unit tests are in place. Thanks! https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; On 2017/01/27 22:39:13, hongchan wrote: > On 2017/01/27 17:39:43, Raymond Toy wrote: > > > Is this the right thing to do? If the FIFO doesn't have enough frames to > > fulfill the request for numberOfFrames, we do nothing. > > No. That means we need to render more frames from the audio graph and fill the > FIFO before the audio thread pulls. > Do you mean you are going to block the rendering thread if there is not enough data? We should not put off decisions on threading any more. Threading needs to be designed into the fifo.
o1ka@ Your help on this CL has been valuable. Thank you so much for that and I understand your priority! One thing: I wanted to introduce the multithreading support in the follow-up CL. This CL already has too much stuff going on, so I think it's better to keep things simple because we're working on the main pipeline of WebAudio. On Mon, Jan 30, 2017 at 3:41 AM <olka@chromium.org> wrote: > Unfortunately I'm under a heavy load now and don't have free cycles to > iterate > through each version of this CL. > > Feel free to ping me if you have any specific questions, otherwise I'll be > happy > to review it when all the safe memory offset math, FIFO logic, threading > and > unit tests are in place. > > Thanks! > > > > > https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp > (right): > > > https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; > On 2017/01/27 22:39:13, hongchan wrote: > > On 2017/01/27 17:39:43, Raymond Toy wrote: > > > > > Is this the right thing to do? If the FIFO doesn't have enough > frames to > > > fulfill the request for numberOfFrames, we do nothing. > > > > No. That means we need to render more frames from the audio graph and > fill the > > FIFO before the audio thread pulls. > > > > Do you mean you are going to block the rendering thread if there is not > enough data? > We should not put off decisions on threading any more. Threading needs > to be designed into the fifo. > > https://codereview.chromium.org/2549093009/ > -- 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.
o1ka@ Your help on this CL has been valuable. Thank you so much for that and I understand your priority! One thing: I wanted to introduce the multithreading support in the follow-up CL. This CL already has too much stuff going on, so I think it's better to keep things simple because we're working on the main pipeline of WebAudio. On Mon, Jan 30, 2017 at 3:41 AM <olka@chromium.org> wrote: > Unfortunately I'm under a heavy load now and don't have free cycles to > iterate > through each version of this CL. > > Feel free to ping me if you have any specific questions, otherwise I'll be > happy > to review it when all the safe memory offset math, FIFO logic, threading > and > unit tests are in place. > > Thanks! > > > > > https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp > (right): > > > https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; > On 2017/01/27 22:39:13, hongchan wrote: > > On 2017/01/27 17:39:43, Raymond Toy wrote: > > > > > Is this the right thing to do? If the FIFO doesn't have enough > frames to > > > fulfill the request for numberOfFrames, we do nothing. > > > > No. That means we need to render more frames from the audio graph and > fill the > > FIFO before the audio thread pulls. > > > > Do you mean you are going to block the rendering thread if there is not > enough data? > We should not put off decisions on threading any more. Threading needs > to be designed into the fifo. > > https://codereview.chromium.org/2549093009/ > -- 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.
> > One thing: I wanted to introduce the multithreading support in the > follow-up CL. This CL already has too much stuff going on, so I think it's > better to keep things simple because we're working on the main pipeline of > WebAudio. > If so - make sure to change FIFO comments :). Multi-threading may affect decision making for FIFO - see my comment above. So it may need a re-work in a follow-up cl.
Yes, I am expecting that, but also it's important not to break the current behavior. I am willing to take some time to see the new FIFO can safely replace the previous one. The last item that I will address in the next CL is the unit test that you suggested. Also I will remove the comment with the 'multi-threading' hint. On Mon, Jan 30, 2017 at 9:14 AM <olka@chromium.org> wrote: > > > > One thing: I wanted to introduce the multithreading support in the > > follow-up CL. This CL already has too much stuff going on, so I think > it's > > better to keep things simple because we're working on the main pipeline > of > > WebAudio. > > > > If so - make sure to change FIFO comments :). > Multi-threading may affect decision making for FIFO - see my comment > above. So > it may need a re-work in a follow-up cl. > > https://codereview.chromium.org/2549093009/ > -- 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.
Yes, I am expecting that, but also it's important not to break the current behavior. I am willing to take some time to see the new FIFO can safely replace the previous one. The last item that I will address in the next CL is the unit test that you suggested. Also I will remove the comment with the 'multi-threading' hint. On Mon, Jan 30, 2017 at 9:14 AM <olka@chromium.org> wrote: > > > > One thing: I wanted to introduce the multithreading support in the > > follow-up CL. This CL already has too much stuff going on, so I think > it's > > better to keep things simple because we're working on the main pipeline > of > > WebAudio. > > > > If so - make sure to change FIFO comments :). > Multi-threading may affect decision making for FIFO - see my comment > above. So > it may need a re-work in a follow-up cl. > > https://codereview.chromium.org/2549093009/ > -- 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/01/30 17:21:25, chromium-reviews wrote: > Yes, I am expecting that, but also it's important not to break the current > behavior. I am willing to take some time to see the new FIFO can safely > replace the previous one. The last item that I will address in the next CL > is the unit test that you suggested. I thought about the test pattern you suggested, and it's unnecessary for the current design. However, it looks really useful when this FIFO supports multithreading so I will use it in the future CL.
> > - The push size is 128 frames (1 RQ), fixed. > > - The pull size can be flexible. > > - In case of overflow (FIFO full while push), the existing frames in FIFO > > will be overwritten and the read index will be forcibly moved to where the > > write index is located. > > I assume this will mean that the next read will get the newly added samples. Is > that right? Should we continue to read from the buffer, returning the old > values? No. When it overflows, the next read will start from ((indexWrite+1) % fifoLength). The overwritten frames cannot be recovered, thus we should provide the next valid frame. On the other hand, the next write will start from |indexRead| when it underflows.
https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:118: // audio device. Please clarify this and explain what is intended to happen when framesToRender > 0. I assume that this means you have to ask the WebAudio to render more audio to fulfill the request. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:96: // To pass the data from FIFO to the audio device callback. nit: This comment is a bit unclear. Maybe say m_outputBus holds the data that will be sent to the audio device callback? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:99: // To push the rendered result from WebAudio graph into the FIFO. Likewise, m_renderBus is used to hold the data that the WebAudio graph will write to the FIFO. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.h:103: // the callback function from the actual audio device. I think you can just say this is a FIFO between the WebAudio and the audio device. There isn't always a size mismatch. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:16: const unsigned kLogCapOverUnderflow = 100; Maybe kMaxMessagesToLog? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: SECURITY_CHECK(m_indexWrite < m_fifoLength); You said the push size is 128. Shouldn't there be a check here for that? Otherwise, there's no enforcement of this requirement. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:76: // is less than the frames to pull, provides the remaining frames plus the s/the// https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: SECURITY_CHECK(framesRequested <= outputBus->length()); You need to add this to your contract. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:81: SECURITY_CHECK(framesRequested <= m_fifoLength); This too. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:134: m_framesAvailable > framesToFill ? m_framesAvailable - framesToFill : 0; Doesn't line 89 guarantees the m_framesAvailable >= framesToFill. Thus, no check needed. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:28: // render quantum (RQ); thus FIFO is needed. "128 frames of render quantum" is awkward. Rephrase. "thus FIFO is needed" => "thus FIFO is needed to handle the general case" or something like that. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // - The maximum FIFO length is 32768 frames (256 RQs). Recall that we encountered an Android device with callback size of 45K or so. Are we going to disallow working on such a device? Also, shouldn't that max constant be defined in this file? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:34: // write index is located. That doesn't really tell me what happens with the data that will be read. Please explain what data will be read. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:36: // output bus will be filled with silence. Thus it will fulfill the request What does "remaining space" mean here? The FIFO is empty. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:44: // clamped. Clamped to what? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:49: // priority of this thread is lower than the actual audio device thread, How do we know the priorities already? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: // values starting from |startingValue|. Then return value will be nit: "from |startingValue|" -> "from |startingValue| and incrementing by 1" https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:49: const char* type; "type" is too generic. Maybe "action"? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:99: // 1. Push 1024 frames first then pull 1024 frames. How does this work? Or does this really mean you push 128 frames 8 times before pulling 1024 frames out? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:104: // recorded and the wrap-around should happen correctly. Use odd channel "odd" mean not even? Or "odd" meaning unusual? I think you mean unusual here. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:111: // second half of the output bus silence. Use odd FIFO size. If you're inspecting the output, you need a test case with more than one channel. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:115: // 4. Push 11, 324 frames. Then pull 11, 324 frames. The most recent output Your contract said you can only push 128 frames. So what is this really testing?
hongchan@chromium.org changed reviewers: - olka@chromium.org
- Added a unit test for the contract. (death test) - Refined/moved comments. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:16: const unsigned kLogCapOverUnderflow = 100; On 2017/01/30 20:59:28, Raymond Toy wrote: > Maybe kMaxMessagesToLog? Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:34: SECURITY_CHECK(m_indexWrite < m_fifoLength); On 2017/01/30 20:59:28, Raymond Toy wrote: > You said the push size is 128. Shouldn't there be a check here for that? > Otherwise, there's no enforcement of this requirement. Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:76: // is less than the frames to pull, provides the remaining frames plus the On 2017/01/30 20:59:28, Raymond Toy wrote: > s/the// Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: SECURITY_CHECK(framesRequested <= outputBus->length()); On 2017/01/30 20:59:28, Raymond Toy wrote: > You need to add this to your contract. I can certainly add, but this is more of sanity check rather than the contract. You cannot request frames that is bigger than the output bucket. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:134: m_framesAvailable > framesToFill ? m_framesAvailable - framesToFill : 0; On 2017/01/30 20:59:28, Raymond Toy wrote: > Doesn't line 89 guarantees the m_framesAvailable >= framesToFill. Thus, no > check needed. Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:28: // render quantum (RQ); thus FIFO is needed. On 2017/01/30 20:59:28, Raymond Toy wrote: > "128 frames of render quantum" is awkward. Rephrase. > > "thus FIFO is needed" => "thus FIFO is needed to handle the general case" or > something like that. Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:29: // - The maximum FIFO length is 32768 frames (256 RQs). On 2017/01/30 20:59:28, Raymond Toy wrote: > Recall that we encountered an Android device with callback size of 45K or so. > Are we going to disallow working on such a device? > > Also, shouldn't that max constant be defined in this file? Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:34: // write index is located. On 2017/01/30 20:59:28, Raymond Toy wrote: > That doesn't really tell me what happens with the data that will be read. > Please explain what data will be read. Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:36: // output bus will be filled with silence. Thus it will fulfill the request On 2017/01/30 20:59:28, Raymond Toy wrote: > What does "remaining space" mean here? The FIFO is empty. "remaining space in the output bus" - that means the bus that will be passed to the audio device. Rephrased the comment a bit. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:44: // clamped. On 2017/01/30 20:59:28, Raymond Toy wrote: > Clamped to what? Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:49: // priority of this thread is lower than the actual audio device thread, On 2017/01/30 20:59:28, Raymond Toy wrote: > How do we know the priorities already? The comments are edited. No mention of multi-threading in this CL. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:19: // values starting from |startingValue|. Then return value will be On 2017/01/30 20:59:28, Raymond Toy wrote: > nit: "from |startingValue|" -> "from |startingValue| and incrementing by 1" Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:49: const char* type; On 2017/01/30 20:59:29, Raymond Toy wrote: > "type" is too generic. Maybe "action"? Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:104: // recorded and the wrap-around should happen correctly. Use odd channel On 2017/01/30 20:59:29, Raymond Toy wrote: > "odd" mean not even? Or "odd" meaning unusual? I think you mean unusual here. Done. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:111: // second half of the output bus silence. Use odd FIFO size. On 2017/01/30 20:59:28, Raymond Toy wrote: > If you're inspecting the output, you need a test case with more than one > channel. All the verification in this test is done through out the channels. For example, if you inspect the frame #10, you are inspecting values in #10 frames in all the channels. Do you think we have to test if copying AudioBus is working correctly? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:115: // 4. Push 11, 324 frames. Then pull 11, 324 frames. The most recent output On 2017/01/30 20:59:28, Raymond Toy wrote: > Your contract said you can only push 128 frames. So what is this really > testing? This is to accommodate o1ka@'s suggestion. Now we have the contract, I will fix the test accordingly.
Almost there. Just a few small questions. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; On 2017/01/27 22:39:14, hongchan wrote: > rtoy@ and I believe 32K is good enough based on the data we collected. I checked the metrics again. There is one case of 46080 in the last 28 days; 7 with 31744(!?!) Perhaps we fundamentally don't care about such huge latencies. But also consider that we support a max sample rate of 384 kHz now, so the fifo may need to be scaled up to accommodate that? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: SECURITY_CHECK(framesRequested <= outputBus->length()); On 2017/02/01 18:07:27, hongchan wrote: > On 2017/01/30 20:59:28, Raymond Toy wrote: > > You need to add this to your contract. > > I can certainly add, but this is more of sanity check rather than the contract. > You cannot request frames that is bigger than the output bucket. Well, you could have defined pull to use the min of outputBus->length() and framesRequested. So it's not clear what the constraints are for pull from reading the header file. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:81: SECURITY_CHECK(framesRequested <= m_fifoLength); On 2017/01/30 20:59:28, Raymond Toy wrote: > This too. I can imagine that if you request more than the fifo length, you could treat it as an underflow and fill in zeroes for the remaining data. You're not doing that, so it's part of the contract. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:195: {0, 0, 0, 0, {{0, 0}}, {{0, 256}, {255, 511}}}}, I think I'd add one "regular operation" test using mono and using quad (or 5.1) For completeness instead of mixing the channel count with the "unusual" configs below. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:200: // - Check if the indexes are wrapping around correctly. Are the outputs checked in these tests? If so, say it since you're saying what's being checked. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:230: {"PULL", 256}, Add test for overflow (and underflow) with weird fifo length and pull sizes. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:239: 2, Maybe use 3 channels here like for the overflow case.
https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; On 2017/02/01 19:02:37, Raymond Toy wrote: > On 2017/01/27 22:39:14, hongchan wrote: > > rtoy@ and I believe 32K is good enough based on the data we collected. > > I checked the metrics again. There is one case of 46080 in the last 28 days; 7 > with 31744(!?!) > > Perhaps we fundamentally don't care about such huge latencies. But also > consider that we support a max sample rate of 384 kHz now, so the fifo may need > to be scaled up to accommodate that? Should FIFO aware of sample rate? Is that what we want? https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:80: SECURITY_CHECK(framesRequested <= outputBus->length()); On 2017/02/01 19:02:38, Raymond Toy wrote: > On 2017/02/01 18:07:27, hongchan wrote: > > On 2017/01/30 20:59:28, Raymond Toy wrote: > > > You need to add this to your contract. > > > > I can certainly add, but this is more of sanity check rather than the > contract. > > You cannot request frames that is bigger than the output bucket. > > Well, you could have defined pull to use the min of outputBus->length() and > framesRequested. So it's not clear what the constraints are for pull from > reading the header file. // - If |framesRequested| is bigger than the length of |outputBus|, it // violates SECURITY_CHECK(). This comment is in the header. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:81: SECURITY_CHECK(framesRequested <= m_fifoLength); On 2017/02/01 19:02:38, Raymond Toy wrote: > On 2017/01/30 20:59:28, Raymond Toy wrote: > > This too. > > I can imagine that if you request more than the fifo length, you could treat it > as an underflow and fill in zeroes for the remaining data. You're not doing > that, so it's part of the contract. // - If |framesRequested| is bigger than FIFO length, it violates // SECURITY_CHECK(). I have this in the header. So it's in the contract. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:195: {0, 0, 0, 0, {{0, 0}}, {{0, 256}, {255, 511}}}}, On 2017/02/01 19:02:38, Raymond Toy wrote: > I think I'd add one "regular operation" test using mono and using quad (or 5.1) > For completeness instead of mixing the channel count with the "unusual" configs > below. Done. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:200: // - Check if the indexes are wrapping around correctly. On 2017/02/01 19:02:38, Raymond Toy wrote: > Are the outputs checked in these tests? If so, say it since you're saying > what's being checked. Done. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:230: {"PULL", 256}, On 2017/02/01 19:02:38, Raymond Toy wrote: > Add test for overflow (and underflow) with weird fifo length and pull sizes. Done. https://codereview.chromium.org/2549093009/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:239: 2, On 2017/02/01 19:02:38, Raymond Toy wrote: > Maybe use 3 channels here like for the overflow case. Done.
lgtm. Your discretion on updating the max size and whether to use a for loop. Thanks for your patience on this! https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; On 2017/02/01 23:09:06, hongchan wrote: > On 2017/02/01 19:02:37, Raymond Toy wrote: > > On 2017/01/27 22:39:14, hongchan wrote: > > > rtoy@ and I believe 32K is good enough based on the data we collected. > > > > I checked the metrics again. There is one case of 46080 in the last 28 days; > 7 > > with 31744(!?!) > > > > Perhaps we fundamentally don't care about such huge latencies. But also > > consider that we support a max sample rate of 384 kHz now, so the fifo may > need > > to be scaled up to accommodate that? > > Should FIFO aware of sample rate? Is that what we want? Well, I was just thinking it needs to be large enough to support 384 kHz. 32k size is just 40ms of buffering. Is that enough? Perhaps. We can always easily increase the limit later. Just want to be sure we have something fairly large. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: while (framesToRender != 0) { Since you decrement by render quantum each time, you know a priori how many times you'll loop. Thus, would this be better as a for loop? Gets rid of the logic in line 145.
olka@chromium.org changed reviewers: + olka@chromium.org
The code looks really polished now :) https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:147: : 0; Is it a contract that framesToRender % kRenderQuantumFrames == 0? (I hope that it's not, because that would cause an extra rebuffering later on) It is is not, then there will be not enough frames to pull at l.152. if framesToRender < AudioUtilities::kRenderQuantumFrames at this point. Unit tests should catch things like that, so we need some unit tests here. You can count pushedFrames (from 0) and make it while(pushedFrames<framesToRender). Or if using for() as rtoy@ suggests, make sure to choose an upper bound for the number of iterations. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:14: const unsigned kMaxMessagesToLog = 100; Put it into an unnamed namespace? https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:74: std::min(m_framesAvailable + inputBusLength, m_fifoLength); DCHECK that relations between m_indexRead, m_indexWrite and m_framesAvailable are correct (one can be calculated from another two) https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:130: m_framesAvailable -= framesToFill; Same here: DCHECK the relations https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: static const size_t kMaxFIFOLength = 65536; Shouldn't the constant should only be declared here, and defined in cpp? Otherwise every file including it has its personal copy. Also, blink namespace seems to be too broad for a global constant. Make it a static member of PushPullFIFO? https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:42: // - The |inputBus| length is 128 frames (1 RQ), fixed. Duplication of the previous line? https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:217: {898, 27, 0, 0, {{898, 898}, {27, 27}}, {{0, 449}, {448, 897}}}}, These values require comments, otherwise it;s really hard to understand what is verified. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:293: {{0, 383}, {256, 639}, {257, 0}, {382, 0}}}}}; Another corner case to consider: Multiple pulls from an empty FIFO. (Whith such a good testing framework we need to configure as may corner cases as we can.)
I think this CL is ready for the review from BUILD.gn owner. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; On 2017/02/01 23:38:55, Raymond Toy wrote: > On 2017/02/01 23:09:06, hongchan wrote: > > On 2017/02/01 19:02:37, Raymond Toy wrote: > > > On 2017/01/27 22:39:14, hongchan wrote: > > > > rtoy@ and I believe 32K is good enough based on the data we collected. > > > > > > I checked the metrics again. There is one case of 46080 in the last 28 > days; > > 7 > > > with 31744(!?!) > > > > > > Perhaps we fundamentally don't care about such huge latencies. But also > > > consider that we support a max sample rate of 384 kHz now, so the fifo may > > need > > > to be scaled up to accommodate that? > > > > Should FIFO aware of sample rate? Is that what we want? > > Well, I was just thinking it needs to be large enough to support 384 kHz. 32k > size is just 40ms of buffering. Is that enough? Perhaps. We can always easily > increase the limit later. Just want to be sure we have something fairly large. Acknowledged. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:123: while (framesToRender != 0) { On 2017/02/01 23:38:55, Raymond Toy wrote: > Since you decrement by render quantum each time, you know a priori how many > times you'll loop. Thus, would this be better as a for loop? Gets rid of the > logic in line 145. Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:147: : 0; On 2017/02/02 16:30:24, o1ka (CET) wrote: > Is it a contract that framesToRender % kRenderQuantumFrames == 0? (I hope that > it's not, because that would cause an extra rebuffering later on) > It is is not, then there will be not enough frames to pull at l.152. if > framesToRender < AudioUtilities::kRenderQuantumFrames at this point. > > Unit tests should catch things like that, so we need some unit tests here. > > > You can count pushedFrames (from 0) and make it > while(pushedFrames<framesToRender). > > Or if using for() as rtoy@ suggests, make sure to choose an upper bound for the > number of iterations. I took both advices and made it a for loop with counting up with the upper bound. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:14: const unsigned kMaxMessagesToLog = 100; On 2017/02/02 16:30:24, o1ka (CET) wrote: > Put it into an unnamed namespace? Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:74: std::min(m_framesAvailable + inputBusLength, m_fifoLength); On 2017/02/02 16:30:24, o1ka (CET) wrote: > DCHECK that relations between m_indexRead, m_indexWrite and m_framesAvailable > are correct (one can be calculated from another two) This doesn't seem to possible because of the heuristic for under/overflow. Purely based on two indexes, we cannot tell if the FIFO is empty or full when they are at the same position. That's why we use m_framesAvailable - to keep track of actual available frames. If you have a better idea please do suggest. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:130: m_framesAvailable -= framesToFill; On 2017/02/02 16:30:24, o1ka (CET) wrote: > Same here: DCHECK the relations Ditto. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:26: static const size_t kMaxFIFOLength = 65536; On 2017/02/02 16:30:25, o1ka (CET) wrote: > Shouldn't the constant should only be declared here, and defined in cpp? > Otherwise every file including it has its personal copy. > Also, blink namespace seems to be too broad for a global constant. > Make it a static member of PushPullFIFO? Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:42: // - The |inputBus| length is 128 frames (1 RQ), fixed. On 2017/02/02 16:30:24, o1ka (CET) wrote: > Duplication of the previous line? Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:217: {898, 27, 0, 0, {{898, 898}, {27, 27}}, {{0, 449}, {448, 897}}}}, On 2017/02/02 16:30:25, o1ka (CET) wrote: > These values require comments, otherwise it;s really hard to understand what is > verified. Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:293: {{0, 383}, {256, 639}, {257, 0}, {382, 0}}}}}; On 2017/02/02 16:30:25, o1ka (CET) wrote: > Another corner case to consider: Multiple pulls from an empty FIFO. (Whith such > a good testing framework we need to configure as may corner cases as we can.) Done.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/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.
FIFO logic lgtm. FIFO unit test coverage does not look sufficient to me though. Possible bugs there will affect all WebAudio playback and will look like "WebAudio plays with glitches", and we won't have any information on pull sizes (= user audio hardware configuration) or fifo state to debug, so the best we can do is to model as many potential situations as possible in unit tests. I understand that now for each test you need to manually calculate all the offsets and expected values, which makes unit test creation time-consuming and error-prone. I would suggest to automate audio bus verification procedure. You will need that anyways when you start with multi-threading tests (you'll need to verify the bus on each pull/push basically). After that adding 15-20 new test configurations covering various corner cases will be easy. Defer to rtoy@ to decide on the importance of unit tests coverage here. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:74: std::min(m_framesAvailable + inputBusLength, m_fifoLength); On 2017/02/02 19:55:23, hongchan wrote: > On 2017/02/02 16:30:24, o1ka (CET) wrote: > > DCHECK that relations between m_indexRead, m_indexWrite and m_framesAvailable > > are correct (one can be calculated from another two) > > This doesn't seem to possible because of the heuristic for under/overflow. > > Purely based on two indexes, we cannot tell if the FIFO is empty or full when > they are at the same position. That's why we use m_framesAvailable - to keep > track of actual available frames. > > If you have a better idea please do suggest. How about DCHECK_EQ((m_indexRead + m_framesAvailable) % m_fifoLength, m_indexWrite)? m_indexWrite is deductible, right? https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:130: m_framesAvailable -= framesToFill; On 2017/02/02 19:55:23, hongchan wrote: > On 2017/02/02 16:30:24, o1ka (CET) wrote: > > Same here: DCHECK the relations > > Ditto. Ditto :) https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:293: {{0, 383}, {256, 639}, {257, 0}, {382, 0}}}}}; On 2017/02/02 19:55:23, hongchan wrote: > On 2017/02/02 16:30:25, o1ka (CET) wrote: > > Another corner case to consider: Multiple pulls from an empty FIFO. (Whith > such > > a good testing framework we need to configure as may corner cases as we can.) > > Done. Right, but how about starting with Pull, without any pushes? https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.h:34: // Maximum FIFO length. (512 RQs) nit: Render Quantum (RQ is a bit confusing) https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:187: {{512, 1, {{"PUSH", 128}, {"PUSH", 128}, {"PULL", 256}}}, Is FIFO length always proportional to 128? If not, test cases for such cases are required. And maximum FIFO length needs tests as well. https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:190: {{512, 2, {{"PUSH", 128}, {"PUSH", 128}, {"PULL", 256}}}, Other corner cases: Pull [FIFO length] samples; FIFO length equal to 128. https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:210: {"PULL", 449}, There are no tests here where PULL is smaller than or equal to 128. It should either be tested, or the contract should be restricted with the corresponding checks. 128 is something we can meet in real life, so it should be tested for sure.
> I would suggest to automate audio bus verification procedure. You will need that > anyways when you start with multi-threading tests (you'll need to verify the bus > on each pull/push basically). I certainly agree with you, but I think it has to be dealt with another CL for multithread support. Other than that, I think I addressed corner cases you mentioned with this patchset and the previous one. Olga, thanks so much for your help on this! Unfortunately I have to nag you a bit more when I work on the multi-thread support. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:74: std::min(m_framesAvailable + inputBusLength, m_fifoLength); On 2017/02/03 10:41:17, o1ka (CET) wrote: > On 2017/02/02 19:55:23, hongchan wrote: > > On 2017/02/02 16:30:24, o1ka (CET) wrote: > > > DCHECK that relations between m_indexRead, m_indexWrite and > m_framesAvailable > > > are correct (one can be calculated from another two) > > > > This doesn't seem to possible because of the heuristic for under/overflow. > > > > Purely based on two indexes, we cannot tell if the FIFO is empty or full when > > they are at the same position. That's why we use m_framesAvailable - to keep > > track of actual available frames. > > > > If you have a better idea please do suggest. > > How about DCHECK_EQ((m_indexRead + m_framesAvailable) % m_fifoLength, > m_indexWrite)? > > m_indexWrite is deductible, right? > Well, I thought you wanted the verification purely based on m_indexRead and m_indexWrite. Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:130: m_framesAvailable -= framesToFill; On 2017/02/03 10:41:17, o1ka (CET) wrote: > On 2017/02/02 19:55:23, hongchan wrote: > > On 2017/02/02 16:30:24, o1ka (CET) wrote: > > > Same here: DCHECK the relations > > > > Ditto. > > Ditto :) Done. https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:293: {{0, 383}, {256, 639}, {257, 0}, {382, 0}}}}}; On 2017/02/03 10:41:17, o1ka (CET) wrote: > On 2017/02/02 19:55:23, hongchan wrote: > > On 2017/02/02 16:30:25, o1ka (CET) wrote: > > > Another corner case to consider: Multiple pulls from an empty FIFO. (Whith > > such > > > a good testing framework we need to configure as may corner cases as we > can.) > > > > Done. > > Right, but how about starting with Pull, without any pushes? I'll add another for this case. Done. https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:187: {{512, 1, {{"PUSH", 128}, {"PUSH", 128}, {"PULL", 256}}}, On 2017/02/03 10:41:17, o1ka (CET) wrote: > Is FIFO length always proportional to 128? If not, test cases for such cases are > required. > > And maximum FIFO length needs tests as well. Many test cases below are using unusual FIFO length. The maximum FIFO length is being test at l.25 above. https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:190: {{512, 2, {{"PUSH", 128}, {"PUSH", 128}, {"PULL", 256}}}, On 2017/02/03 10:41:17, o1ka (CET) wrote: > Other corner cases: Pull [FIFO length] samples; > FIFO length equal to 128. Done. https://codereview.chromium.org/2549093009/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:210: {"PULL", 449}, On 2017/02/03 10:41:17, o1ka (CET) wrote: > There are no tests here where PULL is smaller than or equal to 128. It should > either be tested, or the contract should be restricted with the corresponding > checks. > > 128 is something we can meet in real life, so it should be tested for sure. Done.
hongchan@chromium.org changed reviewers: + haraken@chromium.org
haraken@ PTAL: BUILD.gn This is the first step to bring V8 to WebAudio. The technical review on FIFO design is complete, so please take a look at BUILD.gn.
On 2017/02/03 17:27:22, hongchan wrote: > haraken@ PTAL: BUILD.gn > > This is the first step to bring V8 to WebAudio. The technical review on FIFO > design is complete, so please take a look at BUILD.gn. LGTM
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 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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #15 (id:320001) 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: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org, olka@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2549093009/#ps340001 (title: "Rebased and added CHECKs again 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": 340001, "attempt_start_ts": 1486509003310250,
"parent_rev": "9e27c35258cd2ee9b1a0a06d4cdc677ceb3ad16d", "commit_rev":
"41a5385ea755ea3550ce907716c329b559186a2e"}
Message was sent while issue was closed.
Description was changed from ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 Review-Url: https://codereview.chromium.org/2549093009 Cr-Commit-Position: refs/heads/master@{#448784} Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001) as https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
hongchan: This breaks official tests, see below. (cc rnk fyi) https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:39: "inputBus->length.* == .*kRenderQuantumFrames"); CHECK strings are discarded in official builds, which means this test is failing on bots doing official builds (e.g. https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... but there are many more). See bug linked from https://codereview.chromium.org/1053013009/ and the bug blocked on that bug for other examples. Can you fix, please?
Message was sent while issue was closed.
I see. Thanks for letting me know. I can revert the CL first then try to reland. Would that work? On Wed, Feb 8, 2017 at 11:59 AM <thakis@chromium.org> wrote: > hongchan: This breaks official tests, see below. > > (cc rnk fyi) > > > > https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp > (right): > > > https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:39: > "inputBus->length.* == .*kRenderQuantumFrames"); > CHECK strings are discarded in official builds, which means this test is > failing on bots doing official builds (e.g. > > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... > but there are many more). > > See bug linked from https://codereview.chromium.org/1053013009/ and the > bug blocked on that bug for other examples. > > Can you fix, please? > > https://codereview.chromium.org/2549093009/ > -- 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.
Message was sent while issue was closed.
I see. Thanks for letting me know. I can revert the CL first then try to reland. Would that work? On Wed, Feb 8, 2017 at 11:59 AM <thakis@chromium.org> wrote: > hongchan: This breaks official tests, see below. > > (cc rnk fyi) > > > > https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp > (right): > > > https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:39: > "inputBus->length.* == .*kRenderQuantumFrames"); > CHECK strings are discarded in official builds, which means this test is > failing on bots doing official builds (e.g. > > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... > but there are many more). > > See bug linked from https://codereview.chromium.org/1053013009/ and the > bug blocked on that bug for other examples. > > Can you fix, please? > > https://codereview.chromium.org/2549093009/ > -- 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.
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:340001) has been created in https://codereview.chromium.org/2679413003/ by hongchan@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... Some death tests for CHECK() are failing on the official build..
Message was sent while issue was closed.
Description was changed from ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 Review-Url: https://codereview.chromium.org/2549093009 Cr-Commit-Position: refs/heads/master@{#448784} Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3... ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 Review-Url: https://codereview.chromium.org/2549093009 Cr-Commit-Position: refs/heads/master@{#448784} Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3... ==========
On 2017/02/08 20:13:06, hongchan wrote: > A revert of this CL (patchset #15 id:340001) has been created in > https://codereview.chromium.org/2679413003/ by mailto:hongchan@chromium.org. > > The reason for reverting is: > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... > > Some death tests for CHECK() are failing on the official build.. olka@ haraken@ I am working to fix the official Chrome build bot failure. These CHECKs are stripped out of the official build therefore all the EXPECT_DEATH tests are failing. CHECK_LE(m_fifoLength <= kMaxFIFOLength) CHECK_EQ(inputBus->length() == kRenderQuantumFrames) SECURITY_CHECK(framesRequested <= outputBus->length()) SECURITY_CHECK(framesRequested <= m_fifoLength) It's a bit odd that we even strip out SECURITY_CHECKs out of the official build. I did not know that. Then what's the meaning of having them in the code? I also found 0 instance of EXPECT_DEATH in unit tests within the Blink code. Perhaps I should not use it at all? Perhaps Blink does not encourage to crash even in critical corner cases?
On 2017/02/09 17:18:44, hongchan wrote: > On 2017/02/08 20:13:06, hongchan wrote: > > A revert of this CL (patchset #15 id:340001) has been created in > > https://codereview.chromium.org/2679413003/ by mailto:hongchan@chromium.org. > > > > The reason for reverting is: > > > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... > > > > Some death tests for CHECK() are failing on the official build.. > > olka@ haraken@ > > I am working to fix the official Chrome build bot failure. These CHECKs are > stripped out > of the official build therefore all the EXPECT_DEATH tests are failing. > > CHECK_LE(m_fifoLength <= kMaxFIFOLength) > CHECK_EQ(inputBus->length() == kRenderQuantumFrames) > SECURITY_CHECK(framesRequested <= outputBus->length()) > SECURITY_CHECK(framesRequested <= m_fifoLength) > > It's a bit odd that we even strip out SECURITY_CHECKs out of the official build. > I did not > know that. Then what's the meaning of having them in the code? > > I also found 0 instance of EXPECT_DEATH in unit tests within the Blink code. > Perhaps I > should not use it at all? Perhaps Blink does not encourage to crash even in > critical corner > cases? Should I use CRASH() instead of SECURITY_CHECK() or CHECK()?
On 2017/02/09 17:23:04, hongchan wrote: > On 2017/02/09 17:18:44, hongchan wrote: > > On 2017/02/08 20:13:06, hongchan wrote: > > > A revert of this CL (patchset #15 id:340001) has been created in > > > https://codereview.chromium.org/2679413003/ by mailto:hongchan@chromium.org. > > > > > > The reason for reverting is: > > > > > > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds... > > > > > > Some death tests for CHECK() are failing on the official build.. > > > > olka@ haraken@ > > > > I am working to fix the official Chrome build bot failure. These CHECKs are > > stripped out > > of the official build therefore all the EXPECT_DEATH tests are failing. > > > > CHECK_LE(m_fifoLength <= kMaxFIFOLength) > > CHECK_EQ(inputBus->length() == kRenderQuantumFrames) > > SECURITY_CHECK(framesRequested <= outputBus->length()) > > SECURITY_CHECK(framesRequested <= m_fifoLength) > > > > It's a bit odd that we even strip out SECURITY_CHECKs out of the official > build. > > I did not > > know that. Then what's the meaning of having them in the code? > > > > I also found 0 instance of EXPECT_DEATH in unit tests within the Blink code. > > Perhaps I > > should not use it at all? Perhaps Blink does not encourage to crash even in > > critical corner > > cases? > > Should I use CRASH() instead of SECURITY_CHECK() or CHECK()? Sorry for the confusion. This CL shows how to deal with the official build: https://codereview.chromium.org/1053013009/diff2/1:20001/ppapi/shared_impl/me... I will fix this CL and try to reland soon.
That's one way, but if you click the linked but and its blocker and related bugs you'll see others. Another approach is to never check the death test since it's not that interesting, and yet another is to not have the death test at all since it's actually not testing all that much, and death tests tend to be slow and somewhat annoying. Up to you to choose which of these approaches is the best one here.
On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > That's one way, but if you click the linked but and its blocker and related bugs > you'll see others. Another approach is to never check the death test since it's > not that interesting, and yet another is to not have the death test at all since > it's actually not testing all that much, and death tests tend to be slow and > somewhat annoying. Up to you to choose which of these approaches is the best one > here. In this CL, two reviewers and I specifically wanted to crash the application when there is any issue of out-of-boundary index. So I think we have to keep these tests. rtoy@, olka@ WDYT?
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...
On 2017/02/09 20:48:30, hongchan wrote: > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > > That's one way, but if you click the linked but and its blocker and related > bugs > > you'll see others. Another approach is to never check the death test since > it's > > not that interesting, and yet another is to not have the death test at all > since > > it's actually not testing all that much, and death tests tend to be slow and > > somewhat annoying. Up to you to choose which of these approaches is the best > one > > here. > > In this CL, two reviewers and I specifically wanted to crash the application > when > there is any issue of out-of-boundary index. So I think we have to keep these > tests. > > rtoy@, olka@ WDYT? I think it's useful to verify that we do in fact crash on these conditions since you did put in the appropriate checks.
Do you care that the CHECK text is getting tested in the death test, or is just verifying a crash enough? On Thu, Feb 9, 2017 at 3:55 PM, rtoy@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2017/02/09 20:48:30, hongchan wrote: > > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > > > That's one way, but if you click the linked but and its blocker and > related > > bugs > > > you'll see others. Another approach is to never check the death test > since > > it's > > > not that interesting, and yet another is to not have the death test at > all > > since > > > it's actually not testing all that much, and death tests tend to be > slow and > > > somewhat annoying. Up to you to choose which of these approaches is > the best > > one > > > here. > > > > In this CL, two reviewers and I specifically wanted to crash the > application > > when > > there is any issue of out-of-boundary index. So I think we have to keep > these > > tests. > > > > rtoy@, olka@ WDYT? > > I think it's useful to verify that we do in fact crash on these conditions > since > you did put in the appropriate checks. > > https://codereview.chromium.org/2549093009/ > -- 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.
Do you care that the CHECK text is getting tested in the death test, or is just verifying a crash enough? On Thu, Feb 9, 2017 at 3:55 PM, rtoy@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2017/02/09 20:48:30, hongchan wrote: > > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > > > That's one way, but if you click the linked but and its blocker and > related > > bugs > > > you'll see others. Another approach is to never check the death test > since > > it's > > > not that interesting, and yet another is to not have the death test at > all > > since > > > it's actually not testing all that much, and death tests tend to be > slow and > > > somewhat annoying. Up to you to choose which of these approaches is > the best > > one > > > here. > > > > In this CL, two reviewers and I specifically wanted to crash the > application > > when > > there is any issue of out-of-boundary index. So I think we have to keep > these > > tests. > > > > rtoy@, olka@ WDYT? > > I think it's useful to verify that we do in fact crash on these conditions > since > you did put in the appropriate checks. > > https://codereview.chromium.org/2549093009/ > -- 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.
I personally don't care about the CHECK text as long as we add the right comment on each test. Then we can remove |#if #else| from the test. On Thu, Feb 9, 2017 at 12:58 PM Nico Weber <thakis@chromium.org> wrote: > Do you care that the CHECK text is getting tested in the death test, or is > just verifying a crash enough? > > On Thu, Feb 9, 2017 at 3:55 PM, rtoy@chromium.org via > codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> > wrote: > > On 2017/02/09 20:48:30, hongchan wrote: > > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > > > That's one way, but if you click the linked but and its blocker and > related > > bugs > > > you'll see others. Another approach is to never check the death test > since > > it's > > > not that interesting, and yet another is to not have the death test at > all > > since > > > it's actually not testing all that much, and death tests tend to be > slow and > > > somewhat annoying. Up to you to choose which of these approaches is > the best > > one > > > here. > > > > In this CL, two reviewers and I specifically wanted to crash the > application > > when > > there is any issue of out-of-boundary index. So I think we have to keep > these > > tests. > > > > rtoy@, olka@ WDYT? > > I think it's useful to verify that we do in fact crash on these conditions > since > you did put in the appropriate checks. > > https://codereview.chromium.org/2549093009/ > > > -- 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.
I personally don't care about the CHECK text as long as we add the right comment on each test. Then we can remove |#if #else| from the test. On Thu, Feb 9, 2017 at 12:58 PM Nico Weber <thakis@chromium.org> wrote: > Do you care that the CHECK text is getting tested in the death test, or is > just verifying a crash enough? > > On Thu, Feb 9, 2017 at 3:55 PM, rtoy@chromium.org via > codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> > wrote: > > On 2017/02/09 20:48:30, hongchan wrote: > > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > > > That's one way, but if you click the linked but and its blocker and > related > > bugs > > > you'll see others. Another approach is to never check the death test > since > > it's > > > not that interesting, and yet another is to not have the death test at > all > > since > > > it's actually not testing all that much, and death tests tend to be > slow and > > > somewhat annoying. Up to you to choose which of these approaches is > the best > > one > > > here. > > > > In this CL, two reviewers and I specifically wanted to crash the > application > > when > > there is any issue of out-of-boundary index. So I think we have to keep > these > > tests. > > > > rtoy@, olka@ WDYT? > > I think it's useful to verify that we do in fact crash on these conditions > since > you did put in the appropriate checks. > > https://codereview.chromium.org/2549093009/ > > > -- 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.
Patchset #16 (id:360001) 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: 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, olka@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2549093009/#ps380001 (title: "Death test comparison string dropped 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": 380001, "attempt_start_ts": 1486683503514620,
"parent_rev": "05324cb4575729c503766281107a8a5d75662bdc", "commit_rev":
"52db730e621a1cba97430be2a888feb9a962187a"}
Message was sent while issue was closed.
Description was changed from ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 Review-Url: https://codereview.chromium.org/2549093009 Cr-Commit-Position: refs/heads/master@{#448784} Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3... ========== to ========== This CL is to reduce the complexity of current FIFO structure. 1. The simplified PushPullFIFO replaces old FIFOs into one, and removes the cyclic call structure between AudioDestination and AudioPullFIFO. 2. This refactoring will be helpful when the new WebThread for AudioWorklet is implemented. (push and pull operation now can be separated.) BUG=678678 Review-Url: https://codereview.chromium.org/2549093009 Cr-Original-Commit-Position: refs/heads/master@{#448784} Committed: https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c3... Review-Url: https://codereview.chromium.org/2549093009 Cr-Commit-Position: refs/heads/master@{#449467} Committed: https://chromium.googlesource.com/chromium/src/+/52db730e621a1cba97430be2a888... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:380001) as https://chromium.googlesource.com/chromium/src/+/52db730e621a1cba97430be2a888... |
