Reporting a more accurate buffered time for the video tag

Reporting a more accurate buffered time for the video tag

Instead of showing that the entire file is buffered when the video first loads, this uses current time, current bytes, and buffered bytes to estimate buffered time.

[Alpha Left Google, 2010-05-20 01:46:54]

one last comment and this is good to go. Thanks for doing this!

http://codereview.chromium.org/2085012/diff/3001/2005
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2085012/diff/3001/2005#newcode161
media/base/pipeline_impl.h:161: 
nit: remove this empty line.

[vrk (LEFT CHROMIUM), 2010-05-20 16:49:12]

http://codereview.chromium.org/2085012/diff/3001/2005
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2085012/diff/3001/2005#newcode161
media/base/pipeline_impl.h:161: 
On 2010/05/20 01:46:54, Alpha wrote:
> nit: remove this empty line.

Done.

[Alpha Left Google, 2010-05-20 18:45:38]

Awsome. LGTM.

[scherkus (not reviewing), 2010-05-20 19:28:27]

tiny nits

http://codereview.chromium.org/2085012/diff/10001/11004
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2085012/diff/10001/11004#newcode93
media/base/pipeline_impl.h:93: virtual int64 GetCurrentReadPosition();
do these need to be public or can they be private with the rest of the
filterhost methods?

http://codereview.chromium.org/2085012/diff/10001/11004#newcode366
media/base/pipeline_impl.h:366: // currently reading
nits: we try to refer to functions in comments with () appended to them -- also
ending comments with periods.

http://codereview.chromium.org/2085012/diff/10001/11004#newcode370
media/base/pipeline_impl.h:370: // smooth
ditto w/ comment

http://codereview.chromium.org/2085012/diff/10001/11004#newcode371
media/base/pipeline_impl.h:371: // FIXME: This is a hack ...
we tend to use TODO(username) over FIXME

http://codereview.chromium.org/2085012/diff/10001/11007
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/2085012/diff/10001/11007#newcode191
webkit/glue/webmediaplayer_impl.h:191: virtual const WebKit::WebTimeRanges&
buffered();
did we change the definition of WebKit::WebMediaPlayer?

[vrk (LEFT CHROMIUM), 2010-05-21 18:10:13]

http://codereview.chromium.org/2085012/diff/10001/11004
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2085012/diff/10001/11004#newcode93
media/base/pipeline_impl.h:93: virtual int64 GetCurrentReadPosition();
Turns out these can be private! Though I am a little confused - how come
FFmpegDemuxer can call these methods if they are private? (I guess I am still in
a Java-mindset ...)


On 2010/05/20 19:28:27, scherkus wrote:
> do these need to be public or can they be private with the rest of the
> filterhost methods?

http://codereview.chromium.org/2085012/diff/10001/11004#newcode366
media/base/pipeline_impl.h:366: // currently reading
On 2010/05/20 19:28:27, scherkus wrote:
> nits: we try to refer to functions in comments with () appended to them --
also
> ending comments with periods.

Done.

http://codereview.chromium.org/2085012/diff/10001/11004#newcode370
media/base/pipeline_impl.h:370: // smooth
On 2010/05/20 19:28:27, scherkus wrote:
> ditto w/ comment

Done.

http://codereview.chromium.org/2085012/diff/10001/11004#newcode371
media/base/pipeline_impl.h:371: // FIXME: This is a hack ...
On 2010/05/20 19:28:27, scherkus wrote:
> we tend to use TODO(username) over FIXME

Done.

http://codereview.chromium.org/2085012/diff/10001/11007
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/2085012/diff/10001/11007#newcode191
webkit/glue/webmediaplayer_impl.h:191: virtual const WebKit::WebTimeRanges&
buffered();
Yes, and because of that, this was originally going to be a 2-sided patch, but
now we are doing 3 one-sided patches to get things working.

On 2010/05/20 19:28:27, scherkus wrote:
> did we change the definition of WebKit::WebMediaPlayer?

[Alpha Left Google, 2010-05-21 18:16:17]

