Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
11 years, 7 months ago by Alpha Left Google Modified:
9 years, 6 months ago Reviewers:
awong, scherkus (not reviewing) CC:
chromium-reviews_googlegroups.com, awong, fbarchard Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSeekableBuffer to implement a window of buffer which has short seeking ability.
Defined the base interface SeekableBuffer and a thread safe implemntation of
ThreadSafeSeekableBuffer. This class is to be used for media resource loading
that does the meat of buffer queueing and handles short seeking for short distance
out-of-order read patterns.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=16348
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 74
Patch Set 7 : '' #
Total comments: 20
Patch Set 8 : '' #
Total comments: 8
Patch Set 9 : '' #
Total comments: 59
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 14
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 6
Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #
Created: 11 years, 7 months ago
Messages
Total messages: 18 (0 generated)
Lets talk about the design of this today a bit more before you write too much code :)
I have add the implementation. There's unit tests for read, write and short seek. But I don't have a test that really does producer and consumer in 2 threads..
Couple of architectural concerns. In general, I think we're taking the locks at too fine grained a level. Have we actually benchmarked a performance degredation if we use the lock like a monitor into the class? If not, I suggest we just lock at the entry to the public methods, and forget about trying to release the locks between short operations. The one exception that may make sense is to delay memcpys, but iterating over the buffer queue, etc., should just be done under one lock acquisition unless we're expecting high contention. Also, in this threadsafe kind of API, fill-level query function can at best be advisory. Another possible solution is to drop locking all-together and tell clients they must provide external synchronization (similar to STL containers). I thin I'm most in favor of the last solution if possible. http://codereview.chromium.org/113213/diff/16/1006 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/16/1006#newcode8 Line 8: #include "media/base/seekable_buffer.h" include the header for the current file first, always. http://codereview.chromium.org/113213/diff/16/1006#newcode20 Line 20: ThreadSafeSeekableBuffer::~ThreadSafeSeekableBuffer() { Lock? http://codereview.chromium.org/113213/diff/16/1006#newcode21 Line 21: while (!backward_queue_.empty()) { There should be an stl_util-inl.h that you can use. STLDeleteElements I think http://codereview.chromium.org/113213/diff/16/1006#newcode34 Line 34: return InternalRead(data, size); Usually, it's a bit cleaner to make the internal functions assume locks, so the synchronization logic is directly seeable in the public API. Change InternalRead to InternalRead_Locked, assert the lcok is held, and move the lock grab here? Also saves one lock acquisition in the SeekForward function. http://codereview.chromium.org/113213/diff/16/1006#newcode39 Line 39: AutoLock auto_lock(lock_); move lock acquisition until after the memcpy. http://codereview.chromium.org/113213/diff/16/1006#newcode51 Line 51: AutoLock auto_lock(lock_); So this API doesn't allow people to seek past foward_bytes_? This is okay as long as only one thread consumes buffers. Otherwise, there is no deterministically valid way to use the API because of the lock release between the query and the seek. How about making the seek function return the amount read instead of true/false and telling the caller that they must check? http://codereview.chromium.org/113213/diff/16/1006#newcode66 Line 66: // May be we can just reset the counter in forward queue. I wonder if you'd be better off using an stl list instead of a deque. Then you could keep an iterator to refer to the current position in the list. Unlike deque, stl list guarantees the validity of iterators across insert/erase as long as you don't erase the current iterator. http://codereview.chromium.org/113213/diff/16/1006#newcode78 Line 78: AutoLock auto_lock(lock_); If you just hold the lock over all off SeekBackwards, I think you can the danger on read + seek intermingling. http://codereview.chromium.org/113213/diff/16/1006#newcode124 Line 124: void ThreadSafeSeekableBuffer::SetBackwardCapacity(size_t size) { This function is also not safe to call with seeks. Should be noted in comments, unless we make Seek hold the lock through all its operations. http://codereview.chromium.org/113213/diff/16/1006#newcode151 Line 151: memcpy(data + taken, buffer->data.get() + buffer->taken, copied); If we're allowing ourselves to memcpy inside a lock, why bother releasing the lock between loop iterations? http://codereview.chromium.org/113213/diff/16/1004 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/16/1004#newcode34 Line 34: virtual size_t Read(uint8* buffer, size_t size) = 0; Don't let the comments run into the previous declaration. Add a newline, or collapse the two comments. Same with other spots. Also, the delcaration order of the arguments is backwards. Output parameters are suppsoed to go after input parameters. http://codereview.chromium.org/113213/diff/16/1004#newcode39 Line 39: // Move the read position forward by |size| bytes. When will this return false? http://codereview.chromium.org/113213/diff/16/1004#newcode52 Line 52: // Sets the capacity for forward buffering. If you lower the capacity, what happens? do you drop the data? Do we really need to be able to change capacity? Seems like a strange feature. http://codereview.chromium.org/113213/diff/16/1004#newcode60 Line 60: virtual ~SeekableBuffer() {} Destructor should go at the top. Also, don't inlinet he destructor definition. http://codereview.chromium.org/113213/diff/16/1004#newcode64 Line 64: // Simultaneous read and write operations is allowed, write and seek operations The word "operations" is plural. is -> are allowed, write -> allowed. Write http://codereview.chromium.org/113213/diff/16/1004#newcode65 Line 65: // are also safe to happen at the same time, but there cannot be any "safe to happen at the same time" doesn't quite parse. Do you mean people can call write and seek in parallel? And why can't we make it safe for read/seek to be called in parallel? I think you just need to serialize both calls. It's dangerous, IMO, to add extra concurrency restrictions in the concrete class level and not expose them at the interface level. Also, why bother with an interface class? Do we really expect to have multiple implementations of SeekableBuffer? Maybe collapse the two and just refactor when we need it. Or is it common practice in chromium to preemptive create an interface? http://codereview.chromium.org/113213/diff/16/1004#newcode70 Line 70: // and backward capacity of |backward_capacity|. The values are in number "The values are in bytes" http://codereview.chromium.org/113213/diff/16/1004#newcode93 Line 93: // This method called with |lock_| acquired. Drop the lock_ acquired comment. Name the method with a _Locked suffix, and then AssertAcquired() in the function.
http://codereview.chromium.org/113213/diff/16/1006 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/16/1006#newcode13 Line 13: size_t backward_capacity, size_t forward_capacity) nit: wrap at parentheses ThreadSafeSeekableBuffer::ThreadSafeSeekableBuffer(size_t backward_capacity, size_t forward_capacity) http://codereview.chromium.org/113213/diff/16/1006#newcode44 Line 44: if (forward_bytes_ >= forward_capacity_) what happens now? you've overshot your capacity, however there's nothing stopping me from calling Write over and over and over again relating to my comment in the header, it looks like you actually allow expanding forward capacity so perhaps it's not an error after all :) http://codereview.chromium.org/113213/diff/16/1006#newcode56 Line 56: DCHECK(taken == size); DCHECK_EQ http://codereview.chromium.org/113213/diff/16/1006#newcode74 Line 74: DCHECK(backward_bytes_ >= 0); DCHECK_GE http://codereview.chromium.org/113213/diff/16/1006#newcode83 Line 83: DCHECK(buffer->taken == 0); DCHECK_EQ http://codereview.chromium.org/113213/diff/16/1006#newcode89 Line 89: DCHECK(buffer->taken < buffer->size); DCHECK_LE http://codereview.chromium.org/113213/diff/16/1006#newcode93 Line 93: DCHECK(backward_bytes_ >= 0); DCHECK_GE http://codereview.chromium.org/113213/diff/16/1006#newcode95 Line 95: DCHECK(taken == size); DCHECK_EQ http://codereview.chromium.org/113213/diff/16/1006#newcode136 Line 136: DCHECK(backward_bytes_ >= 0); DCHECK_GE http://codereview.chromium.org/113213/diff/16/1006#newcode150 Line 150: if (data) I'd add a quick comment here reminding us that NULL data means we're seeking forward http://codereview.chromium.org/113213/diff/16/1006#newcode156 Line 156: DCHECK(forward_bytes_ >= 0); DCHECK_GE http://codereview.chromium.org/113213/diff/16/1004 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/16/1004#newcode26 Line 26: // Types of write error. Instead of "Types of write error." how about a quick comment saying where/when they are used? http://codereview.chromium.org/113213/diff/16/1004#newcode37 Line 37: virtual WriteError Write(const uint8* data, size_t size) = 0; I'd consider renaming this Append since write usually implies you're writing at the current position. What are the semantics for appending? If there's only room for 4 bytes and I write 8, do 4 bytes get appended or does nothing get appended? What if appending always works (i.e., forward capacity is expanded to satisfy the request) http://codereview.chromium.org/113213/diff/16/1004#newcode42 Line 42: virtual bool SeekBackward(size_t size) = 0; What about having a single seek method with an integer offset (negative for backwards, positive for forwards). What does it mean if false is returned? Is the position advanced to the very beginning/end, or does it remain in the same location as before? http://codereview.chromium.org/113213/diff/16/1004#newcode58 Line 58: virtual void SetBackwardCapacity(size_t size) = 0; As we discussed yesterday I don't think these methods are needed. If you feel they are, then explain the semantics of changing the capacity (is all data erased? what about if I shrink the capacity? etc) http://codereview.chromium.org/113213/diff/16/1004#newcode60 Line 60: virtual ~SeekableBuffer() {} Instead of splitting everything it into Forward/Backward, would it make more sense to expose a total size + offset? Forward/Backward hints at the implementation (two queues) http://codereview.chromium.org/113213/diff/16/1004#newcode96 Line 96: // An internal read method that is shared by Read and SeekForward. |data| is Read -> Read() SeekForward -> SeekForward() http://codereview.chromium.org/113213/diff/16/1004#newcode108 Line 108: size_t taken; I think "offset" is a more appropriate variable name Also, I'm concerned about the amount of bookkeeping required (every Buffer has an offset). If you only have a single offset, then it might make more sense moving the offset value into the class itself and keeping track of which Buffer is the current buffer (isn't it always forward_queue_.front()?) http://codereview.chromium.org/113213/diff/16/1005 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/16/1005#newcode10 Line 10: namespace media { generally we don't put tests inside our namespace (leave them global) However since you're using a test fixture (SeekableBufferTest) you'll have to wrap your entire test in an anonymous namespace http://codereview.chromium.org/113213/diff/16/1005#newcode50 Line 50: while (read_position < kDataSize) { if you're reading and writing a random amount of data then this test has a potentially unbounded running time (it's possible to always write more than you read) also with your implementation of Write() this test could write as much data as it wanted and you'd allow it could make this more deterministic (i.e., 1000 iterations) http://codereview.chromium.org/113213/diff/16/1005#newcode179 Line 179: EXPECT_GE(read_size, bytes_read); you'll have a stronger test if you check GetForwardBytes() before reading and then assert that: bytes_read == read_size when read_size <= GetForwardBytes() bytes_read == GetForwardBytes() when read_size > GetForwardBytes() http://codereview.chromium.org/113213/diff/16/1005#newcode186 Line 186: EXPECT_EQ(0, buffer_->GetForwardBytes()); sanity check: try and read again and expect 0 bytes http://codereview.chromium.org/113213/diff/16/1005#newcode189 Line 189: } // namespace media
http://codereview.chromium.org/113213/diff/16/1005 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/16/1005#newcode50 Line 50: while (read_position < kDataSize) { On 2009/05/14 21:25:00, scherkus wrote: > if you're reading and writing a random amount of data then this test has a > potentially unbounded running time (it's possible to always write more than you > read) > > also with your implementation of Write() this test could write as much data as > it wanted and you'd allow it > > could make this more deterministic (i.e., 1000 iterations) > Sorry I misinterpreted this. Yes it's possible to write a ton of data, but your reads always increment until kDataSize (for some reason I thought were reading until you exhaust the buffer size and GetForwardBytes() returns 0).
Big changes: 1. Got rid of lock, user of this class should lock 2. Use std::list, kind of easier to maintain... there's still boundary case to handle std::list:end() 3. No more capacity setting 4. One more unit test that checks boundary case for std::list::end for read and seekbackward http://codereview.chromium.org/113213/diff/16/1006 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/16/1006#newcode8 Line 8: #include "media/base/seekable_buffer.h" On 2009/05/14 19:04:48, awong wrote: > include the header for the current file first, always. Done. http://codereview.chromium.org/113213/diff/16/1006#newcode13 Line 13: size_t backward_capacity, size_t forward_capacity) On 2009/05/14 21:25:00, scherkus wrote: > nit: wrap at parentheses > ThreadSafeSeekableBuffer::ThreadSafeSeekableBuffer(size_t backward_capacity, > size_t forward_capacity) Done. http://codereview.chromium.org/113213/diff/16/1006#newcode20 Line 20: ThreadSafeSeekableBuffer::~ThreadSafeSeekableBuffer() { On 2009/05/14 19:04:48, awong wrote: > Lock? Done. http://codereview.chromium.org/113213/diff/16/1006#newcode21 Line 21: while (!backward_queue_.empty()) { On 2009/05/14 19:04:48, awong wrote: > There should be an stl_util-inl.h that you can use. STLDeleteElements I think it's defined in chrome/common which i can't include. http://codereview.chromium.org/113213/diff/16/1006#newcode34 Line 34: return InternalRead(data, size); On 2009/05/14 19:04:48, awong wrote: > Usually, it's a bit cleaner to make the internal functions assume locks, so the > synchronization logic is directly seeable in the public API. > > Change InternalRead to InternalRead_Locked, assert the lcok is held, and move > the lock grab here? > > Also saves one lock acquisition in the SeekForward function. Done. http://codereview.chromium.org/113213/diff/16/1006#newcode56 Line 56: DCHECK(taken == size); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/113213/diff/16/1006#newcode74 Line 74: DCHECK(backward_bytes_ >= 0); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_GE Done. http://codereview.chromium.org/113213/diff/16/1006#newcode83 Line 83: DCHECK(buffer->taken == 0); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/113213/diff/16/1006#newcode89 Line 89: DCHECK(buffer->taken < buffer->size); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_LE Done. http://codereview.chromium.org/113213/diff/16/1006#newcode93 Line 93: DCHECK(backward_bytes_ >= 0); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_GE Done. http://codereview.chromium.org/113213/diff/16/1006#newcode95 Line 95: DCHECK(taken == size); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/113213/diff/16/1006#newcode150 Line 150: if (data) On 2009/05/14 21:25:00, scherkus wrote: > I'd add a quick comment here reminding us that NULL data means we're seeking > forward Done. http://codereview.chromium.org/113213/diff/16/1006#newcode151 Line 151: memcpy(data + taken, buffer->data.get() + buffer->taken, copied); On 2009/05/14 19:04:48, awong wrote: > If we're allowing ourselves to memcpy inside a lock, why bother releasing the > lock between loop iterations? Done. http://codereview.chromium.org/113213/diff/16/1006#newcode156 Line 156: DCHECK(forward_bytes_ >= 0); On 2009/05/14 21:25:00, scherkus wrote: > DCHECK_GE Done. http://codereview.chromium.org/113213/diff/16/1004 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/16/1004#newcode26 Line 26: // Types of write error. On 2009/05/14 21:25:00, scherkus wrote: > Instead of "Types of write error." how about a quick comment saying where/when > they are used? Done. http://codereview.chromium.org/113213/diff/16/1004#newcode34 Line 34: virtual size_t Read(uint8* buffer, size_t size) = 0; On 2009/05/14 19:04:48, awong wrote: > Don't let the comments run into the previous declaration. Add a newline, or > collapse the two comments. Same with other spots. > > Also, the delcaration order of the arguments is backwards. Output parameters > are suppsoed to go after input parameters. Done. http://codereview.chromium.org/113213/diff/16/1004#newcode37 Line 37: virtual WriteError Write(const uint8* data, size_t size) = 0; On 2009/05/14 21:25:00, scherkus wrote: > I'd consider renaming this Append since write usually implies you're writing at > the current position. > > What are the semantics for appending? If there's only room for 4 bytes and I > write 8, do 4 bytes get appended or does nothing get appended? What if > appending always works (i.e., forward capacity is expanded to satisfy the > request) > > Done. http://codereview.chromium.org/113213/diff/16/1004#newcode39 Line 39: // Move the read position forward by |size| bytes. On 2009/05/14 19:04:48, awong wrote: > When will this return false? Done. http://codereview.chromium.org/113213/diff/16/1004#newcode42 Line 42: virtual bool SeekBackward(size_t size) = 0; On 2009/05/14 21:25:00, scherkus wrote: > What about having a single seek method with an integer offset (negative for > backwards, positive for forwards). > > What does it mean if false is returned? Is the position advanced to the very > beginning/end, or does it remain in the same location as before? Done. http://codereview.chromium.org/113213/diff/16/1004#newcode52 Line 52: // Sets the capacity for forward buffering. On 2009/05/14 19:04:48, awong wrote: > If you lower the capacity, what happens? do you drop the data? > > Do we really need to be able to change capacity? Seems like a strange feature. The idea of changing capacity is discarded. http://codereview.chromium.org/113213/diff/16/1004#newcode58 Line 58: virtual void SetBackwardCapacity(size_t size) = 0; On 2009/05/14 21:25:00, scherkus wrote: > As we discussed yesterday I don't think these methods are needed. If you feel > they are, then explain the semantics of changing the capacity (is all data > erased? what about if I shrink the capacity? etc) Done. http://codereview.chromium.org/113213/diff/16/1004#newcode60 Line 60: virtual ~SeekableBuffer() {} On 2009/05/14 19:04:48, awong wrote: > Destructor should go at the top. Also, don't inlinet he destructor definition. Done. http://codereview.chromium.org/113213/diff/16/1004#newcode64 Line 64: // Simultaneous read and write operations is allowed, write and seek operations On 2009/05/14 19:04:48, awong wrote: > The word "operations" is plural. is -> are > > allowed, write -> allowed. Write > removed. http://codereview.chromium.org/113213/diff/16/1004#newcode65 Line 65: // are also safe to happen at the same time, but there cannot be any On 2009/05/14 19:04:48, awong wrote: > "safe to happen at the same time" doesn't quite parse. > > Do you mean people can call write and seek in parallel? > > And why can't we make it safe for read/seek to be called in parallel? I think > you just need to serialize both calls. > > It's dangerous, IMO, to add extra concurrency restrictions in the concrete class > level and not expose them at the interface level. > > Also, why bother with an interface class? Do we really expect to have multiple > implementations of SeekableBuffer? Maybe collapse the two and just refactor > when we need it. Or is it common practice in chromium to preemptive create an > interface? Done. http://codereview.chromium.org/113213/diff/16/1004#newcode70 Line 70: // and backward capacity of |backward_capacity|. The values are in number On 2009/05/14 19:04:48, awong wrote: > "The values are in bytes" Done. http://codereview.chromium.org/113213/diff/16/1004#newcode93 Line 93: // This method called with |lock_| acquired. On 2009/05/14 19:04:48, awong wrote: > Drop the lock_ acquired comment. Name the method with a _Locked suffix, and > then AssertAcquired() in the function. Done. http://codereview.chromium.org/113213/diff/16/1004#newcode96 Line 96: // An internal read method that is shared by Read and SeekForward. |data| is On 2009/05/14 21:25:00, scherkus wrote: > Read -> Read() > SeekForward -> SeekForward() Done. http://codereview.chromium.org/113213/diff/16/1004#newcode108 Line 108: size_t taken; On 2009/05/14 21:25:00, scherkus wrote: > I think "offset" is a more appropriate variable name > > Also, I'm concerned about the amount of bookkeeping required (every Buffer has > an offset). If you only have a single offset, then it might make more sense > moving the offset value into the class itself and keeping track of which Buffer > is the current buffer (isn't it always forward_queue_.front()?) Done. http://codereview.chromium.org/113213/diff/16/1005 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/16/1005#newcode10 Line 10: namespace media { On 2009/05/14 21:25:00, scherkus wrote: > generally we don't put tests inside our namespace (leave them global) > > However since you're using a test fixture (SeekableBufferTest) you'll have to > wrap your entire test in an anonymous namespace Done. http://codereview.chromium.org/113213/diff/16/1005#newcode179 Line 179: EXPECT_GE(read_size, bytes_read); On 2009/05/14 21:25:00, scherkus wrote: > you'll have a stronger test if you check GetForwardBytes() before reading and > then assert that: > bytes_read == read_size when read_size <= GetForwardBytes() > bytes_read == GetForwardBytes() when read_size > GetForwardBytes() > Done. http://codereview.chromium.org/113213/diff/16/1005#newcode186 Line 186: EXPECT_EQ(0, buffer_->GetForwardBytes()); On 2009/05/14 21:25:00, scherkus wrote: > sanity check: try and read again and expect 0 bytes Done. http://codereview.chromium.org/113213/diff/16/1005#newcode189 Line 189: } On 2009/05/14 21:25:00, scherkus wrote: > // namespace media Done.
looking good! http://codereview.chromium.org/113213/diff/23/1012 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/23/1012#newcode24 Line 24: while (!buffers_.empty()) { actually stl_util-inl.h was moved to base so you can use it now http://codereview.chromium.org/113213/diff/23/1012#newcode32 Line 32: return InternalRead(size, data); if data is NULL people will end up seeking.. is this expected? maybe a DCHECK or return 0 http://codereview.chromium.org/113213/diff/23/1012#newcode42 Line 42: return SeekableBuffer::OK; again, may want to consider a bool here if you don't forsee addition result codes http://codereview.chromium.org/113213/diff/23/1012#newcode77 Line 77: // The buffer pointed by current iterator has been consumed, moves backward. moves -> move http://codereview.chromium.org/113213/diff/23/1010 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/23/1010#newcode6 Line 6: // reading a media data source. I'd mention that it must be synchronized externally. http://codereview.chromium.org/113213/diff/23/1010#newcode19 Line 19: // Implementation of a seekable buffer, data is appended to this buffer by I'd probably merge this comment with the one at the top of the file http://codereview.chromium.org/113213/diff/23/1010#newcode27 Line 27: enum Error { nit: seeing as Append() is always successful now, would it make more sense to instead return a bool indicating whether you should call Append() more? http://codereview.chromium.org/113213/diff/23/1010#newcode101 Line 101: BufferQueue::iterator iterator_; iterator_ isn't a terribly descriptive name when this is actually your current buffer, right? http://codereview.chromium.org/113213/diff/23/1010#newcode103 Line 103: size_t current_buffer_taken_; nit: again "taken" is a strange choice of wording... offset seems to make more sense -- to me, at least :) http://codereview.chromium.org/113213/diff/23/1011 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/23/1011#newcode231 Line 231: I'd add a test that never writes anything but tests the essential operations: - Read(0, ptr) - Read(1, ptr) - Read(0, NULL) - Read(1, NULL) - Seek(-1) - Seek(0) - Seek(1) - Forward/backward accessors You'll cover all the strange cases because iterator_ will be equal to both begin() and end()
http://codereview.chromium.org/113213/diff/23/1012 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/23/1012#newcode24 Line 24: while (!buffers_.empty()) { On 2009/05/15 01:00:45, scherkus wrote: > actually stl_util-inl.h was moved to base so you can use it now Done. http://codereview.chromium.org/113213/diff/23/1012#newcode32 Line 32: return InternalRead(size, data); On 2009/05/15 01:00:45, scherkus wrote: > if data is NULL people will end up seeking.. is this expected? > > maybe a DCHECK or return 0 Done. http://codereview.chromium.org/113213/diff/23/1012#newcode42 Line 42: return SeekableBuffer::OK; On 2009/05/15 01:00:45, scherkus wrote: > again, may want to consider a bool here if you don't forsee addition result > codes Done. http://codereview.chromium.org/113213/diff/23/1012#newcode77 Line 77: // The buffer pointed by current iterator has been consumed, moves backward. On 2009/05/15 01:00:45, scherkus wrote: > moves -> move Done. http://codereview.chromium.org/113213/diff/23/1010 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/23/1010#newcode6 Line 6: // reading a media data source. On 2009/05/15 01:00:45, scherkus wrote: > I'd mention that it must be synchronized externally. Done. http://codereview.chromium.org/113213/diff/23/1010#newcode19 Line 19: // Implementation of a seekable buffer, data is appended to this buffer by On 2009/05/15 01:00:45, scherkus wrote: > I'd probably merge this comment with the one at the top of the file Done. http://codereview.chromium.org/113213/diff/23/1010#newcode27 Line 27: enum Error { On 2009/05/15 01:00:45, scherkus wrote: > nit: seeing as Append() is always successful now, would it make more sense to > instead return a bool indicating whether you should call Append() more? Done. http://codereview.chromium.org/113213/diff/23/1010#newcode101 Line 101: BufferQueue::iterator iterator_; On 2009/05/15 01:00:45, scherkus wrote: > iterator_ isn't a terribly descriptive name when this is actually your current > buffer, right? Done. http://codereview.chromium.org/113213/diff/23/1010#newcode103 Line 103: size_t current_buffer_taken_; On 2009/05/15 01:00:45, scherkus wrote: > nit: again "taken" is a strange choice of wording... offset seems to make more > sense -- to me, at least :) Done. I'm sure I like the movie "taken" a lot :P http://codereview.chromium.org/113213/diff/23/1011 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/23/1011#newcode231 Line 231: On 2009/05/15 01:00:45, scherkus wrote: > I'd add a test that never writes anything but tests the essential operations: > - Read(0, ptr) > - Read(1, ptr) > - Read(0, NULL) > - Read(1, NULL) > - Seek(-1) > - Seek(0) > - Seek(1) > - Forward/backward accessors > > You'll cover all the strange cases because iterator_ will be equal to both > begin() and end() I added a simple one as suggested. I can provide more tests if needed.
LGTM, just fix up the comment nits if you do end up moving EvictBackwardBuffer() make to run your unit tests!! http://codereview.chromium.org/113213/diff/29/1017 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/29/1017#newcode135 Line 135: EvictBackwardBuffers(); Can you move this outside the while loop? It seems to me it'd be more efficient to advance your read position a bunch, then call EvictBackwardBuffers() once to free everything else. O(n^2) vs. O(n) sort of thing. Also the comment isn't really explaining much ;) You can either remove it, or explain why you're calling it such as "Free backward buffers as a result of advancing |current_buffer_|." http://codereview.chromium.org/113213/diff/29/1015 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/29/1015#newcode10 Line 10: // This class provides no locking semantic, user of this class should serialize nit: semantic -> semantics, user -> users http://codereview.chromium.org/113213/diff/29/1015#newcode47 Line 47: // can still write into this buffer. "OK means user can still write into this buffer." I don't think we need this comment anymore (you sort of covered this point already) http://codereview.chromium.org/113213/diff/29/1015#newcode71 Line 71: // A helper method to keep backward queue until the required capacity. I don't quite understand what you mean here... maybe something like: "A helper method to free buffers until we're within the backwards capacity."
http://codereview.chromium.org/113213/diff/29/1017 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/29/1017#newcode135 Line 135: EvictBackwardBuffers(); On 2009/05/16 00:14:23, scherkus wrote: > Can you move this outside the while loop? It seems to me it'd be more efficient > to advance your read position a bunch, then call EvictBackwardBuffers() once to > free everything else. O(n^2) vs. O(n) sort of thing. > > Also the comment isn't really explaining much ;) > You can either remove it, or explain why you're calling it such as "Free > backward buffers as a result of advancing |current_buffer_|." doh! since this class within a giant lock I should move this method outside the loop. Although it will still be O(n) (since we don't iterate and remove O(n^2) elements), it is nice to move it out of the loop. The only reason it's in the loop was that I coded this thinking of multithread use and now should change it.. http://codereview.chromium.org/113213/diff/29/1015 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/29/1015#newcode10 Line 10: // This class provides no locking semantic, user of this class should serialize On 2009/05/16 00:14:23, scherkus wrote: > nit: semantic -> semantics, user -> users Done. http://codereview.chromium.org/113213/diff/29/1015#newcode47 Line 47: // can still write into this buffer. On 2009/05/16 00:14:23, scherkus wrote: > "OK means user can still write into this buffer." > > I don't think we need this comment anymore (you sort of covered this point > already) Done. http://codereview.chromium.org/113213/diff/29/1015#newcode71 Line 71: // A helper method to keep backward queue until the required capacity. On 2009/05/16 00:14:23, scherkus wrote: > I don't quite understand what you mean here... maybe something like: > "A helper method to free buffers until we're within the backwards capacity." Done.
http://codereview.chromium.org/113213/diff/1022/1025 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/1022/1025#newcode7 Line 7: #include "media/base/seekable_buffer.h" self-include always goes first. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/113213/diff/1022/1025#newcode25 Line 25: STLDeleteElements<BufferQueue>(&buffers_); The <BufferQueue> is unecessary. STLDeleteElements is a template function where all template types are expressed in the argument list, meaning that type-inference works. http://codereview.chromium.org/113213/diff/1022/1025#newcode38 Line 38: if (forward_bytes_ >= forward_capacity_) This is strange. You allow overrun of the buffer capacity? The semantics seem odd. I would expect you to fail the write and non enqueue the buffer on false. http://codereview.chromium.org/113213/diff/1022/1025#newcode52 Line 52: if (size > forward_bytes_) My head isn't really in a good shape to check boundary conditions. Is size == forward_bytes_ okay? Is there a untitest checking this (will try to see for myself in a bit) http://codereview.chromium.org/113213/diff/1022/1025#newcode55 Line 55: DCHECK_EQ(taken, size); why not just return the offset moved in all 3 Seek interfaces? Converting to a boolean just loses information without any real gain I think. http://codereview.chromium.org/113213/diff/1022/1025#newcode63 Line 63: size_t taken = 0; Implemneation comment explaining the loop logic. I shouldn't have to think while reading code. :) http://codereview.chromium.org/113213/diff/1022/1025#newcode65 Line 65: if (current_buffer_ == buffers_.end()) So...if we've played to the very end of the buffer, can we no longer seek backwards? Should this be current_buffer == buffers_.begin() && current_buffer_offset_ == 0? http://codereview.chromium.org/113213/diff/1022/1025#newcode69 Line 69: current_buffer_offset_ -= consumed; yeah...I'm not going to work thtrough this in my head. :) You need a comment so us readers don't need to think. http://codereview.chromium.org/113213/diff/1022/1025#newcode118 Line 118: forward_bytes_ -= copied; non-trivial bookeeping! Comment! http://codereview.chromium.org/113213/diff/1022/1023 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/1022/1023#newcode6 Line 6: // reading a media data source. By reading from it, the read position moves By reading -> When reading http://codereview.chromium.org/113213/diff/1022/1023#newcode10 Line 10: // This class provides no locking semantics, users of this class should "This class provides no locking semantics" is not very specific. It does have locking semantics. The semantic is "you need to do it yourself." :) Instead say "This class is not inherently threadsafe. Concurrent access must be externally serialized." http://codereview.chromium.org/113213/diff/1022/1023#newcode29 Line 29: // greater than |forward_capacity|, this value is used to report a buffer This is a run-on sentence. It should be 3 sentences. I also don't understand what how to use the "forward_bytes()" to report a buffer-full situation. This comment feels more like a class-level comment. Can we shuffle the information a bit so there is a large comment at the top that explains how this class is used, what the different values do, and maybe even give an example of common expected usages? Then the comment for the constructor can just focus on explaning what hte constructor will do rather than also explaining how the class works. http://codereview.chromium.org/113213/diff/1022/1023#newcode33 Line 33: // position that exceeds |backward_capacity| are evicted automatically. Are there any guarantees on when eviction occurs? Can we request eviction? Describe in class comment. http://codereview.chromium.org/113213/diff/1022/1023#newcode38 Line 38: // Read into |buffer| with the maximum size of |size| from the current "Read a maximum of |size| bytes into buffer" is more clear wording. http://codereview.chromium.org/113213/diff/1022/1023#newcode42 Line 42: // Append |data| with |size| bytes into this buffer. If this buffer "into this" -> "to this" http://codereview.chromium.org/113213/diff/1022/1023#newcode49 Line 49: // Move the read position by an offset of |size| bytes. of |offset| bytes. Fix in rest of comment. http://codereview.chromium.org/113213/diff/1022/1023#newcode51 Line 51: // forward_bytes(), return false and the current position remains the same. "return false and do not update the current position." When using "and" both clauses should have parallel structure. "return false" is an action, so the second clause should be phrased as an action too. http://codereview.chromium.org/113213/diff/1022/1023#newcode56 Line 56: // shifted by |offset| bytes. Reword the comment for the "valid" and "invalid" case. Something like Moves the read position by an offset of |size| bytes. If |offset| is positive, the position is moved foward. If negative, the position is moved backwards. The offset must not attempt to move beyond forward_capacity() when seeking foward, or backwards_capacity() when seeking backards. If the capacity in either direction is exceeded, the function returns false, and the current position is not updated. http://codereview.chromium.org/113213/diff/1022/1023#newcode60 Line 60: size_t forward_bytes() { return forward_bytes_; } const function? Same in functions below. http://codereview.chromium.org/113213/diff/1022/1023#newcode66 Line 66: size_t forward_capacity() { return forward_capacity_; } Add comments for these as well. http://codereview.chromium.org/113213/diff/1022/1023#newcode75 Line 75: // |data| is NULL if this method is called for seeking. Describe what the function actually does, not just who uses it. It's really good that you put who uses it though. That was non-obvious. :) http://codereview.chromium.org/113213/diff/1022/1023#newcode78 Line 78: // Internal seek forward method. Useless comment. There is no information here that isn't given by the function name, it's type, and protection level. Either remove it or make it useful. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments http://codereview.chromium.org/113213/diff/1022/1023#newcode86 Line 86: Buffer(size_t len) : data(new uint8[len]), size(len) {} explicit for all single parameter cosntructors to avoid creating an user-defined implicit type conversion. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const... http://codereview.chromium.org/113213/diff/1022/1024 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/1022/1024#newcode16 Line 16: seed_ = static_cast<int32>(base::Time::Now().ToInternalValue()); Anytime a unittest has a random generator, always print out the random seed. That way, on a flakey failure, we can reproduce the test given the log. In our case, it's pretty trivial, but it's still a good practice. Also, there's no reason for this to be saved as a member varaible, and I don't think you've managed to seed the random number generator. http://codereview.chromium.org/113213/diff/1022/1024#newcode30 Line 30: virtual void TearDown() { Don't declare it if it does nothing. http://codereview.chromium.org/113213/diff/1022/1024#newcode41 Line 41: scoped_array<uint8> temp_; temp_ is a bad variable name. Pick something more sensible and add a comment. http://codereview.chromium.org/113213/diff/1022/1024#newcode47 Line 47: TEST_F(SeekableBufferTest, RandomReadWrite) { Have we tried testing a 0-sized buffer? How about constructing with invalid arguments? eg, a negative forward or backward buffer? http://codereview.chromium.org/113213/diff/1022/1024#newcode60 Line 60: EXPECT_FALSE(should_append); You'll wnat to used a SCOPED_TRACE if you have EXPECT_* inside a loop, otherwise you wont' know which iteration the loop is on when this fails.
Fixed most of the comments, except for the SCOPED_TRACE will talk directly about this. http://codereview.chromium.org/113213/diff/1022/1025 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/1022/1025#newcode7 Line 7: #include "media/base/seekable_buffer.h" On 2009/05/16 01:26:06, awong wrote: > self-include always goes first. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > Done. http://codereview.chromium.org/113213/diff/1022/1025#newcode25 Line 25: STLDeleteElements<BufferQueue>(&buffers_); On 2009/05/16 01:26:06, awong wrote: > The <BufferQueue> is unecessary. STLDeleteElements is a template function where > all template types are expressed in the argument list, meaning that > type-inference works. Done. http://codereview.chromium.org/113213/diff/1022/1025#newcode38 Line 38: if (forward_bytes_ >= forward_capacity_) On 2009/05/16 01:26:06, awong wrote: > This is strange. You allow overrun of the buffer capacity? The semantics seem > odd. I would expect you to fail the write and non enqueue the buffer on false. As mentioned in the comment, we allow overrun of the buffer, we advise the user to not write anymore. It is because we cannot afford losing data since a new url request has very heavy penalty. In fact BufferedResourceLoader would stop writing to this buffer if false is returned. http://codereview.chromium.org/113213/diff/1022/1025#newcode52 Line 52: if (size > forward_bytes_) On 2009/05/16 01:26:06, awong wrote: > My head isn't really in a good shape to check boundary conditions. Is size == > forward_bytes_ okay? Is there a untitest checking this (will try to see for > myself in a bit) There's a unit test check for this, size == forward_bytes is a valid operation. http://codereview.chromium.org/113213/diff/1022/1025#newcode55 Line 55: DCHECK_EQ(taken, size); On 2009/05/16 01:26:06, awong wrote: > why not just return the offset moved in all 3 Seek interfaces? Converting to a > boolean just loses information without any real gain I think. It's not really useful to know the offset that actually moved. The use case of this class only needs to know success or not. http://codereview.chromium.org/113213/diff/1022/1025#newcode63 Line 63: size_t taken = 0; On 2009/05/16 01:26:06, awong wrote: > Implemneation comment explaining the loop logic. I shouldn't have to think > while reading code. :) Done. http://codereview.chromium.org/113213/diff/1022/1025#newcode65 Line 65: if (current_buffer_ == buffers_.end()) On 2009/05/16 01:26:06, awong wrote: > So...if we've played to the very end of the buffer, can we no longer seek > backwards? > > Should this be current_buffer == buffers_.begin() && current_buffer_offset_ == > 0? I changed the code so that current_buffer_ is set to buffers_.begin() at Append(). After that current_buffer_ will never be buffers_.end(), this check only will be true when SeekBackward() is called before any data is appended. I can make this as a DCHECK and have the if statement before the loop. http://codereview.chromium.org/113213/diff/1022/1025#newcode69 Line 69: current_buffer_offset_ -= consumed; On 2009/05/16 01:26:06, awong wrote: > yeah...I'm not going to work thtrough this in my head. :) You need a comment so > us readers don't need to think. Done. http://codereview.chromium.org/113213/diff/1022/1025#newcode118 Line 118: forward_bytes_ -= copied; On 2009/05/16 01:26:06, awong wrote: > non-trivial bookeeping! Comment! > > Done. http://codereview.chromium.org/113213/diff/1022/1023 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/1022/1023#newcode6 Line 6: // reading a media data source. By reading from it, the read position moves On 2009/05/16 01:26:06, awong wrote: > By reading -> When reading Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode10 Line 10: // This class provides no locking semantics, users of this class should On 2009/05/16 01:26:06, awong wrote: > "This class provides no locking semantics" is not very specific. It does have > locking semantics. The semantic is "you need to do it yourself." :) > > Instead say > > "This class is not inherently threadsafe. Concurrent access must be externally > serialized." Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode29 Line 29: // greater than |forward_capacity|, this value is used to report a buffer On 2009/05/16 01:26:06, awong wrote: > This is a run-on sentence. It should be 3 sentences. > > I also don't understand what how to use the "forward_bytes()" to report a > buffer-full situation. > > This comment feels more like a class-level comment. Can we shuffle the > information a bit so there is a large comment at the top that explains how this > class is used, what the different values do, and maybe even give an example of > common expected usages? > > Then the comment for the constructor can just focus on explaning what hte > constructor will do rather than also explaining how the class works. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode33 Line 33: // position that exceeds |backward_capacity| are evicted automatically. On 2009/05/16 01:26:06, awong wrote: > Are there any guarantees on when eviction occurs? Can we request eviction? > Describe in class comment. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode38 Line 38: // Read into |buffer| with the maximum size of |size| from the current On 2009/05/16 01:26:06, awong wrote: > "Read a maximum of |size| bytes into buffer" > > is more clear wording. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode42 Line 42: // Append |data| with |size| bytes into this buffer. If this buffer On 2009/05/16 01:26:06, awong wrote: > "into this" -> "to this" Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode49 Line 49: // Move the read position by an offset of |size| bytes. On 2009/05/16 01:26:06, awong wrote: > of |offset| bytes. Fix in rest of comment. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode51 Line 51: // forward_bytes(), return false and the current position remains the same. On 2009/05/16 01:26:06, awong wrote: > "return false and do not update the current position." > > When using "and" both clauses should have parallel structure. "return false" is > an action, so the second clause should be phrased as an action too. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode56 Line 56: // shifted by |offset| bytes. On 2009/05/16 01:26:06, awong wrote: > Reword the comment for the "valid" and "invalid" case. Something like > > Moves the read position by an offset of |size| bytes. If |offset| is positive, > the position is moved foward. If negative, the position is moved backwards. The > offset must not attempt to move beyond forward_capacity() when seeking foward, > or backwards_capacity() when seeking backards. > > If the capacity in either direction is exceeded, the function returns false, and > the current position is not updated. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode60 Line 60: size_t forward_bytes() { return forward_bytes_; } On 2009/05/16 01:26:06, awong wrote: > const function? > > Same in functions below. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode66 Line 66: size_t forward_capacity() { return forward_capacity_; } On 2009/05/16 01:26:06, awong wrote: > Add comments for these as well. Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode75 Line 75: // |data| is NULL if this method is called for seeking. On 2009/05/16 01:26:06, awong wrote: > Describe what the function actually does, not just who uses it. > > It's really good that you put who uses it though. That was non-obvious. :) Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode78 Line 78: // Internal seek forward method. On 2009/05/16 01:26:06, awong wrote: > Useless comment. There is no information here that isn't given by the function > name, it's type, and protection level. Either remove it or make it useful. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments > Done. http://codereview.chromium.org/113213/diff/1022/1023#newcode86 Line 86: Buffer(size_t len) : data(new uint8[len]), size(len) {} On 2009/05/16 01:26:06, awong wrote: > explicit for all single parameter cosntructors to avoid creating an user-defined > implicit type conversion. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const... > Done. http://codereview.chromium.org/113213/diff/1022/1024 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/1022/1024#newcode16 Line 16: seed_ = static_cast<int32>(base::Time::Now().ToInternalValue()); On 2009/05/16 01:26:06, awong wrote: > Anytime a unittest has a random generator, always print out the random seed. > That way, on a flakey failure, we can reproduce the test given the log. > > In our case, it's pretty trivial, but it's still a good practice. > > Also, there's no reason for this to be saved as a member varaible, and I don't > think you've managed to seed the random number generator. Done. http://codereview.chromium.org/113213/diff/1022/1024#newcode30 Line 30: virtual void TearDown() { On 2009/05/16 01:26:06, awong wrote: > Don't declare it if it does nothing. Done. http://codereview.chromium.org/113213/diff/1022/1024#newcode41 Line 41: scoped_array<uint8> temp_; On 2009/05/16 01:26:06, awong wrote: > temp_ is a bad variable name. Pick something more sensible and add a comment. Done. http://codereview.chromium.org/113213/diff/1022/1024#newcode47 Line 47: TEST_F(SeekableBufferTest, RandomReadWrite) { On 2009/05/16 01:26:06, awong wrote: > Have we tried testing a 0-sized buffer? > > How about constructing with invalid arguments? eg, a negative forward or > backward buffer? A 0-size buffer is tested in the last test. We can't construct the buffer with negative values since they are size_t. http://codereview.chromium.org/113213/diff/1022/1024#newcode60 Line 60: EXPECT_FALSE(should_append); On 2009/05/16 01:26:06, awong wrote: > You'll wnat to used a SCOPED_TRACE if you have EXPECT_* inside a loop, > otherwise you wont' know which iteration the loop is on when this fails. Not so sure about SCOPED_TRACE, will talk more about this.
http://codereview.chromium.org/113213/diff/1022/1025 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/1022/1025#newcode38 Line 38: if (forward_bytes_ >= forward_capacity_) On 2009/05/18 09:13:29, Alpha wrote: > On 2009/05/16 01:26:06, awong wrote: > > This is strange. You allow overrun of the buffer capacity? The semantics > seem > > odd. I would expect you to fail the write and non enqueue the buffer on > false. > > As mentioned in the comment, we allow overrun of the buffer, we advise the user > to not write anymore. It is because we cannot afford losing data since a new url > request has very heavy penalty. In fact BufferedResourceLoader would stop > writing to this buffer if false is returned. Right right...forgot this was an append API. Still, I think returning size is more general. It's similar in style to the printf, scanf APIs. The user is asked to check that the returned size == the passed in size to see if the full buffer can be written. If return != size does not equal, then the user can decide how best to handle -- including drop the extra data. Currently, the API says "well, you wrote part of the data, and I've dropped some for you, but you don't know waht." http://codereview.chromium.org/113213/diff/1022/1025#newcode55 Line 55: DCHECK_EQ(taken, size); On 2009/05/18 09:13:29, Alpha wrote: > On 2009/05/16 01:26:06, awong wrote: > > why not just return the offset moved in all 3 Seek interfaces? Converting to > a > > boolean just loses information without any real gain I think. > > It's not really useful to know the offset that actually moved. The use case of > this class only needs to know success or not. I see, since you don't seek if it'd move you past the end, this makes sense. http://codereview.chromium.org/113213/diff/48/1039#newcode84 Line 84: return true; as since -> since http://codereview.chromium.org/113213/diff/48/1039#newcode95 Line 95: DCHECK_GE(backward_bytes_, 0u); Isn't this always true? http://codereview.chromium.org/113213/diff/48/1039#newcode134 Line 134: } Write this as a positive statement. |current_buffer_| will always be valid after the buffer has been appended with data at least once. http://codereview.chromium.org/113213/diff/48/1039#newcode143 Line 143: Run-on sentence. http://codereview.chromium.org/113213/diff/48/1037 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/48/1037#newcode7 Line 7: // In order to support backward and forward seeking, this class buffers data in Put a newline between paragraphs please. This is hard to read. http://codereview.chromium.org/113213/diff/48/1037#newcode11 Line 11: // namely |backward_capacity| and |forward_capacity|. -namely http://codereview.chromium.org/113213/diff/48/1037#newcode13 Line 13: // advances and thus there will be more data in the backward direction, report check grammar
http://codereview.chromium.org/113213/diff/1022/1025 File media/base/seekable_buffer.cc (right): http://codereview.chromium.org/113213/diff/1022/1025#newcode38 Line 38: if (forward_bytes_ >= forward_capacity_) On 2009/05/18 20:30:41, awong wrote: > On 2009/05/18 09:13:29, Alpha wrote: > > On 2009/05/16 01:26:06, awong wrote: > > > This is strange. You allow overrun of the buffer capacity? The semantics > > seem > > > odd. I would expect you to fail the write and non enqueue the buffer on > > false. > > > > As mentioned in the comment, we allow overrun of the buffer, we advise the > user > > to not write anymore. It is because we cannot afford losing data since a new > url > > request has very heavy penalty. In fact BufferedResourceLoader would stop > > writing to this buffer if false is returned. > > Right right...forgot this was an append API. > > Still, I think returning size is more general. It's similar in style to the > printf, scanf APIs. The user is asked to check that the returned size == the > passed in size to see if the full buffer can be written. > > If return != size does not equal, then the user can decide how best to handle -- > including drop the extra data. Currently, the API says "well, you wrote part of > the data, > and I've dropped some for you, but you don't know waht." Append is always successful and data will always be appended, false just means that it is advised not to append any more, I added some comments to explain it. http://codereview.chromium.org/113213/diff/48/1039#newcode84 Line 84: return true; On 2009/05/18 20:30:42, awong wrote: > as since -> since Done. http://codereview.chromium.org/113213/diff/48/1039#newcode95 Line 95: DCHECK_GE(backward_bytes_, 0u); On 2009/05/18 20:30:42, awong wrote: > Isn't this always true? This should be always true, just to make sure we don't subtract too much from it. http://codereview.chromium.org/113213/diff/48/1039#newcode134 Line 134: } On 2009/05/18 20:30:42, awong wrote: > Write this as a positive statement. > > |current_buffer_| will always be valid after the buffer has been appended with > data at least once. Done. http://codereview.chromium.org/113213/diff/48/1039#newcode143 Line 143: On 2009/05/18 20:30:42, awong wrote: > Run-on sentence. Done. http://codereview.chromium.org/113213/diff/48/1037 File media/base/seekable_buffer.h (right): http://codereview.chromium.org/113213/diff/48/1037#newcode7 Line 7: // In order to support backward and forward seeking, this class buffers data in On 2009/05/18 20:30:42, awong wrote: > Put a newline between paragraphs please. This is hard to read. Done. http://codereview.chromium.org/113213/diff/48/1037#newcode11 Line 11: // namely |backward_capacity| and |forward_capacity|. On 2009/05/18 20:30:42, awong wrote: > -namely Done. http://codereview.chromium.org/113213/diff/48/1037#newcode13 Line 13: // advances and thus there will be more data in the backward direction, report On 2009/05/18 20:30:42, awong wrote: > check grammar Done.
updated the unit tests, it should be pretty clear what the DCHECKS are doing now.
lgtm One small nit (repeated once). Also, I think you need to sync before submitting. http://codereview.chromium.org/113213/diff/60/62 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/60/62#newcode56 Line 56: EXPECT_EQ(should_append, buffer_->forward_bytes() < kBufferSize) << Unlike all other binary-operators, for <<, the newline is usually but before the <<. http://codereview.chromium.org/113213/diff/60/62#newcode250 Line 250: EXPECT_EQ(should_append, buffer_->forward_bytes() < kBufferSize) << Same here http://codereview.chromium.org/113213/diff/60/64 File media/media.gyp (right): http://codereview.chromium.org/113213/diff/60/64#newcode76 Line 76: 'base/yuv_scale.h', Are the yuv_scale and filter/video_thread changes expected for media.gyp? Do you need to sync?
http://codereview.chromium.org/113213/diff/60/62 File media/base/seekable_buffer_unittest.cc (right): http://codereview.chromium.org/113213/diff/60/62#newcode56 Line 56: EXPECT_EQ(should_append, buffer_->forward_bytes() < kBufferSize) << On 2009/05/18 22:29:00, awong wrote: > Unlike all other binary-operators, for <<, the newline is usually but before the > <<. Done. http://codereview.chromium.org/113213/diff/60/62#newcode250 Line 250: EXPECT_EQ(should_append, buffer_->forward_bytes() < kBufferSize) << On 2009/05/18 22:29:00, awong wrote: > Same here Done. http://codereview.chromium.org/113213/diff/60/64 File media/media.gyp (right): http://codereview.chromium.org/113213/diff/60/64#newcode76 Line 76: 'base/yuv_scale.h', sync'ed in the last upload. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
