Ignore seek operations to the current time in pause state

Ignore seek operations to the current time during pause state. This change
is required to remove the noseekrequired logic in HTMLMediaElement.cpp

BUG=266631
Committed: https://crrev.com/f881f652f007ea0480eb0a0fa72a8e5efa00c9ae
Cr-Commit-Position: refs/heads/master@{#304019}

[Srirama, 2014-10-29 04:58:23]

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

[Srirama, 2014-10-29 04:58:24]

As discussed i have uploaded the changes here for discussion. These are not
final changes and there are timeout problems with test cases because we are not
sending seeking and seeked events.

[philipj_slow, 2014-10-29 08:23:44]

What did you want to discuss about this CL? It seems like the correct fix should
involve OnPipelineSeeked in some form.

[Srirama, 2014-10-29 08:55:59]

On 2014/10/29 08:23:44, philipj wrote:
> What did you want to discuss about this CL? It seems like the correct fix
should
> involve OnPipelineSeeked in some form.

Yes, you are right, in normal cases it will be invoked from media thread
automatically. So in this case should i do something like posting a task to the
same thread? I just checked the webmediaplayer_impl file yesterday if something
similar is already there, but couldn't figure out.
I will check again.

[philipj_slow, 2014-10-29 10:39:35]

On 2014/10/29 08:55:59, Srirama wrote:
> On 2014/10/29 08:23:44, philipj wrote:
> > What did you want to discuss about this CL? It seems like the correct fix
> should
> > involve OnPipelineSeeked in some form.
> 
> Yes, you are right, in normal cases it will be invoked from media thread
> automatically. So in this case should i do something like posting a task to
the
> same thread? I just checked the webmediaplayer_impl file yesterday if
something
> similar is already there, but couldn't figure out.
> I will check again.

Yeah, in order to not have the callback while HTMLMediaElement::seek() is
executing, posting a task sounds good.

[Srirama, 2014-10-29 11:39:14]

I have changed to post an internal task for OnPipelineBufferingStateChanged
which will trigger seeking and seeked events. Post task for OnPipelineSeeked is
not enough and it again depends on OnPipelineBufferingStateChanged for
propagating the state to htmlmediaelement and OnPipelineBufferingStateChanged
alone is sufficient to do the job.

PTAL

[Srirama, 2014-10-29 11:41:19]

Also i have verified the mediasource-duration test with 1000 iterations, with
these changes, htmlmediaelement changes and without
https://codereview.chromium.org/440803002 and it is working fine.

[philipj_slow, 2014-10-29 13:13:41]

Faking a readyState change when there may not have been one seems a bit risky,
but I'll leave it to the OWNERS (maybe add scherkus?) to say.

[Srirama, 2014-10-29 14:33:41]

On 2014/10/29 13:13:41, philipj wrote:
> Faking a readyState change when there may not have been one seems a bit risky,
> but I'll leave it to the OWNERS (maybe add scherkus?) to say.

I have verified by passing all the seeks to media engine (removing
noseekrequired logic in 
HTMLMediaElement and without changes in webmediaplayer_impl), then we get a 
bufferstatechange event with BUFFERING_HAVE_ENOUGH even when we seek to current
time.
I am not sure if there is any other case. Also lets wait for comments from
DaleCurtis as you said.

[DaleCurtis, 2014-10-29 18:31:46]

Another option might be to just call client->onTimeChanged() instead of
manipulating the readyState, but I haven't thought that through.

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:346: else {
move else to upper line.

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:347: main_task_runner_->PostTask(FROM_HERE,
Formatting is weird, have you run "git cl format" or used clang-format?

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:350: BUFFERING_HAVE_ENOUGH));
What about just using whatever the current buffering state is? I.e. Directly
call SetReadyState(network_state_);

This also precludes a time changed event from happening, is the HTMLMediaElement
okay with that?

[philipj_slow, 2014-10-29 19:14:48]

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:350: BUFFERING_HAVE_ENOUGH));
On 2014/10/29 18:31:46, DaleCurtis wrote:
> What about just using whatever the current buffering state is? I.e. Directly
> call SetReadyState(network_state_);
> 
> This also precludes a time changed event from happening, is the
HTMLMediaElement
> okay with that?

Hmm, better have a test for that. There should be seeking, timeupdate and seeked
events, in that order.

[Srirama, 2014-10-30 03:25:48]

