Seeking media fragment URI before loadeddata event

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 9c001f5f1ccb05d5aecf15b6d685fd7ed57d5885..39e57e77f4eba3668c8df8ab8fe11b9e86f5a446 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -334,7 +334,6 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_cachedTime(MediaPlayer::invalidTime())
     , m_cachedTimeWallClockUpdateTime(0)
     , m_minimumWallClockTimeToCacheMediaTime(0)
-    , m_fragmentStartTime(MediaPlayer::invalidTime())
     , m_fragmentEndTime(MediaPlayer::invalidTime())
     , m_pendingActionFlags(0)
     , m_userGestureRequiredForPlay(false)
@@ -1813,22 +1812,41 @@ void HTMLMediaElement::setReadyState(ReadyState state)
     if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
         createPlaceholderTracksIfNecessary();
 
-        prepareMediaFragmentURI();
-
         selectInitialTracksIfNecessary();
 
+        MediaFragmentURIParser fragmentParser(m_currentSrc);
+
         m_duration = duration();
         scheduleEvent(EventTypeNames::durationchange);
 
+        double end = fragmentParser.endTime();

[amogh.bihani, 2014/09/05 09:05:43]

Does end==0 needs to be taken care of? As per line 2430 (which uses >= for
comparison) I don't think so, but wanted to confirm first. If it is not needed
then we can also remove 'double end' and save one more variable. :)

something like
m_fragmentEndtime = fragmentParser.endTime();
if (m_fragmentEndTime != invalidTime())
    m_fraggmentEndTime = std::min((std::max(0, m_fraggmentEndTime), duration);

[philipj_slow, 2014/09/05 22:49:11]

At this point it seems like we should just set m_fragmentEndTime =
fragmentParser.endTime() and it seems like
HTMLMediaElement::playbackProgressTimerFired will behave just fine. Clamping to
the duration seems pretty useless, I'm not sure why that was done originally.

[amogh.bihani, 2014/09/08 09:33:46]

Done.

+        if (end != MediaPlayer::invalidTime() && end > 0)
+            m_fragmentEndTime = std::min(end, m_duration);
+        else
+            m_fragmentEndTime = MediaPlayer::invalidTime();
+
         if (isHTMLVideoElement())
             scheduleEvent(EventTypeNames::resize);
         scheduleEvent(EventTypeNames::loadedmetadata);
 
+        bool jumped = false;
         if (m_defaultPlaybackStartPosition > 0) {
             seek(m_defaultPlaybackStartPosition);
+            jumped = true;
             m_defaultPlaybackStartPosition = 0;
         }
 
+        if (!jumped) {
+            double start = fragmentParser.startTime();
+            if (start !=MediaPlayer::invalidTime() && start > 0) {

[philipj_slow, 2014/09/05 22:49:12]

Missing whitespace after !=

+                m_sentEndEvent = false;
+                UseCounter::count(document(), UseCounter::HTMLMediaElementSeekToFragmentStart);
+                seek(start);
+                jumped = true;

[philipj_slow, 2014/09/05 22:49:11]

Don't need to set jumped, the spec doesn't AFAICT.

[amogh.bihani, 2014/09/08 09:33:45]

Step 10 says "if jumped is still false, seek to that time and let jumped be
true."

I had not implemented step 13 as I thought it is a big change and should be in a
new CL, although it turns out it wasn't so I am adding that in next PS.

[philipj_slow, 2014/09/08 14:42:31]

Oh sorry, sloppy spec reading on my part. That the use of jumped wasn't
implemented was indeed why I missed this.

+            }
+            // FIXME: Add support for selecting tracks by ID with the Media Fragments track dimension.
+        }
+
         if (hasMediaControls())
             mediaControls()->reset();
         if (renderer())
@@ -1842,7 +1860,6 @@ void HTMLMediaElement::setReadyState(ReadyState state)
         shouldUpdateDisplayState = true;
         scheduleEvent(EventTypeNames::loadeddata);
         setShouldDelayLoadEvent(false);
-        applyMediaFragmentURI();
     }
 
     bool isPotentiallyPlaying = potentiallyPlaying();
@@ -3869,42 +3886,6 @@ bool HTMLMediaElement::isBlockedOnMediaController() const
     return false;
 }
 
-void HTMLMediaElement::prepareMediaFragmentURI()
-{
-    MediaFragmentURIParser fragmentParser(m_currentSrc);
-    double dur = duration();
-
-    double start = fragmentParser.startTime();
-    if (start != MediaFragmentURIParser::invalidTimeValue() && start > 0) {
-        m_fragmentStartTime = start;
-        if (m_fragmentStartTime > dur)
-            m_fragmentStartTime = dur;
-    } else
-        m_fragmentStartTime = MediaPlayer::invalidTime();
-
-    double end = fragmentParser.endTime();
-    if (end != MediaFragmentURIParser::invalidTimeValue() && end > 0 && end > m_fragmentStartTime) {
-        m_fragmentEndTime = end;
-        if (m_fragmentEndTime > dur)
-            m_fragmentEndTime = dur;
-    } else
-        m_fragmentEndTime = MediaPlayer::invalidTime();
-
-    // FIXME: Add support for selecting tracks by ID with the Media Fragments track dimension.
-
-    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);
-    }
-}
-
 WebMediaPlayer::CORSMode HTMLMediaElement::corsMode() const
 {
     const AtomicString& crossOriginMode = fastGetAttribute(crossoriginAttr);