Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
389623002:
Add a block based Audio FIFO. (Closed)
|
Created:
6 years, 5 months ago by no longer working on chromium Modified:
6 years, 5 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd a block based Audio FIFO.
This new AudioBlockFifo() is made for the input code to avoid copying audio data during Push() and Consume().
Contrast to the existing AudioFifo, which requires a AudioBus* as input param for its Push() and Consume() methods to copy the data from/to the FIFO, this new AudioBlockFifo keeps blocks of AudioBus, it accepts interleaved data as input for its Push() method, and its Consume() method return an AudioBus for consumption. So the copy operations in this AudioBlockFifo() is 1 versus 3 in AudioFifo().
NOTRY=true
BUG=393199
TEST=media_unittests --gtest_filter="*AudioBlockFifo*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283518
Patch Set 1 : #
Total comments: 16
Patch Set 2 : addressed the comments. #
Total comments: 24
Patch Set 3 : used indices and addressed the comments #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 14 (0 generated)
Dale, could you please review this CL? We talked about such a FIFO in https://codereview.chromium.org/358823002. Note, I have not hook it up to any existing code yet, I will do it as a follow-up. Thanks, SX
https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:41: AudioFifo::GetSizes(write_pos_, block_frames_, frames, &append_frames, You can simplify this entire method to just the following: uint8* source_ptr = source; while (frames_to_push > 0) { const int frames_this_pass = std::min(frames_ - write_pos_, frames_to_push); audio_blocks_[head]->FromInterleavedPartial(source_ptr, write_pos_, frames_this_pass, bytes_per_sample); write_pos_ += frames_this_pass; frames_to_push -= frames_this_pass; source_ptr += frames_this_pass * bytes_per_frame; if (write_pos_ == frames_) { head = (head + 1) % audio_blocks_.size(); CHECK_NE(head, tail); write_pos_ = 0; } } https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:65: DCHECK(filled_blocks()); Likewise, this just becomes: CHECK_LE(tail, head); const AudioBus* output = audio_blocks_[tail]; if (tail == head) audio_blocks_[tail]->ZeroFramesPartial(write_pos_, frames - write_pos_); else tail = (tail + 1) % audio_blocks_.size(); return output; https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:73: while (!filled_blocks_.empty()) { And finally: write_pos_ = tail = head = 0; https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:86: DCHECK_GE(static_cast<int>(empty_blocks_.size() * block_frames_), write_pos_); I'd just make this frames_ - write_pos_; https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.h (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:42: int filled_blocks() const; Hacker_style requires the methods to be defined in the header. How about available_blocks() instead? https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:44: int empty_frames() const; Ditto. This also needs a comment since it's not really clear what it means relative to the pre-allocated blocks. I think you really just want this to mean the number of unfilled frames in the current block. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:52: std::queue<AudioBus*> empty_blocks_; Why do you need these? Shouldn't you just need a head and tail index which you increment % blocks? In the beginning head == tail == 0. On Push() which fills a bus completely, head = (head + 1) % blocks; On Consume(), tail = (tail + 1) % blocks. If ever head == tail, crash.
Thanks Dale, please see the comments inline. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:41: AudioFifo::GetSizes(write_pos_, block_frames_, frames, &append_frames, On 2014/07/11 22:33:32, DaleCurtis wrote: > You can simplify this entire method to just the following: > > uint8* source_ptr = source; > while (frames_to_push > 0) { > const int frames_this_pass = std::min(frames_ - write_pos_, frames_to_push); > > audio_blocks_[head]->FromInterleavedPartial(source_ptr, write_pos_, > frames_this_pass, bytes_per_sample); > > write_pos_ += frames_this_pass; > frames_to_push -= frames_this_pass; > source_ptr += frames_this_pass * bytes_per_frame; > > if (write_pos_ == frames_) { > head = (head + 1) % audio_blocks_.size(); > CHECK_NE(head, tail); > write_pos_ = 0; > } > } Done with using a write loop instead of recursive method, also get rid of calling AudioFifo::GetSizes() and AudioFifo::UpdatePos() since they can be replaced by one line of code. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:65: DCHECK(filled_blocks()); On 2014/07/11 22:33:32, DaleCurtis wrote: > Likewise, this just becomes: > > CHECK_LE(tail, head); > const AudioBus* output = audio_blocks_[tail]; > if (tail == head) > audio_blocks_[tail]->ZeroFramesPartial(write_pos_, frames - write_pos_); > else > tail = (tail + 1) % audio_blocks_.size(); > > return output; Lets move the discussion to the relevant comment in the header. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.cc:86: DCHECK_GE(static_cast<int>(empty_blocks_.size() * block_frames_), write_pos_); On 2014/07/11 22:33:32, DaleCurtis wrote: > I'd just make this frames_ - write_pos_; The comment is addressed in the head unfilled_frames() is used to tell the unfilled frames in the whole fifo. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.h (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:42: int filled_blocks() const; On 2014/07/11 22:33:33, DaleCurtis wrote: > Hacker_style requires the methods to be defined in the header. How about > available_blocks() instead? Done. Just a though, since I change the empty_frames() to unfilled_frames(), do this change your mind that filled_blocks() might be more clear? https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:44: int empty_frames() const; On 2014/07/11 22:33:33, DaleCurtis wrote: > Ditto. This also needs a comment since it's not really clear what it means > relative to the pre-allocated blocks. I think you really just want this to mean > the number of unfilled frames in the current block. I want this to tell the number of unfilled frames in the whole FIFO. This is useful for the clients to know how many more data they can write to the FIFO. I added a comment and changed the name to unfilled_frames() since "empty" might be a bit vague. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:52: std::queue<AudioBus*> empty_blocks_; On 2014/07/11 22:33:33, DaleCurtis wrote: > Why do you need these? Shouldn't you just need a head and tail index which you > increment % blocks? > > In the beginning head == tail == 0. On Push() which fills a bus completely, > head = (head + 1) % blocks; On Consume(), tail = (tail + 1) % blocks. If ever > head == tail, crash. Indexes might work but require more complicated logics. For example, if blocks == 2, and user calls Push() to fill up all the blocks, then head become 0, since there has been any Consume() yet, so tail will also be 0. Take another conner case, blocks == 1, head and tail will be always 0, then we need a way to know the the bus has been completely filled or not. Using the queues is a bit simpler from readability perspective, I am still open to switch to index but it might need more than head and tail, please let me know how strongly you think.
https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.h (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:42: int filled_blocks() const; On 2014/07/14 11:28:27, xians1 wrote: > On 2014/07/11 22:33:33, DaleCurtis wrote: > > Hacker_style requires the methods to be defined in the header. How about > > available_blocks() instead? > > Done. Just a though, since I change the empty_frames() to unfilled_frames(), do > this change your mind that filled_blocks() might be more clear? I still like available better, but it's up to you. It can't be a hacker style though as mentioned on the new patch set. https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:52: std::queue<AudioBus*> empty_blocks_; On 2014/07/14 11:28:27, xians1 wrote: > On 2014/07/11 22:33:33, DaleCurtis wrote: > > Why do you need these? Shouldn't you just need a head and tail index which > you > > increment % blocks? > > > > In the beginning head == tail == 0. On Push() which fills a bus completely, > > head = (head + 1) % blocks; On Consume(), tail = (tail + 1) % blocks. If > ever > > head == tail, crash. > > Indexes might work but require more complicated logics. > For example, if blocks == 2, and user calls Push() to fill up all the blocks, > then head become 0, since there has been any Consume() yet, so tail will also be > 0. > > Take another conner case, blocks == 1, head and tail will be always 0, then we > need a way to know the the bus has been completely filled or not. > > > Using the queues is a bit simpler from readability perspective, I am still open > to switch to index but it might need more than head and tail, please let me know > how strongly you think. Good point, you can just drop the head==tail checks in favor of tracking an available_frames_ value which you add and remove from during push / consume. You can then make the unfilled_frames() method a hacker_style() method. I think it's easier to reason about indices moving around in a vector than it is to reason about which block is where in the queues. It's hard for me to follow the push/pop maze and I prefer using as few data structures as possible. I won't block this CL though if you prefer using the multi-queue approach tough. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:14: // Create |blocks| of audio buses and push them to the containers. Since you know the capacity, call audio_blocks_.reserve(blocks) here to avoid unnecessary growth of the vector. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:24: void AudioBlockFifo::Push(const void* source, int frames, Is this actually allowed clang-format style? I thought only one parameter per line. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:35: CHECK(current_block); Seems unnecessary? Just add a DCHECK(!unfilled_blocks_.empty()) above. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:47: // The current block is completely filled, move it from |empety_blocks_| empty_blocks_ https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:76: DCHECK_GE(static_cast<int>(unfilled_blocks_.size() * block_frames_), Since you're making calculations here, this can't be a hacker_style method. How about GetUnfilledFrames or GetAvailableFrames()? https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... File media/base/audio_block_fifo_unittest.cc (right): https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:17: const int bytes_per_sample = 2; Global static const? https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:22: for (int filled_frames =max_frames - fifo->unfilled_frames(); formatting. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:26: EXPECT_EQ(fifo->unfilled_frames(), max_frames - filled_frames); expect_eq order is swapped (expected, actual). https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:27: EXPECT_EQ(fifo->available_blocks(), Ditto. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:32: protected: must be under private. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:38: static const int kChannels = 6; No need for static const within functions. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:77: { Seems unnecessary to block these?
HI Dale, PTAL. Thanks, https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:14: // Create |blocks| of audio buses and push them to the containers. On 2014/07/14 20:14:27, DaleCurtis wrote: > Since you know the capacity, call audio_blocks_.reserve(blocks) here to avoid > unnecessary growth of the vector. Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:24: void AudioBlockFifo::Push(const void* source, int frames, On 2014/07/14 20:14:27, DaleCurtis wrote: > Is this actually allowed clang-format style? I thought only one parameter per > line. I saw both in the current chromium code, so I guess it is allowed. Anyway, I changed it one param per line. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:35: CHECK(current_block); On 2014/07/14 20:14:27, DaleCurtis wrote: > Seems unnecessary? Just add a DCHECK(!unfilled_blocks_.empty()) above. Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:47: // The current block is completely filled, move it from |empety_blocks_| On 2014/07/14 20:14:27, DaleCurtis wrote: > empty_blocks_ comment was updated. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo.cc:76: DCHECK_GE(static_cast<int>(unfilled_blocks_.size() * block_frames_), On 2014/07/14 20:14:27, DaleCurtis wrote: > Since you're making calculations here, this can't be a hacker_style method. How > about GetUnfilledFrames or GetAvailableFrames()? GetUnfilledFrames() sounds good since available_blocks() is used to tell the available blocks of data for consumption. FYI, it seems frames() in audio_fifo.h is hacker_style but has implementation in .cc: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_f... Do you want me to fix it as well? https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... File media/base/audio_block_fifo_unittest.cc (right): https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:17: const int bytes_per_sample = 2; On 2014/07/14 20:14:27, DaleCurtis wrote: > Global static const? It is only used in this function, there is no need to make it global. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:22: for (int filled_frames =max_frames - fifo->unfilled_frames(); On 2014/07/14 20:14:27, DaleCurtis wrote: > formatting. Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:26: EXPECT_EQ(fifo->unfilled_frames(), max_frames - filled_frames); On 2014/07/14 20:14:27, DaleCurtis wrote: > expect_eq order is swapped (expected, actual). Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:27: EXPECT_EQ(fifo->available_blocks(), On 2014/07/14 20:14:27, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:32: protected: On 2014/07/14 20:14:27, DaleCurtis wrote: > must be under private. Done. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:38: static const int kChannels = 6; On 2014/07/14 20:14:27, DaleCurtis wrote: > No need for static const within functions. I changed it to const int in order to make it clear that the values won't be changed afterwards. Hope it is fine. https://codereview.chromium.org/389623002/diff/40001/media/base/audio_block_f... media/base/audio_block_fifo_unittest.cc:77: { On 2014/07/14 20:14:27, DaleCurtis wrote: > Seems unnecessary to block these? It is simply a way to help understanding the tests, I removed it since you might not think it was useful.
https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... File media/base/audio_block_fifo.h (right): https://codereview.chromium.org/389623002/diff/20001/media/base/audio_block_f... media/base/audio_block_fifo.h:52: std::queue<AudioBus*> empty_blocks_; On 2014/07/14 20:14:26, DaleCurtis wrote: > On 2014/07/14 11:28:27, xians1 wrote: > > On 2014/07/11 22:33:33, DaleCurtis wrote: > > > Why do you need these? Shouldn't you just need a head and tail index which > > you > > > increment % blocks? > > > > > > In the beginning head == tail == 0. On Push() which fills a bus completely, > > > head = (head + 1) % blocks; On Consume(), tail = (tail + 1) % blocks. If > > ever > > > head == tail, crash. > > > > Indexes might work but require more complicated logics. > > For example, if blocks == 2, and user calls Push() to fill up all the blocks, > > then head become 0, since there has been any Consume() yet, so tail will also > be > > 0. > > > > Take another conner case, blocks == 1, head and tail will be always 0, then we > > need a way to know the the bus has been completely filled or not. > > > > > > Using the queues is a bit simpler from readability perspective, I am still > open > > to switch to index but it might need more than head and tail, please let me > know > > how strongly you think. > > Good point, you can just drop the head==tail checks in favor of tracking an > available_frames_ value which you add and remove from during push / consume. > You can then make the unfilled_frames() method a hacker_style() method. I think > it's easier to reason about indices moving around in a vector than it is to > reason about which block is where in the queues. It's hard for me to follow the > push/pop maze and I prefer using as few data structures as possible. I won't > block this CL though if you prefer using the multi-queue approach tough. :) Both work for me. But since you prefer indices, I just switch it over to indices As you might have already noticed from the new version, I need to use an extra member variable to keep track how many blocks of data can be consumed, since we don't want to provide zero to the clients when there is not enough data in the FIFO, hope it is fine.
lgtm https://codereview.chromium.org/389623002/diff/80001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/80001/media/base/audio_block_f... media/base/audio_block_fifo.cc:77: int unfilled_frames = nit: const
The CQ bit was checked by xians@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/389623002/100001
Thanks, landing now. https://codereview.chromium.org/389623002/diff/80001/media/base/audio_block_f... File media/base/audio_block_fifo.cc (right): https://codereview.chromium.org/389623002/diff/80001/media/base/audio_block_f... media/base/audio_block_fifo.cc:77: int unfilled_frames = On 2014/07/15 21:52:19, DaleCurtis wrote: > nit: const Done.
The CQ bit was unchecked by xians@chromium.org
The CQ bit was checked by xians@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/389623002/100001
Message was sent while issue was closed.
Change committed as 283518 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
