Remove PosterWaitingForVideo which is redundant

Remove PosterWaitingForVideo from DisplayMode which is redundant

Bug=342621
Originally introduced in http://trac.webkit.org/changeset/83667

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188211

[Srirama, 2014-08-30 14:36:57]

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

[Srirama, 2014-08-30 14:36:58]

PTAL

[philipj_slow, 2014-09-01 10:01:00]

In the description you can point to the commit which introduced this:
http://trac.webkit.org/changeset/8366

[philipj_slow, 2014-09-01 11:09:17]

The only use of the Poster state that looks like it can be affected is in
HTMLMediaElement::seek:

bool noSeekRequired = !seekableRanges->length() || (time == now && displayMode()
!= Poster);

With this change, there are cases where noSeekRequired will be  false where it
was previously true. The code in question was removed and revert back in
https://codereview.chromium.org/431273003 so is there any chance that this
change will be reverted for the same reasons? You run
http/tests/media/media-source/mediasource-duration.html with --iterations=1000
locally to investigate.

https://codereview.chromium.org/525773002/diff/1/Source/core/html/HTMLVideoEl...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/525773002/diff/1/Source/core/html/HTMLVideoEl...
Source/core/html/HTMLVideoElement.cpp:185: mode = Poster;
Since oldMode is Poster, can't we just return instead of setting mode = Poster?

[Srirama, 2014-09-01 13:27:55]

I guess you are referring to http://trac.webkit.org/changeset/83667
Updated the description with this.
I ran the test 1000 times as suggested but there seems to be no change, it fails
always as expected. The state PosterWaitingForVideo seems to be not covered as
part of tests.
But your point looks to be valid and i think it is better to wait till i land
https://codereview.chromium.org/456343002/ to make things clear.

https://codereview.chromium.org/525773002/diff/1/Source/core/html/HTMLVideoEl...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/525773002/diff/1/Source/core/html/HTMLVideoEl...
Source/core/html/HTMLVideoElement.cpp:185: mode = Poster;
On 2014/09/01 11:09:17, philipj wrote:
> Since oldMode is Poster, can't we just return instead of setting mode =
Poster?

Acknowledged.

[acolwell GONE FROM CHROMIUM, 2014-09-17 21:01:51]

acolwell@chromium.org changed reviewers:
- acolwell@chromium.org

[philipj_slow, 2014-10-27 08:09:49]

Ping

[Srirama, 2014-10-27 09:54:22]

On 2014/10/27 08:09:49, philipj wrote:
> Ping

Sorry for the delay. As you said i have verified mediasource-duration test with
1000 iterations and there is no effect of this patch. So should i go ahead and
land this or should i wait for
https://code.google.com/p/chromium/issues/detail?id=399523
Looks like it really takes time for me to land that one. For the complete
discussions you can have a quick look at 
https://code.google.com/p/chromium/issues/detail?id=399523 if you have time.

[philipj_slow, 2014-10-27 10:07:26]

On 2014/10/27 09:54:22, Srirama wrote:
> On 2014/10/27 08:09:49, philipj wrote:
> > Ping
> 
> Sorry for the delay. As you said i have verified mediasource-duration test
with
> 1000 iterations and there is no effect of this patch. So should i go ahead and
> land this or should i wait for
> https://code.google.com/p/chromium/issues/detail?id=399523
> Looks like it really takes time for me to land that one. For the complete
> discussions you can have a quick look at 
> https://code.google.com/p/chromium/issues/detail?id=399523 if you have time.

What is blocking re-landing the removal of the noSeekRequired bits of
HTMLMediaElement::seek? From skimming the comments of the bug I can't find the
concrete problem.

[Srirama, 2014-10-27 10:26:24]

On 2014/10/27 10:07:26, philipj wrote:
> On 2014/10/27 09:54:22, Srirama wrote:
> > On 2014/10/27 08:09:49, philipj wrote:
> > > Ping
> > 
> > Sorry for the delay. As you said i have verified mediasource-duration test
> with
> > 1000 iterations and there is no effect of this patch. So should i go ahead
and
> > land this or should i wait for
> > https://code.google.com/p/chromium/issues/detail?id=399523
> > Looks like it really takes time for me to land that one. For the complete
> > discussions you can have a quick look at 
> > https://code.google.com/p/chromium/issues/detail?id=399523 if you have time.
> 
> What is blocking re-landing the removal of the noSeekRequired bits of
> HTMLMediaElement::seek? From skimming the comments of the bug I can't find the
> concrete problem.

