Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(655)

Issue 2549093009: Introduce PushPullFIFO class and remove other FIFOs (Closed)

Created:
3 years, 9 months ago by hongchan
Modified:
3 years, 7 months ago
Reviewers:
haraken, Nico, o1ka, Raymond Toy
CC:
blink-reviews, chromium-reviews, Reid Kleckner
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+645 lines, -429 lines) Patch
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/audio/AudioDestination.h View 1 2 4 chunks +10 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/audio/AudioDestination.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +39 lines, -34 lines 0 comments Download
D third_party/WebKit/Source/platform/audio/AudioFIFO.h View 1 1 chunk +0 lines, -84 lines 0 comments Download
D third_party/WebKit/Source/platform/audio/AudioFIFO.cpp View 1 1 chunk +0 lines, -146 lines 0 comments Download
D third_party/WebKit/Source/platform/audio/AudioPullFIFO.h View 1 1 chunk +0 lines, -84 lines 0 comments Download
D third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp View 1 1 chunk +0 lines, -69 lines 0 comments Download
A third_party/WebKit/Source/platform/audio/PushPullFIFO.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +85 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +145 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +363 lines, -0 lines 0 comments Download

Messages

Total messages: 105 (43 generated)
hongchan
PTAL. olka@ PushPullFIFO here does not have fancy features like AudioShifter, but at least it ...
3 years, 8 months ago (2017-01-05 23:24:52 UTC) #5
Raymond Toy
https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode104 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:104: if (destinationData.size() != m_numberOfOutputChannels || Should there be a ...
3 years, 8 months ago (2017-01-06 22:22:03 UTC) #7
o1ka
Could you please clarify naming of variables? The CL is a bit tricky to read. ...
3 years, 8 months ago (2017-01-09 14:09:03 UTC) #8
hongchan
Thanks so much for the review. The new FIFO is largely based on AudioFIFO.h/cpp. However, ...
3 years, 8 months ago (2017-01-09 21:53:16 UTC) #9
Raymond Toy
https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/40001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode104 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:104: if (destinationData.size() != m_numberOfOutputChannels || On 2017/01/09 21:53:15, hongchan ...
3 years, 8 months ago (2017-01-09 22:46:02 UTC) #10
Raymond Toy
The subject says "deprecate other FIFOs". All other FIFOs are gone, right, so maybe say ...
3 years, 8 months ago (2017-01-09 22:47:44 UTC) #11
o1ka
Since FIFO interface very minimal, you can have unit tests right now. A single-threaded version ...
3 years, 8 months ago (2017-01-10 10:07:20 UTC) #12
Raymond Toy
https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp#newcode33 third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:33: return; On 2017/01/10 10:07:19, o1ka wrote: > Why these ...
3 years, 8 months ago (2017-01-10 16:18:15 UTC) #13
o1ka
https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Source/platform/audio/PushPullFIFO.h File third_party/WebKit/Source/platform/audio/PushPullFIFO.h (right): https://codereview.chromium.org/2549093009/diff/60001/third_party/WebKit/Source/platform/audio/PushPullFIFO.h#newcode33 third_party/WebKit/Source/platform/audio/PushPullFIFO.h:33: size_t framesInFIFO() const { return m_framesInFIFO; } On 2017/01/10 ...
3 years, 8 months ago (2017-01-10 16:41:39 UTC) #14
hongchan
Thanks very much for your review so far. I think now we have settled with ...
3 years, 8 months ago (2017-01-10 21:15:58 UTC) #15
hongchan
PTAL. In this patch set, I have done: - Converted size_t to int - Added ...
3 years, 8 months ago (2017-01-12 18:18:55 UTC) #16
Raymond Toy
I'm a bit concerned about the underflow not being an error. I think previously we ...
3 years, 8 months ago (2017-01-12 19:29:40 UTC) #17
o1ka
https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/100001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp#newcode28 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/Source/platform/audio/PushPullFIFO.cpp#newcode29 third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:29: int remainder ...
3 years, 8 months ago (2017-01-13 11:23:44 UTC) #18
hongchan
o1ka@ Thanks so much for your review! It has been tremendously helpful. Summary of PS6: ...
3 years, 8 months ago (2017-01-13 23:29:23 UTC) #19
hongchan
Friendly ping. I would like to add more unit tests. Please take a look at ...
3 years, 8 months ago (2017-01-23 17:02:31 UTC) #20
Raymond Toy
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp#newcode24 third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { What does "IncrementalIndex" mean here? ...
3 years, 8 months ago (2017-01-23 17:52:26 UTC) #21
hongchan
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp#newcode24 third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { On 2017/01/23 17:52:25, Raymond Toy ...
3 years, 8 months ago (2017-01-23 19:37:36 UTC) #22
Raymond Toy
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp#newcode24 third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:24: void fillBusWithIncrementalIndex(AudioBus* targetBus) { On 2017/01/23 19:37:36, hongchan wrote: ...
3 years, 8 months ago (2017-01-23 21:53:53 UTC) #23
hongchan
https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/120001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp#newcode124 third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp:124: } On 2017/01/23 21:53:53, Raymond Toy wrote: > On ...
3 years, 8 months ago (2017-01-23 22:28:36 UTC) #24
o1ka
https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/140001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode120 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 ...
3 years, 8 months ago (2017-01-24 11:09:02 UTC) #25
hongchan
Sorry about massive (+ messy) changes since the previous patchset. Here is the summary: - ...
3 years, 7 months ago (2017-01-26 22:33:18 UTC) #26
o1ka
https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode105 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:105: return; As I said before, I feel strongly against ...
3 years, 7 months ago (2017-01-27 13:47:54 UTC) #27
Raymond Toy
https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/160001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode124 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: : 0; Is this the right thing to do? ...
3 years, 7 months ago (2017-01-27 17:39:44 UTC) #28
hongchan
I added some comments on the contract for FIFO: (RQ = WebAudio Render Quantum, 128 ...
3 years, 7 months ago (2017-01-27 22:39:15 UTC) #30
Raymond Toy
On 2017/01/27 22:39:15, hongchan wrote: > I added some comments on the contract for FIFO: ...
3 years, 7 months ago (2017-01-27 23:10:13 UTC) #31
o1ka
Unfortunately I'm under a heavy load now and don't have free cycles to iterate through ...
3 years, 7 months ago (2017-01-30 11:41:36 UTC) #32
blink-reviews
o1ka@ Your help on this CL has been valuable. Thank you so much for that ...
3 years, 7 months ago (2017-01-30 16:51:34 UTC) #33
chromium-reviews
o1ka@ Your help on this CL has been valuable. Thank you so much for that ...
3 years, 7 months ago (2017-01-30 16:51:35 UTC) #34
o1ka
> > One thing: I wanted to introduce the multithreading support in the > follow-up ...
3 years, 7 months ago (2017-01-30 17:14:29 UTC) #35
blink-reviews
Yes, I am expecting that, but also it's important not to break the current behavior. ...
3 years, 7 months ago (2017-01-30 17:21:24 UTC) #36
chromium-reviews
Yes, I am expecting that, but also it's important not to break the current behavior. ...
3 years, 7 months ago (2017-01-30 17:21:25 UTC) #37
hongchan
On 2017/01/30 17:21:25, chromium-reviews wrote: > Yes, I am expecting that, but also it's important ...
3 years, 7 months ago (2017-01-30 18:22:42 UTC) #38
hongchan
> > - The push size is 128 frames (1 RQ), fixed. > > - ...
3 years, 7 months ago (2017-01-30 18:33:05 UTC) #39
Raymond Toy
https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode118 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:118: // audio device. Please clarify this and explain what ...
3 years, 7 months ago (2017-01-30 20:59:29 UTC) #40
hongchan
- Added a unit test for the contract. (death test) - Refined/moved comments. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File ...
3 years, 7 months ago (2017-02-01 18:07:28 UTC) #42
Raymond Toy
Almost there. Just a few small questions. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp#newcode13 third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t ...
3 years, 7 months ago (2017-02-01 19:02:38 UTC) #43
hongchan
https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (right): https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp#newcode13 third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:13: const size_t kMaxFIFOLength = 32768; On 2017/02/01 19:02:37, Raymond ...
3 years, 7 months ago (2017-02-01 23:09:06 UTC) #44
Raymond Toy
lgtm. Your discretion on updating the max size and whether to use a for loop. ...
3 years, 7 months ago (2017-02-01 23:38:55 UTC) #45
o1ka
The code looks really polished now :) https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2549093009/diff/260001/third_party/WebKit/Source/platform/audio/AudioDestination.cpp#newcode147 third_party/WebKit/Source/platform/audio/AudioDestination.cpp:147: : 0; ...
3 years, 7 months ago (2017-02-02 16:30:25 UTC) #47
hongchan
I think this CL is ready for the review from BUILD.gn owner. https://codereview.chromium.org/2549093009/diff/200001/third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp ...
3 years, 7 months ago (2017-02-02 19:55:23 UTC) #48
o1ka
FIFO logic lgtm. FIFO unit test coverage does not look sufficient to me though. Possible ...
3 years, 7 months ago (2017-02-03 10:41:17 UTC) #53
hongchan
> I would suggest to automate audio bus verification procedure. You will need that > ...
3 years, 7 months ago (2017-02-03 17:20:12 UTC) #54
hongchan
haraken@ PTAL: BUILD.gn This is the first step to bring V8 to WebAudio. The technical ...
3 years, 7 months ago (2017-02-03 17:27:22 UTC) #56
haraken
On 2017/02/03 17:27:22, hongchan wrote: > haraken@ PTAL: BUILD.gn > > This is the first ...
3 years, 7 months ago (2017-02-03 18:34:37 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2549093009/340001
3 years, 7 months ago (2017-02-07 23:11:23 UTC) #73
commit-bot: I haz the power
Committed patchset #15 (id:340001) as https://chromium.googlesource.com/chromium/src/+/41a5385ea755ea3550ce907716c329b559186a2e
3 years, 7 months ago (2017-02-07 23:29:01 UTC) #76
Nico
hongchan: This breaks official tests, see below. (cc rnk fyi) https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp File third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp (right): https://codereview.chromium.org/2549093009/diff/340001/third_party/WebKit/Source/platform/audio/PushPullFIFOTest.cpp#newcode39 ...
3 years, 7 months ago (2017-02-08 19:59:28 UTC) #78
blink-reviews
I see. Thanks for letting me know. I can revert the CL first then try ...
3 years, 7 months ago (2017-02-08 20:10:44 UTC) #79
chromium-reviews
I see. Thanks for letting me know. I can revert the CL first then try ...
3 years, 7 months ago (2017-02-08 20:10:45 UTC) #80
hongchan
A revert of this CL (patchset #15 id:340001) has been created in https://codereview.chromium.org/2679413003/ by hongchan@chromium.org. ...
3 years, 7 months ago (2017-02-08 20:13:06 UTC) #81
hongchan
On 2017/02/08 20:13:06, hongchan wrote: > A revert of this CL (patchset #15 id:340001) has ...
3 years, 7 months ago (2017-02-09 17:18:44 UTC) #83
hongchan
On 2017/02/09 17:18:44, hongchan wrote: > On 2017/02/08 20:13:06, hongchan wrote: > > A revert ...
3 years, 7 months ago (2017-02-09 17:23:04 UTC) #84
hongchan
On 2017/02/09 17:23:04, hongchan wrote: > On 2017/02/09 17:18:44, hongchan wrote: > > On 2017/02/08 ...
3 years, 7 months ago (2017-02-09 20:37:07 UTC) #85
Nico
That's one way, but if you click the linked but and its blocker and related ...
3 years, 7 months ago (2017-02-09 20:41:27 UTC) #86
hongchan
On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: > That's one way, but if ...
3 years, 7 months ago (2017-02-09 20:48:30 UTC) #87
Raymond Toy
On 2017/02/09 20:48:30, hongchan wrote: > On 2017/02/09 20:41:27, Nico (ooo Thu Feb 9) wrote: ...
3 years, 7 months ago (2017-02-09 20:55:14 UTC) #90
Nico
Do you care that the CHECK text is getting tested in the death test, or ...
3 years, 7 months ago (2017-02-09 20:58:43 UTC) #91
Nico
Do you care that the CHECK text is getting tested in the death test, or ...
3 years, 7 months ago (2017-02-09 20:58:44 UTC) #92
blink-reviews
I personally don't care about the CHECK text as long as we add the right ...
3 years, 7 months ago (2017-02-09 21:02:04 UTC) #93
chromium-reviews
I personally don't care about the CHECK text as long as we add the right ...
3 years, 7 months ago (2017-02-09 21:02:05 UTC) #94
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2549093009/380001
3 years, 7 months ago (2017-02-09 23:39:03 UTC) #102
commit-bot: I haz the power
3 years, 7 months ago (2017-02-09 23:48:16 UTC) #105
Message was sent while issue was closed.
Committed patchset #16 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/52db730e621a1cba97430be2a888...

Powered by Google App Engine
This is Rietveld 408576698