Fix media timeline so that thumb never exceeds buffered data

Fix media timeline so that thumb never exceeds buffered data

Caps PipelineImpl's current time to the time of the end of the last frame
buffered so that the thumb no longer continues progress when stalled.

BUG=99915
TEST=Playing video with limited bandwidth.


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

[vrk (LEFT CHROMIUM), 2012-01-10 02:43:12]

A few of my edits to PipelineImpl are a little strange/ugly; I'll explain in
person why I did what I did!

[acolwell GONE FROM CHROMIUM, 2012-01-10 22:08:10]

looks good and makes me happy. Just a few simple things to fix.

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

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:244: elapsed = std::min(elapsed, max_clock_time_);
It feels like this and the duration clamping should be in Clock. WDYT?

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:484: DCHECK(IsRunning());
DCHECK_LE(time, max_time);

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:506: DCHECK(!waiting_for_clock_update_);
DCHECK_GE(clock->Elapsed(), max_time) ?

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

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:233: // Callback elecuted by video renderer to update
clock time.
s/elecuted/executed/

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:160:
.WillOnce(Invoke(&RunFilterCallback4));
ick.. I wish we could find a better way to do this. :(

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:59: algorithm_.reset(NULL);
audio_time_cb_ & underflow_callback_ should probably be Reset() in here.

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:91:
MOCK_METHOD2(OnAudioTimeCallback, void(base::TimeDelta, base::TimeDelta));
It might be nice to have this be an actual method that verifies that time <=
max_time always holds.

http://codereview.chromium.org/9155003/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:65: 
statistics_callback_ & video_time_cb_ should probably be Reset() in here.

[scherkus (not reviewing), 2012-01-10 22:22:32]

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

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:244: elapsed = std::min(elapsed, max_clock_time_);
On 2012/01/10 22:08:11, acolwell wrote:
> It feels like this and the duration clamping should be in Clock. WDYT?

drive-by: yes

[vrk (LEFT CHROMIUM), 2012-01-13 02:48:41]

Ok! Please take another look. 

I'm not toootally confident that the Clock API changes make sense. In
particular: My changes let you set a media duration on the Clock, and it also
lets you call SetTime with times greater than the media duration, *but* calls to
Elapsed() will be capped at the duration. If that's weird, I could instead
modify PipelineImpl to check to make sure values sent to SetTime() do not exceed
the duration of the video.

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

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:244: elapsed = std::min(elapsed, max_clock_time_);
On 2012/01/10 22:22:33, scherkus wrote:
> On 2012/01/10 22:08:11, acolwell wrote:
> > It feels like this and the duration clamping should be in Clock. WDYT?
> 
> drive-by: yes

and Done!

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:244: elapsed = std::min(elapsed, max_clock_time_);
On 2012/01/10 22:08:11, acolwell wrote:
> It feels like this and the duration clamping should be in Clock. WDYT?

Done.

I put the duration in the Clock as well. Some of the calls look a little funky
(like clock_->SetMaxTime(clock_->media_duration()) so let me know what you
think.

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:484: DCHECK(IsRunning());
On 2012/01/10 22:08:11, acolwell wrote:
> DCHECK_LE(time, max_time);

Done.

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:506: DCHECK(!waiting_for_clock_update_);
On 2012/01/10 22:08:11, acolwell wrote:
> DCHECK_GE(clock->Elapsed(), max_time) ?

Done.

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

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:233: // Callback elecuted by video renderer to update
clock time.
On 2012/01/10 22:08:11, acolwell wrote:
> s/elecuted/executed/

Done.

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:160:
.WillOnce(Invoke(&RunFilterCallback4));
On 2012/01/10 22:08:11, acolwell wrote:
> ick.. I wish we could find a better way to do this. :(

ya :( 

was trying to think of ways to make this less gross, but my initial stabs just
made things worse, haha, so I stuck with the already-existing pattern!

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:59: algorithm_.reset(NULL);
On 2012/01/10 22:08:11, acolwell wrote:
> audio_time_cb_ & underflow_callback_ should probably be Reset() in here.

Done.

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:91:
MOCK_METHOD2(OnAudioTimeCallback, void(base::TimeDelta, base::TimeDelta));
On 2012/01/10 22:08:11, acolwell wrote:
> It might be nice to have this be an actual method that verifies that time <=
> max_time always holds.

Done.

http://codereview.chromium.org/9155003/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:65: 
On 2012/01/10 22:08:11, acolwell wrote:
> statistics_callback_ & video_time_cb_ should probably be Reset() in here.

Done.

[acolwell GONE FROM CHROMIUM, 2012-01-13 22:51:21]

looking good. I get more excited every iteration I see on the CL!

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode49
media/base/clock.cc:49: DCHECK_LE(current_time.ToInternalValue(),
max_time.ToInternalValue());
How about DCHECK(current_time <= max_time)? The ToInternalValue() feels ugly.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode65
media/base/clock.cc:65: if (max_time_.ToInternalValue() > 0)
How about using != kNoTimestamp instead of > 0?

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode68
media/base/clock.cc:68: if (duration_.ToInternalValue() > 0 && elapsed >
duration_)
How about using != kNoTimestamp instead of > 0?

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h
File media/base/clock.h (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode54
media/base/clock.h:54: void SetTime(base::TimeDelta current_time,
base::TimeDelta max_time);
Comment should mention that the values are clamped by duration_

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode57
media/base/clock.h:57: max_time_ = max_time;
Implementation should be in .cc & should be clamped to 0 <= max_time <=
duration_

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode63
media/base/clock.h:63: void set_media_duration(base::TimeDelta duration) {
duration_ = duration; }
How about dropping media?
Impl in .cc & DCHECK(duration >= base::TimeDelta())
We should probably also add a DCHECK() to make sure duration_ doesn't get
smaller if it has already been set. If that was allowed it could cause the clock
to go backwards!

http://codereview.chromium.org/9155003/diff/19003/media/base/clock_unittest.cc
File media/base/clock_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock_unittest.c...
media/base/clock_unittest.cc:227: 
Please add some unit tests for set_max_time enforcement.

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

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:347: clock_->set_media_duration(kZero);
Replace this and the call below with a clock_->Reset()?

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:903: clock_->set_max_time(seek_timestamp_);
I'm pretty sure this needs to be inside the locked block below  or TSAN will be
unhappy. 

Why is this needed? It seems like it could cause weird problems if one seeks far
ahead into the future. It would temporarily override the value set by the audio
& video time update calls. I think we still have to worry about those coming in
until the pipeline_filter_->Pause() completes.

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:946: clock_->SetTime(clock_->media_duration(),
clock_->media_duration());
How about clock_->EndOfStream() or something similar? You might consider
transitioning from playing to paused state inside the clock, but I don't know if
that will have an adverse effect elsewhere.

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1179: clock_->SetTime(demuxer_->GetStartTime(),
clock_->media_duration());
I think we might want to use demuxer_->GetStartTime() here for the max too. The
comments above lead me to believe we don't want the clock to move forward so
setting both params to that makes sure that happens.

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:261: new_current_time > host()->GetTime()))
{
Perhaps enforce this last check in PipelineImpl so this class doesn't rely on
FilterHost::GetTime(). It's on the termination list too. ;)

[vrk (LEFT CHROMIUM), 2012-01-21 00:54:14]

PTAL acolwell!

When I tried to add more unit tests, I realized my original attempt at adding a
max time to Clock was not very robust... so the innards of Clock got changed
quite a bit! Then after the Clock changes, the Clock unit tests were really
crummy to write, so I rewrote the unit tests as well!

Sorry for the large changes :) hopefully for the better, though! Let me know
what you think.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode49
media/base/clock.cc:49: DCHECK_LE(current_time.ToInternalValue(),
max_time.ToInternalValue());
On 2012/01/13 22:51:21, acolwell wrote:
> How about DCHECK(current_time <= max_time)? The ToInternalValue() feels ugly.

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode65
media/base/clock.cc:65: if (max_time_.ToInternalValue() > 0)
On 2012/01/13 22:51:21, acolwell wrote:
> How about using != kNoTimestamp instead of > 0?

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.cc#newcode68
media/base/clock.cc:68: if (duration_.ToInternalValue() > 0 && elapsed >
duration_)
On 2012/01/13 22:51:21, acolwell wrote:
> How about using != kNoTimestamp instead of > 0?

Actually, I modified logic of this method to return 0 when duration is not set
(to match the preexisting behavior).

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h
File media/base/clock.h (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode54
media/base/clock.h:54: void SetTime(base::TimeDelta current_time,
base::TimeDelta max_time);
On 2012/01/13 22:51:21, acolwell wrote:
> Comment should mention that the values are clamped by duration_

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode57
media/base/clock.h:57: max_time_ = max_time;
On 2012/01/13 22:51:21, acolwell wrote:
> Implementation should be in .cc & should be clamped to 0 <= max_time <=
> duration_

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock.h#newcode63
media/base/clock.h:63: void set_media_duration(base::TimeDelta duration) {
duration_ = duration; }
On 2012/01/13 22:51:21, acolwell wrote:
> How about dropping media?
> Impl in .cc & DCHECK(duration >= base::TimeDelta())
> We should probably also add a DCHECK() to make sure duration_ doesn't get
> smaller if it has already been set. If that was allowed it could cause the
clock
> to go backwards!

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/clock_unittest.cc
File media/base/clock_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/base/clock_unittest.c...
media/base/clock_unittest.cc:227: 
On 2012/01/13 22:51:21, acolwell wrote:
> Please add some unit tests for set_max_time enforcement.

Done... and actually this was such an annoying test to add that I rewrote most
of this class to make test-adding easier!

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

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:347: clock_->set_media_duration(kZero);
On 2012/01/13 22:51:21, acolwell wrote:
> Replace this and the call below with a clock_->Reset()?

Done.

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:903: clock_->set_max_time(seek_timestamp_);
On 2012/01/13 22:51:21, acolwell wrote:
> I'm pretty sure this needs to be inside the locked block below  or TSAN will
be
> unhappy. 
> 
> Why is this needed? It seems like it could cause weird problems if one seeks
far
> ahead into the future. It would temporarily override the value set by the
audio
> & video time update calls. I think we still have to worry about those coming
in
> until the pipeline_filter_->Pause() completes.

I believe the reason why I did this was to prevent the thumb from moving forward
in between the start of a seek (where the thumb moves immediately) and the video
catching up to the seek....

... but unfortunately it's hard for me to confirm this remotely :( On second
thought it seems like it would cause problems, as you said. For now I have
deleted the line, and I will test again when I get back to my desk!

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:946: clock_->SetTime(clock_->media_duration(),
clock_->media_duration());
On 2012/01/13 22:51:21, acolwell wrote:
> How about clock_->EndOfStream() or something similar? 
Done.

> You might consider
> transitioning from playing to paused state inside the clock, but I don't know
if
> that will have an adverse effect elsewhere.

Considering there's a call to Pause() right above this call, looks like it'll be
okay :) I smooshed that into the EndOfStream impl as well.

http://codereview.chromium.org/9155003/diff/19003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1179: clock_->SetTime(demuxer_->GetStartTime(),
clock_->media_duration());
On 2012/01/13 22:51:21, acolwell wrote:
> I think we might want to use demuxer_->GetStartTime() here for the max too.
The
> comments above lead me to believe we don't want the clock to move forward so
> setting both params to that makes sure that happens.

Done.

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:261: new_current_time > host()->GetTime()))
{
On 2012/01/13 22:51:21, acolwell wrote:
> Perhaps enforce this last check in PipelineImpl so this class doesn't rely on
> FilterHost::GetTime(). It's on the termination list too. ;)

I was thinking the same thing, but it looks like it's not especially easy to do
right now. Moving the "new_current_time > host()->GetTime()" into Pipeline is
easy, but since the condition is ||ed not &&ed, I would need to also move the
last_fill_buffer_time_ != last_fill_buffer_time into PipelineImpl...

Or, I suppose I could add a bool parameter to the callback? WDYT?

[acolwell GONE FROM CHROMIUM, 2012-01-24 18:13:59]

Looks good. Just a few tiny questions and then I think we're good to go.

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9155003/diff/19003/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:261: new_current_time > host()->GetTime()))
{
On 2012/01/21 00:54:14, Victoria Kirst wrote:
> On 2012/01/13 22:51:21, acolwell wrote:
> > Perhaps enforce this last check in PipelineImpl so this class doesn't rely
on
> > FilterHost::GetTime(). It's on the termination list too. ;)
> 
> I was thinking the same thing, but it looks like it's not especially easy to
do
> right now. Moving the "new_current_time > host()->GetTime()" into Pipeline is
> easy, but since the condition is ||ed not &&ed, I would need to also move the
> last_fill_buffer_time_ != last_fill_buffer_time into PipelineImpl...
> 
> Or, I suppose I could add a bool parameter to the callback? WDYT?
Hmm.. ok. I think we should leave this for another CL. Nevermind.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc#newcode44
media/base/clock.cc:44: if (current_time == kNoTimestamp()) {
Should we convert this into a DCHECK? Any idea why this is here or why it was
added?

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc#newcode120
media/base/clock.cc:120: if (IsTimeProgressing())
I think this function body can be replaced with
UpdateReferencePoints(Elapsed()). If so just inline IsTimeProgressing() in
Elapsed() since that's the only place it is used.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.cc
File media/base/clock_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:178:
clock_.SetTime(base::TimeDelta::FromSeconds(12), clock_.Duration());
kUpdatedTime?

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:235: clock_.SetMaxTime(clock_.Duration());
Why Duration() here instead of kMaxTime1?

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:254: }
\o/ these tests are so much clearer!!!

http://codereview.chromium.org/9155003/diff/39002/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:696: pipeline_->SetClockForTesting(new
Clock(&StaticClockFunction));
I think if you move this before InitializeDemuxer() the duration will be set
properly and you won't need the line below.

