Change how <video> fetch a resource to make it friendly to sparse caching

Change how <video> fetch a resource to make it friendly to sparse caching

BUG=16013
TEST=test_shell_tests --gtest_filter=Buffered*
     Also video on a server that doesn't support range request will still work.

How <video> used to fetch a resource:
1. Fetch the whole file to get the header
2. Fetch a small range (1, 1) to determine if server supports range request
3. If [2] was successful, then fetch the file with range request if necessary
   If [2] failed, prevent range request by telling ffmpeg this is streaming

New way of fetching a resource for <video>:
1. Fetch (0, 1023) to get the header (This needs more experiment).
2. If [1] was successful, then request later on will be made partial
   If [1] failed, prevent range request by telling ffmpeg this is streaming

By doing this change we can eliminate one request before we can start the
file. And with the help of sparse cache, we would be able to reuse the first
1KB fetched even we need to fetch the index at the end of file.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27587

[scherkus (not reviewing), 2009-09-29 06:36:48]

http://codereview.chromium.org/248012/diff/1/2
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/248012/diff/1/2#newcode233
Line 233: if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
hrmm this method is getting a little messy but I'm not sure how to clean up the
logic

http://codereview.chromium.org/248012/diff/1/2#newcode569
Line 569: loader_ = CreateLoader(0, 1024);
what's 1024?  why only 1024 and not more?  etc...

should elaborate on the TODO as well

http://codereview.chromium.org/248012/diff/1/2#newcode739
Line 739: // If the server didn't reply with an instance , it is likely this
an instance , -> an instance,

what's an instance again?  Is it simply an expected response to a request (i.e.,
206 if it was range, 200 if it wasn't).  It's a bit of a generic word

http://codereview.chromium.org/248012/diff/1/2#newcode754
Line 754: if (error == net::OK && loader_->partial_response()) {
since you set partial_response_ = range_requested_, this means we don't support
non range requested responses, correct?

http://codereview.chromium.org/248012/diff/1/2#newcode755
Line 755: // Once the range request has start successfully, we can proceed with
start -> started

http://codereview.chromium.org/248012/diff/1/3
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/248012/diff/1/3#newcode86
Line 86: // Returns true if the response for this loader is a partial response.
could you elaborate what a "partial response" means?  Is this a 206 response or
something?

http://codereview.chromium.org/248012/diff/1/3#newcode142
Line 142: bool completed_;
can we get some explanation on these variables?  I know you've documented the
functions above, but I believe range_requested_ has no explanation

http://codereview.chromium.org/248012/diff/1/4
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/248012/diff/1/4#newcode654
Line 654: // 3. Then the new loader will be started.
update this comment to say what the bit about partial_response() means

[Alpha Left Google, 2009-09-29 22:59:32]

http://codereview.chromium.org/248012/diff/1/2
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/248012/diff/1/2#newcode233
Line 233: if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
On 2009/09/29 06:36:48, scherkus wrote:
> hrmm this method is getting a little messy but I'm not sure how to clean up
the
> logic

Yeah.. Maybe taking this out to a new function.. but this isn't too long that
worth the change.. It's clear enough to me so I'd just leave it as such.

http://codereview.chromium.org/248012/diff/1/2#newcode569
Line 569: loader_ = CreateLoader(0, 1024);
On 2009/09/29 06:36:48, scherkus wrote:
> what's 1024?  why only 1024 and not more?  etc...
> 
> should elaborate on the TODO as well

Done.

http://codereview.chromium.org/248012/diff/1/2#newcode739
Line 739: // If the server didn't reply with an instance , it is likely this
On 2009/09/29 06:36:48, scherkus wrote:
> an instance , -> an instance,
> 
> what's an instance again?  Is it simply an expected response to a request
(i.e.,
> 206 if it was range, 200 if it wasn't).  It's a bit of a generic word

Should be instance size, I'm using a similar terminology as RFC 2616 14.16. What
it means is the full length of the resource requested.

http://codereview.chromium.org/248012/diff/1/2#newcode739
Line 739: // If the server didn't reply with an instance , it is likely this
On 2009/09/29 06:36:48, scherkus wrote:
> an instance , -> an instance,
> 
> what's an instance again?  Is it simply an expected response to a request
(i.e.,
> 206 if it was range, 200 if it wasn't).  It's a bit of a generic word

Should be instance size, this is defined in

http://codereview.chromium.org/248012/diff/1/2#newcode754
Line 754: if (error == net::OK && loader_->partial_response()) {
On 2009/09/29 06:36:48, scherkus wrote:
> since you set partial_response_ = range_requested_, this means we don't
support
> non range requested responses, correct?

This callback is invoked with the prerequisite that:
1. Server supports range request
2. We have verified the above fact (by doing the first 1K request)
3. We made a range request again

So if we don't get a partial response for this callback, it violates what we
have verified, thus an error.

We still support non range request server, just that this method wouldn't be
called in such case.

http://codereview.chromium.org/248012/diff/1/2#newcode755
Line 755: // Once the range request has start successfully, we can proceed with
On 2009/09/29 06:36:48, scherkus wrote:
> start -> started

Done.

http://codereview.chromium.org/248012/diff/1/2#newcode755
Line 755: // Once the range request has start successfully, we can proceed with
On 2009/09/29 06:36:48, scherkus wrote:
> start -> started

Done.

http://codereview.chromium.org/248012/diff/1/3
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/248012/diff/1/3#newcode86
Line 86: // Returns true if the response for this loader is a partial response.
On 2009/09/29 06:36:48, scherkus wrote:
> could you elaborate what a "partial response" means?  Is this a 206 response
or
> something?

Done.

[scherkus (not reviewing), 2009-09-29 23:10:34]

LGTM some tiny nits

http://codereview.chromium.org/248012/diff/4001/5001
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/248012/diff/4001/5001#newcode573
Line 573: loader_ = CreateLoader(0, 1024);
ok.. might have to request this be made a constant :)

kHeaderBytes or kInitialBytes or something

http://codereview.chromium.org/248012/diff/4001/5002
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/248012/diff/4001/5002#newcode142
Line 142: // True if resource loading is deffered.
deffered -> deferred

http://codereview.chromium.org/248012/diff/4001/5002#newcode145
Line 145: // True if resource loading is completed.
is -> has

http://codereview.chromium.org/248012/diff/4001/5002#newcode148
Line 148: // True if a range request is made.
is -> was

http://codereview.chromium.org/248012/diff/4001/5002#newcode151
Line 151: // True if response data received is a partial range.
is -> was

http://codereview.chromium.org/248012/diff/4001/5003
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/248012/diff/4001/5003#newcode470
Line 470: EXPECT_CALL(*data_source_, CreateLoader(0, 1024))
you can also use the constant here etc...