As you know when we try to land that CL, it created an assert in chromium side.
Then we figured out that one of Dale's fixes
https://codereview.chromium.org/440803002 is actualling fixing that assert
issue. But acolwell wanted to know how exactly is that fixing our assert issue.
So i was trying to figure that out with the help of DaleCurtis as i don't have
much idea on the chromium side. And from comment 15 it was diverted a bit and
Dale said we need to add this noseekrequired logic somewhere else to avoid this
issue. Acolwell wanted this logic to be added in chromium side rather than blink
side and Dale said he is ok to do it except for programmatic seeks.

[philipj_slow, 2014-10-27 12:09:42]

OK. Aaron is no longer in the Chromium project, so can you ask Dale if adding
such checks to WebMediaPlayerImpl would be sufficient, or if there's something
else standing in the way?

[Srirama, 2014-10-27 12:24:43]

On 2014/10/27 12:09:42, philipj wrote:
> OK. Aaron is no longer in the Chromium project, so can you ask Dale if adding
> such checks to WebMediaPlayerImpl would be sufficient, or if there's something
> else standing in the way?

Ok, i will ask him, but before that, as per acolwell's comments (comment#17)
"Since there are thread hops involved, seeking to "now" can only theoretically
be avoided once you are already on the media thread and in the proper context to
actually initiate the seek. This means that the logic would likely need to be
somewhere inside Pipeline and similar parts of the Android code."
having that check in webmediaplayer_impl will take care of the above thing? We
will be still in blink context right.
May be do we need to go one more layer below till pipeline.cc:seek() to take
care of actual thread hops and context as mentioned in above comment?

[philipj_slow, 2014-10-27 13:11:38]

On 2014/10/27 12:24:43, Srirama wrote:
> On 2014/10/27 12:09:42, philipj wrote:
> > OK. Aaron is no longer in the Chromium project, so can you ask Dale if
adding
> > such checks to WebMediaPlayerImpl would be sufficient, or if there's
something
> > else standing in the way?
> 
> Ok, i will ask him, but before that, as per acolwell's comments (comment#17)
> "Since there are thread hops involved, seeking to "now" can only theoretically
> be avoided once you are already on the media thread and in the proper context
to
> actually initiate the seek. This means that the logic would likely need to be
> somewhere inside Pipeline and similar parts of the Android code."
> having that check in webmediaplayer_impl will take care of the above thing? We
> will be still in blink context right.
> May be do we need to go one more layer below till pipeline.cc:seek() to take
> care of actual thread hops and context as mentioned in above comment?

If the only objective is to make seek to currentTime a no-op while paused then
any layer ought to be OK. I don't think trying to avoid extra seeking while
playing makes much sense, the chance of currentTime actually being the same by
the time one gets to the thread where it can be compared ought to be roughly
zero, so why bother? I'm guessing that avoid network traffic on seeks to 0 is
the only really important case.

[Srirama, 2015-01-12 07:04:58]

Patchset #2 (id:20001) has been deleted

[philipj_slow, 2015-01-12 09:34:43]

I guess this is no longer blocked on any other changes, can you rebase and run
the bots?

[Srirama, 2015-01-12 09:46:00]

On 2015/01/12 09:34:43, philipj wrote:
> I guess this is no longer blocked on any other changes, can you rebase and run
> the bots?

Yes, already done. Is there any chance of "noseekrequired" patch being reverted
or can i assume that two days of testing is good enough for that to stick?

[philipj_slow, 2015-01-12 10:10:28]

There's always a chance of revert, but this is LGTM if you want to land this and
just revert both if that turns out to be necessary later.

[Srirama, 2015-01-12 10:43:10]

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

[commit-bot: I haz the power, 2015-01-12 10:44:20]

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

[commit-bot: I haz the power, 2015-01-12 10:47:36]

Committed patchset #2 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188211