Fire canplaythrough event at the proper time for audio/video

Fire canplaythrough event at the proper time for audio/video

In this CL, the browser fires the canplaythrough event based on an
approximation of the download speed of the media instead of firing the
event right away.

BUG=73609
TEST=NONE

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

[vrk (LEFT CHROMIUM), 2011-10-27 01:08:48]

PTAL!

I need to do a bit more testing/probably need to add tests, but let me know what
you think of the approach and the criteria to issue canplaythrough!

[vrk (LEFT CHROMIUM), 2011-10-27 23:57:48]

Made modifications based on in-person conversation with acolwell! Please take
another look, thanks!!

[acolwell GONE FROM CHROMIUM, 2011-10-28 18:24:13]

looking pretty good. Just a few questions/comments.

http://codereview.chromium.org/8399023/diff/2001/media/base/demuxer.h
File media/base/demuxer.h (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/demuxer.h#newcode60
media/base/demuxer.h:60: // Returns the bitrate of media file. May be obtained
from container or
s/content bitrate/bitrate of media file/ since ChunkDemuxer doesn't deal with a
media file per se, but may want to expose the bitrate of the content it is
receiving.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:578: base::AutoLock auto_lock(lock_);
Why are we doing these operations here instead of in NotifyNetworkEventTask?

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1413: message_loop_->PostTask(FROM_HERE,
Do we need this PostTask? Aren't we always on the message_loop_ when this is
called? If not, what would we have to do to make it true.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1430: if (is_downloading_data_ && seconds_elapsed >
1) {
Make the time elapsed threshold a constant and lets use something like 5 seconds
to start with. Also include a comment above the constant indicating that this
value was pulled out of thin error and will need tuning

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1432: DCHECK(bytes_downloaded > 0);
Should this really be a DCHECK? Shouldn't we just put this as part of the
condition above. What if there happens to be some sort of network stall where we
don't get bytes for a little bit? I don't think we want to crash in that case.
We just want to keep the old estimate.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1440: if (has_notified_can_play_through_ ||
total_bytes_ == 0)
Might want to add a comment explaining what total_bytes_ == 0 reflects (ie live
streaming or that we were unable to determine the total size of the stream)

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1454: if (download_rate > bitrate * 8)
Shouldn't this be (8 * download_rate > bitrate) or (download_rate > bitrate / 8)
?

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1463: float seconds_buffered = (buffered_time -
current_time).InSecondsF();
std::max(..., 0) to protect the computation in release builds.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1464: int bytes_downloaded_after_buffer_is_consumed
=
s/after/while/ ?

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:343: // Returns an approximation of the download
speed of the resource loader in
Remove "of the resource loader" since that is an implementation detail of how
the FFmpegDemuxer -> BufferedDataSource->BufferedResourceLoader stack works.
Technically the pipeline should not be aware of such things.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:346: int ApproximateDownloadRate_Locked();
Might want to change this to ApproximateDownloadByteRate() so that the units are
clear at the call sites.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:498: unsigned long long starting_bytes_loaded_;
how about making this int64

[vrk (LEFT CHROMIUM), 2011-11-01 21:57:34]

Addressed acolwell's comments and uploaded the latest version of the download
rate estimation logic, now a bit more robust.

There are probably some bugs and kinks to work out still, but going to hold off
on further implementation until after we resolve the discussion on the WHATWG
list (as discussed offline).

http://codereview.chromium.org/8399023/diff/2001/media/base/demuxer.h
File media/base/demuxer.h (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/demuxer.h#newcode60
media/base/demuxer.h:60: // Returns the bitrate of media file. May be obtained
from container or
On 2011/10/28 18:24:13, acolwell wrote:
> s/content bitrate/bitrate of media file/ since ChunkDemuxer doesn't deal with
a
> media file per se, but may want to expose the bitrate of the content it is
> receiving.

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:578: base::AutoLock auto_lock(lock_);
On 2011/10/28 18:24:13, acolwell wrote:
> Why are we doing these operations here instead of in NotifyNetworkEventTask?

Hmm, no real reason; I guess I was doing this here because the
NotifyNetworkEventTask didn't take the "is_downloading_data" param.

Updated NotifyNetworkEventTask accordingly.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1413: message_loop_->PostTask(FROM_HERE,
On 2011/10/28 18:24:13, acolwell wrote:
> Do we need this PostTask? Aren't we always on the message_loop_ when this is
> called?

I believe we are not on this thread for 2 out of 3 call sites:
SetBufferedBytes() (render thread) and NotifyEnded (audio thread).

> If not, what would we have to do to make it true.
Post NotifyCanPlayThrough to message_loop_ at the calling sites?

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1430: if (is_downloading_data_ && seconds_elapsed >
1) {
On 2011/10/28 18:24:13, acolwell wrote:
> Make the time elapsed threshold a constant and lets use something like 5
seconds
> to start with. Also include a comment above the constant indicating that this
> value was pulled out of thin error and will need tuning

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1432: DCHECK(bytes_downloaded > 0);
On 2011/10/28 18:24:13, acolwell wrote:
> Should this really be a DCHECK? Shouldn't we just put this as part of the
> condition above. What if there happens to be some sort of network stall where
we
> don't get bytes for a little bit? I don't think we want to crash in that case.
> We just want to keep the old estimate.

Oops, the ">" was meant to be a ">=". Am fine with changing it to an if test
instead.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1440: if (has_notified_can_play_through_ ||
total_bytes_ == 0)
On 2011/10/28 18:24:13, acolwell wrote:
> Might want to add a comment explaining what total_bytes_ == 0 reflects (ie
live
> streaming or that we were unable to determine the total size of the stream)

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1454: if (download_rate > bitrate * 8)
On 2011/10/28 18:24:13, acolwell wrote:
> Shouldn't this be (8 * download_rate > bitrate) or (download_rate > bitrate /
8)
> ?

D'OH! Yes, absolutely. Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1463: float seconds_buffered = (buffered_time -
current_time).InSecondsF();
On 2011/10/28 18:24:13, acolwell wrote:
> std::max(..., 0) to protect the computation in release builds.

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1464: int bytes_downloaded_after_buffer_is_consumed
=
On 2011/10/28 18:24:13, acolwell wrote:
> s/after/while/ ?

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:343: // Returns an approximation of the download
speed of the resource loader in
On 2011/10/28 18:24:13, acolwell wrote:
> Remove "of the resource loader" since that is an implementation detail of how
> the FFmpegDemuxer -> BufferedDataSource->BufferedResourceLoader stack works.
> Technically the pipeline should not be aware of such things.

Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:346: int ApproximateDownloadRate_Locked();
On 2011/10/28 18:24:13, acolwell wrote:
> Might want to change this to ApproximateDownloadByteRate() so that the units
are
> clear at the call sites.

That's a good idea. Done.

http://codereview.chromium.org/8399023/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:498: unsigned long long starting_bytes_loaded_;
On 2011/10/28 18:24:13, acolwell wrote:
> how about making this int64

Done.

[vrk (LEFT CHROMIUM), 2011-11-08 22:11:09]

PTAL! Modified the CanPlayThrough heuristics to fit what we chatted about in
person last week. Also shoved all the logic relating to download rate into a new
class. I think it's cleaner and less prone to error, but I'm open to
alternatives as well!

Let me know what you think!

[acolwell GONE FROM CHROMIUM, 2011-11-09 00:50:21]

Looking good. Just a few comments & questions.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:24: if (!end_.second.is_null())
Change to CHECK based on out of band discussion.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:30: if (!start_.second.is_null())
ditto

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:36: if (seconds_elapsed() > 0.0 &&
bytes_downloaded() > 0)
This will hide situations like not getting any bytes for 10 seconds. Why do we
want to do this?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:48: return end_.first - start_.first;
no is_null() checks? I think this breaks if start_ is set, but end_ is not.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:61: start_ = end_;
does end_ need to be set to null here?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:88: buffered_bytes_ = buffered_bytes;
Probably should add a CHECK for your monotonically nondecreasing constraint
here.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:93: if (current_sample_.start_point().first
> buffered_bytes)
Doesn't this violate your monotonically nondecreasing expectation? Is this for
handling seeks? If so you may want to provide a way to explicitly signal a seek
to this object.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:108: if (is_downloading_data !=
is_downloading_data_)
consider reversing condition & early return.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:114: // There are max |kNumberOfSamples|;
might as well recompute each time.
remove ;

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:150: if (seconds_elapsed_in_window() > 0.0)
I don't think we need the DCHECK AND the if().

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:162: if (has_notified_can_play_through_ ||
total_bytes_ == 0)
so does this mean that autoplay can't be implemented for live streams?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:63: const Point& start_point() const { return
start_; }
nit accessor style: set_start(), set_end(), start(), end()

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:77: // Returns true if Sample is null, false
otherwise.
How about "Returns true if Sample has not been initialized."? This matches the
description in base::Time::is_null().

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline.h#newco...
media/base/pipeline.h:49: typedef base::Callback<void(NetworkEvent)>
NetworkEventCB;
I figured this bool wouldn't last long. ;)

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:372: is_downloading_data_ = false;
Does download_rate_monitor_ need to be reset here?

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:425: base::AutoLock auto_lock(lock_);
This is neaded because the SetXXXBytes() calls are on the Demuxer thread? You
should add a comment indicating why the lock is needed eventhough we are on the
pipeline thread here.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:598: is_downloading_data));
Is is_downloading_data still needed given that it's value is encoded in type? We
have the lock above so we could update is_downloading_data_ here.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.h#...
media/base/pipeline_impl.h:265: void NotifyNetworkEventTask(NetworkEvent type,
bool is_downloading_data);
Do we still need the bool?

http://codereview.chromium.org/8399023/diff/24001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:367: int ChunkDemuxer::GetBitrate() {
add a TODO(acolwell) : Implement bitrate reporting.

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:743: if (!can_play_through_)
I just want to double check what is going on here. On the initial play we'll
likely go to HaveFutureData and at some point later a network event will trigger
the transition to HaveEnoughData. Any seek after that time will go directly to
HaveEnoughData. 

What if a second seek happens before the "can play through" network event fires?
Won't this mean that the canplaythrough will happen after the second seek? Is
that ok?

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:809: if (!can_play_through_) {
Do we really only want to do this once or is it ok to do this transition if
ready_state_ != WebKit::WebMediaPlayer::HaveEnoughData?

[scherkus (not reviewing), 2011-11-09 02:55:15]

few nits but looking good!

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:145: // XXX: If the data is arriving really
bursty-ly, say getting a big chunk
hmm... hard to say whether we'll see that in practice

we require 5 samples at a minimum, correct?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:9: #include <stdio.h>
what's stdio.h used for?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:20: class DownloadRateMonitor {
class looks like a pretty good candidate for unit tests :)

if you want to wait until we iron out some details that's OK

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:29: void Start(const CanPlayThroughCB&
canplaythrough_cb, int media_bitrate);
is there no stop?

what happens if I call these other methods without calling Start()?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:55: typedef std::pair<int64, base::Time>
Point;
favour a two-member struct over std::pair

struct Point {
  base::Time time;
  int64 bytes_buffered;
};

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1431: &PipelineImpl::NotifyCanPlayThrough, this));
nit: can put this on previous line

