Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
155193:
BufferQueue class to hide audio data micromanagement from scaling algorithms. (Closed)
|
Created:
11 years, 5 months ago by kylep Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionBufferQueue class to hide audio data micromanagement from scaling algorithms. May be useful in other contexts.
BUG=16011
TEST=src/media/base/buffer_queue_unittest.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20211
Patch Set 1 #Patch Set 2 : '' #
Total comments: 43
Patch Set 3 : '' #
Total comments: 28
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 13
Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 2
Patch Set 10 : '' #
Messages
Total messages: 12 (0 generated)
I'd appreciate a code review from you on this new class. Thanks. -Kyle
my first comment is the file names are misleading. I'm pretty sure this applies to audio, and not general data source buffering?
Looks solid. Here's a bunch of nitpicks. http://codereview.chromium.org/155193/diff/1007/1008 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1007/1008#newcode20 Line 20: if (IsEmpty()) Is this worthy of a DCHECK? http://codereview.chromium.org/155193/diff/1007/1008#newcode23 Line 23: // Perform changes on |size_| here to save operations. Does our API allow for consuming? I'm tempted to want to just CHECK fail on these kinds of issues. They shouldn't call Consume if they aren't checking whether or not they consume. In general, APIs that try to be "nice" and make things "safer" usually leads to unpredictable behavior because people don't notice when they code logic errors. http://codereview.chromium.org/155193/diff/1007/1008#newcode31 Line 31: while (bytes > 0) { bytes isn't a very descriptive name. what kind of bytes are they? chocolate? :) http://codereview.chromium.org/155193/diff/1007/1008#newcode38 Line 38: data_offset_ += bytes; Hmm...I didn't think this through, but usually if you have to have a break in the middle of the loop, the loop's condition is a bit complex. You have 2 break statements, and 2 levels of nesting of ifs. Can this be simplified? A good ideal is to have, for each loop, one condition checked in one spot. http://codereview.chromium.org/155193/diff/1007/1008#newcode53 Line 53: if (IsEmpty()) Oh, and another benefit of making the APIs less forgiving of programmer stupidity...less error checking code. Less complexity = less bugs. http://codereview.chromium.org/155193/diff/1007/1008#newcode62 Line 62: while (bytes > 0) { I wonder if a for loop makes more sense? Also, try to choose some more desciptive variable names. "index" is as descriptive as "i". http://codereview.chromium.org/155193/diff/1007/1008#newcode76 Line 76: if (index > queue_.size()) Same comment regarding having multiple loop conditions being checked in multiple spots. Extend that principle to include the sentinel variable. It's usually ideal to try and increment the sentinel in one, or maybe at most 2 spots. http://codereview.chromium.org/155193/diff/1007/1008#newcode97 Line 97: // Since we keep track of the number of bytes, this is easier than calling What if you get 0-lengthed buffers? http://codereview.chromium.org/155193/diff/1007/1009 File media/base/buffer_queue.h (right): http://codereview.chromium.org/155193/diff/1007/1009#newcode7 Line 7: // as a contiguous region. Make sure to comment on the thread safety of this class. Specifically, this calss is not threadsafe and needs external locking. :) http://codereview.chromium.org/155193/diff/1007/1009#newcode14 Line 14: #include "base/ref_counted.h" Why do we need this? Do we just need scoped_ref_ptr.h? http://codereview.chromium.org/155193/diff/1007/1009#newcode23 Line 23: virtual ~BufferQueue(); Is this class meant to be subclassed or not? If it is meant to be subclassed, then make everything public virtual unless you have a pretty good reason not to. http://codereview.chromium.org/155193/diff/1007/1009#newcode27 Line 27: void Consume(size_t bytes); Hmm...Consume is separate from Copy. This isn't a bad design, however, usually people split up the functions because there's some problem with doing both in the same one. For instancee in STL, you cannot pop and top the queue due to exception safety issues caused by returning via copy. Here, I'm less certain what the tradeoffs are. Any particular reason to prefer this over a single "Read" or "Dequeue" function? http://codereview.chromium.org/155193/diff/1007/1009#newcode29 Line 29: // Copies |bytes| bytes from |queue_| to |dest|. Returns the number of bytes Copies |bytes| -> Copies at most |bytes| Also, avoid referencing implementation details (ie., private variables) in your comments. I shouldn't have to understand the implementation to understand your comment. http://codereview.chromium.org/155193/diff/1007/1009#newcode34 Line 34: // |buffer_in|. Make sure you note that you take a reference on the buffer. http://codereview.chromium.org/155193/diff/1007/1009#newcode37 Line 37: // Clears |queue_| and resets |size_| to 0. Call this Clear() since that's the verb you use in the comment? Also, try to focus on the semantics of the function, not the specific action. So this thing releases all in the queue. You don't need to specifically state which variables change how. http://codereview.chromium.org/155193/diff/1007/1009#newcode48 Line 48: std::deque< scoped_refptr<Buffer> > queue_; std::dequeue< scope_ -> std::dequeue<scoped_ remove the leading space. You only need the trailing one to avoid the >> ambiguity. http://codereview.chromium.org/155193/diff/1007/1010 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1007/1010#newcode20 Line 20: // Tests ctor. Good set of tests! In general, it's better to create separate TEST or TEST_F for each specific case. It keeps the tests shorter, less prone to affect one another, and makes it look cool when you see a bunch of tests passed instead of just one big one. :) Also, when there's a failure, seeing BufferQueueTest_CtorInitialization failed is much more descriptive than just BufferQueueTest_Basic failed. LOoking at the code, maybe make one separate test for each comment taht says // Tests ....? Might also look at TEST_F instead of TEST if there is any comment setup/teardown.
Also, I'm using this only for audio data, but I see no reason it can't be retasked for other buffer queuing needs. Perhaps the description of the CL is misleading? :p http://codereview.chromium.org/155193/diff/1007/1008 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1007/1008#newcode20 Line 20: if (IsEmpty()) On 2009/07/08 02:44:17, awong wrote: > Is this worthy of a DCHECK? Done. http://codereview.chromium.org/155193/diff/1007/1008#newcode23 Line 23: // Perform changes on |size_| here to save operations. On 2009/07/08 02:44:17, awong wrote: > Does our API allow for consuming? I'm tempted to want to just CHECK fail on > these kinds of issues. They shouldn't call Consume if they aren't checking > whether or not they consume. > > In general, APIs that try to be "nice" and make things "safer" usually leads to > unpredictable behavior because people don't notice when they code logic errors. Thanks; good advice. This also simplified my logic below. http://codereview.chromium.org/155193/diff/1007/1008#newcode31 Line 31: while (bytes > 0) { On 2009/07/08 02:44:17, awong wrote: > bytes isn't a very descriptive name. what kind of bytes are they? chocolate? > :) Yes. Chocolate data. What's wrong with bytes? It's the number of bytes I'm consuming. A counter that ticks down as I consume them. Should I call it "bytesToBeConsumed"? Renamed as such. http://codereview.chromium.org/155193/diff/1007/1008#newcode38 Line 38: data_offset_ += bytes; On 2009/07/08 02:44:17, awong wrote: > Hmm...I didn't think this through, but usually if you have to have a break in > the middle of the loop, the loop's condition is a bit complex. You have 2 break > statements, and 2 levels of nesting of ifs. Can this be simplified? > > A good ideal is to have, for each loop, one condition checked in one spot. Done. http://codereview.chromium.org/155193/diff/1007/1008#newcode53 Line 53: if (IsEmpty()) On 2009/07/08 02:44:17, awong wrote: > Oh, and another benefit of making the APIs less forgiving of programmer > stupidity...less error checking code. > > Less complexity = less bugs. Done. http://codereview.chromium.org/155193/diff/1007/1008#newcode62 Line 62: while (bytes > 0) { On 2009/07/08 02:44:17, awong wrote: > I wonder if a for loop makes more sense? > > Also, try to choose some more desciptive variable names. "index" is as > descriptive as "i". > Okay. Changed it to a for loop and renamed index "queueIndex". It's probably cleaner. http://codereview.chromium.org/155193/diff/1007/1008#newcode76 Line 76: if (index > queue_.size()) On 2009/07/08 02:44:17, awong wrote: > Same comment regarding having multiple loop conditions being checked in multiple > spots. Extend that principle to include the sentinel variable. It's usually > ideal to try and increment the sentinel in one, or maybe at most 2 spots. > Does the for loop make this irrelevant? http://codereview.chromium.org/155193/diff/1007/1008#newcode97 Line 97: // Since we keep track of the number of bytes, this is easier than calling On 2009/07/08 02:44:17, awong wrote: > What if you get 0-lengthed buffers? Then we have no data... and the size should be zero. I renamed size_ to size_in_bytes_. http://codereview.chromium.org/155193/diff/1007/1009 File media/base/buffer_queue.h (right): http://codereview.chromium.org/155193/diff/1007/1009#newcode7 Line 7: // as a contiguous region. On 2009/07/08 02:44:17, awong wrote: > Make sure to comment on the thread safety of this class. Specifically, this > calss is not threadsafe and needs external locking. :) Done. http://codereview.chromium.org/155193/diff/1007/1009#newcode14 Line 14: #include "base/ref_counted.h" On 2009/07/08 02:44:17, awong wrote: > Why do we need this? Do we just need scoped_ref_ptr.h? scoped_refptr is in that file. http://codereview.chromium.org/155193/diff/1007/1009#newcode23 Line 23: virtual ~BufferQueue(); On 2009/07/08 02:44:17, awong wrote: > Is this class meant to be subclassed or not? If it is meant to be subclassed, > then make everything public virtual unless you have a pretty good reason not to. > Isn't it unsafe to make the destructor non-virtual? I don't intend to subclass this, but if other people want to repurpose it, I don't want them to have to change too much. http://codereview.chromium.org/155193/diff/1007/1009#newcode27 Line 27: void Consume(size_t bytes); On 2009/07/08 02:44:17, awong wrote: > Hmm...Consume is separate from Copy. > > This isn't a bad design, however, usually people split up the functions because > there's some problem with doing both in the same one. For instancee in STL, you > cannot pop and top the queue due to exception safety issues caused by returning > via copy. > > Here, I'm less certain what the tradeoffs are. Any particular reason to prefer > this over a single "Read" or "Dequeue" function? Yes. For my purposes, I will be overlapping/crossfading the data in the queue, so I will copy over a bunch, but only consume half of what I copied (at 0.5x speed, for example). http://codereview.chromium.org/155193/diff/1007/1009#newcode29 Line 29: // Copies |bytes| bytes from |queue_| to |dest|. Returns the number of bytes On 2009/07/08 02:44:17, awong wrote: > Copies |bytes| -> Copies at most |bytes| > > Also, avoid referencing implementation details (ie., private variables) in your > comments. I shouldn't have to understand the implementation to understand your > comment. Done. http://codereview.chromium.org/155193/diff/1007/1009#newcode34 Line 34: // |buffer_in|. On 2009/07/08 02:44:17, awong wrote: > Make sure you note that you take a reference on the buffer. Done. http://codereview.chromium.org/155193/diff/1007/1009#newcode37 Line 37: // Clears |queue_| and resets |size_| to 0. On 2009/07/08 02:44:17, awong wrote: > Call this Clear() since that's the verb you use in the comment? > > Also, try to focus on the semantics of the function, not the specific action. So > this thing releases all in the queue. You don't need to specifically state > which variables change how. Done. http://codereview.chromium.org/155193/diff/1007/1009#newcode48 Line 48: std::deque< scoped_refptr<Buffer> > queue_; On 2009/07/08 02:44:17, awong wrote: > std::dequeue< scope_ -> std::dequeue<scoped_ > > remove the leading space. You only need the trailing one to avoid the >> > ambiguity. I believe the style guide says you should (or at least can) make it symmetrical? http://codereview.chromium.org/155193/diff/1007/1010 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1007/1010#newcode20 Line 20: // Tests ctor. On 2009/07/08 02:44:17, awong wrote: > Good set of tests! > > In general, it's better to create separate TEST or TEST_F for each specific > case. It keeps the tests shorter, less prone to affect one another, and makes > it look cool when you see a bunch of tests passed instead of just one big one. > :) > > Also, when there's a failure, seeing BufferQueueTest_CtorInitialization failed > is much more descriptive than just BufferQueueTest_Basic failed. > > LOoking at the code, maybe make one separate test for each comment taht says // > Tests ....? > > Might also look at TEST_F instead of TEST if there is any comment > setup/teardown. > Done.
sweet! http://codereview.chromium.org/155193/diff/9/10 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/9/10#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. should be 2009 http://codereview.chromium.org/155193/diff/9/10#newcode19 Line 19: void BufferQueue::Consume(size_t bytesToBeConsumed) { bytes_to_be_consumed http://codereview.chromium.org/155193/diff/9/10#newcode29 Line 29: // Calculate number of usable bytesToBeConsumed in the front of the |queue_|. 80 chars http://codereview.chromium.org/155193/diff/9/10#newcode62 Line 62: // Modify counts and pointers period at end of comment http://codereview.chromium.org/155193/diff/9/11 File media/base/buffer_queue.h (right): http://codereview.chromium.org/155193/diff/9/11#newcode30 Line 30: // Advances front pointer |bytesToBeConsumed| bytes and discards "consumed" bytes_to_be_consumed http://codereview.chromium.org/155193/diff/9/11#newcode32 Line 32: void Consume(size_t bytesToBeConsumed); bytes_to_be_consumed http://codereview.chromium.org/155193/diff/9/12 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/9/12#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. should be 2009 http://codereview.chromium.org/155193/diff/9/12#newcode10 Line 10: using media::BufferQueue; nowadays we seem to place our unit tests inside the corressponding namespace so here you can wrap the whole thing inside the media namespace and drop the using statements http://codereview.chromium.org/155193/diff/9/12#newcode40 Line 40: }; private: DISALLOW_COPY_AND_ASSIGN(BufferQueueTest); http://codereview.chromium.org/155193/diff/9/12#newcode43 Line 43: TEST_F(BufferQueueTest, TestCtor) { nit: the "Test" prefix is redundant when working from these tests from the command line they'll look like: BufferQueueTest.TestCtor
Much nicer! got a few more comments. http://codereview.chromium.org/155193/diff/1007/1008 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1007/1008#newcode20 Line 20: if (IsEmpty()) On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > Is this worthy of a DCHECK? > > Done. Actually, if we are going to assume that size_in_bytes > bytesTobeConsumed, we need CHECK this. Otherwise, in production code, this check will be disabled and we might underrun the buffer. BTW, should this be > or >=? http://codereview.chromium.org/155193/diff/1007/1008#newcode31 Line 31: while (bytes > 0) { On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > bytes isn't a very descriptive name. what kind of bytes are they? chocolate? > > :) > > Yes. Chocolate data. What's wrong with bytes? It's the number of bytes I'm > consuming. A counter that ticks down as I consume them. Should I call it > "bytesToBeConsumed"? Renamed as such. local varaibles should use _ naming, not camelcase. The issue was indeed that I bytes by itself doesn't indicate bytes that have been read, bytes left to read, etc. bytes_to_be_consumed fixes that. bytes_to_consume would be shorter. http://codereview.chromium.org/155193/diff/1007/1008#newcode38 Line 38: data_offset_ += bytes; On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > Hmm...I didn't think this through, but usually if you have to have a break in > > the middle of the loop, the loop's condition is a bit complex. You have 2 > break > > statements, and 2 levels of nesting of ifs. Can this be simplified? > > > > A good ideal is to have, for each loop, one condition checked in one spot. > > Done. This still ends up having the loop termination in 2 spots. Can we move the checks into one spot? Maybe something like this? size_t bytes_left = bytes_to_consume; while (!queue_.empty() && bytes_left != 0) { ... and don't do any breaks in here? } Is that possible? http://codereview.chromium.org/155193/diff/1007/1008#newcode62 Line 62: while (bytes > 0) { On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > I wonder if a for loop makes more sense? > > > > Also, try to choose some more desciptive variable names. "index" is as > > descriptive as "i". > > > > Okay. Changed it to a for loop and renamed index "queueIndex". It's probably > cleaner. :-/ I should be more clear in my notes. What I meant was, if you're going to say something like "index" you might as well say "i". So, basically, either make it descriptive, or don't waste text. :) In this case, you could call it "i" since you're in a for-loop and it's just as meaningful IMO. http://codereview.chromium.org/155193/diff/1007/1008#newcode76 Line 76: if (index > queue_.size()) On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > Same comment regarding having multiple loop conditions being checked in > multiple > > spots. Extend that principle to include the sentinel variable. It's usually > > ideal to try and increment the sentinel in one, or maybe at most 2 spots. > > > > Does the for loop make this irrelevant? yep! http://codereview.chromium.org/155193/diff/1007/1008#newcode97 Line 97: // Since we keep track of the number of bytes, this is easier than calling On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > What if you get 0-lengthed buffers? > > Then we have no data... and the size should be zero. I renamed size_ to > size_in_bytes_. Right...but if someone enqueues a 0-lengthed buffer, then size_in_bytes_ == 0, but queue_() is not empty. You end up with a quasi-memory leak no? Maybe we should just not enqueue 0-lengthed buffers? http://codereview.chromium.org/155193/diff/1007/1009 File media/base/buffer_queue.h (right): http://codereview.chromium.org/155193/diff/1007/1009#newcode34 Line 34: // |buffer_in|. On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > Make sure you note that you take a reference on the buffer. > > Done. "adds a reference" is better phrased as "keeps a reference". http://codereview.chromium.org/155193/diff/1007/1009#newcode48 Line 48: std::deque< scoped_refptr<Buffer> > queue_; On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > std::dequeue< scope_ -> std::dequeue<scoped_ > > > > remove the leading space. You only need the trailing one to avoid the >> > > ambiguity. > > I believe the style guide says you should (or at least can) make it symmetrical? Okay...the rest of our code doesn't seem to do this though. I'm fine eitehrw ay. http://codereview.chromium.org/155193/diff/1007/1010 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1007/1010#newcode20 Line 20: // Tests ctor. On 2009/07/08 18:28:25, kylep wrote: > On 2009/07/08 02:44:17, awong wrote: > > Good set of tests! > > > > In general, it's better to create separate TEST or TEST_F for each specific > > case. It keeps the tests shorter, less prone to affect one another, and makes > > it look cool when you see a bunch of tests passed instead of just one big one. > > :) > > > > Also, when there's a failure, seeing BufferQueueTest_CtorInitialization failed > > is much more descriptive than just BufferQueueTest_Basic failed. > > > > LOoking at the code, maybe make one separate test for each comment taht says > // > > Tests ....? > > > > Might also look at TEST_F instead of TEST if there is any comment > > setup/teardown. > > > > Done. much nicer :) http://codereview.chromium.org/155193/diff/9/10 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/9/10#newcode26 Line 26: size_t front_remaining; Declare this inside the loop. You don't need this variable outside the loop, and its value isn't particularly meaning full either with that context either. http://codereview.chromium.org/155193/diff/9/12 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/9/12#newcode16 Line 16: ASSERT_TRUE(kNewDataSize > kDataSize); Ooh! Testing the validity of the test data! I like :) Usually, it's best to move this into its own test case. Setup should be for intializing things for each test. None of this code is required per test, but as it is written, it will be called once per test. http://codereview.chromium.org/155193/diff/9/12#newcode34 Line 34: BufferQueue queue; queue -> queue_ http://codereview.chromium.org/155193/diff/9/12#newcode36 Line 36: const char kData[] = "hello"; Erp? This compiles? Surprising. (1) These should be static (2) In c++, I don't think you're allowed to inline a definition of a variable _except_ for integer variants. Make these static and move all the static definitions out of the class?
Also, modified the description of the CL. Is that a legitimate way to announce the test? http://codereview.chromium.org/155193/diff/9/10 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/9/10#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. On 2009/07/08 18:43:30, scherkus wrote: > should be 2009 Done. http://codereview.chromium.org/155193/diff/9/10#newcode19 Line 19: void BufferQueue::Consume(size_t bytesToBeConsumed) { On 2009/07/08 18:43:30, scherkus wrote: > bytes_to_be_consumed Done. http://codereview.chromium.org/155193/diff/9/10#newcode26 Line 26: size_t front_remaining; On 2009/07/08 19:00:52, awong wrote: > Declare this inside the loop. You don't need this variable outside the loop, > and its value isn't particularly meaning full either with that context either. Done. http://codereview.chromium.org/155193/diff/9/10#newcode29 Line 29: // Calculate number of usable bytesToBeConsumed in the front of the |queue_|. On 2009/07/08 18:43:30, scherkus wrote: > 80 chars Done. This was a weird replace all error. http://codereview.chromium.org/155193/diff/9/10#newcode62 Line 62: // Modify counts and pointers On 2009/07/08 18:43:30, scherkus wrote: > period at end of comment Done. http://codereview.chromium.org/155193/diff/9/11 File media/base/buffer_queue.h (right): http://codereview.chromium.org/155193/diff/9/11#newcode30 Line 30: // Advances front pointer |bytesToBeConsumed| bytes and discards "consumed" On 2009/07/08 18:43:30, scherkus wrote: > bytes_to_be_consumed Done. http://codereview.chromium.org/155193/diff/9/11#newcode32 Line 32: void Consume(size_t bytesToBeConsumed); On 2009/07/08 18:43:30, scherkus wrote: > bytes_to_be_consumed Done. http://codereview.chromium.org/155193/diff/9/12 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/9/12#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. On 2009/07/08 18:43:30, scherkus wrote: > should be 2009 Done. http://codereview.chromium.org/155193/diff/9/12#newcode10 Line 10: using media::BufferQueue; On 2009/07/08 18:43:30, scherkus wrote: > nowadays we seem to place our unit tests inside the corressponding namespace > > so here you can wrap the whole thing inside the media namespace and drop the > using statements Done. http://codereview.chromium.org/155193/diff/9/12#newcode16 Line 16: ASSERT_TRUE(kNewDataSize > kDataSize); On 2009/07/08 19:00:52, awong wrote: > Ooh! Testing the validity of the test data! I like :) > > Usually, it's best to move this into its own test case. Setup should be for > intializing things for each test. None of this code is required per test, but as > it is written, it will be called once per test. Done. http://codereview.chromium.org/155193/diff/9/12#newcode34 Line 34: BufferQueue queue; On 2009/07/08 19:00:52, awong wrote: > queue -> queue_ Done. http://codereview.chromium.org/155193/diff/9/12#newcode36 Line 36: const char kData[] = "hello"; On 2009/07/08 19:00:52, awong wrote: > Erp? This compiles? Surprising. > > (1) These should be static > (2) In c++, I don't think you're allowed to inline a definition of a variable > _except_ for integer variants. Make these static and move all the static > definitions out of the class? No. This was all crap. I fixed it. http://codereview.chromium.org/155193/diff/9/12#newcode40 Line 40: }; On 2009/07/08 18:43:30, scherkus wrote: > private: > DISALLOW_COPY_AND_ASSIGN(BufferQueueTest); Done. http://codereview.chromium.org/155193/diff/9/12#newcode43 Line 43: TEST_F(BufferQueueTest, TestCtor) { On 2009/07/08 18:43:30, scherkus wrote: > nit: the "Test" prefix is redundant > > when working from these tests from the command line they'll look like: > BufferQueueTest.TestCtor Done.
near-lgtm, but some nits also awong will probably have a look http://codereview.chromium.org/155193/diff/1037/1038 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1037/1038#newcode55 Line 55: // case for front due to data_offset_. data_offset_ -> |data_offset_| http://codereview.chromium.org/155193/diff/1037/1040 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1037/1040#newcode16 Line 16: : kDataSize(0), indent by 2 spaces http://codereview.chromium.org/155193/diff/1037/1040#newcode21 Line 21: kData = "hello"; since you have a constructor, you can get rid of SetUp() and move this code into the constructor http://codereview.chromium.org/155193/diff/1037/1040#newcode25 Line 25: kNewDataSize = kNewData.size() + 1; also I'm pretty sure you can do this stuff as static constants class Foo { static const char kData[]; static const size_t kDataSize; }; const char Foo::kData[] = "hello"; const size_t Foo::kDataSize = arraysize(Foo::kData); // includes the null char
Almost there! 3 more comments. http://codereview.chromium.org/155193/diff/1037/1038 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1037/1038#newcode23 Line 23: // Perform changes on |size_| here to save operations. "to save operations" doesn't completely explain the reasoning. You should say explain why it is semantically correct...basically because you guaranteed earier that you will be able to consume x bytse, you can just decrement the counter...though hopefully phrased nicer. http://codereview.chromium.org/155193/diff/1037/1038#newcode72 Line 72: dest += current_remaining; You could remove one mutation by not modifying dest and instead doing memcpy(dest + copeid, current, current_remaining); It's nicer not to not modified passed in parameters anyways. http://codereview.chromium.org/155193/diff/1037/1040 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1037/1040#newcode43 Line 43: size_t kDataSize; If you're actually using a std::string, you don't need kDatasize(). The string already has it.
Why doesn't gcl upload run a full gcl lint? http://codereview.chromium.org/155193/diff/1037/1038 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/1037/1038#newcode23 Line 23: // Perform changes on |size_| here to save operations. On 2009/07/08 22:36:51, awong wrote: > "to save operations" doesn't completely explain the reasoning. > > You should say explain why it is semantically correct...basically because you > guaranteed earier that you will be able to consume x bytse, you can just > decrement the counter...though hopefully phrased nicer. Done. http://codereview.chromium.org/155193/diff/1037/1038#newcode55 Line 55: // case for front due to data_offset_. On 2009/07/08 22:12:00, scherkus wrote: > data_offset_ -> |data_offset_| Done. http://codereview.chromium.org/155193/diff/1037/1038#newcode72 Line 72: dest += current_remaining; On 2009/07/08 22:36:51, awong wrote: > You could remove one mutation by not modifying dest and instead doing > > memcpy(dest + copeid, current, current_remaining); > > It's nicer not to not modified passed in parameters anyways. Done. http://codereview.chromium.org/155193/diff/1037/1040 File media/base/buffer_queue_unittest.cc (right): http://codereview.chromium.org/155193/diff/1037/1040#newcode16 Line 16: : kDataSize(0), On 2009/07/08 22:12:00, scherkus wrote: > indent by 2 spaces Done. http://codereview.chromium.org/155193/diff/1037/1040#newcode21 Line 21: kData = "hello"; On 2009/07/08 22:12:00, scherkus wrote: > since you have a constructor, you can get rid of SetUp() and move this code into > the constructor Done. http://codereview.chromium.org/155193/diff/1037/1040#newcode25 Line 25: kNewDataSize = kNewData.size() + 1; On 2009/07/08 22:12:00, scherkus wrote: > also I'm pretty sure you can do this stuff as static constants > > class Foo { > static const char kData[]; > static const size_t kDataSize; > }; > > const char Foo::kData[] = "hello"; > const size_t Foo::kDataSize = arraysize(Foo::kData); // includes the null char Done. C++ and char[] always make me angry. I can never get the syntax correct.
LGTM 2 more commentse, but I don't need to rereview after you address them. http://codereview.chromium.org/155193/diff/38/1050 File media/base/buffer_queue.cc (right): http://codereview.chromium.org/155193/diff/38/1050#newcode50 Line 50: size_t current_remaining; Initialize these all to a known value even though they get overwritten, just as a way to provide defined behavior incase someone introduces a bug later. http://codereview.chromium.org/155193/diff/38/1050#newcode58 Line 58: current_remaining = queue_.front()->GetDataSize() - data_offset_; BTw, if you want to remove this if statement, you could just start with the i = 1, and do the specialized logic outside of the loop.
LGTM -- fix up awongs stuff and submit! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
