Fix MediaSource.duration setter behavior to match the current spec.

Fix MediaSource.duration setter behavior to match the current spec.

This change fixes non-compliant MSE behavior when the duration is set
to a smaller value. The spec calls for remove() to get called in this
case which wasn't happening before. This causes SourceBuffer.updating
to become true which prevents other operations like endOfStream() from
being allowed until the remove completes. Tests that relied on this
broken behavior have been updated.

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

[acolwell GONE FROM CHROMIUM, 2014-06-06 23:20:05]

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2843: SeekSkipPermission seekSkip =
(duration < oldDuration) ? SkipNotAllowed : SkipAllowed;
This change and ones related to it are needed because we flipped the order of
the m_attachedElement->durationChanged(duration) and
m_webMediaSource->setDuration(duration) calls in MediaSource::setDuration() to
match the spec. This now means that the webMediaSource() knows about the new
duration before the HTMLMediaElement does so it clamps the currentTime to the
new duration.

[adamk, 2014-06-07 17:46:59]

Sorry, I don't think I'm going to be able to get to this before I leave on
vacation. Perhaps philipj could review?

[acolwell GONE FROM CHROMIUM, 2014-06-09 17:19:29]

On 2014/06/07 17:46:59, adamk (ooo until june 16) wrote:
> Sorry, I don't think I'm going to be able to get to this before I leave on
> vacation. Perhaps philipj could review?

No problem.

philipj: Please take a look.

[acolwell GONE FROM CHROMIUM, 2014-06-13 17:02:23]

philipj: ping. PTAL

[philipj_slow, 2014-06-16 13:26:19]

LGTM with nits and discussio that doesn't need to block landing.

Sorry for the delay, I ignored most review mail last week and don't have filters
to pick out things directed specifically at me. I will rectify this.

My high-level comment on this is that the extra requestSeek and seekSkip
parameters are a bit unfortunate, but I don't have a concrete alternative. Maybe
MediaSource could set the duration on its WebMediaSource which would in turn
call mediaPlayerDurationChanged() with the new duration and the old currentTime,
to get around the clamping problem. It seems like all backends should require
that, since otherwise they're at least momentarily in a state where
currentTime() can exceed duration(), which just seems wrong.