http://codereview.chromium.org/8399023/diff/24001/media/filters/dummy_demuxer.cc
File media/filters/dummy_demuxer.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/filters/dummy_demuxer...
media/filters/dummy_demuxer.cc:52: // Bitrate is unknown.
nit: no need for comment

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:810: can_play_through_ = true;
so if we determine that our connection is "good enough" to satisfy the
can_play_through_ event then we assume we'll always be able to can play through
on future seeks?

[vrk (LEFT CHROMIUM), 2011-11-11 02:51:06]

Thanks for the review! Updated the CL to address comments.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:24: if (!end_.second.is_null())
On 2011/11/09 00:50:22, acolwell wrote:
> Change to CHECK based on out of band discussion.

I actually got rid of SetStartPoint altogether and changed some things around...
Take a look; I don't know if I did the right things! I'll talk to you in person
about it :)

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:30: if (!start_.second.is_null())
On 2011/11/09 00:50:22, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:36: if (seconds_elapsed() > 0.0 &&
bytes_downloaded() > 0)
On 2011/11/09 00:50:22, acolwell wrote:
> This will hide situations like not getting any bytes for 10 seconds. Why do we
> want to do this?

You're right, we don't want to hide that situation; this should be >= 0.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:48: return end_.first - start_.first;
On 2011/11/09 00:50:22, acolwell wrote:
> no is_null() checks? I think this breaks if start_ is set, but end_ is not.

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:61: start_ = end_;
On 2011/11/09 00:50:22, acolwell wrote:
> does end_ need to be set to null here?

