Suppress slider thumb jumping around during seeking...

Suppress slider thumb jumping around during seeking

BUG=19396
TEST=Open a video, seek to any position, the thumb should stops a bit and then goes forward from there.

Preventing the slider thumb from jumping around after seek
by freezing the clock until we get a valid time update from
the audio renderer.


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

[Alpha Left Google, 2009-08-19 19:36:13]

no test yet, just shows the idea..

[Alpha Left Google, 2009-08-20 06:26:28]

this code is good for testing again.

[scherkus (not reviewing), 2009-08-20 21:40:46]

fix initial comments, but looking good!

http://codereview.chromium.org/173072/diff/21/22
File media/base/clock.h (right):

http://codereview.chromium.org/173072/diff/21/22#newcode44
Line 44: static const base::TimeDelta kInvalidTimestamp;
nit: constants before methods:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

also we need a comment here

http://codereview.chromium.org/173072/diff/21/23
File media/base/clock_impl.cc (right):

http://codereview.chromium.org/173072/diff/21/23#newcode12
Line 12: base::TimeDelta::FromMicroseconds(-1);
thinking about this, -1 still isn't quite good enough

we need to use something extreme such as:
std::numeric_limits<int64>::min()

http://codereview.chromium.org/173072/diff/21/23#newcode39
Line 39: DCHECK(time != kInvalidTimestamp);
nit: DCHECK_NE

also what happens in release builds?  perhaps we should return or will it be ok
if we set it to an invalid timestamp?

http://codereview.chromium.org/173072/diff/21/24
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/173072/diff/21/24#newcode77
Line 77: waiting_for_clock_update_(false),
don't forget to set this back to false in ResetState()

http://codereview.chromium.org/173072/diff/21/24#newcode356
Line 356: clock_.SetTime(time);
nit: even though this is locked, perhaps we should set time before playing

otherwise clock would report:
t=5 (Paused)
...
t=5
t=6 (Play)
t=7
...
t=0 (SetTime)
t=1
t=2
...

http://codereview.chromium.org/173072/diff/21/24#newcode777
Line 777: bool has_audio = IsRendered(mime_type::kMajorTypeAudio);
comment here

http://codereview.chromium.org/173072/diff/21/24#newcode779
Line 779: waiting_for_clock_update_ = has_audio;
nit: you can directly implement IsRendered() while holding the lock:

AutoLock auto_lock(lock_);
waiting_for_clock_update_ =
    rendered_mime_types_.find(mime_type::kMajorTypeAudio) !=
rendered_mime_types_.end();
if (!waiting_for_clock_update_)
  clock_.Play();

http://codereview.chromium.org/173072/diff/21/28
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/173072/diff/21/28#newcode312
Line 312: // for an update of the clock greater than or equals to the elapsed
time to
equals -> equal

http://codereview.chromium.org/173072/diff/21/27
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/173072/diff/21/27#newcode80
Line 80: estimated_next_timestamp_ = Clock::kInvalidTimestamp;
hmm I don't like how these classes have to know about clock

I would recommend moving kInvalidTimestamp to media/base/buffers.h