I think client_->timeChanged() will not solve the purpose. yesterday i have
tried with OnPipelineSeeked(internally invokes client_->timeChanged()) event but
it is of no use, so i changed it to bufferingstatechange.
I will try with the 2nd option and check it with a test.

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:346: else {
On 2014/10/29 18:31:46, DaleCurtis wrote:
> move else to upper line.

Acknowledged.

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:347: main_task_runner_->PostTask(FROM_HERE,
On 2014/10/29 18:31:46, DaleCurtis wrote:
> Formatting is weird, have you run "git cl format" or used clang-format?

Sorry, i will take care now.

https://codereview.chromium.org/685993002/diff/20001/media/blink/webmediaplay...
media/blink/webmediaplayer_impl.cc:350: BUFFERING_HAVE_ENOUGH));
On 2014/10/29 19:14:48, philipj wrote:
> On 2014/10/29 18:31:46, DaleCurtis wrote:
> > What about just using whatever the current buffering state is? I.e. Directly
> > call SetReadyState(network_state_);
> > 
> > This also precludes a time changed event from happening, is the
> HTMLMediaElement
> > okay with that?
> 
> Hmm, better have a test for that. There should be seeking, timeupdate and
seeked
> events, in that order.

I will try with this option and will add a test

[Srirama, 2014-10-30 10:28:24]

Patchset #3 (id:40001) has been deleted

[Srirama, 2014-10-30 11:16:42]

SetReadyState with current state is not solving the issue. Timeout issue is
coming.
Please find below the logs taken when we pass the seek to media engine without
filtering. As we can see, media engine is invoking buffering state with
BUFFERING_HAVE_ENOUGH when we call seek to same time. So the current change
should be fine right?
Also i have updated the test case
LayoutTests/media/video-seek-to-current-position.html with timeupdate event in
blink patch https://codereview.chromium.org/456343002/.
This patch alone cannot have test case as this will not have any effect until
blink patch lands.
[12296:12296:1030/155432:612080526078:ERROR:webmediaplayer_impl.cc(958)]
SetReadyState(0)
[12296:12296:1030/155432:612080533286:ERROR:webmediaplayer_impl.cc(958)]
SetReadyState(1)
[12296:12296:1030/155432:612080534528:ERROR:webmediaplayer_impl.cc(343)] seek:
old_time: 0, new_time: 1000000
[12296:12296:1030/155432:612080541561:ERROR:webmediaplayer_impl.cc(782)]
OnPipelineBufferingStateChanged(1)
[12296:12296:1030/155432:612080541674:ERROR:webmediaplayer_impl.cc(958)]
SetReadyState(4)
[12296:12296:1030/155432:612080542323:ERROR:webmediaplayer_impl.cc(958)]
SetReadyState(1)
[12296:12296:1030/155432:612080542444:ERROR:webmediaplayer_impl.cc(343)] seek:
old_time: 1000000, new_time: 1000000
[12296:12296:1030/155432:612080548944:ERROR:webmediaplayer_impl.cc(782)]
OnPipelineBufferingStateChanged(1)
[12296:12296:1030/155432:612080549023:ERROR:webmediaplayer_impl.cc(958)]
SetReadyState(4)

[DaleCurtis, 2014-10-30 18:21:55]

When you tested, were you PostTask'ing the setReadyState() or timeChanged()
calls?

[Srirama, 2014-10-31 03:13:10]

On 2014/10/30 18:21:55, DaleCurtis wrote:
> When you tested, were you PostTask'ing the setReadyState() or timeChanged()
> calls?

setReadyState(), because as i said before timeChanged() isn't solving the issue.

[Srirama, 2014-10-31 03:18:37]

On 2014/10/31 03:13:10, Srirama wrote:
> On 2014/10/30 18:21:55, DaleCurtis wrote:
> > When you tested, were you PostTask'ing the setReadyState() or timeChanged()
> > calls?
> 
> setReadyState(), because as i said before timeChanged() isn't solving the
issue.

when we look at the logs for the first seek, setReadyState is called with
HaveEnoughData(4) and then immediately reset to HaveMetaData(1). So for the 2nd
seek (seek to same time) setReadyState is invoked with current state which is
HaveMetaData and it is timing out without firing seeking and seeked events.
Where as when we do posttask for bufferingstate with BUFFERING_HAVE_ENOUGH
internally it invokes readystate with HaveEnoughData.

[DaleCurtis, 2014-10-31 21:53:02]

I see; this lgtm then. Please add some tests which verify the event order with
paused seeks to the same time as philipj@ pointed out.