It doesn't *need* to be, but maybe it makes more sense to have it that way.
Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:88: buffered_bytes_ = buffered_bytes;
On 2011/11/09 00:50:22, acolwell wrote:
> Probably should add a CHECK for your monotonically nondecreasing constraint
> here.

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:93: if (current_sample_.start_point().first
> buffered_bytes)
I'll answer this slightly out of order :)
On 2011/11/09 00:50:22, acolwell wrote:
> Is this for handling seeks? 
More generally, it's for anything that causes the resource loader to be
recreated, which often includes a seek. The resource loader is also recreated
when the video pauses and buffers for a long time then plays again.

> Doesn't this violate your monotonically nondecreasing expectation?
Yes... I think I don't want/need that expectation anymore :) It makes sense for
a buffer to drop (like if buffer data gets evicted). The expectation I really
care about is for the timestamps to be monotonically nondecreasing. I modified
the comment to say so.

> If so you may want to provide a way to explicitly signal a seek
> to this object.
Now I Stop() the monitor when a Seek starts and Start() again when playback
starts again.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:108: if (is_downloading_data !=
is_downloading_data_)
On 2011/11/09 00:50:22, acolwell wrote:
> consider reversing condition & early return.

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:114: // There are max |kNumberOfSamples|;
might as well recompute each time.
On 2011/11/09 00:50:22, acolwell wrote:
> remove ;

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:145: // XXX: If the data is arriving really
bursty-ly, say getting a big chunk
On 2011/11/09 02:55:15, scherkus wrote:
> hmm... hard to say whether we'll see that in practice
> 
> we require 5 samples at a minimum, correct?

