Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2590823007:
Clean up and refactor platform/AudioDestination (Closed)
|
Created:
4 years ago by hongchan Modified:
3 years, 11 months ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up and refactor platform/AudioDestination
- Code clean up, refactoring and reordering.
- Add/update the comment. Some of them are irrelevant or out-of-date.
BUG=676386
Committed: https://crrev.com/c44e4f0a2120f24a6701298868befa1b2adc1ce8
Cr-Commit-Position: refs/heads/master@{#441270}
Patch Set 1 : Refactor, clean up and more comments #Patch Set 2 : Remove redundant initializers #
Total comments: 28
Patch Set 3 : Addressing feedback 1 #
Total comments: 3
Patch Set 4 : Addressing feedback 2 #
Messages
Total messages: 36 (18 generated)
Description was changed from ========== Clean up and refactor platform/AudioDestination - Code clean up and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 ========== to ========== Clean up and refactor platform/AudioDestination - Code clean up, refactoring and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 ==========
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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.
PTAL.
Please don't reorder things unless they need to be. It makes reviewing much harder. Also, can we wait on this? This is going to totally screw the larger latency hint CL. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO Size; determined arbitrarily. That's not really true (arbitrarily). It was an estimate of the largest callback buffer size that we would ever need. And the FIFO size must be greater than this. The current UMA stats indicates that this is, in fact, probably too small. There are Android devices out there with a size of 8000 or so. We might need to make this larger. In another CL. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if maxChannelCount() equals 0, then numberOfOutputChannels must be 2. Where did this comment go? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t m_callbackBufferSize; Why move these things around? The order isn't important is it? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer and the Blink's WebAudio module. It has a FIFO to adapt the s/and the/and/ https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different frequency of WebAudio renderer and the actual hardware audio "frequency" may not be the right word here. Maybe "processing block sizes"? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual void start(); Why move these around? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool isPlaying() { return m_isPlaying; } Why are these no longer static? And why change the order?
This is a clean up, refactoring, reordering CL as I mentioned. The ordering does not matter, then we should go for the better readability. That is also why I tried to contain all the changes in two files. I can certainly wait on the other work, but it will delay the introduction of the new FIFO, which is quite critical for the AudioWorklet to work. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO Size; determined arbitrarily. On 2016/12/22 16:59:59, Raymond Toy wrote: > That's not really true (arbitrarily). It was an estimate of the largest > callback buffer size that we would ever need. And the FIFO size must be greater > than this. > > The current UMA stats indicates that this is, in fact, probably too small. > There are Android devices out there with a size of 8000 or so. We might need to > make this larger. In another CL. Acknowledged. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if maxChannelCount() equals 0, then numberOfOutputChannels must be 2. On 2016/12/22 17:00:00, Raymond Toy wrote: > Where did this comment go? First, not sure what these comments mean here. None of logics explained here is not in this class at all. I believe this is happening in the media renderer side, and the comment is wrongly placed. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t m_callbackBufferSize; On 2016/12/22 17:00:00, Raymond Toy wrote: > Why move these things around? The order isn't important is it? The right/meaningful order is certainly important for the readability. I separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and OutputPosition. Like I said, this will be a messy CL but I think it is important to have the good readability just like other parts of Blink. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer and the Blink's WebAudio module. It has a FIFO to adapt the On 2016/12/22 16:59:59, Raymond Toy wrote: > s/and the/and/ Acknowledged. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different frequency of WebAudio renderer and the actual hardware audio On 2016/12/22 16:59:59, Raymond Toy wrote: > "frequency" may not be the right word here. Maybe "processing block sizes"? Acknowledged. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual void start(); On 2016/12/22 17:00:00, Raymond Toy wrote: > Why move these around? render() and provideInput() are the core methods of this class. Then start/stop(). Getters are at the end. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool isPlaying() { return m_isPlaying; } On 2016/12/22 16:59:59, Raymond Toy wrote: > Why are these no longer static? And why change the order? They have never been static. These are regular member functions.
https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool isPlaying() { return m_isPlaying; } On 2016/12/22 17:19:49, hongchan wrote: > On 2016/12/22 16:59:59, Raymond Toy wrote: > > Why are these no longer static? And why change the order? > > They have never been static. These are regular member functions. Sorry. I misread this and the following functions.
On 2016/12/22 17:19:50, hongchan wrote: > This is a clean up, refactoring, reordering CL as I mentioned. The ordering does > not matter, then we should go for the better readability. That is also why I > tried to contain all the changes in two files. > > I can certainly wait on the other work, but it will delay the introduction of > the new FIFO, which is quite critical for the AudioWorklet to work. Can we do this in two separate CLs then? One to remove the input stuff, one to reorder. That's certainly much easier to review and easier to determine correctness. Or maybe as two separate patches. (Well, too late for this one.) But I agree; if the CL is ready to go, it's ready; don't wait for other CLs that aren't ready. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO Size; > determined arbitrarily. > On 2016/12/22 16:59:59, Raymond Toy wrote: > > That's not really true (arbitrarily). It was an estimate of the largest > > callback buffer size that we would ever need. And the FIFO size must be > greater > > than this. > > > > The current UMA stats indicates that this is, in fact, probably too small. > > There are Android devices out there with a size of 8000 or so. We might need > to > > make this larger. In another CL. > > Acknowledged. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if > maxChannelCount() equals 0, then numberOfOutputChannels must be 2. > On 2016/12/22 17:00:00, Raymond Toy wrote: > > Where did this comment go? > > First, not sure what these comments mean here. None of logics explained here is > not in this class at all. I believe this is happening in the media renderer > side, and the comment is wrongly placed. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t > m_callbackBufferSize; > On 2016/12/22 17:00:00, Raymond Toy wrote: > > Why move these things around? The order isn't important is it? > > The right/meaningful order is certainly important for the readability. I > separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and > OutputPosition. > > Like I said, this will be a messy CL but I think it is important to have the > good readability just like other parts of Blink. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer and > the Blink's WebAudio module. It has a FIFO to adapt the > On 2016/12/22 16:59:59, Raymond Toy wrote: > > s/and the/and/ > > Acknowledged. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different > frequency of WebAudio renderer and the actual hardware audio > On 2016/12/22 16:59:59, Raymond Toy wrote: > > "frequency" may not be the right word here. Maybe "processing block sizes"? > > Acknowledged. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual void > start(); > On 2016/12/22 17:00:00, Raymond Toy wrote: > > Why move these around? > > render() and provideInput() are the core methods of this class. Then > start/stop(). Getters are at the end. > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool isPlaying() > { return m_isPlaying; } > On 2016/12/22 16:59:59, Raymond Toy wrote: > > Why are these no longer static? And why change the order? > > They have never been static. These are regular member functions.
On 2016/12/22 17:32:20, Raymond Toy wrote: > On 2016/12/22 17:19:50, hongchan wrote: > > This is a clean up, refactoring, reordering CL as I mentioned. The ordering > does > > not matter, then we should go for the better readability. That is also why I > > tried to contain all the changes in two files. > > > > I can certainly wait on the other work, but it will delay the introduction of > > the new FIFO, which is quite critical for the AudioWorklet to work. > > Can we do this in two separate CLs then? One to remove the input stuff, one to > reorder. That's certainly much easier to review and easier to determine > correctness. Or maybe as two separate patches. (Well, too late for this one.) > > But I agree; if the CL is ready to go, it's ready; don't wait for other CLs that > aren't ready. > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO > Size; > > determined arbitrarily. > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > That's not really true (arbitrarily). It was an estimate of the largest > > > callback buffer size that we would ever need. And the FIFO size must be > > greater > > > than this. > > > > > > The current UMA stats indicates that this is, in fact, probably too small. > > > There are Android devices out there with a size of 8000 or so. We might > need > > to > > > make this larger. In another CL. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if > > maxChannelCount() equals 0, then numberOfOutputChannels must be 2. > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > Where did this comment go? > > > > First, not sure what these comments mean here. None of logics explained here > is > > not in this class at all. I believe this is happening in the media renderer > > side, and the comment is wrongly placed. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t > > m_callbackBufferSize; > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > Why move these things around? The order isn't important is it? > > > > The right/meaningful order is certainly important for the readability. I > > separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and > > OutputPosition. > > > > Like I said, this will be a messy CL but I think it is important to have the > > good readability just like other parts of Blink. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer > and > > the Blink's WebAudio module. It has a FIFO to adapt the > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > s/and the/and/ > > > > Acknowledged. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different > > frequency of WebAudio renderer and the actual hardware audio > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > "frequency" may not be the right word here. Maybe "processing block sizes"? > > > > Acknowledged. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual void > > start(); > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > Why move these around? > > > > render() and provideInput() are the core methods of this class. Then > > start/stop(). Getters are at the end. > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool > isPlaying() > > { return m_isPlaying; } > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > Why are these no longer static? And why change the order? > > > > They have never been static. These are regular member functions. Just to be sure, the input component was removed in the previous CL: https://codereview.chromium.org/2590983003/ This is just a clean up CL.
On 2016/12/22 17:37:56, hongchan wrote: > On 2016/12/22 17:32:20, Raymond Toy wrote: > > On 2016/12/22 17:19:50, hongchan wrote: > > > This is a clean up, refactoring, reordering CL as I mentioned. The ordering > > does > > > not matter, then we should go for the better readability. That is also why I > > > tried to contain all the changes in two files. > > > > > > I can certainly wait on the other work, but it will delay the introduction > of > > > the new FIFO, which is quite critical for the AudioWorklet to work. > > > > Can we do this in two separate CLs then? One to remove the input stuff, one > to > > reorder. That's certainly much easier to review and easier to determine > > correctness. Or maybe as two separate patches. (Well, too late for this one.) > > > > But I agree; if the CL is ready to go, it's ready; don't wait for other CLs > that > > aren't ready. > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO > > Size; > > > determined arbitrarily. > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > That's not really true (arbitrarily). It was an estimate of the largest > > > > callback buffer size that we would ever need. And the FIFO size must be > > > greater > > > > than this. > > > > > > > > The current UMA stats indicates that this is, in fact, probably too small. > > > > > There are Android devices out there with a size of 8000 or so. We might > > need > > > to > > > > make this larger. In another CL. > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if > > > maxChannelCount() equals 0, then numberOfOutputChannels must be 2. > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > Where did this comment go? > > > > > > First, not sure what these comments mean here. None of logics explained here > > is > > > not in this class at all. I believe this is happening in the media renderer > > > side, and the comment is wrongly placed. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t > > > m_callbackBufferSize; > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > Why move these things around? The order isn't important is it? > > > > > > The right/meaningful order is certainly important for the readability. I > > > separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and > > > OutputPosition. > > > > > > Like I said, this will be a messy CL but I think it is important to have the > > > good readability just like other parts of Blink. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer > > and > > > the Blink's WebAudio module. It has a FIFO to adapt the > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > s/and the/and/ > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different > > > frequency of WebAudio renderer and the actual hardware audio > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > "frequency" may not be the right word here. Maybe "processing block > sizes"? > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual void > > > start(); > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > Why move these around? > > > > > > render() and provideInput() are the core methods of this class. Then > > > start/stop(). Getters are at the end. > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool > > isPlaying() > > > { return m_isPlaying; } > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > Why are these no longer static? And why change the order? > > > > > > They have never been static. These are regular member functions. > > Just to be sure, the input component was removed in the previous CL: > https://codereview.chromium.org/2590983003/ > > This is just a clean up CL. But you did remove the m_inputId stuff here. And when you rename things (m_renderBus -> m_outputBus?) AND move them around, it makes it much harder to know what happened.
On 2016/12/22 18:05:14, Raymond Toy wrote: > On 2016/12/22 17:37:56, hongchan wrote: > > On 2016/12/22 17:32:20, Raymond Toy wrote: > > > On 2016/12/22 17:19:50, hongchan wrote: > > > > This is a clean up, refactoring, reordering CL as I mentioned. The > ordering > > > does > > > > not matter, then we should go for the better readability. That is also why > I > > > > tried to contain all the changes in two files. > > > > > > > > I can certainly wait on the other work, but it will delay the introduction > > of > > > > the new FIFO, which is quite critical for the AudioWorklet to work. > > > > > > Can we do this in two separate CLs then? One to remove the input stuff, one > > to > > > reorder. That's certainly much easier to review and easier to determine > > > correctness. Or maybe as two separate patches. (Well, too late for this > one.) > > > > > > But I agree; if the CL is ready to go, it's ready; don't wait for other CLs > > that > > > aren't ready. > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO > > > Size; > > > > determined arbitrarily. > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > That's not really true (arbitrarily). It was an estimate of the largest > > > > > callback buffer size that we would ever need. And the FIFO size must be > > > > greater > > > > > than this. > > > > > > > > > > The current UMA stats indicates that this is, in fact, probably too > small. > > > > > > > There are Android devices out there with a size of 8000 or so. We might > > > need > > > > to > > > > > make this larger. In another CL. > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or if > > > > maxChannelCount() equals 0, then numberOfOutputChannels must be 2. > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > Where did this comment go? > > > > > > > > First, not sure what these comments mean here. None of logics explained > here > > > is > > > > not in this class at all. I believe this is happening in the media > renderer > > > > side, and the comment is wrongly placed. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t > > > > m_callbackBufferSize; > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > Why move these things around? The order isn't important is it? > > > > > > > > The right/meaningful order is certainly important for the readability. I > > > > separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and > > > > OutputPosition. > > > > > > > > Like I said, this will be a messy CL but I think it is important to have > the > > > > good readability just like other parts of Blink. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // > renderer > > > and > > > > the Blink's WebAudio module. It has a FIFO to adapt the > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > s/and the/and/ > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // > different > > > > frequency of WebAudio renderer and the actual hardware audio > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > "frequency" may not be the right word here. Maybe "processing block > > sizes"? > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual > void > > > > start(); > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > Why move these around? > > > > > > > > render() and provideInput() are the core methods of this class. Then > > > > start/stop(). Getters are at the end. > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool > > > isPlaying() > > > > { return m_isPlaying; } > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > Why are these no longer static? And why change the order? > > > > > > > > They have never been static. These are regular member functions. > > > > Just to be sure, the input component was removed in the previous CL: > > https://codereview.chromium.org/2590983003/ > > > > This is just a clean up CL. > But you did remove the m_inputId stuff here. Yes, this input ID string is actually nothing - and this is different from removing the empty input array. If you want that change to be separated I can do that, but I think it is too trivial. > And when you rename things (m_renderBus -> m_outputBus?) AND move them around, > it makes it much harder to know what happened. Sorry for the inconvenience - so you want 'renaming' and 'reodering' to be two different CLs?
On 2016/12/22 18:09:48, hongchan wrote: > On 2016/12/22 18:05:14, Raymond Toy wrote: > > On 2016/12/22 17:37:56, hongchan wrote: > > > On 2016/12/22 17:32:20, Raymond Toy wrote: > > > > On 2016/12/22 17:19:50, hongchan wrote: > > > > > This is a clean up, refactoring, reordering CL as I mentioned. The > > ordering > > > > does > > > > > not matter, then we should go for the better readability. That is also > why > > I > > > > > tried to contain all the changes in two files. > > > > > > > > > > I can certainly wait on the other work, but it will delay the > introduction > > > of > > > > > the new FIFO, which is quite critical for the AudioWorklet to work. > > > > > > > > Can we do this in two separate CLs then? One to remove the input stuff, > one > > > to > > > > reorder. That's certainly much easier to review and easier to determine > > > > correctness. Or maybe as two separate patches. (Well, too late for this > > one.) > > > > > > > > But I agree; if the CL is ready to go, it's ready; don't wait for other > CLs > > > that > > > > aren't ready. > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // > FIFO > > > > Size; > > > > > determined arbitrarily. > > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > > That's not really true (arbitrarily). It was an estimate of the > largest > > > > > > callback buffer size that we would ever need. And the FIFO size must > be > > > > > greater > > > > > > than this. > > > > > > > > > > > > The current UMA stats indicates that this is, in fact, probably too > > small. > > > > > > > > > There are Android devices out there with a size of 8000 or so. We > might > > > > need > > > > > to > > > > > > make this larger. In another CL. > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:100: // or > if > > > > > maxChannelCount() equals 0, then numberOfOutputChannels must be 2. > > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > > Where did this comment go? > > > > > > > > > > First, not sure what these comments mean here. None of logics explained > > here > > > > is > > > > > not in this class at all. I believe this is happening in the media > > renderer > > > > > side, and the comment is wrongly placed. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:110: size_t > > > > > m_callbackBufferSize; > > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > > Why move these things around? The order isn't important is it? > > > > > > > > > > The right/meaningful order is certainly important for the readability. I > > > > > separated them into 3 blocks: WebAudioDevice, WebAudio rendering, and > > > > > OutputPosition. > > > > > > > > > > Like I said, this will be a messy CL but I think it is important to have > > the > > > > > good readability just like other parts of Blink. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/audio/AudioDestination.h > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // > > renderer > > > > and > > > > > the Blink's WebAudio module. It has a FIFO to adapt the > > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > > s/and the/and/ > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // > > different > > > > > frequency of WebAudio renderer and the actual hardware audio > > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > > "frequency" may not be the right word here. Maybe "processing block > > > sizes"? > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:80: virtual > > void > > > > > start(); > > > > > On 2016/12/22 17:00:00, Raymond Toy wrote: > > > > > > Why move these around? > > > > > > > > > > render() and provideInput() are the core methods of this class. Then > > > > > start/stop(). Getters are at the end. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/audio/AudioDestination.h:85: bool > > > > isPlaying() > > > > > { return m_isPlaying; } > > > > > On 2016/12/22 16:59:59, Raymond Toy wrote: > > > > > > Why are these no longer static? And why change the order? > > > > > > > > > > They have never been static. These are regular member functions. > > > > > > Just to be sure, the input component was removed in the previous CL: > > > https://codereview.chromium.org/2590983003/ > > > > > > This is just a clean up CL. > > > > But you did remove the m_inputId stuff here. > > Yes, this input ID string is actually nothing - and this is different from > removing the empty input array. > If you want that change to be separated I can do that, but I think it is too > trivial. > > > And when you rename things (m_renderBus -> m_outputBus?) AND move them around, > > it makes it much harder to know what happened. > > Sorry for the inconvenience - so you want 'renaming' and 'reodering' to be two > different CLs? I'll suck it up and review as is. It will just take longer because I have to track down things that look like they changed but didn't because they were moved. And other stuff like that.
https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (left): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:73: m_outputPosition() { What happened to this initializer? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:79: sampleRate, this, inputDeviceId, std::move(securityOrigin))); Why define these variables that are only used once? Why not just pass them in directly to the function? https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:87: NOTREACHED(); Is this really necessary? You already have a DCHECK that the size is valid. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: static float hardwareSampleRate(); This is a new public API. Who needs it?
https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (left): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:73: m_outputPosition() { On 2016/12/22 18:38:39, Raymond Toy wrote: > What happened to this initializer? This is a non-pointer member variable. So we don't need to use the initializer - it will be implicitly initialized at the construction. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:42: // FIFO Size; determined arbitrarily. On 2016/12/22 17:19:49, hongchan wrote: > On 2016/12/22 16:59:59, Raymond Toy wrote: > > That's not really true (arbitrarily). It was an estimate of the largest > > callback buffer size that we would ever need. And the FIFO size must be > greater > > than this. > > > > The current UMA stats indicates that this is, in fact, probably too small. > > There are Android devices out there with a size of 8000 or so. We might need > to > > make this larger. In another CL. > > Acknowledged. Added comments as well. Done. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:79: sampleRate, this, inputDeviceId, std::move(securityOrigin))); On 2016/12/22 18:38:39, Raymond Toy wrote: > Why define these variables that are only used once? Why not just pass them in > directly to the function? Sure. I'll do that. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:87: NOTREACHED(); On 2016/12/22 18:38:39, Raymond Toy wrote: > Is this really necessary? You already have a DCHECK that the size is valid. I can remove it, however I am not sure what we should do if the buffer size calculation fails. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:48: // renderer and the Blink's WebAudio module. It has a FIFO to adapt the On 2016/12/22 16:59:59, Raymond Toy wrote: > s/and the/and/ Done. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:49: // different frequency of WebAudio renderer and the actual hardware audio On 2016/12/22 17:19:49, hongchan wrote: > On 2016/12/22 16:59:59, Raymond Toy wrote: > > "frequency" may not be the right word here. Maybe "processing block sizes"? > > Acknowledged. Done. https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: static float hardwareSampleRate(); On 2016/12/22 18:38:39, Raymond Toy wrote: > This is a new public API. Who needs it? This class does. Here's what I am thinking: - AudioDestination represents the audio hardware within Blink-WebAudio. - It proxies the hardware information such as buffer size, sample rate and max channel count; so we don't have to call Platform::current()-> to query the hardware information. - We did not expose the sample rate, so I added it as a static getter.
https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: static float hardwareSampleRate(); On 2016/12/22 18:59:46, hongchan wrote: > On 2016/12/22 18:38:39, Raymond Toy wrote: > > This is a new public API. Who needs it? > > This class does. Here's what I am thinking: Yes, but these could be private. > > - AudioDestination represents the audio hardware within Blink-WebAudio. > - It proxies the hardware information such as buffer size, sample rate and max > channel count; so we don't have to call Platform::current()-> to query the > hardware information. > - We did not expose the sample rate, so I added it as a static getter. Unless there's already an external user for these, I'd prefer to make these private methods. When a user needs them we can make them public then. Is there a CL coming very soon that needs these? https://codereview.chromium.org/2590823007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:44: // TODO(hongchan): This was estimated based on the largest callback buffer size crbug.com/670747 https://codereview.chromium.org/2590823007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:220: return true; Instead of lines 217-220, maybe just do return m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames <= kFIFOSize Update comment on line 214 appropriately.
https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2590823007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: static float hardwareSampleRate(); On 2016/12/22 21:46:06, Raymond Toy wrote: > On 2016/12/22 18:59:46, hongchan wrote: > > On 2016/12/22 18:38:39, Raymond Toy wrote: > > > This is a new public API. Who needs it? > > > > This class does. Here's what I am thinking: > > Yes, but these could be private. > > > > - AudioDestination represents the audio hardware within Blink-WebAudio. > > - It proxies the hardware information such as buffer size, sample rate and max > > channel count; so we don't have to call Platform::current()-> to query the > > hardware information. > > - We did not expose the sample rate, so I added it as a static getter. > > Unless there's already an external user for these, I'd prefer to make these > private methods. When a user needs them we can make them public then. Is there > a CL coming very soon that needs these? I'll make hardwareBufferSize private, but leave hardwareSampleRate as it is. https://codereview.chromium.org/2590823007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2590823007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:220: return true; On 2016/12/22 21:46:06, Raymond Toy wrote: > Instead of lines 217-220, maybe just do > > return m_callbackBufferSize + AudioUtilities::kRenderQuantumFrames <= kFIFOSize > > Update comment on line 214 appropriately. Done.
lgtm
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
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": 60001, "attempt_start_ts": 1483490684079150,
"parent_rev": "16cdf43dc0b01e5d2b5edc866bbc09a9f3564074", "commit_rev":
"8130191b73f8e7a207840d3d7bbec9243b9d4825"}
Message was sent while issue was closed.
Description was changed from ========== Clean up and refactor platform/AudioDestination - Code clean up, refactoring and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 ========== to ========== Clean up and refactor platform/AudioDestination - Code clean up, refactoring and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 Review-Url: https://codereview.chromium.org/2590823007 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clean up and refactor platform/AudioDestination - Code clean up, refactoring and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 Review-Url: https://codereview.chromium.org/2590823007 ========== to ========== Clean up and refactor platform/AudioDestination - Code clean up, refactoring and reordering. - Add/update the comment. Some of them are irrelevant or out-of-date. BUG=676386 Committed: https://crrev.com/c44e4f0a2120f24a6701298868befa1b2adc1ce8 Cr-Commit-Position: refs/heads/master@{#441270} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c44e4f0a2120f24a6701298868befa1b2adc1ce8 Cr-Commit-Position: refs/heads/master@{#441270} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
