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

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index 54ebb9466532426b8b362d8f52407a83b3755922..e5657c43b4852d6cbc922608d1703caba6155a08 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -1645,7 +1645,10 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
 
     if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
         prepareMediaFragmentURI();
+
+        m_duration = duration();
         scheduleEvent(EventTypeNames::durationchange);
+
         if (isHTMLVideoElement(*this))
             scheduleEvent(EventTypeNames::resize);
         scheduleEvent(EventTypeNames::loadedmetadata);
@@ -1749,9 +1752,9 @@ void HTMLMediaElement::prepareToPlay()
         startDelayedLoad();
 }
 
-void HTMLMediaElement::seek(double time, ExceptionState& exceptionState)
+void HTMLMediaElement::seek(SeekSkipPermission seekSkip, double time, ExceptionState& exceptionState)
 {
-    WTF_LOG(Media, "HTMLMediaElement::seek(%f)", time);
+    WTF_LOG(Media, "HTMLMediaElement::seek(%d, %f)", seekSkip, time);
 
     // 4.8.10.9 Seeking
 
@@ -1807,9 +1810,8 @@ void HTMLMediaElement::seek(double time, ExceptionState& exceptionState)
     // Short circuit seeking to the current time by just firing the events if no seek is required.
     // Don't skip calling the media engine if we are in poster mode because a seek should always
     // cancel poster display.
-    bool noSeekRequired = !seekableRanges->length() || (time == now && displayMode() != Poster);
-
-    if (noSeekRequired) {
+    if (!seekableRanges->length()
+        || (seekSkip == SkipAllowed && time == now && displayMode() != Poster)) {

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

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.

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

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.
Thanks for noticing this. I'll make sure to preserve this case.

[philipj_slow, 2014/06/17 11:59:06]

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

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

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. :)

         if (time == now) {
             scheduleEvent(EventTypeNames::seeking);
             if (previousSeekStillPending)
@@ -1927,7 +1929,7 @@ void HTMLMediaElement::setCurrentTime(double time, ExceptionState& exceptionStat
         exceptionState.throwDOMException(InvalidStateError, "The element is slaved to a MediaController.");
         return;
     }
-    seek(time, exceptionState);
+    seek(SkipAllowed, time, exceptionState);
 }
 
 double HTMLMediaElement::duration() const
@@ -2054,7 +2056,7 @@ void HTMLMediaElement::playInternal()
         scheduleDelayedAction(LoadMediaResource);
 
     if (endedPlayback())
-        seek(0, IGNORE_EXCEPTION);
+        seek(SkipAllowed, 0, IGNORE_EXCEPTION);
 
     if (m_mediaController)
         m_mediaController->bringElementUpToSpeed(this);
@@ -2782,7 +2784,7 @@ void HTMLMediaElement::mediaPlayerTimeChanged()
         if (loop() && !m_mediaController) {
             m_sentEndEvent = false;
             //  then seek to the earliest possible position of the media resource and abort these steps.
-            seek(0, IGNORE_EXCEPTION);
+            seek(SkipAllowed, 0, IGNORE_EXCEPTION);
         } else {
             // If the media element does not have a current media controller, and the media element
             // has still ended playback, and the direction of playback is still forwards, and paused
@@ -2811,18 +2813,22 @@ void HTMLMediaElement::mediaPlayerTimeChanged()
 void HTMLMediaElement::mediaPlayerDurationChanged()
 {
     WTF_LOG(Media, "HTMLMediaElement::mediaPlayerDurationChanged");
-    durationChanged(duration());
+    durationChanged(duration(), currentTime() > duration());

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

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.

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

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.

[philipj_slow, 2014/06/17 11:59:06]

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]

Done.

 }
 
-void HTMLMediaElement::durationChanged(double duration)
+void HTMLMediaElement::durationChanged(double duration, bool requestSeek)
 {
-    WTF_LOG(Media, "HTMLMediaElement::durationChanged(%f)", duration);
+    WTF_LOG(Media, "HTMLMediaElement::durationChanged(%f, %d)", duration, requestSeek);
 
     // Abort if duration unchanged.
     if (m_duration == duration)
         return;
 
+    double oldDuration = m_duration;
     m_duration = duration;
+
+    WTF_LOG(Media, "HTMLMediaElement::durationChanged : %f -> %f", oldDuration, duration);
+
     scheduleEvent(EventTypeNames::durationchange);
 
     if (hasMediaControls())
@@ -2830,8 +2836,13 @@ void HTMLMediaElement::durationChanged(double duration)
     if (renderer())
         renderer()->updateFromElement();
 
-    if (currentTime() > duration)
-        seek(duration, IGNORE_EXCEPTION);
+    if (requestSeek) {
+        // Prevent seek skipping if the duration is getting smaller. The webMediaPlayer()
+        // may be clamping currentTime to duration() so the normal "are we seeking to the current location?"
+        // skip check may not be reliable.
+        SeekSkipPermission seekSkip = (duration < oldDuration) ? SkipNotAllowed : SkipAllowed;

[acolwell GONE FROM CHROMIUM, 2014/06/06 23:20:04]

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. 

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

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

+        seek(seekSkip, duration, IGNORE_EXCEPTION);
+    }
 }
 
 void HTMLMediaElement::mediaPlayerPlaybackStateChanged()
@@ -3599,7 +3610,7 @@ void HTMLMediaElement::applyMediaFragmentURI()
     if (m_fragmentStartTime != MediaPlayer::invalidTime()) {
         m_sentEndEvent = false;
         UseCounter::count(document(), UseCounter::HTMLMediaElementSeekToFragmentStart);
-        seek(m_fragmentStartTime, IGNORE_EXCEPTION);
+        seek(SkipAllowed, m_fragmentStartTime, IGNORE_EXCEPTION);
     }
 }