This actually does happen in practice, in the contrived example in the bug
report:
http://flim.org/~kinetik/tests/bug627153/

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:150: if (seconds_elapsed_in_window() > 0.0)
On 2011/11/09 00:50:22, acolwell wrote:
> I don't think we need the DCHECK AND the if().

Without the if, there might be a divide-by-0 in Release mode.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:162: if (has_notified_can_play_through_ ||
total_bytes_ == 0)
On 2011/11/09 00:50:22, acolwell wrote:
> so does this mean that autoplay can't be implemented for live streams?

What it means is, one wouldn't be able to implement their own version of
autoplay by waiting for the canplaythrough event on a live stream. Sounds like a
potential issue to raise regarding the spec?

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:9: #include <stdio.h>
On 2011/11/09 02:55:15, scherkus wrote:
> what's stdio.h used for?

Uhh, debug printfs! Removed :)

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:20: class DownloadRateMonitor {
On 2011/11/09 02:55:15, scherkus wrote:
> class looks like a pretty good candidate for unit tests :)
I agree!

> if you want to wait until we iron out some details that's OK
Yup will do!

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:29: void Start(const CanPlayThroughCB&
canplaythrough_cb, int media_bitrate);
On 2011/11/09 02:55:15, scherkus wrote:
> is there no stop?
> 
> what happens if I call these other methods without calling Start()?

Added Stop(), added comments about Start() and Stop() to the relevant methods in
the header. Let me know if I should add more comments!

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:55: typedef std::pair<int64, base::Time>
Point;
On 2011/11/09 02:55:15, scherkus wrote:
> favour a two-member struct over std::pair
> 
> struct Point {
>   base::Time time;
>   int64 bytes_buffered;
> };

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:63: const Point& start_point() const { return
start_; }
On 2011/11/09 00:50:22, acolwell wrote:
> nit accessor style: set_start(), set_end(), start(), end()

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/download_rate_mo...
media/base/download_rate_monitor.h:77: // Returns true if Sample is null, false
otherwise.
On 2011/11/09 00:50:22, acolwell wrote:
> How about "Returns true if Sample has not been initialized."? This matches the
> description in base::Time::is_null().

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline.h#newco...
media/base/pipeline.h:49: typedef base::Callback<void(NetworkEvent)>
NetworkEventCB;
On 2011/11/09 00:50:22, acolwell wrote:
> I figured this bool wouldn't last long. ;)

:)

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:372: is_downloading_data_ = false;
On 2011/11/09 00:50:22, acolwell wrote:
> Does download_rate_monitor_ need to be reset here?

Yes, done.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:425: base::AutoLock auto_lock(lock_);
On 2011/11/09 00:50:22, acolwell wrote:
> This is neaded because the SetXXXBytes() calls are on the Demuxer thread?

Actually the SetXXXBytes method calls (and SetLoaded, and SetNetworkActivity)
originate from BufferedDataSource and so are called on the renderer thread. But
yes, those methods are the reason for the lock.