[vrk (LEFT CHROMIUM), 2012-01-25 00:09:32]

I also fixed a couple of tests I had broken/flakified; I added comments to
highlight what changes were made.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc#newcode44
media/base/clock.cc:44: if (current_time == kNoTimestamp()) {
On 2012/01/24 18:13:59, acolwell wrote:
> Should we convert this into a DCHECK? Any idea why this is here or why it was
> added?

Introduced in this CL: http://codereview.chromium.org/173072

It was originally a DCHECK, then there was conversation to make it an if
instead. http://codereview.chromium.org/173072/diff/21/23#newcode39

But yeah, judging by the code, it looks like having a current_time ==
kNoTimestamp() is an indication of programmer error. Changing it to a DCHECK.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock.cc#newcode120
media/base/clock.cc:120: if (IsTimeProgressing())
On 2012/01/24 18:13:59, acolwell wrote:
> I think this function body can be replaced with
> UpdateReferencePoints(Elapsed()). If so just inline IsTimeProgressing() in
> Elapsed() since that's the only place it is used.

Yes, you are right! Done.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.cc
File media/base/clock_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:178:
clock_.SetTime(base::TimeDelta::FromSeconds(12), clock_.Duration());
On 2012/01/24 18:13:59, acolwell wrote:
> kUpdatedTime?

Done.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:235: clock_.SetMaxTime(clock_.Duration());
On 2012/01/24 18:13:59, acolwell wrote:
> Why Duration() here instead of kMaxTime1?

I wanted to test what happens if the MaxTime gets set to something large, then
gets set to something smaller. (This happens in the code when the pipeline is
done initializing and there's no audio, so max time is set to duration as the
clock runs wild and free... until the first SetMax gets called!)

I could make this a separate test if you want!

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:254: }
On 2012/01/24 18:13:59, acolwell wrote:
> \o/ these tests are so much clearer!!!

