Pass seek event through pipeline during loading state

Pass seek event through pipeline during loading state

When try to land https://codereviougew.chromium.org/456343002/ the test
controls-restrained-media-controller.html is failing sometimes as the
frame is not ready before the canplay event. Reason being canplay event
is fired before VideoFrameCompositor::UpdateCurrentFrame if the seek event
is not passed to pipeline during loading state. Also removed
OnPipelineSeeked which is redundant now.

Required for landing https://codereview.chromium.org/456343002/

Committed: https://crrev.com/ccf67181d8ccd356d86019b6768e5a9096956fbf
Cr-Commit-Position: refs/heads/master@{#310483}

[Srirama, 2014-12-26 11:23:25]

srirama.m@samsung.com changed reviewers:
+ dalecurtis@chromium.org, philipj@opera.com

[Srirama, 2014-12-26 11:23:25]

Investigated further on why frame is not ready during
controls-restrained-media-controller.html test.
Sometimes the canplay event is fired before
VideoFrameCompositor::UpdateCurrentFrame is executed even though FrameReady
callback has been called from pipeline.
So delaying OnPipelineBufferingStateChanged event makes sure that frame is
actually ready at blink side before the canplay event.

[Srirama, 2014-12-29 05:04:10]

Patchset #2 (id:20001) has been deleted

[Srirama, 2014-12-29 09:35:00]

Patchset #4 (id:80001) has been deleted

[Srirama, 2014-12-29 11:42:29]

Patchset #3 (id:60001) has been deleted

[Srirama, 2014-12-29 11:42:37]

Patchset #2 (id:40001) has been deleted

[DaleCurtis, 2015-01-05 19:13:06]

This doesn't look right.  Can you instead just perform the seek if the state
isn't already BUFFERING_HAVE_ENOUGH ?

[Srirama, 2015-01-06 13:33:07]

On 2015/01/05 19:13:06, DaleCurtis wrote:
> This doesn't look right.  Can you instead just perform the seek if the state
> isn't already BUFFERING_HAVE_ENOUGH ?

I couldn't find a way to check for buffering state directly, so i have checked
with readystate. Even the readystate is reset to ReadyStateHaveMetadata just
before every seek operation in WebMediaPlayerImpl::seek().
So i have stored the old value before the reset.
If this is fine i will modify the description and go ahead.

[DaleCurtis, 2015-01-06 19:18:55]

https://codereview.chromium.org/787373008/diff/100001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/787373008/diff/100001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:334: if (paused_time_ != seek_time ||
old_state != ReadyStateHaveEnoughData) {
Can you see if this allows the removal of OnPipelineSeeked() now? I.e. move the
inverse of this state check to the PostTask for
OnPipelineBufferingStateChanged(). 

if (paused_time_ != seek_time) {
  paused_time_ = seek_time;
} else if (old_state == ReadyStateHaveEnoughData) {
  // PostTask(OnPipelineBufferingStateChanged);
  return;
}

This will avoid redoing the seek to the beginning if load is still in progress. 
I also don't remember exactly why the OnPipelineSeeked() is necessary. The load
event should eventually completely and issue the correct buffering state change.

[Srirama, 2015-01-07 06:46:00]

On 2015/01/06 19:18:55, DaleCurtis wrote:
>
https://codereview.chromium.org/787373008/diff/100001/media/blink/webmediapla...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/787373008/diff/100001/media/blink/webmediapla...
> media/blink/webmediaplayer_impl.cc:334: if (paused_time_ != seek_time ||
> old_state != ReadyStateHaveEnoughData) {
> Can you see if this allows the removal of OnPipelineSeeked() now? I.e. move
the
> inverse of this state check to the PostTask for
> OnPipelineBufferingStateChanged(). 
> 
> if (paused_time_ != seek_time) {
>   paused_time_ = seek_time;
> } else if (old_state == ReadyStateHaveEnoughData) {
>   // PostTask(OnPipelineBufferingStateChanged);
>   return;
> }
> 
> This will avoid redoing the seek to the beginning if load is still in
progress. 
> I also don't remember exactly why the OnPipelineSeeked() is necessary. The
load
> event should eventually completely and issue the correct buffering state
change.

I have added OnPipelineSeeked event to fix the failure on blink side but it is
of no use. So i have removed it now and the test cases are passing even without
that. And i have shuffled the condition as suggested.

[Dale Curtis, 2015-01-07 19:23:14]

dalecurtis@google.com changed reviewers:
+ dalecurtis@google.com

[Dale Curtis, 2015-01-07 19:23:15]

lgtm % comment update.

https://codereview.chromium.org/787373008/diff/120001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/787373008/diff/120001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:330: // In paused state ignore the seek
operations to current time and generate
Please update the comment here to remove the OnPipelineSeeked() mention as well
as indicate why we only send the buffering state update when old_state is
already correct. => I.e., that a soon to be fired buffering state change will
take care of this.

[DaleCurtis, 2015-01-07 20:43:53]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@google.com

[DaleCurtis, 2015-01-07 20:43:53]

(from the right account) lgtm

[Srirama, 2015-01-08 05:31:38]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2015-01-08 05:32:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787373008/140001

[commit-bot: I haz the power, 2015-01-08 07:34:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-08 07:34:12]

Exceeded time limit waiting for builds to trigger.

[Srirama, 2015-01-08 10:52:05]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2015-01-08 10:54:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787373008/140001

[commit-bot: I haz the power, 2015-01-08 11:59:42]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-01-08 12:00:36]

Patchset 4 (id:??) landed as
https://crrev.com/ccf67181d8ccd356d86019b6768e5a9096956fbf
Cr-Commit-Position: refs/heads/master@{#310483}