http://codereview.chromium.org/173072/diff/21/27#newcode118
Line 118: if (input->GetDuration() == Clock::kInvalidTimestamp ||
duration can be an invalid timestamp?  wouldn't a zero duration make more sense?
 when does this occur?

http://codereview.chromium.org/173072/diff/21/27#newcode134
Line 134: // if it is valid (i.e. >= 0).  Otherwise the error will stack
if we're using kInvalidTimestamp, than any timestamp != kInvalidTimestamp is
valid

[Alpha Left Google, 2009-08-20 22:23:58]

http://codereview.chromium.org/173072/diff/21/22
File media/base/clock.h (right):

http://codereview.chromium.org/173072/diff/21/22#newcode44
Line 44: static const base::TimeDelta kInvalidTimestamp;
On 2009/08/20 21:40:46, scherkus wrote:
> nit: constants before methods:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> also we need a comment here

Done.

http://codereview.chromium.org/173072/diff/21/23
File media/base/clock_impl.cc (right):

http://codereview.chromium.org/173072/diff/21/23#newcode12
Line 12: base::TimeDelta::FromMicroseconds(-1);
On 2009/08/20 21:40:46, scherkus wrote:
> thinking about this, -1 still isn't quite good enough
> 
> we need to use something extreme such as:
> std::numeric_limits<int64>::min()

Done.

http://codereview.chromium.org/173072/diff/21/23#newcode39
Line 39: DCHECK(time != kInvalidTimestamp);
On 2009/08/20 21:40:46, scherkus wrote:
> nit: DCHECK_NE
> 
> also what happens in release builds?  perhaps we should return or will it be
ok
> if we set it to an invalid timestamp?

there's no printing overloaded for TimeDelta, I can DCHECK the internal value
instead. Setting to invalid timestamp is difinitely not ok.. I'll use if then.

http://codereview.chromium.org/173072/diff/21/24
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/173072/diff/21/24#newcode77
Line 77: waiting_for_clock_update_(false),
On 2009/08/20 21:40:46, scherkus wrote:
> don't forget to set this back to false in ResetState()

Done.

http://codereview.chromium.org/173072/diff/21/24#newcode356
Line 356: clock_.SetTime(time);
On 2009/08/20 21:40:46, scherkus wrote:
> nit: even though this is locked, perhaps we should set time before playing
> 
> otherwise clock would report:
> t=5 (Paused)
> ...
> t=5
> t=6 (Play)
> t=7
> ...
> t=0 (SetTime)
> t=1
> t=2
> ...
> 
> 

Done.

http://codereview.chromium.org/173072/diff/21/28
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/173072/diff/21/28#newcode312
Line 312: // for an update of the clock greater than or equals to the elapsed
time to
On 2009/08/20 21:40:46, scherkus wrote:
> equals -> equal

Done.

http://codereview.chromium.org/173072/diff/21/27
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/173072/diff/21/27#newcode80
Line 80: estimated_next_timestamp_ = Clock::kInvalidTimestamp;
On 2009/08/20 21:40:46, scherkus wrote:
> hmm I don't like how these classes have to know about clock
> 
> I would recommend moving kInvalidTimestamp to media/base/buffers.h

Done.

http://codereview.chromium.org/173072/diff/21/27#newcode118
Line 118: if (input->GetDuration() == Clock::kInvalidTimestamp ||
On 2009/08/20 21:40:46, scherkus wrote:
> duration can be an invalid timestamp?  wouldn't a zero duration make more
sense?
>  when does this occur?

This is just being extra careful, I can remove it.

http://codereview.chromium.org/173072/diff/21/27#newcode134
Line 134: // if it is valid (i.e. >= 0).  Otherwise the error will stack
On 2009/08/20 21:40:46, scherkus wrote:
> if we're using kInvalidTimestamp, than any timestamp != kInvalidTimestamp is
> valid

Done.

[scherkus (not reviewing), 2009-08-20 23:19:44]

http://codereview.chromium.org/173072/diff/1014/1015
File media/base/clock_impl.cc (right):

http://codereview.chromium.org/173072/diff/1014/1015#newcode35
Line 35: if (time.InMicroseconds() < 0) {
the only invalid timestamp is kInvalidTimestamp

http://codereview.chromium.org/173072/diff/1014/1018
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/173072/diff/1014/1018#newcode36
Line 36: const base::TimeDelta StreamSample::kInvalidTimestamp =
I don't see the change for declaring this in StreamSample

Also this isn't the best file to be declaring this.. you may have to add a
buffers.cc that contains this line of code :P

Also I'm wondering if we should just declare this variable globally.  We're
already protected with namespace media.. I'm not sure what the style guide says
about this :(

[Alpha Left Google, 2009-08-20 23:36:36]

http://codereview.chromium.org/173072/diff/1014/1015
File media/base/clock_impl.cc (right):

http://codereview.chromium.org/173072/diff/1014/1015#newcode35
Line 35: if (time.InMicroseconds() < 0) {
On 2009/08/20 23:19:44, scherkus wrote:
> the only invalid timestamp is kInvalidTimestamp

Done.

[Alpha Left Google, 2009-08-21 01:04:49]

updated..

[scherkus (not reviewing), 2009-08-21 01:07:39]

LGTM