Though your question leads me to ask: Looking at the code, it appears that the
demuxer runs on the pipeline's thread. Is that not true?

> You should add a comment indicating why the lock is needed eventhough we are
on the
> pipeline thread here.

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:598: is_downloading_data));
On 2011/11/09 00:50:22, acolwell wrote:
> Is is_downloading_data still needed given that it's value is encoded in type?

AH oops, you're right! It served a purpose when the canplaythrough logic was
still in this class. Deleted. 

> We have the lock above so we could update is_downloading_data_ here.

Actually the lock is for download_rate_monitor_.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1431: &PipelineImpl::NotifyCanPlayThrough, this));
On 2011/11/09 02:55:15, scherkus wrote:
> nit: can put this on previous line

Done.

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/8399023/diff/24001/media/base/pipeline_impl.h#...
media/base/pipeline_impl.h:265: void NotifyNetworkEventTask(NetworkEvent type,
bool is_downloading_data);
On 2011/11/09 00:50:22, acolwell wrote:
> Do we still need the bool?

Nope! Removed.

http://codereview.chromium.org/8399023/diff/24001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:367: int ChunkDemuxer::GetBitrate() {
On 2011/11/09 00:50:22, acolwell wrote:
> add a TODO(acolwell) : Implement bitrate reporting.

Done.

http://codereview.chromium.org/8399023/diff/24001/media/filters/dummy_demuxer.cc
File media/filters/dummy_demuxer.cc (right):

http://codereview.chromium.org/8399023/diff/24001/media/filters/dummy_demuxer...
media/filters/dummy_demuxer.cc:52: // Bitrate is unknown.
On 2011/11/09 02:55:15, scherkus wrote:
> nit: no need for comment

Done.

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:743: if (!can_play_through_)
On 2011/11/09 00:50:22, acolwell wrote:
> I just want to double check what is going on here. On the initial play we'll
> likely go to HaveFutureData and at some point later a network event will
trigger
> the transition to HaveEnoughData. Any seek after that time will go directly to
> HaveEnoughData. 
> 
> What if a second seek happens before the "can play through" network event
fires?
> Won't this mean that the canplaythrough will happen after the second seek? Is
> that ok?