[Srirama, 2014-11-01 03:46:28]

On 2014/10/31 21:53:02, DaleCurtis wrote:
> I see; this lgtm then. Please add some tests which verify the event order with
> paused seeks to the same time as philipj@ pointed out.

Thanks DaleCutis.
@Philip: Do i need to add test for this CL or should i cover it as part of blink
CL https://codereview.chromium.org/456343002/.
As i said before this CL is of no effect without the blink change.

[Srirama, 2014-11-10 09:11:34]

On 2014/11/01 03:46:28, Srirama wrote:
> On 2014/10/31 21:53:02, DaleCurtis wrote:
> > I see; this lgtm then. Please add some tests which verify the event order
with
> > paused seeks to the same time as philipj@ pointed out.
> 
> Thanks DaleCutis.
> @Philip: Do i need to add test for this CL or should i cover it as part of
blink
> CL https://codereview.chromium.org/456343002/.
> As i said before this CL is of no effect without the blink change.

@Philip: Ping

[philipj_slow, 2014-11-13 11:54:17]

Non-owner LGTM.

[Srirama, 2014-11-13 11:55:23]

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

[commit-bot: I haz the power, 2014-11-13 11:55:34]

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

[commit-bot: I haz the power, 2014-11-13 12:40:19]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-11-13 12:40:59]

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

[philipj_slow, 2014-11-13 14:47:54]

On 2014/11/01 03:46:28, Srirama wrote:
> On 2014/10/31 21:53:02, DaleCurtis wrote:
> > I see; this lgtm then. Please add some tests which verify the event order
with
> > paused seeks to the same time as philipj@ pointed out.
> 
> Thanks DaleCutis.
> @Philip: Do i need to add test for this CL or should i cover it as part of
blink
> CL https://codereview.chromium.org/456343002/.
> As i said before this CL is of no effect without the blink change.

A test with the Blink change sounds good.

[Srirama, 2014-11-19 05:39:34]

Patchset #4 (id:80001) has been deleted

[Srirama, 2014-11-19 17:11:05]

This has been reverted in https://codereview.chromium.org/726523003/ because the
test MediaTest.VideoBear3gpAacH264/0 was failing (timing out).
Luckily i was able to simulate the issue with linux_chromium_chrome_rel bot, not
sure how it was missed when the patch was actually landed.
So i did a bit of investigation with the help of logs with the
linux_chromium_chrome_rel bot. I found out that the issue is actually with the
pipeline_.GetMediaTime() in pause() function in webmediaplayer_impl.
In the test case
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/data/pl...
there is a function SeekTestStep() which is invoked on 'ended' event. In this
function there is a seek to 0.9*duration value. So ideally it should not be the
"seek to sametime" scenario but it is happening that way because of wrong value
from pipeline_.GetMediaTime() which is not returning a value less than the
duration even though playback has been completed.
So i did a small change to verify in
https://codereview.chromium.org/740663002/#ps140001 and it is passing the test
case now.
I used pipeline_.GetMediaDuration() instead of pipeline_.GetMediaTime() when the
playback has ended.

Please suggest me whether i can proceed with this change or not.

[Srirama, 2014-11-19 17:14:30]

On 2014/11/19 17:11:05, Srirama wrote:
> This has been reverted in https://codereview.chromium.org/726523003/ because
the
> test MediaTest.VideoBear3gpAacH264/0 was failing (timing out).
> Luckily i was able to simulate the issue with linux_chromium_chrome_rel bot,
not
> sure how it was missed when the patch was actually landed.
> So i did a bit of investigation with the help of logs with the
> linux_chromium_chrome_rel bot. I found out that the issue is actually with the
> pipeline_.GetMediaTime() in pause() function in webmediaplayer_impl.
> In the test case
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/data/pl...
> there is a function SeekTestStep() which is invoked on 'ended' event. In this
> function there is a seek to 0.9*duration value. So ideally it should not be
the
> "seek to sametime" scenario but it is happening that way because of wrong
value
> from pipeline_.GetMediaTime() which is not returning a value less than the

Typo**** from pipeline_.GetMediaTime() which is returning a value less than the

> duration even though playback has been completed.
> So i did a small change to verify in
> https://codereview.chromium.org/740663002/#ps140001 and it is passing the test
> case now.
> I used pipeline_.GetMediaDuration() instead of pipeline_.GetMediaTime() when
the
> playback has ended.
> 
> Please suggest me whether i can proceed with this change or not.