Allow HTMLMediaElement.currentTime to be set before the transition to HAVE_METADATA

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 91ef6708e6f5e78594ef9b36ad751aec9955cc39..9c001f5f1ccb05d5aecf15b6d685fd7ed57d5885 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -324,6 +324,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_duration(std::numeric_limits<double>::quiet_NaN())
     , m_lastTimeUpdateEventWallTime(0)
     , m_lastTimeUpdateEventMovieTime(std::numeric_limits<double>::max())
+    , m_defaultPlaybackStartPosition(0)
     , m_loadState(WaitingForSource)
     , m_deferredLoadState(NotDeferred)
     , m_deferredLoadTimer(this, &HTMLMediaElement::deferredLoadTimerFired)
@@ -1822,6 +1823,12 @@ void HTMLMediaElement::setReadyState(ReadyState state)
         if (isHTMLVideoElement())
             scheduleEvent(EventTypeNames::resize);
         scheduleEvent(EventTypeNames::loadedmetadata);
+
+        if (m_defaultPlaybackStartPosition > 0) {
+            seek(m_defaultPlaybackStartPosition);
+            m_defaultPlaybackStartPosition = 0;
+        }
+
         if (hasMediaControls())
             mediaControls()->reset();
         if (renderer())
@@ -1922,18 +1929,12 @@ void HTMLMediaElement::prepareToPlay()
         startDeferredLoad();
 }
 
-void HTMLMediaElement::seek(double time, ExceptionState& exceptionState)
+void HTMLMediaElement::seek(double time)
 {
     WTF_LOG(Media, "HTMLMediaElement::seek(%f)", time);
 
     // 4.8.10.9 Seeking
 

[amogh.bihani, 2014/09/08 10:26:38]

While working on media fragments URI it came out that when media controller has
2 resources and the second resource has #t=2 then both resources seek till t=2
but if the first resource has #t=2, then when the media controller seeks till
t=2, second resource is still at HAVE_NOTHING, because of which it does not seek
till t=2. However,if we set here
if (HAVE_NOTHING) {
  m_defaultPlaybackStartPostition = time,
  return;
}
then the other resource would come to t=2 when it reaches loadedmetadata.

I could not find anything in the spec which would suggest what is the normal
behaviour.
What should be done here?

[philipj_slow, 2014/09/08 15:00:58]

It sounds like the problem is that setting MediaController.currentTime will
invoke HTMLMediaElement.seek(), but it's the HTMLMediaElement.currentTime setter
that saves the default playback start position in the HAVE_NOTHING case? And
moving that logic to seek() "fixes" the problem?

What I don't understand is how #t=2 on one element ends up seeking both elements
attached to a MediaController. applyMediaFragmentURI() just calls seek(), and
that doesn't do anything with the MediaController. Per spec I also don't see a
connection. Please help me out :)

[amogh.bihani, 2014/09/09 05:30:52]

Yes applyMediaFragmentURI seeks only the corresponding media but when we add
another media to the media group or MediaController then we call
addMediaElement() which internally calls bringElementUpToSpeed(). Now this calls
seek() for the new media attached. So if the first element has t=2, second will
also seek to that position (but the problem is that it is still in HAVE_NOTHING
state :/)

In step 13, it says that the media controller should seek to the media element's
initial playback position. So when second media has t=2, even the first moves to
that position.

Even in first scenario when we start the playback, the second media seeks to t=2
and then both start playing together, so I guess seeking it before hand would
look good and make both the scenarios uniform. Plus with this we won't be
fetching the resource which wont be played and save some data and power
(especially in hand held devices).

[philipj_slow, 2014/09/09 08:39:37]

I think the root problem is that, regardless of media fragments and the default
playback start position, attaching a media element with readyState HAVE_NOTHING
to a media controller will try to seek the media element but fail. If only later
calls play() it'll seek again, but if left paused it will stay in the
out-of-sync state.

If you want to file a spec bug about this, do so at http://whatwg.org/newbug and
post the bug number here.

Unless this CL exposes a problem that couldn't happen before, which doesn't seem
to be the case, I'm OK with leaving the problem unsolved.

-    // 1 - If the media element's readyState is HAVE_NOTHING, then raise an InvalidStateError exception.
-    if (m_readyState == HAVE_NOTHING) {
-        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.
     if (m_preload < MediaPlayer::Auto && m_readyState < HAVE_FUTURE_DATA)
         prepareToPlay();
@@ -1979,7 +1980,7 @@ 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);
+    bool noSeekRequired = !seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition && displayMode() != Poster);