On 2010/05/21 18:10:13, Victoria Kirst wrote:
> http://codereview.chromium.org/2085012/diff/10001/11004
> File media/base/pipeline_impl.h (right):
> 
> http://codereview.chromium.org/2085012/diff/10001/11004#newcode93
> media/base/pipeline_impl.h:93: virtual int64 GetCurrentReadPosition();
> Turns out these can be private! Though I am a little confused - how come
> FFmpegDemuxer can call these methods if they are private? (I guess I am still
in
> a Java-mindset ...)
> 
> 
> On 2010/05/20 19:28:27, scherkus wrote:
> > do these need to be public or can they be private with the rest of the
> > filterhost methods?
> 
> http://codereview.chromium.org/2085012/diff/10001/11004#newcode366
> media/base/pipeline_impl.h:366: // currently reading
> On 2010/05/20 19:28:27, scherkus wrote:
> > nits: we try to refer to functions in comments with () appended to them --
> also
> > ending comments with periods.
> 
> Done.
> 
> http://codereview.chromium.org/2085012/diff/10001/11004#newcode370
> media/base/pipeline_impl.h:370: // smooth
> On 2010/05/20 19:28:27, scherkus wrote:
> > ditto w/ comment
> 
> Done.
> 
> http://codereview.chromium.org/2085012/diff/10001/11004#newcode371
> media/base/pipeline_impl.h:371: // FIXME: This is a hack ...
> On 2010/05/20 19:28:27, scherkus wrote:
> > we tend to use TODO(username) over FIXME
> 
> Done.
> 
> http://codereview.chromium.org/2085012/diff/10001/11007
> File webkit/glue/webmediaplayer_impl.h (right):
> 
> http://codereview.chromium.org/2085012/diff/10001/11007#newcode191
> webkit/glue/webmediaplayer_impl.h:191: virtual const WebKit::WebTimeRanges&
> buffered();
> Yes, and because of that, this was originally going to be a 2-sided patch, but
> now we are doing 3 one-sided patches to get things working.
> 
> On 2010/05/20 19:28:27, scherkus wrote:
> > did we change the definition of WebKit::WebMediaPlayer?

LGTM.

[scherkus (not reviewing), 2010-05-21 18:44:08]

LGTM w/ nits

Alpha can probably land it for you :)

http://codereview.chromium.org/2085012/diff/10001/11004
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2085012/diff/10001/11004#newcode93
media/base/pipeline_impl.h:93: virtual int64 GetCurrentReadPosition();
On 2010/05/21 18:10:13, Victoria Kirst wrote:
> Turns out these can be private! Though I am a little confused - how come
> FFmpegDemuxer can call these methods if they are private? (I guess I am still
in
> a Java-mindset ...)
> 
> 
> On 2010/05/20 19:28:27, scherkus wrote:
> > do these need to be public or can they be private with the rest of the
> > filterhost methods?

It's via the magic of casting and strong type checking!!

The filters have a reference to a FilterHost where the methods are all declared
public.  From its perspective it doesn't really care how these methods are
implemented (private, etc..).

PipelineImpl implements those methods, but C++ doesn't care whether they're
public/protected/private (subclasses can change the access level of any method
at will).  As long as they're implemented the C++ compiler is happy.

Basically public/protected/private is a compile-time feature because at the end
of the day we're just jumping to various functions and all that access level
stuff is thrown out the door.

http://codereview.chromium.org/2085012/diff/23001/24006
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/2085012/diff/23001/24006#newcode443
webkit/glue/webmediaplayer_impl.cc:443: // This will be deleted once the
WebKit::WebMediaPlayer interface is updated.
Add TODO(vrk)

http://codereview.chromium.org/2085012/diff/23001/24007
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/2085012/diff/23001/24007#newcode194
webkit/glue/webmediaplayer_impl.h:194: // two-sided commit.
Add TODO(vrk)

[fbarchard, 2010-05-21 20:59:50]

LGTM
Its not a perfect solution, because the purpose of showing what is buffered is
the user knows where they can seek quickly.  That would include the disk cache.
In the future, we should 'autobuffer' more but perhaps to the disk cache, not
memory.