Talked in person. It appears I can get get rid of this SetReadyState call after
seek altogether! In the original code, it looks like it is always in the
HAVE_ENOUGH_DATA state whenever the code reached that point - which would also
make sense why CanPlayThrough is only fired once. I deleted the call and the
bool field.

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:809: if (!can_play_through_) {
On 2011/11/09 00:50:22, acolwell wrote:
> Do we really only want to do this once or is it ok to do this transition if
> ready_state_ != WebKit::WebMediaPlayer::HaveEnoughData?

For now, I think we really only want to do this once, *but* this is the wrong
place to enforce it (it's already enforced in DownloadRateMonitor). Fixed.

http://codereview.chromium.org/8399023/diff/24001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:810: can_play_through_ = true;
On 2011/11/09 02:55:15, scherkus wrote:
> so if we determine that our connection is "good enough" to satisfy the
> can_play_through_ event then we assume we'll always be able to can play
through
> on future seeks?

Yes. It's not necessarily a good assumption, but I think it might be good enough
for an initial implementation. I'd like to leave the task of reevaluating the
"canplaythrough" condition and possibly firing it multiple times to a different
CL.

[acolwell GONE FROM CHROMIUM, 2011-11-11 21:05:39]

Looks good. Just add some unit tests please.

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:178: if (seconds_elapsed_in_window() > 0.0)
remove DCHECK or if depending on whether you think this can actually happen
during correct operation.

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:71: void set_end(const BufferingPoint&
new_start);
new_end

[vrk (LEFT CHROMIUM), 2011-11-15 16:55:16]

Added tests.

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:178: if (seconds_elapsed_in_window() > 0.0)
On 2011/11/11 21:05:39, acolwell wrote:
> remove DCHECK or if depending on whether you think this can actually happen
> during correct operation.

Done.

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/33004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:71: void set_end(const BufferingPoint&
new_start);
On 2011/11/11 21:05:39, acolwell wrote:
> new_end

Done.

[acolwell GONE FROM CHROMIUM, 2011-11-15 21:43:10]

LGTM

Most of the comments are documentation nits at this point.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:180: // seconds until bitrate is calculated
... is that ok?
nit: Should probably remove the "is that ok?". Could also replace If w/ "In the
unlikely case"

Given the logic in UpdateSampleWindow(), if all goes well this will start
returning data after 5 seconds right?

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:231: return true;
Does this leave a gap between 2-5 seconds where canplaythrough can't fire?

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.h:22: typedef base::Callback<void()>
CanPlayThroughCB;
nit: Consider just using base::Closure. I don't think you need a separate type
here.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:64: 1000,
nit: Consider removing this parameter since it is 1000 almost everywhere. The
one place it isn't could be handled by multiplying bytes_per_packet by 100.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:68: TEST_F(DownloadRateMonitorTest,
DownloadRateGreaterThanBitrate_Pause) {
I don't understand what behavior this is testing. Please add a comment. Are we
just making sure that canplaythough will fire even if there is a huge delay
during download?

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:84: 2 * download_byte_rate,
nit: Consider having SimulateNetwork() return the final bytes_buffered value. I
think if you had a variable capture the return value from the call above and
pass it in here this code would be easier to follow.

[vrk (LEFT CHROMIUM), 2011-11-15 22:41:35]

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor.cc (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:180: // seconds until bitrate is calculated
... is that ok?
On 2011/11/15 21:43:10, acolwell wrote:
> nit: Should probably remove the "is that ok?". Could also replace If w/ "In
the
> unlikely case"
> 
> Given the logic in UpdateSampleWindow(), if all goes well this will start
> returning data after 5 seconds right?

Done and yes.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.cc:231: return true;
On 2011/11/15 21:43:10, acolwell wrote:
> Does this leave a gap between 2-5 seconds where canplaythrough can't fire?

Talked offline - yes, and this is intentional.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor.h:22: typedef base::Callback<void()>
CanPlayThroughCB;
On 2011/11/15 21:43:10, acolwell wrote:
> nit: Consider just using base::Closure. I don't think you need a separate type
> here.

Done.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:64: 1000,
On 2011/11/15 21:43:10, acolwell wrote:
> nit: Consider removing this parameter since it is 1000 almost everywhere. The
> one place it isn't could be handled by multiplying bytes_per_packet by 100.

I kept the param because I wanted to test in particular a big download that
happens in a very short amount of time - to make sure the DownloadRateMonitor
doesn't wait for more data because the time window isn't big enough.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:68: TEST_F(DownloadRateMonitorTest,
DownloadRateGreaterThanBitrate_Pause) {
On 2011/11/15 21:43:10, acolwell wrote:
> I don't understand what behavior this is testing. Please add a comment. Are we
> just making sure that canplaythough will fire even if there is a huge delay
> during download? 

Yup! If the user pauses and the pipeline stops downloading data, then the
DownloadRateMonitor should not get confused and think that the download was
stalling. Added a comment saying such.

http://codereview.chromium.org/8399023/diff/42006/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:84: 2 * download_byte_rate,
On 2011/11/15 21:43:10, acolwell wrote:
> nit: Consider having SimulateNetwork() return the final bytes_buffered value.
I
> think if you had a variable capture the return value from the call above and
> pass it in here this code would be easier to follow.

Good idea, done.

[scherkus (not reviewing), 2011-11-16 02:15:17]

LGTM w/ nits

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
File media/base/download_rate_monitor.h (right):

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:22: explicit DownloadRateMonitor();
nit: explicit isn't needed

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:41: void SetTotalBytes(int64 total_bytes) {
total_bytes_ = total_bytes; }
trivial setters/getters should be unix_hacker() style and typically you don't
even need comments as it's quite obvious what's going on

set_total_bytes()

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:44: void SetLoaded(bool loaded) { loaded_ =
loaded; }
set_loaded()

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
media/base/download_rate_monitor.h:66: explicit Sample();
no explicit

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
File media/base/download_rate_monitor_unittest.cc (right):

http://codereview.chromium.org/8399023/diff/47004/media/base/download_rate_mo...
media/base/download_rate_monitor_unittest.cc:31: int SimulateNetwork(const
base::Time& starting_time,
nit: you call base::Time::FromDoubleT() every time so what about making this a
double and doing the conversion here?

your tests should end up less indent-heavy

http://codereview.chromium.org/8399023/diff/47004/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/8399023/diff/47004/media/media.gyp#newcode116
media/media.gyp:116: 'base/download_rate_monitor.h',
djb2 comes before download*

http://codereview.chromium.org/8399023/diff/47004/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8399023/diff/47004/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:805: default:
indentation