Yay!! :)

http://codereview.chromium.org/9155003/diff/39002/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:696: pipeline_->SetClockForTesting(new
Clock(&StaticClockFunction));
On 2012/01/24 18:13:59, acolwell wrote:
> I think if you move this before InitializeDemuxer() the duration will be set
> properly and you won't need the line below.

Doing this actually made the test fail... because it exposed a subtle bug!
Statement moved and bug fixed, thanks!

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc#newcode48
media/base/clock.cc:48: bool paused_for_set_time = false;
Needed to add this logic to fix a flakiness that was introduced in
media/media-ended.html. (Doesn't fix the (already-existing) occasional timeout,
unfortunately!)

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:914: clock_->SetMaxTime(clock_->Duration());
(this is the change that was made to fix the bug referenced in
media/base/pipeline_unittest.cc)

[acolwell GONE FROM CHROMIUM, 2012-01-25 05:20:29]

almost..there... Just want to sync up on the SetTime() change.

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.cc
File media/base/clock_unittest.cc (right):

http://codereview.chromium.org/9155003/diff/39002/media/base/clock_unittest.c...
media/base/clock_unittest.cc:235: clock_.SetMaxTime(clock_.Duration());
On 2012/01/25 00:09:32, Victoria Kirst wrote:
> On 2012/01/24 18:13:59, acolwell wrote:
> > Why Duration() here instead of kMaxTime1?
> 
> I wanted to test what happens if the MaxTime gets set to something large, then
> gets set to something smaller. (This happens in the code when the pipeline is
> done initializing and there's no audio, so max time is set to duration as the
> clock runs wild and free... until the first SetMax gets called!)
>
Ok that is what I thought might be the case. Just wanted to check.
 
