Fixing dropped seeks.

Fixing dropped seeks.

BUG=92251, chromium-os:19888
TEST=None


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

[SeRya, 2011-09-13 19:36:26]

Assignment a value to currentTime property of Media element is ignored if
previous seek has not finished.

Please, review.

[Ami GONE FROM CHROMIUM, 2011-09-13 20:12:57]

This is also http://code.google.com/p/chromium/issues/detail?id=92251 which I
was about to fix differently: by tracking all pending seeks and executing them
in order.  But I like your solution a *lot* better (b/c webkit seems very
trigger-happy when it comes to issuing seek()s, far more than seem warranted by
user actions).  If your solution works I'll be very happy.

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

http://codereview.chromium.org/7890012/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:874:
seek_callback.Run(PIPELINE_ERROR_INVALID_STATE);
Does this still get called, considering the guard you added to
webmediaplayerimpl?

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:713: seeking_ = false;
Curious about the reason for this move.

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:247: float pending_seek_seconds_;
Add a comment to the effect that this is the *last* pending seek?  (since
subsequent seek()s overwrite previous ones)

[SeRya, 2011-09-13 22:14:20]

Thank you for the review.

I also slightly changed code to avoid sending notification that seek has done if
a pending seek exists. For the following reason:

Then user drags the slider it does may seeks. Some of them are dropped. If the
user released the slider one of that seeks finishes after a while and then final
seek gets started. If the pre-final seeks send notification temporarily moves
the slider to the position of that seek.

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

http://codereview.chromium.org/7890012/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:874:
seek_callback.Run(PIPELINE_ERROR_INVALID_STATE);
On 2011/09/13 20:12:58, Ami Fischman wrote:
> Does this still get called, considering the guard you added to
> webmediaplayerimpl?

You are right, it's not called. Reverted that lines.

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:713: seeking_ = false;
On 2011/09/13 20:12:58, Ami Fischman wrote:
> Curious about the reason for this move.

Player should leave "seeking" state whether seek succeeded or not. Otherwise all
the next seeks will be forever pending.

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/7890012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:247: float pending_seek_seconds_;
On 2011/09/13 20:12:58, Ami Fischman wrote:
> Add a comment to the effect that this is the *last* pending seek?  (since
> subsequent seek()s overwrite previous ones)

Done.

[Ami GONE FROM CHROMIUM, 2011-09-13 22:16:59]

LGTM
Thanks!

[scherkus (not reviewing), 2011-09-14 18:52:09]

++LGTM

this was one of the options I had in mind for fixing this bug!  glad to see that
it works :)

I'd also run layout tests as this definitely might cause some results to change
in the scrubbing/seeking tests

[commit-bot: I haz the power, 2011-09-14 20:33:06]

List of reviewers changed.

[commit-bot: I haz the power, 2011-09-15 00:06:07]

Change committed as 101197