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

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1627 matching lines @@
     } else {
         if (wasPotentiallyPlaying && m_readyState < HAVE_FUTURE_DATA) {
             // 4.8.10.8
             scheduleTimeupdateEvent(false);
             scheduleEvent(EventTypeNames::waiting);
         }
     }
 
     if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
         prepareMediaFragmentURI();
+
+        m_duration = duration();
         scheduleEvent(EventTypeNames::durationchange);
+
         if (isHTMLVideoElement(*this))
             scheduleEvent(EventTypeNames::resize);
         scheduleEvent(EventTypeNames::loadedmetadata);
         if (hasMediaControls())
             mediaControls()->reset();
         if (renderer())
             renderer()->updateFromElement();
     }
 
     bool shouldUpdateDisplayState = false;
@@ skipping 83 matching lines @@
 {
     WTF_LOG(Media, "HTMLMediaElement::prepareToPlay(%p)", this);
     if (m_havePreparedToPlay)
         return;
     m_havePreparedToPlay = true;
 
     if (m_delayingLoadForPreloadNone)
         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
 
     // 1 - If the media element's readyState is HAVE_NOTHING, then raise an InvalidStateError exception.
     if (m_readyState == HAVE_NOTHING || !m_player) {
         exceptionState.throwDOMException(InvalidStateError, "The element's readyState is HAVE_NOTHING.");
         return;
     }
 
     // If the media engine has been told to postpone loading data, let it go ahead now.
@@ skipping 35 matching lines @@
 
     // 7 - If the (possibly now changed) new playback position is not in one of the ranges given in the
     // seekable attribute, then let it be the position in one of the ranges given in the seekable attribute
     // that is the nearest to the new playback position. ... If there are no ranges given in the seekable
     // attribute then set the seeking IDL attribute to false and abort these steps.
     RefPtr<TimeRanges> seekableRanges = seekable();
 
     // 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)
                 return;
             // FIXME: There must be a stable state before timeupdate+seeked are dispatched and seeking
             // is reset to false. See http://crbug.com/266631
             scheduleTimeupdateEvent(false);
             scheduleEvent(EventTypeNames::seeked);
         }
         m_seeking = false;
@@ skipping 97 matching lines @@
 
     return m_cachedTime;
 }
 
 void HTMLMediaElement::setCurrentTime(double time, ExceptionState& exceptionState)
 {
     if (m_mediaController) {
         exceptionState.throwDOMException(InvalidStateError, "The element is slaved to a MediaController.");
         return;
     }
-    seek(time, exceptionState);
+    seek(SkipAllowed, time, exceptionState);
 }
 
 double HTMLMediaElement::duration() const
 {
     if (!m_player || m_readyState < HAVE_METADATA)
         return numeric_limits<double>::quiet_NaN();
 
     // FIXME: Refactor so m_duration is kept current (in both MSE and
     // non-MSE cases) once we have transitioned from HAVE_NOTHING ->
     // HAVE_METADATA. Currently, m_duration may be out of date for at least MSE
@@ skipping 106 matching lines @@
 
 void HTMLMediaElement::playInternal()
 {
     WTF_LOG(Media, "HTMLMediaElement::playInternal");
 
     // 4.8.10.9. Playing the media resource
     if (!m_player || m_networkState == NETWORK_EMPTY)
         scheduleDelayedAction(LoadMediaResource);
 
     if (endedPlayback())
-        seek(0, IGNORE_EXCEPTION);
+        seek(SkipAllowed, 0, IGNORE_EXCEPTION);
 
     if (m_mediaController)
         m_mediaController->bringElementUpToSpeed(this);
 
     if (m_paused) {
         m_paused = false;
         invalidateCachedTime();
         scheduleEvent(EventTypeNames::play);
 
         if (m_readyState <= HAVE_CURRENT_DATA)
@@ skipping 707 matching lines @@
     double now = currentTime();
     double dur = duration();
 
     // When the current playback position reaches the end of the media resource when the direction of
     // playback is forwards, then the user agent must follow these steps:
     if (!std::isnan(dur) && dur && now >= dur && m_playbackRate > 0) {
         // If the media element has a loop attribute specified and does not have a current media controller,
         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
             // is false,
             if (!m_mediaController && !m_paused) {
                 // changes paused to true and fires a simple event named pause at the media element.
                 m_paused = true;
                 scheduleEvent(EventTypeNames::pause);
             }
             // Queue a task to fire a simple event named ended at the media element.
             if (!m_sentEndEvent) {
                 m_sentEndEvent = true;
                 scheduleEvent(EventTypeNames::ended);
             }
             // If the media element has a current media controller, then report the controller state
             // for the media element's current media controller.
             updateMediaController();
         }
     }
     else
         m_sentEndEvent = false;
 
     updatePlayState();
 }
 
 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())
         mediaControls()->reset();
     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()
 {
     WTF_LOG(Media, "HTMLMediaElement::mediaPlayerPlaybackStateChanged");
 
     if (!m_player || m_pausedInternal)
         return;
 
     if (m_player->paused())
@@ skipping 747 matching lines @@
 
     if (m_fragmentStartTime != MediaPlayer::invalidTime() && m_readyState < HAVE_FUTURE_DATA)
         prepareToPlay();
 }
 
 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);
     }
 }
 
 WebMediaPlayer::CORSMode HTMLMediaElement::corsMode() const
 {
     const AtomicString& crossOriginMode = fastGetAttribute(crossoriginAttr);
     if (crossOriginMode.isNull())
         return WebMediaPlayer::CORSModeUnspecified;
     if (equalIgnoringCase(crossOriginMode, "use-credentials"))
         return WebMediaPlayer::CORSModeUseCredentials;
@@ skipping 41 matching lines @@
     visitor->trace(m_error);
     visitor->trace(m_currentSourceNode);
     visitor->trace(m_nextChildNodeToConsider);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
     WillBeHeapSupplementable<HTMLMediaElement>::trace(visitor);
     HTMLElement::trace(visitor);
 }
 
 }