> I could make this a separate test if you want!
No. This is fine.

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc#newcode48
media/base/clock.cc:48: bool paused_for_set_time = false;
On 2012/01/25 00:09:32, Victoria Kirst wrote:
> Needed to add this logic to fix a flakiness that was introduced in
> media/media-ended.html. (Doesn't fix the (already-existing) occasional
timeout,
> unfortunately!)
Whoa! Something doesn't seem right here. IM me when you get a chance so we can
talk about it. I think you might be getting into trouble because you are using
SetMaxTime() here. I think you can just copy the max_time_ & underflow_
assignments from that method and you'll be good. I might be missing something
though.

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:914: clock_->SetMaxTime(clock_->Duration());
On 2012/01/25 00:09:32, Victoria Kirst wrote:
> (this is the change that was made to fix the bug referenced in
> media/base/pipeline_unittest.cc)
oooo...tricky. Do we need this in DisableAudioRendererTask() too?

[vrk (LEFT CHROMIUM), 2012-01-26 00:06:53]

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc
File media/base/clock.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/clock.cc#newcode48
media/base/clock.cc:48: bool paused_for_set_time = false;
On 2012/01/25 05:20:30, acolwell wrote:
> On 2012/01/25 00:09:32, Victoria Kirst wrote:
> > Needed to add this logic to fix a flakiness that was introduced in
> > media/media-ended.html. (Doesn't fix the (already-existing) occasional
> timeout,
> > unfortunately!)
> Whoa! Something doesn't seem right here. IM me when you get a chance so we can
> talk about it. I think you might be getting into trouble because you are using
> SetMaxTime() here. I think you can just copy the max_time_ & underflow_
> assignments from that method and you'll be good. I might be missing something
> though.

