Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
88047:
Buffered data source that does range request to provide data to media pipelin... (Closed)
|
Created:
11 years, 8 months ago by Alpha Left Google Modified:
9 years, 6 months ago CC:
chromium-reviews_googlegroups.com, fbarchard Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionBuffered data source that does range request to provide data to media pipeline
The implementation of buffered data source that uses buffered resource loader
to download a media object and does buffering. This implementation knows
when to do defer loading and when to restarts the loading. The implementation
uses one buffered resource loading for one data source and does no connection
recovery.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14818
Patch Set 1 #Patch Set 2 : '' #
Total comments: 69
Patch Set 3 : '' #
Total comments: 1
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 50
Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 10
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #
Messages
Total messages: 16 (0 generated)
I did a redesign of my prototype so it took some time to send this CL. I changed the design so there's a BufferedResourceLoader that is used by BufferedDataSource to do resource loading, the intention is to isolate all resource loading logic to BufferedResourceLoader so BufferedDataSource can have multiple resource loaders and does connection recovery. I'll need to write some unit tests and upload them later. Please review the code first. :)
Looks pretty good. Have some comments about memory ownership and locking. http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode188 Line 188: return true; Should all of these be in the loop? Please add comment explaining thread semantics and why/how these variables are changing outside of the lock. I also wonder if taking the lock 3 times in the loop is necessary...It's not like you're doing anything long in here. Taking it once at the top of the loop might be enough. http://codereview.chromium.org/88047/diff/1009/1010#newcode209 Line 209: buffer->taken += taken; So, it's okay to modify the buffer even while it's still on the buffer queue? http://codereview.chromium.org/88047/diff/1009/1010#newcode383 Line 383: if (stopped_) Don't need this. stopped_ = true is idempotent. http://codereview.chromium.org/88047/diff/1009/1010#newcode392 Line 392: resource_loader = buffered_resource_loader_; Why is this in a separate lock block? http://codereview.chromium.org/88047/diff/1009/1010#newcode447 Line 447: int trials = 2; add comment explaining why "2" is a sane trial number. http://codereview.chromium.org/88047/diff/1009/1010#newcode490 Line 490: *position_out = position_; does this need lock? same below? http://codereview.chromium.org/88047/diff/1009/1011 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/1009/1011#newcode9 Line 9: #include <string> ordering http://codereview.chromium.org/88047/diff/1009/1011#newcode37 Line 37: ~BufferedResourceLoader(); virtual? http://codereview.chromium.org/88047/diff/1009/1011#newcode54 Line 54: // the required amount of bytes is read or the loader is stopped or this "only the required amount of bytes is read or the" -> "only after the required amount of bytes is read, the" No more than one "or" per sentence. :) Also, doesn't this reutrn after the timeout is reached? http://codereview.chromium.org/88047/diff/1009/1011#newcode62 Line 62: bool SeekForward(int64 position); what is the unit of position? I'm guessing it's the "offset." Is that bytes? time? http://codereview.chromium.org/88047/diff/1009/1011#newcode65 Line 65: // has the first byte or is required to download from. I don't understand this comment...can you reword it a bit? http://codereview.chromium.org/88047/diff/1009/1011#newcode76 Line 76: // Get the content length of the instance after this loader has been started. What unit is this? http://codereview.chromium.org/88047/diff/1009/1011#newcode81 Line 81: virtual void OnUploadProgress(uint64 position, uint64 size) {} Is this supposed to have no implementation? Should you add a TODO? http://codereview.chromium.org/88047/diff/1009/1011#newcode103 Line 103: // A deep copy constructor. Why not use a vector<uint8>? http://codereview.chromium.org/88047/diff/1009/1011#newcode137 Line 137: Lock lock_; DISALLOW_COPY_AND_ASSIGN? Also, add comment to lock_ saying what it's protecting so we can know when it should be grabbed. http://codereview.chromium.org/88047/diff/1009/1011#newcode151 Line 151: ///////////////////////////////////////////////////////////////////////////// It seems like most files in chrome don't use /////////////////// for section separators, or really section separators at all. Can we remove these? http://codereview.chromium.org/88047/diff/1009/1011#newcode167 Line 167: friend class media::FilterFactoryImpl1<BufferedDataSource, Impll? And if this compiles as is, do we actually need FilterFactoryImpl to be a friend? http://codereview.chromium.org/88047/diff/1009/1011#newcode192 Line 192: scoped_refptr<BufferedResourceLoader> buffered_resource_loader_; I don't fully understand all of this, but why does this need to be refcounted? Can we modify the code to guarantee any of the following: (a) the object exists for the while lifetime of the BufferedDataSource (make BufferResourceLoader reusable?) (b) the object isn't created or deleted except for from one thread It'd be nice to avoid needing refcounts...
http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode383 Line 383: if (stopped_) On 2009/04/21 20:05:31, awong wrote: > Don't need this. stopped_ = true is idempotent. eeek..completely misread this. Ignore this comment.
http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode188 Line 188: return true; On 2009/04/21 20:05:31, awong wrote: > Should all of these be in the loop? > > Please add comment explaining thread semantics and why/how these variables are > changing outside of the lock. Done. > > I also wonder if taking the lock 3 times in the loop is necessary...It's not > like you're doing anything long in here. Taking it once at the top of the loop > might be enough. I like the idea of making critical section as small as possible especially locking a large object, i.e. buffers_. I allow data to be appended to |buffers_| while I'm in the loop. Also note that buffer_event_.Wait() needs to be outside of lock or there will be a deadlock. So the paradigm is: 1. Take an object with lock 2. Operate with the object 3. Lock if I need to delete it 4. If object is not available then wait The first 3 can be grouped to use one lock if it's more clear to you. http://codereview.chromium.org/88047/diff/1009/1010#newcode209 Line 209: buffer->taken += taken; On 2009/04/21 20:05:31, awong wrote: > So, it's okay to modify the buffer even while it's still on the buffer queue? Only append operations may happen while we are operating on the first buffer object, so it's safe to change the content in the first buffer. Read() and SeekForward() happens on Demuxer thread. AppendToBuffer() happens on Render thread. http://codereview.chromium.org/88047/diff/1009/1010#newcode383 Line 383: if (stopped_) On 2009/04/21 20:05:31, awong wrote: > Don't need this. stopped_ = true is idempotent. Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode392 Line 392: resource_loader = buffered_resource_loader_; On 2009/04/21 20:05:31, awong wrote: > Why is this in a separate lock block? Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode447 Line 447: int trials = 2; On 2009/04/21 20:05:31, awong wrote: > add comment explaining why "2" is a sane trial number. Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode490 Line 490: *position_out = position_; On 2009/04/21 20:05:31, awong wrote: > does this need lock? same below? > Not really, these methods are called in the same thread: - Read - GetPosition - SetPosition - GetSize http://codereview.chromium.org/88047/diff/1009/1011 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/1009/1011#newcode9 Line 9: #include <string> On 2009/04/21 20:05:31, awong wrote: > ordering Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode37 Line 37: ~BufferedResourceLoader(); On 2009/04/21 20:05:31, awong wrote: > virtual? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode54 Line 54: // the required amount of bytes is read or the loader is stopped or this On 2009/04/21 20:05:31, awong wrote: > "only the required amount of bytes is read or the" -> "only after the required > amount of bytes is read, the" > > No more than one "or" per sentence. :) > > Also, doesn't this reutrn after the timeout is reached? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode62 Line 62: bool SeekForward(int64 position); On 2009/04/21 20:05:31, awong wrote: > what is the unit of position? I'm guessing it's the "offset." Is that bytes? > time? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode65 Line 65: // has the first byte or is required to download from. On 2009/04/21 20:05:31, awong wrote: > I don't understand this comment...can you reword it a bit? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode76 Line 76: // Get the content length of the instance after this loader has been started. On 2009/04/21 20:05:31, awong wrote: > What unit is this? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode81 Line 81: virtual void OnUploadProgress(uint64 position, uint64 size) {} On 2009/04/21 20:05:31, awong wrote: > Is this supposed to have no implementation? Should you add a TODO? We don't need to implement this. It there because it's pure virtual. http://codereview.chromium.org/88047/diff/1009/1011#newcode103 Line 103: // A deep copy constructor. On 2009/04/21 20:05:31, awong wrote: > Why not use a vector<uint8>? The reason why I'm doing this is because the operations on the buffer is mostly peek from front and push to back. Using vector of bytes is heavy for this usage. The operations on the buffer queue has to be atomic, that means the expansion and shrinking of the vector has to be atomic too, that would require copying of memory in magnitude of MBs to be atomic. Making it a queue of Buffer objects reduces the copying by a factor of ~32k (say each Buffer is 32k size), it makes pop from front and pushing to back much more efficient. Please take a look into SeekForward() and Read(), because of this data structure I'm able to work on Buffer objects outside of lock. http://codereview.chromium.org/88047/diff/1009/1011#newcode137 Line 137: Lock lock_; On 2009/04/21 20:05:31, awong wrote: > DISALLOW_COPY_AND_ASSIGN? > > Also, add comment to lock_ saying what it's protecting so we can know when it > should be grabbed. Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode151 Line 151: ///////////////////////////////////////////////////////////////////////////// On 2009/04/21 20:05:31, awong wrote: > It seems like most files in chrome don't use /////////////////// for section > separators, or really section separators at all. > > Can we remove these? Done. http://codereview.chromium.org/88047/diff/1009/1011#newcode167 Line 167: friend class media::FilterFactoryImpl1<BufferedDataSource, On 2009/04/21 20:05:31, awong wrote: > Impll? > Impl1 means this is a template that constructs with one parameter. > And if this compiles as is, do we actually need FilterFactoryImpl to be a > friend? We keep ctor and dtor private so only this FilterFactor can construct us. http://codereview.chromium.org/88047/diff/1009/1011#newcode192 Line 192: scoped_refptr<BufferedResourceLoader> buffered_resource_loader_; On 2009/04/21 20:05:31, awong wrote: > I don't fully understand all of this, but why does this need to be refcounted? > Can we modify the code to guarantee any of the following: > (a) the object exists for the while lifetime of the BufferedDataSource (make > BufferResourceLoader reusable?) > (b) the object isn't created or deleted except for from one thread > > It'd be nice to avoid needing refcounts... Reducing refcounted objects is a good idea. By having it not refcounted we have the following draw backs: 1. BufferedResourceLoader will be less self contained. Reasons: 1. SetDefersLoading(true/false) has to done on render thread. BufferedResourceLoader can't post task on render to do it on its own because it's not refcounted. 2. ResourceLoaderBridge::Cancel() and destruction has to be on render thread, save reason above. It's simpler to let ResourceLoaderBridge to run on their own once we have multiple instances of them. The design is actually going for this route.
http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode188 Line 188: return true; On 2009/04/21 21:06:11, Alpha wrote: > On 2009/04/21 20:05:31, awong wrote: > > Should all of these be in the loop? > > > > Please add comment explaining thread semantics and why/how these variables are > > changing outside of the lock. > > Done. > > > > I also wonder if taking the lock 3 times in the loop is necessary...It's not > > like you're doing anything long in here. Taking it once at the top of the > loop > > might be enough. > > I like the idea of making critical section as small as possible especially > locking a large object, i.e. buffers_. I allow data to be appended to |buffers_| > while I'm in the loop. Also note that buffer_event_.Wait() needs to be outside > of lock or there will be a deadlock. So the paradigm is: > 1. Take an object with lock > 2. Operate with the object > 3. Lock if I need to delete it > 4. If object is not available then wait > > The first 3 can be grouped to use one lock if it's more clear to you. > > I agree with keeping the critical sections short, but all you're doing in the critical sections here are some very basic assignments and aditions, subtractions, etc. In this situation, I wonder if the grabbing/releaseing of the locks with all the pipeline/cache changes that entails would actually overwhelm any benefit of having a shorter section...especially if we don't know how much lock contention there would be. I agree holding over Wait is bad. But the rest, your operations are so small, I'd rather go for a simpler lock structure and optimize when we see an actual performance impact. http://codereview.chromium.org/88047/diff/1009/1010#newcode209 Line 209: buffer->taken += taken; On 2009/04/21 21:06:11, Alpha wrote: > On 2009/04/21 20:05:31, awong wrote: > > So, it's okay to modify the buffer even while it's still on the buffer queue? > > Only append operations may happen while we are operating on the first buffer > object, so it's safe to change the content in the first buffer. > > Read() and SeekForward() happens on Demuxer thread. > AppendToBuffer() happens on Render thread. > Document this in the header file for this class, and put a comment here pointing to that documentation. Looking at it without context, I don't know how to verify your operation is safe. http://codereview.chromium.org/88047/diff/1009/1011 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/1009/1011#newcode103 Line 103: // A deep copy constructor. On 2009/04/21 21:06:11, Alpha wrote: > On 2009/04/21 20:05:31, awong wrote: > > Why not use a vector<uint8>? > > The reason why I'm doing this is because the operations on the buffer is mostly > peek from front and push to back. Using > vector of bytes is heavy for this usage. The operations on the buffer queue has > to be atomic, that means the expansion and shrinking of the vector has to be > atomic too, that would require copying of memory in magnitude of MBs to be > atomic. Making it a queue of Buffer objects reduces the copying by a factor of > ~32k (say each Buffer is 32k size), it makes pop from front and pushing to back > much more efficient. Please take a look into SeekForward() and Read(), because > of this data structure I'm able to work on Buffer objects outside of lock. ...I'm not certain I understand your rationale here still. So, a vector isn't going to be any less efficient if you create it with the correct size. There will not be any "growing." In terms of putting in a queue, you're already enqueing Buffer*, not Buffer. In fact, I don't think you are invoking this copy constructor anywhere, are you? Lastly, about the thread safety, your guarantee is that only one thread will access any given buffer object through the handle in the queue at any given time. I don't think there's any synchronization guarantees in your code about multiple access to one specific buffer object...I don't think the thread safety of vector matters here... http://codereview.chromium.org/88047/diff/1009/1011#newcode167 Line 167: friend class media::FilterFactoryImpl1<BufferedDataSource, On 2009/04/21 21:06:11, Alpha wrote: > On 2009/04/21 20:05:31, awong wrote: > > Impll? > > > > Impl1 means this is a template that constructs with one parameter. > > > And if this compiles as is, do we actually need FilterFactoryImpl to be a > > friend? > > We keep ctor and dtor private so only this FilterFactor can construct us. > Oops..misread Impl1 as Impll. Makes sense. http://codereview.chromium.org/88047/diff/11/12 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/11/12#newcode181 Line 181: // Seeking backward. I feel like the thread safety assumptions about functions should be documented in the header file explicitly. It's complicated enough that it's non-obvoius from the API which functions can be called when. Consider adding comments like // Callable only from the Render thread to your function documentation.
looking good but please please please write a unit test for your HttpResponseHeaders change... your function is definitely complex enough and http_response_headers_unittest.cc is ready and waiting for you to write one :) ericroman seems like a good guy to ping for a net unit test (he wrote one a few months back) http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode21 Line 21: const char* kHttpScheme = "http"; chrome prefers to declare const strings as arrays: Good: const char kFoo[] = "bar"; Bad: const char kFoo* = "bar"; http://codereview.chromium.org/88047/diff/1009/1010#newcode72 Line 72: NOTIMPLEMENTED() << "Suffix length range request not implemented."; add braces here http://codereview.chromium.org/88047/diff/1009/1010#newcode98 Line 98: NewRunnableMethod(this, this can fit on a single line http://codereview.chromium.org/88047/diff/1009/1010#newcode183 Line 183: if (stopped_) this is a twisting maze of logic agree with awong on explaining what's going on here or maybe combining the if statements/adding quick comments http://codereview.chromium.org/88047/diff/1009/1010#newcode236 Line 236: // TODO(hclam): do the right thing about defer loading here. what's the right thing can you explain a bit more? http://codereview.chromium.org/88047/diff/1009/1010#newcode448 Line 448: while (trials > 0) { what about a for loop? http://codereview.chromium.org/88047/diff/1009/1011 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/1009/1011#newcode4 Line 4: // remove extra // here http://codereview.chromium.org/88047/diff/1009/1011#newcode21 Line 21: #include "net/base/file_stream.h" are you sure you're using all these header files? I don't think I see platform_file or the net includes being used http://codereview.chromium.org/88047/diff/1009/1011#newcode111 Line 111: if (data) no need to check: C++ lets you call delete on NULL pointers http://codereview.chromium.org/88047/diff/1009/1011#newcode129 Line 129: bool defered_; defered_ -> deferred_ http://codereview.chromium.org/88047/diff/1009/1011#newcode134 Line 134: std::string uri_; uri_ -> url_ (chrome is trying to standardize on url) http://codereview.chromium.org/88047/diff/1009/1011#newcode145 Line 145: WebMediaPlayerDelegateImpl* delegate) { indent by 2 http://codereview.chromium.org/88047/diff/1009/1011#newcode146 Line 146: return new media::FilterFactoryImpl1<BufferedDataSource, un-indent by 2 http://codereview.chromium.org/88047/diff/1009/1012 File net/http/http_response_headers.cc (right): http://codereview.chromium.org/88047/diff/1009/1012#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. bump copyright year http://codereview.chromium.org/88047/diff/1009/1012#newcode972 Line 972: // From RFC 2616 14.16 can you add a comment here with the spec, similar to GetContentLength() you may want to grab ericroman to look at this also.... UNIT TESTS!!! There's already a http_response_headers_unittest.cc waiting for you :) http://codereview.chromium.org/88047/diff/1009/1012#newcode996 Line 996: return false; this looks like its dangling... may want to add braces http://codereview.chromium.org/88047/diff/1009/1012#newcode1005 Line 1005: if (LowerCaseEqualsASCII(content_range_val.begin() + space_position + 1, can you actually construct the strings instead of using iterators inside of the LowerCaseEqualsASCII/StringToInt64? I have no idea what's going on here haha http://codereview.chromium.org/88047/diff/1009/1013 File net/http/http_response_headers.h (right): http://codereview.chromium.org/88047/diff/1009/1013#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. bump copyright year
http://codereview.chromium.org/88047/diff/1009/1010 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/1009/1010#newcode21 Line 21: const char* kHttpScheme = "http"; On 2009/04/21 21:36:31, scherkus wrote: > chrome prefers to declare const strings as arrays: > Good: const char kFoo[] = "bar"; > Bad: const char kFoo* = "bar"; Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode72 Line 72: NOTIMPLEMENTED() << "Suffix length range request not implemented."; On 2009/04/21 21:36:31, scherkus wrote: > add braces here Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode98 Line 98: NewRunnableMethod(this, On 2009/04/21 21:36:31, scherkus wrote: > this can fit on a single line Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode183 Line 183: if (stopped_) On 2009/04/21 21:36:31, scherkus wrote: > this is a twisting maze of logic > > agree with awong on explaining what's going on here or maybe combining the if > statements/adding quick comments Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode188 Line 188: return true; On 2009/04/21 21:32:10, awong wrote: > On 2009/04/21 21:06:11, Alpha wrote: > > On 2009/04/21 20:05:31, awong wrote: > > > Should all of these be in the loop? > > > > > > Please add comment explaining thread semantics and why/how these variables > are > > > changing outside of the lock. > > > > Done. > > > > > > I also wonder if taking the lock 3 times in the loop is necessary...It's not > > > like you're doing anything long in here. Taking it once at the top of the > > loop > > > might be enough. > > > > I like the idea of making critical section as small as possible especially > > locking a large object, i.e. buffers_. I allow data to be appended to > |buffers_| > > while I'm in the loop. Also note that buffer_event_.Wait() needs to be outside > > of lock or there will be a deadlock. So the paradigm is: > > 1. Take an object with lock > > 2. Operate with the object > > 3. Lock if I need to delete it > > 4. If object is not available then wait > > > > The first 3 can be grouped to use one lock if it's more clear to you. > > > > > > I agree with keeping the critical sections short, but all you're doing in the > critical sections here are some very basic assignments and aditions, > subtractions, etc. In this situation, I wonder if the grabbing/releaseing of > the locks with all the pipeline/cache changes that entails would actually > overwhelm any benefit of having a shorter section...especially if we don't know > how much lock contention there would be. > > I agree holding over Wait is bad. But the rest, your operations are so small, > I'd rather go for a simpler lock structure and optimize when we see an actual > performance impact. Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode209 Line 209: buffer->taken += taken; On 2009/04/21 21:32:10, awong wrote: > On 2009/04/21 21:06:11, Alpha wrote: > > On 2009/04/21 20:05:31, awong wrote: > > > So, it's okay to modify the buffer even while it's still on the buffer > queue? > > > > Only append operations may happen while we are operating on the first buffer > > object, so it's safe to change the content in the first buffer. > > > > Read() and SeekForward() happens on Demuxer thread. > > AppendToBuffer() happens on Render thread. > > > > Document this in the header file for this class, and put a comment here pointing > to that documentation. > > Looking at it without context, I don't know how to verify your operation is > safe. Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode236 Line 236: // TODO(hclam): do the right thing about defer loading here. On 2009/04/21 21:36:31, scherkus wrote: > what's the right thing > > can you explain a bit more? Done. http://codereview.chromium.org/88047/diff/1009/1010#newcode448 Line 448: while (trials > 0) { On 2009/04/21 21:36:31, scherkus wrote: > what about a for loop? Done. http://codereview.chromium.org/88047/diff/1009/1012 File net/http/http_response_headers.cc (right): http://codereview.chromium.org/88047/diff/1009/1012#newcode972 Line 972: // From RFC 2616 14.16 On 2009/04/21 21:36:31, scherkus wrote: > can you add a comment here with the spec, similar to GetContentLength() > > you may want to grab ericroman to look at this > > also.... UNIT TESTS!!! There's already a http_response_headers_unittest.cc > waiting for you :) Definitely unit tests! I wanted you guys to look into BufferedDataSource first :P http://codereview.chromium.org/88047/diff/1009/1012#newcode1005 Line 1005: if (LowerCaseEqualsASCII(content_range_val.begin() + space_position + 1, On 2009/04/21 21:36:31, scherkus wrote: > can you actually construct the strings instead of using iterators inside of the > LowerCaseEqualsASCII/StringToInt64? > > I have no idea what's going on here haha Unforunately constructing the strings I still need to use the iterators, so there's just some extra lines without much help. I added some commencts here about which part does what.
I forgot that initialization needs to be asynchronous, I've changed some code so that BufferedResourceLoader::Start() can operate in both synchronous (for seek and connection restart) and asynchronous mode (for the very first request to determine server capabilities) I've already noticed some problems with this patch, mainly reloading the page over http we don't get response from the server. I think this is unrelated to this CL, will look into that later.
On 2009/04/23 02:08:58, Alpha wrote: > I forgot that initialization needs to be asynchronous, I've changed some code so > that BufferedResourceLoader::Start() can operate in both synchronous (for seek > and connection restart) and asynchronous mode (for the very first request to > determine server capabilities) > > I've already noticed some problems with this patch, mainly reloading the page > over http we don't get response from the server. I think this is unrelated to > this CL, will look into that later. Forget my comments about reloading, it's a problem of disk cache, I disabled it for now until we have better disk cache.
some driveby comments http://codereview.chromium.org/88047/diff/5005/4005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/5005/4005#newcode24 Line 24: const int kPositionNotSpecified = -1; might want to define this as an int64 instead of int, to be consistent with how it is used. http://codereview.chromium.org/88047/diff/5005/4005#newcode38 Line 38: content_length_(0), why use 0 instead of kPositionNotSpecified? does it matter? http://codereview.chromium.org/88047/diff/5005/4005#newcode159 Line 159: memcpy(data + taken, buffer->data + buffer->taken, copy); I dont understand how the caller uses this, but seems odd not to start writing into the start of |data|.... as long as this doesnt risk buffer overrun. http://codereview.chromium.org/88047/diff/5005/4005#newcode288 Line 288: void BufferedResourceLoader::OnReceivedRedirect(const GURL& new_url) { } I reckon you should implement a body here, to keep track what the final response URL is. Since the URL you end up loading may differ from what you requested: some parts of you code enforce the URL scheme must be one of {http, https, file}. This enforcement needs to be done across redirects too, since you may redirect to a ftp:// url here. http://codereview.chromium.org/88047/diff/5005/4005#newcode297 Line 297: URLPattern url_pattern; Why URLPattern and not GURL? http://codereview.chromium.org/88047/diff/5005/4005#newcode301 Line 301: if (url_pattern.scheme() != kFileScheme) { (if this were a GURL, you would use SchemeIsFile()). http://codereview.chromium.org/88047/diff/5005/4005#newcode308 Line 308: if (info.headers->response_code() != kHttpPartialContent || I guess the assumption here is that the response was a http or https URL (for which response_code has meaning). See comment above about redirects. http://codereview.chromium.org/88047/diff/5005/4005#newcode328 Line 328: content_length_ = info.content_length; may want to mention that info.content_length may be -1. http://codereview.chromium.org/88047/diff/5005/4005#newcode330 Line 330: if (first_byte_position != -1) maybe use your constant here, kPositionNotSpecified http://codereview.chromium.org/88047/diff/5005/4005#newcode463 Line 463: URLPattern url_pattern; Why not GURL ? http://codereview.chromium.org/88047/diff/5005/4005#newcode465 Line 465: if (!LowerCaseEqualsASCII(url_pattern.scheme(), kHttpScheme) && (if this were GURL, you would use GURL::SchemeIs(x)). http://codereview.chromium.org/88047/diff/5005/4006 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/5005/4006#newcode29 Line 29: // full and un-defers the resource loading it is under buffered. wording nit: - loading it is under buffered. + loading if it is under buffered. http://codereview.chromium.org/88047/diff/5005/4006#newcode42 Line 42: // If |start_callbacl| is NULL, this method operates in synchronous mode and typo nit: - |start_callbacl| + |start_callback| http://codereview.chromium.org/88047/diff/5005/4006#newcode43 Line 43: // it returns true if loaders has started successfully, false otherwise. It wording nit: - it returns true if loaders has started successfully + it returns true if the load has started successfully http://codereview.chromium.org/88047/diff/5005/4006#newcode50 Line 50: bool Start(Callback1<bool>::Type* start_callback); You might want to consider using the same paradigm for this as the net module (use CompletionCallback). The way that would look is: int Start(CompletionCallback* callback); - Returns net::ERR_IOPENDING if Start is to complete asynchronously (in which case |callback| will be invoked with the result code later). - Returns an error code (negative integer from net_error_list.h) if the call failed synchronously. - Returns non-negative number if the call completed successfully synchronously. (generally this is net::OK which is zero, or for stuff like read/write it is a byte count). http://codereview.chromium.org/88047/diff/5005/4006#newcode52 Line 52: // Stop this loader. Wakes up all synchronous actions. use passive tense: - Stop this loader. + Stops this loader. http://codereview.chromium.org/88047/diff/5005/4006#newcode61 Line 61: size_t Read(uint8* buffer, size_t size); How are errors represented, as 0? In this case, is there any distinction between end of stream and failure? Might want to consider using a signed type as the result, so the negative range can be used for error codes. http://codereview.chromium.org/88047/diff/5005/4006#newcode71 Line 71: // Returns the position in bytes that this loader is download from. wording nit: - is download from + is downloading from http://codereview.chromium.org/88047/diff/5005/4006#newcode74 Line 74: // Get and set the buffering limit of this loader. use passive tense: - Get and set + Gets and sets http://codereview.chromium.org/88047/diff/5005/4006#newcode78 Line 78: // Get and set the timeout for the synchronous operations. use passive tense: - Get and set + Gets and sets http://codereview.chromium.org/88047/diff/5005/4006#newcode82 Line 82: // Get the content length in bytes of the instance after this loader has been use passive tense - Get and set + Gets and sets http://codereview.chromium.org/88047/diff/5005/4006#newcode116 Line 116: delete [] data; I would suggest doing the allocation of data as part of Buffer too. I.e. change the constructor to just take a length. (Then both allocation and free are details of Buffer). http://codereview.chromium.org/88047/diff/5005/4006#newcode119 Line 119: size_t taken; this could use a comment. http://codereview.chromium.org/88047/diff/5005/4006#newcode141 Line 141: std::string url_; In general, all the code that uses URLs should represent them with a GURL. Additionally, add a comment on what this URL is -- is it the requested URL, or the response URL? (see comment about redirects). http://codereview.chromium.org/88047/diff/5005/4006#newcode198 Line 198: std::string url_; Ditto for using GURL.
Sorry for fixing this CL late. http://codereview.chromium.org/88047/diff/5005/4005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/5005/4005#newcode24 Line 24: const int kPositionNotSpecified = -1; On 2009/04/23 22:51:39, eroman wrote: > might want to define this as an int64 instead of int, to be consistent with how > it is used. Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode38 Line 38: content_length_(0), On 2009/04/23 22:51:39, eroman wrote: > why use 0 instead of kPositionNotSpecified? does it matter? Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode159 Line 159: memcpy(data + taken, buffer->data + buffer->taken, copy); On 2009/04/23 22:51:39, eroman wrote: > I dont understand how the caller uses this, but seems odd not to start writing > into the start of |data|.... as long as this doesnt risk buffer overrun. This method fetches buffer and copy them into |data| as much as possible so the copying progresses in |data|. http://codereview.chromium.org/88047/diff/5005/4005#newcode288 Line 288: void BufferedResourceLoader::OnReceivedRedirect(const GURL& new_url) { } On 2009/04/23 22:51:39, eroman wrote: > I reckon you should implement a body here, to keep track what the final response > URL is. > > Since the URL you end up loading may differ from what you requested: some parts > of you code enforce the URL scheme must be one of {http, https, file}. This > enforcement needs to be done across redirects too, since you may redirect to a > ftp:// url here. > Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode297 Line 297: URLPattern url_pattern; On 2009/04/23 22:51:39, eroman wrote: > Why URLPattern and not GURL? Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode301 Line 301: if (url_pattern.scheme() != kFileScheme) { On 2009/04/23 22:51:39, eroman wrote: > (if this were a GURL, you would use SchemeIsFile()). Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode308 Line 308: if (info.headers->response_code() != kHttpPartialContent || On 2009/04/23 22:51:39, eroman wrote: > I guess the assumption here is that the response was a http or https URL (for > which response_code has meaning). See comment above about redirects. Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode328 Line 328: content_length_ = info.content_length; On 2009/04/23 22:51:39, eroman wrote: > may want to mention that info.content_length may be -1. Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode330 Line 330: if (first_byte_position != -1) On 2009/04/23 22:51:39, eroman wrote: > maybe use your constant here, kPositionNotSpecified Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode463 Line 463: URLPattern url_pattern; On 2009/04/23 22:51:39, eroman wrote: > Why not GURL ? Done. http://codereview.chromium.org/88047/diff/5005/4005#newcode465 Line 465: if (!LowerCaseEqualsASCII(url_pattern.scheme(), kHttpScheme) && On 2009/04/23 22:51:39, eroman wrote: > (if this were GURL, you would use GURL::SchemeIs(x)). Done. http://codereview.chromium.org/88047/diff/5005/4006 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/5005/4006#newcode29 Line 29: // full and un-defers the resource loading it is under buffered. On 2009/04/23 22:51:39, eroman wrote: > wording nit: > > - loading it is under buffered. > + loading if it is under buffered. Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode42 Line 42: // If |start_callbacl| is NULL, this method operates in synchronous mode and On 2009/04/23 22:51:39, eroman wrote: > typo nit: > > - |start_callbacl| > + |start_callback| Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode43 Line 43: // it returns true if loaders has started successfully, false otherwise. It On 2009/04/23 22:51:39, eroman wrote: > wording nit: > > - it returns true if loaders has started successfully > + it returns true if the load has started successfully Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode50 Line 50: bool Start(Callback1<bool>::Type* start_callback); On 2009/04/23 22:51:39, eroman wrote: > You might want to consider using the same paradigm for this as the net module > (use CompletionCallback). The way that would look is: > > int Start(CompletionCallback* callback); > > - Returns net::ERR_IOPENDING if Start is to complete asynchronously (in which > case |callback| will be invoked with the result code later). > > - Returns an error code (negative integer from net_error_list.h) if the call > failed synchronously. > > - Returns non-negative number if the call completed successfully synchronously. > (generally this is net::OK which is zero, or for stuff like read/write it is a > byte count). > Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode52 Line 52: // Stop this loader. Wakes up all synchronous actions. On 2009/04/23 22:51:39, eroman wrote: > use passive tense: > > - Stop this loader. > + Stops this loader. Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode61 Line 61: size_t Read(uint8* buffer, size_t size); On 2009/04/23 22:51:39, eroman wrote: > How are errors represented, as 0? > > In this case, is there any distinction between end of stream and failure? > > Might want to consider using a signed type as the result, so the negative range > can be used for error codes. > Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode71 Line 71: // Returns the position in bytes that this loader is download from. On 2009/04/23 22:51:39, eroman wrote: > wording nit: > > - is download from > + is downloading from Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode74 Line 74: // Get and set the buffering limit of this loader. On 2009/04/23 22:51:39, eroman wrote: > use passive tense: > > - Get and set > + Gets and sets Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode78 Line 78: // Get and set the timeout for the synchronous operations. On 2009/04/23 22:51:39, eroman wrote: > use passive tense: > > - Get and set > + Gets and sets Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode82 Line 82: // Get the content length in bytes of the instance after this loader has been On 2009/04/23 22:51:39, eroman wrote: > use passive tense > > - Get and set > + Gets and sets Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode116 Line 116: delete [] data; On 2009/04/23 22:51:39, eroman wrote: > I would suggest doing the allocation of data as part of Buffer too. I.e. change > the constructor to just take a length. (Then both allocation and free are > details of Buffer). Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode119 Line 119: size_t taken; On 2009/04/23 22:51:39, eroman wrote: > this could use a comment. Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode141 Line 141: std::string url_; On 2009/04/23 22:51:39, eroman wrote: > In general, all the code that uses URLs should represent them with a GURL. > > Additionally, add a comment on what this URL is -- is it the requested URL, or > the response URL? (see comment about redirects). Done. http://codereview.chromium.org/88047/diff/5005/4006#newcode198 Line 198: std::string url_; On 2009/04/23 22:51:39, eroman wrote: > Ditto for using GURL. Done.
thanks for making those changes; a couple more nits. http://codereview.chromium.org/88047/diff/9007/8005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/9007/8005#newcode440 Line 440: start_callback_->RunWithParams(Tuple1<int>(error)); or simply: start_callback_->Run(error); http://codereview.chromium.org/88047/diff/9007/8005#newcode479 Line 479: if (!url_.SchemeIs(kHttpScheme) && !url_.SchemeIs(kHttpsScheme) && might extract this to a helper, since it is also used by BufferedResourceLoader. http://codereview.chromium.org/88047/diff/9007/8005#newcode547 Line 547: url_.spec(), position_, can bufferedresourceloader take a GURL? http://codereview.chromium.org/88047/diff/9007/8006 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/9007/8006#newcode192 Line 192: // |success| is true if the request has started successfully or false. comment needs updating.
http://codereview.chromium.org/88047/diff/9007/8005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/9007/8005#newcode440 Line 440: start_callback_->RunWithParams(Tuple1<int>(error)); On 2009/04/28 19:40:18, eroman wrote: > or simply: > start_callback_->Run(error); Done. http://codereview.chromium.org/88047/diff/9007/8005#newcode479 Line 479: if (!url_.SchemeIs(kHttpScheme) && !url_.SchemeIs(kHttpsScheme) && On 2009/04/28 19:40:18, eroman wrote: > might extract this to a helper, since it is also used by BufferedResourceLoader. Done. http://codereview.chromium.org/88047/diff/9007/8005#newcode547 Line 547: url_.spec(), position_, On 2009/04/28 19:40:18, eroman wrote: > can bufferedresourceloader take a GURL? Done. http://codereview.chromium.org/88047/diff/9007/8006 File chrome/renderer/media/buffered_data_source.h (right): http://codereview.chromium.org/88047/diff/9007/8006#newcode192 Line 192: // |success| is true if the request has started successfully or false. On 2009/04/28 19:40:18, eroman wrote: > comment needs updating. Done.
just missing some notifications for the filter host http://codereview.chromium.org/88047/diff/9007/8005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/9007/8005#newcode615 Line 615: host_->InitializationComplete(); if IsSeekable() is true, then add a call here to set the size of the resource: host_->SetTotalBytes(total_bytes_); also is there a way to tell how much we have buffered? if not you may want to lie for the time being (so seeking will work) and add a TODO: host_->SetBufferedBytes(total_bytes_);
http://codereview.chromium.org/88047/diff/9007/8005 File chrome/renderer/media/buffered_data_source.cc (right): http://codereview.chromium.org/88047/diff/9007/8005#newcode615 Line 615: host_->InitializationComplete(); On 2009/04/28 21:46:29, scherkus wrote: > if IsSeekable() is true, then add a call here to set the size of the resource: > host_->SetTotalBytes(total_bytes_); > > also is there a way to tell how much we have buffered? if not you may want to > lie for the time being (so seeking will work) and add a TODO: > host_->SetBufferedBytes(total_bytes_); yup, I want to lie. :)
LGTM also I think you want to remove the svn:executable bit from your source files svn pd svn:executable [files] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