[amogh.bihani, 2014/09/04 13:44:06]

Because of line 1944 we get time == now and hence seek never happens. In PS1, I
had mistakenly returned m_defaultPlaybackStartPosition in line 2082, hence it
was working :P.

Sorry it took nearly the whole day to debug this and briefly go through the
Fragment URI spec, hence I could not work much on handling fragment URI. I have
created a rought patch at https://codereview.chromium.org/539103002/.
This works fine but I have one doubt in it. Please help me there.

[philipj_slow, 2014/09/04 13:53:40]

Ah, right, this is a bit tricky now. The spec has split the concept of current
playback position and official playback position. The currentTime() exposed to
scripts is the official playback position, but using that internally is going to
cause little issues like this.

Maybe this isn't the place, but I would like to have
[ImplementedAs=officialPlaybackPosition] attribute double currentTime in the IDL
and only officialPlaybackPosition() and currentPlaybackPosition() internally so
that each call sites makes to make an explicit choice, hopefully matching the
spec.

[amogh.bihani, 2014/09/08 10:26:38]

Should I add a TODO here that this condition should be removed when official
playback position is implemented or first implement it and then land this on top
of that?

[philipj_slow, 2014/09/08 15:00:58]

If you want to work on official playback position that'd be nice, but it's
likely to be a bit complicated so I wouldn't block this CL on it. To fix this
problem, I would perhaps use "double now = m_cachedTime" above and add a comment
at that point saying that it's required to avoid getting the default playback
start position from currentTime().

[amogh.bihani, 2014/09/09 05:30:52]

Acknowledged.

 
     if (noSeekRequired) {
         if (time == now) {
@@ -2074,6 +2075,9 @@ double HTMLMediaElement::currentTime() const
     static const double minCachedDeltaForWarning = 0.01;
 #endif
 
+    if (m_defaultPlaybackStartPosition)
+        return m_defaultPlaybackStartPosition;
+
     if (m_readyState == HAVE_NOTHING)
         return 0;
 
@@ -2102,7 +2106,15 @@ void HTMLMediaElement::setCurrentTime(double time, ExceptionState& exceptionStat
         exceptionState.throwDOMException(InvalidStateError, "The element is slaved to a MediaController.");
         return;
     }
-    seek(time, exceptionState);
+
+    // If the media element's readyState is HAVE_NOTHING, then set the default
+    // playback start position to that time.
+    if (m_readyState == HAVE_NOTHING) {
+        m_defaultPlaybackStartPosition = time;
+        return;
+    }
+
+    seek(time);
 }
 
 double HTMLMediaElement::duration() const
@@ -2241,7 +2253,7 @@ void HTMLMediaElement::playInternal()
         scheduleDelayedAction(LoadMediaResource);
 
     if (endedPlayback())
-        seek(0, IGNORE_EXCEPTION);
+        seek(0);
 
     if (m_mediaController)
         m_mediaController->bringElementUpToSpeed(this);
@@ -3103,7 +3115,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(0);
         } 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
@@ -3157,7 +3169,7 @@ void HTMLMediaElement::durationChanged(double duration, bool requestSeek)
         renderer()->updateFromElement();
 
     if (requestSeek)
-        seek(duration, IGNORE_EXCEPTION);
+        seek(duration);
 }
 
 void HTMLMediaElement::mediaPlayerPlaybackStateChanged()
@@ -3188,10 +3200,10 @@ void HTMLMediaElement::mediaPlayerRequestSeek(double time)
 {
     // The player is the source of this seek request.
     if (m_mediaController) {
-        m_mediaController->setCurrentTime(time, IGNORE_EXCEPTION);
+        m_mediaController->setCurrentTime(time);
         return;
     }
-    setCurrentTime(time, IGNORE_EXCEPTION);
+    setCurrentTime(time, ASSERT_NO_EXCEPTION);
 }
 
 // MediaPlayerPresentation methods
@@ -3889,7 +3901,7 @@ void HTMLMediaElement::applyMediaFragmentURI()
     if (m_fragmentStartTime != MediaPlayer::invalidTime()) {
         m_sentEndEvent = false;
         UseCounter::count(document(), UseCounter::HTMLMediaElementSeekToFragmentStart);
-        seek(m_fragmentStartTime, IGNORE_EXCEPTION);
+        seek(m_fragmentStartTime);
     }
 }