Talked offline; Done!

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/9155003/diff/48001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:914: clock_->SetMaxTime(clock_->Duration());
On 2012/01/25 05:20:30, acolwell wrote:
> On 2012/01/25 00:09:32, Victoria Kirst wrote:
> > (this is the change that was made to fix the bug referenced in
> > media/base/pipeline_unittest.cc)
> oooo...tricky. Do we need this in DisableAudioRendererTask() too?

Good catch! I am pretty positive that we do, so I added it... Unfortunately I
can't test it because in the AudioRendererImpl -> AudioDevice migration,
propogating DisableAudioRenderer() was omitted! (crbug.com/111409)

[acolwell GONE FROM CHROMIUM, 2012-01-26 01:11:28]

LGTM \o/

[commit-bot: I haz the power, 2012-02-02 00:46:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9155003/54001

[commit-bot: I haz the power, 2012-02-02 00:46:26]

Can't apply patch for file
content/renderer/media/audio_renderer_impl_unittest.cc.
While running patch -p1 --forward --force;
patching file content/renderer/media/audio_renderer_impl_unittest.cc
Hunk #1 FAILED at 107.
1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/media/audio_renderer_impl_unittest.cc.rej

[commit-bot: I haz the power, 2012-02-02 01:26:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9155003/55022

[commit-bot: I haz the power, 2012-02-02 02:52:43]

Change committed as 120151