I haven't studied the problem in enough detail to know if this makes actual
sense.

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/media-source/mediasource-getvideoplaybackquality.html
(right):

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-getvideoplaybackquality.html:46:
assert_false(sourceBuffer.updating, "sourceBuffer.updating");
nit: this and the following lines are indented with tabs

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-remove.html (right):

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-remove.html:142:
mediaSource.duration = 10;
Can you assert_less_than(mediaSource.duration, 10) before this? It explains why
updating is still false afterwards, which I didn't understand at first.

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-remove.html:144:
assert_false(sourceBuffer.updating, "updating");
nit: misindented from here

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1814: || (seekSkip == SkipAllowed && time
== now && displayMode() != Poster)) {
Do you know why we actually need this short circuiting? I've seen it before and
wondered, and now it's made slightly more complicated still. This CL doesn't
need to block on it, but should we try getting rid of this? I think the backend
needs to handle seeking to currentTime anyway, because if playback is ongoing it
could happen to be equal when the request reaches the backend but not when the
check is done here...

As an aside, the !seekableRanges->length() case is actually in the spec, but it
shuoldn't fire any events.

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2816: durationChanged(duration(),
currentTime() > duration());
This was already the case, but isn't it weird that the player can ever return a
currentTime() > duration()? The "webMediaPlayer() may be clamping currentTime to
duration()" comment makes it seem not true, but I can't actually find the
clamping explicitly in webmediaplayer_ms.cc or webmediaplayer_impl.cc.

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2843: SeekSkipPermission seekSkip =
(duration < oldDuration) ? SkipNotAllowed : SkipAllowed;
On 2014/06/06 23:20:04, acolwell wrote:
> This change and ones related to it are needed because we flipped the order of
> the m_attachedElement->durationChanged(duration) and
> m_webMediaSource->setDuration(duration) calls in MediaSource::setDuration() to
> match the spec. This now means that the webMediaSource() knows about the new
> duration before the HTMLMediaElement does so it clamps the currentTime to the
> new duration. 

OK, that explains why requestSeek is initialized when it is in
MediaSource::durationChangeAlgorithm.

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:390: // 6. Update the media
controller duration to new duration and run the HTMLMediaElement duration change
algorithm.
Spec nit: The "media controller duration" is a calculated attribute defined in
terms of the slaved elements, I'm not sure what it means to set it. I think the
"media resource end position" is the appropriate concept here.

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.h (right):

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.h:125: // Implements the duration
algorithm.
nit: duration change algorithm

[acolwell GONE FROM CHROMIUM, 2014-06-17 01:24:02]

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-remove.html (right):

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-remove.html:142:
mediaSource.duration = 10;
On 2014/06/16 13:26:19, philipj wrote:
> Can you assert_less_than(mediaSource.duration, 10) before this? It explains
why
> updating is still false afterwards, which I didn't understand at first.

Done.

https://codereview.chromium.org/319213002/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-remove.html:144:
assert_false(sourceBuffer.updating, "updating");
On 2014/06/16 13:26:18, philipj wrote:
> nit: misindented from here

Done.

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1814: || (seekSkip == SkipAllowed && time
== now && displayMode() != Poster)) {
On 2014/06/16 13:26:19, philipj wrote:
> Do you know why we actually need this short circuiting? I've seen it before
and
> wondered, and now it's made slightly more complicated still. This CL doesn't
> need to block on it, but should we try getting rid of this? I think the
backend
> needs to handle seeking to currentTime anyway, because if playback is ongoing
it
> could happen to be equal when the request reaches the backend but not when the
> check is done here...

I've never been a fan of this block of code. I'll remove it in a different CL
and then rebase this change on top of that.

> 
> As an aside, the !seekableRanges->length() case is actually in the spec, but
it
> shuoldn't fire any events.
Thanks for noticing this. I'll make sure to preserve this case.

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2816: durationChanged(duration(),
currentTime() > duration());
On 2014/06/16 13:26:19, philipj wrote:
> This was already the case, but isn't it weird that the player can ever return
a
> currentTime() > duration()? The "webMediaPlayer() may be clamping currentTime
to
> duration()" comment makes it seem not true, but I can't actually find the
> clamping explicitly in webmediaplayer_ms.cc or webmediaplayer_impl.cc.

With the current code this condition should always return false. I just put it
here to be consistent with the spec. The clamping to duration happens WAY down
in media::Clock::ClampToValidTimeRange().

I agree that the duration change logic needs a little attention. Ideally, I'd
like to avoid every WebMediaPlayer implementation from having to handle the
"seek on duration truncation case." I just haven't come up with a good solution
yet.

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.cpp:390: // 6. Update the media
controller duration to new duration and run the HTMLMediaElement duration change
algorithm.
On 2014/06/16 13:26:19, philipj wrote:
> Spec nit: The "media controller duration" is a calculated attribute defined in
> terms of the slaved elements, I'm not sure what it means to set it. I think
the
> "media resource end position" is the appropriate concept here.

Good point. I'll update the spec.

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/MediaSource.h (right):

https://codereview.chromium.org/319213002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/MediaSource.h:125: // Implements the duration
algorithm.
On 2014/06/16 13:26:19, philipj wrote:
> nit: duration change algorithm

Done.

[philipj_slow, 2014-06-17 11:59:07]

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1814: || (seekSkip == SkipAllowed && time
== now && displayMode() != Poster)) {
On 2014/06/17 01:24:02, acolwell wrote:
> On 2014/06/16 13:26:19, philipj wrote:
> > Do you know why we actually need this short circuiting? I've seen it before
> and
> > wondered, and now it's made slightly more complicated still. This CL doesn't
> > need to block on it, but should we try getting rid of this? I think the
> backend
> > needs to handle seeking to currentTime anyway, because if playback is
ongoing
> it
> > could happen to be equal when the request reaches the backend but not when
the
> > check is done here...
> 
> I've never been a fan of this block of code. I'll remove it in a different CL
> and then rebase this change on top of that.
> 
> > 
> > As an aside, the !seekableRanges->length() case is actually in the spec, but
> it
> > shuoldn't fire any events.
> Thanks for noticing this. I'll make sure to preserve this case.

Thanks, I hope it turns out that the short circuiting really isn't needed.

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2816: durationChanged(duration(),
currentTime() > duration());
On 2014/06/17 01:24:02, acolwell wrote:
> On 2014/06/16 13:26:19, philipj wrote:
> > This was already the case, but isn't it weird that the player can ever
return
> a
> > currentTime() > duration()? The "webMediaPlayer() may be clamping
currentTime
> to
> > duration()" comment makes it seem not true, but I can't actually find the
> > clamping explicitly in webmediaplayer_ms.cc or webmediaplayer_impl.cc.
> 
> With the current code this condition should always return false. I just put it
> here to be consistent with the spec. The clamping to duration happens WAY down
> in media::Clock::ClampToValidTimeRange().
> 
> I agree that the duration change logic needs a little attention. Ideally, I'd
> like to avoid every WebMediaPlayer implementation from having to handle the
> "seek on duration truncation case." I just haven't come up with a good
solution
> yet.

A FIXME or ASSERT to clarify the current status of things here would be nice.

[acolwell GONE FROM CHROMIUM, 2014-06-17 22:39:02]

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1814: || (seekSkip == SkipAllowed && time
== now && displayMode() != Poster)) {
On 2014/06/17 11:59:06, philipj wrote:
> On 2014/06/17 01:24:02, acolwell wrote:
> > On 2014/06/16 13:26:19, philipj wrote:
> > > Do you know why we actually need this short circuiting? I've seen it
before
> > and
> > > wondered, and now it's made slightly more complicated still. This CL
doesn't
> > > need to block on it, but should we try getting rid of this? I think the
> > backend
> > > needs to handle seeking to currentTime anyway, because if playback is
> ongoing
> > it
> > > could happen to be equal when the request reaches the backend but not when
> the
> > > check is done here...
> > 
> > I've never been a fan of this block of code. I'll remove it in a different
CL
> > and then rebase this change on top of that.
> > 
> > > 
> > > As an aside, the !seekableRanges->length() case is actually in the spec,
but
> > it
> > > shuoldn't fire any events.
> > Thanks for noticing this. I'll make sure to preserve this case.
> 
> Thanks, I hope it turns out that the short circuiting really isn't needed.
I've removed the short circuiting in https://codereview.chromium.org/340553004/
. It resulted in a small zak shave, but the end result is better spec compliance
in a few places. :)

