Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10912079:
Adds AudioFifo class to Chrome media. (Closed)
|
Created:
8 years, 3 months ago by henrika (OOO until Aug 14) Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, scherkus (not reviewing), tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPorts the WebCore::AudioFIFO in WebKit to Chrome style.
This class will be use in an upcoming AudioPullFifo class.
BUG=none
TEST=media_unittests --gtest_filter=AudioFifoTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155171
Patch Set 1 #
Total comments: 52
Patch Set 2 : Changes based on review by Chris #Patch Set 3 : Changes based on review by Dale #
Total comments: 7
Patch Set 4 : Added frames_to_consum to Consume() #
Total comments: 25
Patch Set 5 : More changes proposed by Dale #
Messages
Total messages: 14 (0 generated)
Please review a first version of media::AudioFifo. The API is not identical to the original version and I'v tried to clean up the code somewhat. The main internal architecture is however similar to the WebKit version. Here are some additional notes: Append() returns false when we overflow and I don't do any attempt to use parts of the input and "fill up as much as I can". Remove() returns false of the #frames in the destination AudioBus is larger than what the FIFO contains (underrun). Again, I don't flush out the available content here either. This class is not thread safe. Should I add locks or derive from NonThreadSafe to make it more clear?
Henrik, looks great, especially the tests! I have a couple of nits. Dale: could you please look especially hard at the final test https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:34: audio_bus_->Zero(); I don't think Zero() is necessary since Create() should return an already zeroed bus https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:62: memcpy(&dest[write_pos_], &src[0], append_size * sizeof(src[0])); why "&dest[write_pos_]" instead of "dest + write_pos" and "&src[0]" instead of "src" https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:65: memcpy(&dest[0], &src[append_size], wrap_size * sizeof(src[0])); why not "dest" instead of "&dest[0]" https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:92: // Remove all channels from the FIFO and copy the content to the "Remove all channels from the FIFO" sounds a little misleading. How about: "For all channels, remove the requested amount of data..." https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:99: memcpy(&dest[0], &src[read_pos_], remove_size * sizeof(src[0])); "dest" instead of "&dest[0]", etc. here and in memcpy() below https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.h File media/base/audio_fifo.h (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:16: // and guaranteed to be aligned by AudioBus::kChannelAlignment. The allocated comment nit: I'd simply remove this comment about alignment, since this is really a property of AudioBus and not this class. In other words, Append() and Remove() both deal with copying from and to AudioBus objects which themselves have the property of alignment. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:17: // memory is utilized as a circular queue. nit: My preference is to avoid the word "queue" and instead use "buffer" or "ring buffer", since queue usually involves operations on single elements instead of variable length amounts of data. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:25: // Returns false if the allocated space is not sufficient. and I would add that "partial data is not written" for this failure case https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:29: // them to |destination|. and "Returns false if there's not enough data, not copying partial data" https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:36: int size() const { return size_; } I would tend to call this "available()" or "frames_available()" here, since "size()" makes me think of the total FIFO size (what you call max_size() below) https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:43: // The actual FIFO is an audio bus implemented as a circular queue. queue -> buffer https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:47: int size_; size_ -> available_ https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo_... File media/base/audio_fifo_unittest.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo_... media/base/audio_fifo_unittest.cc:149: TEST_F(AudioFifoTest, AppdendAndRemove) { typo: AppdendAndRemove
Thanks Chris. Uploaded new patch and answered your questions. Waiting for feedback from Dale as well. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:34: audio_bus_->Zero(); removed. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:62: memcpy(&dest[write_pos_], &src[0], append_size * sizeof(src[0])); I prefer the "C++ way" and not the "C way" since &foo[0] can deal with cases like: char char_array[]; std::vector<char> std_array; memcpy(&char_array[0], &src[0], size); // OK memcpy(&std_array[0], &src[0], size); // OK memcpy(char_array + 0, &src[0], size); // OK, the compiler will cast char_array to a pointer. memcpy(std_array + 0, &src[0], size); // Error since std_array is not a pointer. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:65: memcpy(&dest[0], &src[append_size], wrap_size * sizeof(src[0])); ditto http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:92: // Remove all channels from the FIFO and copy the content to the On 2012/09/04 20:07:20, Chris Rogers wrote: > "Remove all channels from the FIFO" sounds a little misleading. How about: "For > all channels, remove the requested amount of data..." Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:99: memcpy(&dest[0], &src[read_pos_], remove_size * sizeof(src[0])); ditto http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode16 media/base/audio_fifo.h:16: // and guaranteed to be aligned by AudioBus::kChannelAlignment. The allocated On 2012/09/04 20:07:20, Chris Rogers wrote: > comment nit: I'd simply remove this comment about alignment, since this is > really a property of AudioBus and not this class. In other words, Append() and > Remove() both deal with copying from and to AudioBus objects which themselves > have the property of alignment. Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode17 media/base/audio_fifo.h:17: // memory is utilized as a circular queue. On 2012/09/04 20:07:20, Chris Rogers wrote: > nit: My preference is to avoid the word "queue" and instead use "buffer" or > "ring buffer", since queue usually involves operations on single elements > instead of variable length amounts of data. Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode25 media/base/audio_fifo.h:25: // Returns false if the allocated space is not sufficient. On 2012/09/04 20:07:20, Chris Rogers wrote: > and I would add that "partial data is not written" for this failure case Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode29 media/base/audio_fifo.h:29: // them to |destination|. On 2012/09/04 20:07:20, Chris Rogers wrote: > and "Returns false if there's not enough data, not copying partial data" Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode36 media/base/audio_fifo.h:36: int size() const { return size_; } I will wait for feedback from Dale as well. But available() does not feel OK to me since I am not referring to "free space in the FIFO" but "how much is used". http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode43 media/base/audio_fifo.h:43: // The actual FIFO is an audio bus implemented as a circular queue. On 2012/09/04 20:07:20, Chris Rogers wrote: > queue -> buffer Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode47 media/base/audio_fifo.h:47: int size_; I am inspired by: STL where queue::size() returns the number of elements in the queue. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest.cc File media/base/audio_fifo_unittest.cc (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest... media/base/audio_fifo_unittest.cc:149: TEST_F(AudioFifoTest, AppdendAndRemove) { On 2012/09/04 20:07:20, Chris Rogers wrote: > typo: AppdendAndRemove Done.
In general looks good. I don't have any functional complaints, just API level comments. Testing is nice :) https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:13: int* size, int* wrap_size) { Indent to start with (. (Or move the line above down here and keep 4 space indent) https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:14: if (pos + src_size > max_size) { CHECK() size, wrap_size? https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:34: audio_bus_->Zero(); On 2012/09/04 20:07:20, Chris Rogers wrote: > I don't think Zero() is necessary since Create() should return an already zeroed > bus Create() doesn't Zero, so if it needs to be Zero'd that must be done here. I don't it's necessary though since we'll just Underflow if no data has been added. otherwise the data will be written to anyways through Append(). https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:52: int append_size = 0; Can these names include "frames" or something more specific than size? At a glance I interpreted this as the size in bytes. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.cc:98: // Append part of (or the complete) source to the destination. Update comment. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.h File media/base/audio_fifo.h (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:26: bool Append(const AudioBus* source); Do we expect a recoverable / normal case where Append() (or Remove()) will fail? Otherwise it's probably better to simply CHECK() and have these return void. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:30: bool Remove(AudioBus* destination); Kind of a misleading name, since it doesn't remove the data, just copies it out. The comment reinforces this by stating that it first removes and then copies the frames out. The original Push/Consume names might be better in the sense that they work around the issue by making the implementation details opaque. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:36: int size() const { return size_; } On 2012/09/04 20:07:20, Chris Rogers wrote: > I would tend to call this "available()" or "frames_available()" here, since > "size()" makes me think of the total FIFO size (what you call max_size() below) +1 for frames_available(). size() is inconsistent with AudioBus, so I'd prefer whatever the name chosen, frames() is included in it. frames_available makes the most sense to me since you are advertising the number of frames that are available for removal/consuming. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:37: int max_size() const { return audio_bus_->frames(); } Needed publicly? https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:38: int channels() const { return audio_bus_->channels(); } Ditto. https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo.... media/base/audio_fifo.h:40: bool full() const { return (size_ == max_size()); } Ditto / needed at all? https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo_... File media/base/audio_fifo_unittest.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo_... media/base/audio_fifo_unittest.cc:29: const int kChannels = 6; static here and below. Used elsewhere too, so maybe global? https://chromiumcodereview.appspot.com/10912079/diff/1/media/base/audio_fifo_... media/base/audio_fifo_unittest.cc:116: while (!fifo.full()) { How long does this take to run? Not sure we need 480 iterations :)
- Removed helper methods. - Modified member names. - New public API: Append->Push, Remove->Consume. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:13: int* size, int* wrap_size) { On 2012/09/05 10:34:18, DaleCurtis wrote: > Indent to start with (. (Or move the line above down here and keep 4 space > indent) Done. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.cc#newco... media/base/audio_fifo.cc:14: if (pos + src_size > max_size) { See comment above about using CHECK() in this class. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode26 media/base/audio_fifo.h:26: bool Append(const AudioBus* source); I discussed this off-line with Chris and his suggestion was to make it bool. It feels scary to add CHECK() in parts that will run in the browser process and are connected to audio buffering. Could we really do that even if we wanted? http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode30 media/base/audio_fifo.h:30: bool Remove(AudioBus* destination); Chris and I discussed this as well and landed at the current naming. You have a point. Let me think. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode36 media/base/audio_fifo.h:36: int size() const { return size_; } OK, now I understand the point of using available. I'll change this name. Check out the next version. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode37 media/base/audio_fifo.h:37: int max_size() const { return audio_bus_->frames(); } nope; I just liked it. Will remove. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode38 media/base/audio_fifo.h:38: int channels() const { return audio_bus_->channels(); } got it, will remove. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode40 media/base/audio_fifo.h:40: bool full() const { return (size_ == max_size()); } I dropped two of them but would like to keep these. They are useful for testing and would like to avoid making the test a friend. Other ideas? http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest.cc File media/base/audio_fifo_unittest.cc (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest... media/base/audio_fifo_unittest.cc:29: const int kChannels = 6; I actually use different for each test. OK to keep as is? http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest... media/base/audio_fifo_unittest.cc:116: while (!fifo.full()) { It takes 4ms on my machine. Reduced it to 64 => 1ms ;-)
http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode26 media/base/audio_fifo.h:26: bool Append(const AudioBus* source); On 2012/09/05 12:49:04, henrika wrote: > I discussed this off-line with Chris and his suggestion was to make it bool. It > feels scary to add CHECK() in parts that will run in the browser process and are > connected to audio buffering. Could we really do that even if we wanted? I'd just prefer to not have to worry about the return value (vs CHECK()) if this is a state we could only reach by programmer error. I admit I haven't thought it all the way through though, so if you and Chris have already hashed it out, that's fine with me. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo.h#newcode40 media/base/audio_fifo.h:40: bool full() const { return (size_ == max_size()); } On 2012/09/05 12:49:04, henrika wrote: > I dropped two of them but would like to keep these. They are useful for testing > and would like to avoid making the test a friend. Other ideas? Seems you dropped them anyway? If you need them that's fine with me, but friending tests is usually the way to go to avoid unnecessary API. http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest.cc File media/base/audio_fifo_unittest.cc (right): http://codereview.chromium.org/10912079/diff/1/media/base/audio_fifo_unittest... media/base/audio_fifo_unittest.cc:29: const int kChannels = 6; On 2012/09/05 12:49:04, henrika wrote: > I actually use different for each test. > > OK to keep as is? Should still be static, non-global is fine with me. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:30: // Returns false if it is not possible to fill up the complete |destination|. Reword, maybe: "Returns false if there are not enough frames available to completely fill the |destination|." Though now that I think about it more, this is where the bool gets a bit confusing. Since wouldn't it make more sense to return all frames available() even if fewer than destination->frames() are available? Then return the amount of frames filled. Are you expecting callers to create a new AudioBus() which matches the drained frame count every time? Consider AudioPullFIFO, since the current behavior would mean you can't empty this FIFO before requesting more frames. Which, depending on how you sized the FIFO originally means you'll overflow during Push. I think it would be nicer to return all the frames available. Even on Push(), sending the frames available and DLOG(ERROR)'ing a message might result in less of a glitch. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:37: // Number of actual audio bus frames in the FIFO. Drop the "bus". http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:47: // This size is set by |frames| in the constructor. s/size/value/
Based on off-line discussions I will modify the public API to: // Consumes |frames_to_consume| audio frames from the FIFO and copies // them to |destination|. // Returns false if the FIFO does not contain |frames_to_consume| frames // or if there is not sufficient space in |destination| to store the frames. bool Consume(AudioBus* destination, int frames_to_consume); which is more inline with the WebKit port and also feels required to be able to add support for AudioPullFIFO. This change affects the tests quite much and I will have to modify them. Will land new version tomorrow. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:37: // Number of actual audio bus frames in the FIFO. On 2012/09/05 13:09:22, DaleCurtis wrote: > Drop the "bus". Done. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:47: // This size is set by |frames| in the constructor. On 2012/09/05 13:09:22, DaleCurtis wrote: > s/size/value/ Done.
Per discussion yesterday, LGTM. We'll iterate and improve in AudioPullFIFO.
Modified Consume() and also cleaned up the unit test to make it more clear. Andrew: need OK from you to land. Chris: any input? FYI - this guy might change when I start on the PullFifo. Should be safe to land as is since it is not used in Chrome today. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:30: // Returns false if it is not possible to fill up the complete |destination|. Modified the API.
I'm also a media/ OWNER, so you don't need to wait for scherkus unless you want to. LGTM. http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10912079/diff/10001/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:37: // Number of actual audio bus frames in the FIFO. On 2012/09/05 15:26:32, henrika wrote: > On 2012/09/05 13:09:22, DaleCurtis wrote: > > Drop the "bus". > > Done. Looks unchanged still? http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:7: #include "media/audio/audio_parameters.h" alphabetical order (should be below base include) http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:12: static void GetSizes( Docs. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:25: static int UpdatePos(int pos, int step, int max_size) { Docs. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... File media/base/audio_fifo_unittest.cc (right): http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... media/base/audio_fifo_unittest.cc:192: // Push one audio bus to the FIFO and fill it with 1's. I think this section is already tested by the Consume() (and FramesInFifo) test above? http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... media/base/audio_fifo_unittest.cc:212: extra line.
Thanks Dale. Fixed. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:7: #include "media/audio/audio_parameters.h" Fixed. Don't need this one at all actually. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:12: static void GetSizes( On 2012/09/06 11:14:13, DaleCurtis wrote: > Docs. Done. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:25: static int UpdatePos(int pos, int step, int max_size) { On 2012/09/06 11:14:13, DaleCurtis wrote: > Docs. Done. http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... File media/base/audio_fifo_unittest.cc (right): http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... media/base/audio_fifo_unittest.cc:192: // Push one audio bus to the FIFO and fill it with 1's. Well, almost. I check the content here as well not only the size ;-) http://codereview.chromium.org/10912079/diff/13001/media/base/audio_fifo_unit... media/base/audio_fifo_unittest.cc:212: On 2012/09/06 11:14:13, DaleCurtis wrote: > extra line. Done.
few post-land nits/questions https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... File media/base/audio_fifo.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:7: #include "media/audio/audio_parameters.h" a->z ordering https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:12: static void GetSizes( docs please https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:25: static int UpdatePos(int pos, int step, int max_size) { docs https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:80: DLOG(ERROR) << "FIFO underrun."; this is something that I feel we may want to bump up into a CHECK() is the calling going to be significantly more complex if they have to inspect available frames in the FIFO before calling Consume()? https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:87: DLOG(ERROR) << "Insufficient space in destination."; ditto https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... File media/base/audio_fifo.h (right): https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.h:32: bool Consume(AudioBus* destination, int frames_to_consume); are callees expected to check their own size / frames_in_fifo() prior to calling Consume()? if so, should we crash instead of returning bool? it looks like it's being used to signal two separate classes of errors (not enough in destination / not enough in fifo) In general I try to avoid "return a bool indicating success" functions because people tend to forget to check the result :) https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.h:38: int frames_in_fifo() const { return frames_in_fifo_; } s/frames_in_fifo/frames/ ? we're using a FIFO object so not sure why we need to repeat the _in_fifo() bit https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... File media/base/audio_fifo_unittest.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo_unittest.cc:212: nit: remove extra blank line
Thanks Andrew, will fix all this in an upcoming AudioPullFifo CL. On Thu, Sep 6, 2012 at 3:53 PM, <scherkus@chromium.org> wrote: > few post-land nits/questions > > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.cc<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc> > File media/base/audio_fifo.cc (right): > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.**cc#newcode7<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc#newcode7> > media/base/audio_fifo.cc:7: #include "media/audio/audio_parameters.**h" > a->z ordering > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.**cc#newcode12<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc#newcode12> > > media/base/audio_fifo.cc:12: static void GetSizes( > docs please > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.**cc#newcode25<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc#newcode25> > > media/base/audio_fifo.cc:25: static int UpdatePos(int pos, int step, int > max_size) { > docs > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.**cc#newcode80<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc#newcode80> > media/base/audio_fifo.cc:80: DLOG(ERROR) << "FIFO underrun."; > this is something that I feel we may want to bump up into a CHECK() > > is the calling going to be significantly more complex if they have to > inspect available frames in the FIFO before calling Consume()? > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.**cc#newcode87<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.cc#newcode87> > media/base/audio_fifo.cc:87: DLOG(ERROR) << "Insufficient space in > destination."; > ditto > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.h<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.h> > File media/base/audio_fifo.h (right): > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.h#**newcode32<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.h#newcode32> > media/base/audio_fifo.h:32: bool Consume(AudioBus* destination, int > frames_to_consume); > are callees expected to check their own size / frames_in_fifo() prior to > calling Consume()? > > if so, should we crash instead of returning bool? it looks like it's > being used to signal two separate classes of errors (not enough in > destination / not enough in fifo) > > In general I try to avoid "return a bool indicating success" functions > because people tend to forget to check the result :) > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo.h#**newcode38<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo.h#newcode38> > media/base/audio_fifo.h:38: int frames_in_fifo() const { return > frames_in_fifo_; } > s/frames_in_fifo/frames/ ? > > we're using a FIFO object so not sure why we need to repeat the > _in_fifo() bit > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo_**unittest.cc<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo_unittest.cc> > File media/base/audio_fifo_**unittest.cc (right): > > https://chromiumcodereview.**appspot.com/10912079/diff/** > 13001/media/base/audio_fifo_**unittest.cc#newcode212<https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_fifo_unittest.cc#newcode212> > media/base/audio_fifo_**unittest.cc:212: > nit: remove extra blank line > > https://chromiumcodereview.**appspot.com/10912079/<https://chromiumcodereview... >
Fixed and added to AudioPullFifo CL. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... File media/base/audio_fifo.cc (right): https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:7: #include "media/audio/audio_parameters.h" On 2012/09/06 13:53:57, scherkus wrote: > a->z ordering Done. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:12: static void GetSizes( On 2012/09/06 13:53:57, scherkus wrote: > docs please Done. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:25: static int UpdatePos(int pos, int step, int max_size) { On 2012/09/06 13:53:57, scherkus wrote: > docs Done. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:80: DLOG(ERROR) << "FIFO underrun."; Got it. Will modify. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.cc:87: DLOG(ERROR) << "Insufficient space in destination."; see above https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... File media/base/audio_fifo.h (right): https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.h:32: bool Consume(AudioBus* destination, int frames_to_consume); Are you suggesting that I modify to void and add CHECK() for these two failing cases? And yes, the user can either do the sanity check you list or go for it and check the return value ;-) Disussed off-line; will change. https://chromiumcodereview.appspot.com/10912079/diff/13001/media/base/audio_f... media/base/audio_fifo.h:38: int frames_in_fifo() const { return frames_in_fifo_; } LOL, that's my melody as well but not all reviewers like the initial proposal since it could be mixed up with the max number of frames it can contain ;-) I'll be happy to change. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