https://codereview.chromium.org/319213002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2816: durationChanged(duration(),
currentTime() > duration());
On 2014/06/17 11:59:06, philipj wrote:
> On 2014/06/17 01:24:02, acolwell wrote:
> > On 2014/06/16 13:26:19, philipj wrote:
> > > This was already the case, but isn't it weird that the player can ever
> return
> > a
> > > currentTime() > duration()? The "webMediaPlayer() may be clamping
> currentTime
> > to
> > > duration()" comment makes it seem not true, but I can't actually find the
> > > clamping explicitly in webmediaplayer_ms.cc or webmediaplayer_impl.cc.
> > 
> > With the current code this condition should always return false. I just put
it
> > here to be consistent with the spec. The clamping to duration happens WAY
down
> > in media::Clock::ClampToValidTimeRange().
> > 
> > I agree that the duration change logic needs a little attention. Ideally,
I'd
> > like to avoid every WebMediaPlayer implementation from having to handle the
> > "seek on duration truncation case." I just haven't come up with a good
> solution
> > yet.
> 
> A FIXME or ASSERT to clarify the current status of things here would be nice.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-17 22:39:10]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-17 22:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/319213002/100001

[acolwell GONE FROM CHROMIUM, 2014-06-17 23:13:26]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-17 23:13:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/319213002/120001

[commit-bot: I haz the power, 2014-06-18 01:25:32]

Change committed as 176373