MSE: Fix logic bugs with high precision duration

third_party/WebKit/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
@@ skipping 1643 matching lines @@
 
     selectInitialTracksIfNecessary();
 
     MediaFragmentURIParser fragmentParser(m_currentSrc);
     m_fragmentEndTime = fragmentParser.endTime();
 
     // Set the current playback position and the official playback position to
     // the earliest possible position.
     setOfficialPlaybackPosition(earliestPossiblePosition());
 
-    m_duration = duration();
+    m_duration = m_webMediaPlayer->duration();
     scheduleEvent(EventTypeNames::durationchange);
 
     if (isHTMLVideoElement())
       scheduleEvent(EventTypeNames::resize);
     scheduleEvent(EventTypeNames::loadedmetadata);
 
     bool jumped = false;
     if (m_defaultPlaybackStartPosition > 0) {
       seek(m_defaultPlaybackStartPosition);
       jumped = true;
@@ skipping 278 matching lines @@
 }
 
 double HTMLMediaElement::officialPlaybackPosition() const {
   // Hold updates to official playback position while paused or waiting for more
   // data. The underlying media player may continue to make small advances in
   // currentTime (e.g. as samples in the last rendered audio buffer are played
   // played out), but advancing currentTime while paused/waiting sends a mixed
   // signal about the state of playback.
   bool waitingForData = m_readyState <= kHaveCurrentData;
   if (m_officialPlaybackPositionNeedsUpdate && !m_paused && !waitingForData) {
-    // Internal player position may advance slightly beyond duration because
-    // many files use imprecise duration. Clamp official position to duration.
-    double newPosition = std::min(duration(), currentPlaybackPosition());
-    setOfficialPlaybackPosition(newPosition);
+    setOfficialPlaybackPosition(currentPlaybackPosition());
   }
 
 #if LOG_OFFICIAL_TIME_STATUS
   static const double minCachedDeltaForWarning = 0.01;
   double delta =
       std::abs(m_officialPlaybackPosition - currentPlaybackPosition());
   if (delta > minCachedDeltaForWarning) {
     BLINK_MEDIA_LOG << "currentTime(" << (void*)this
                     << ") - WARNING, cached time is " << delta
                     << "seconds off of media time when paused/waiting";
   }
 #endif
 
   return m_officialPlaybackPosition;
 }
 
 void HTMLMediaElement::setOfficialPlaybackPosition(double position) const {
 #if LOG_OFFICIAL_TIME_STATUS
   BLINK_MEDIA_LOG << "setOfficialPlaybackPosition(" << (void*)this
                   << ") was:" << m_officialPlaybackPosition
                   << " now:" << position;
 #endif
 
-  m_officialPlaybackPosition = position;
+  // Internal player position may advance slightly beyond duration because
+  // many files use imprecise duration. Clamp official position to duration when
+  // known. Duration may be unknown when readyState < HAVE_METADATA.
+  m_officialPlaybackPosition =
+      !std::isnan(duration()) ? std::min(duration(), position) : position;

[wolenetz, 2017/01/09 22:19:35]

nit: swap the ternary clauses to remove leading '!' and simplify readability?

Also, when duration() is NaN, must position be 0? (For MSE, I think so, but I'm
less certain about non-MSE where earliest playback position might not be 0.) If
so, sounds like a good DCHECK to add.

[chcunningham, 2017/01/10 18:49:29]

Done.
I think you might be right about non-MSE, so leaving no DCHECK.

+
+  if (m_officialPlaybackPosition != position) {
+    BLINK_MEDIA_LOG << "setOfficialPlaybackPosition(" << (void*)this
+                    << ") position:" << position
+                    << " truncated to duration:" << m_officialPlaybackPosition;
+  }
 
   // Once set, official playback position should hold steady until the next
   // stable state. We approximate this by using a microtask to mark the
   // need for an update after the current (micro)task has completed. When
   // needed, the update is applied in the next call to
   // officialPlaybackPosition().
   m_officialPlaybackPositionNeedsUpdate = false;
   Microtask::enqueueMicrotask(
       WTF::bind(&HTMLMediaElement::requireOfficialPlaybackPositionUpdate,
                 wrapWeakPersistent(this)));
@@ skipping 21 matching lines @@
   // playback start position to that time.
   if (m_readyState == kHaveNothing) {
     m_defaultPlaybackStartPosition = time;
     return;
   }
 
   seek(time);
 }
 
 double HTMLMediaElement::duration() const {
-  // FIXME: remove m_webMediaPlayer check once we figure out how
-  // m_webMediaPlayer is going out of sync with readystate.
-  // m_webMediaPlayer is cleared but readystate is not set to kHaveNothing.
-  if (!m_webMediaPlayer || m_readyState < kHaveMetadata)
-    return std::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 kHaveNothing ->
-  // kHaveMetadata. Currently, m_duration may be out of date for at least MSE
-  // case because MediaSource and SourceBuffer do not notify the element
-  // directly upon duration changes caused by endOfStream, remove, or append
-  // operations; rather the notification is triggered by the WebMediaPlayer
-  // implementation observing that the underlying engine has updated duration
-  // and notifying the element to consult its MediaSource for current
-  // duration. See http://crbug.com/266644
-
-  if (m_mediaSource)
-    return m_mediaSource->duration();
-
-  return webMediaPlayer()->duration();
+  return m_duration;
 }
 
 bool HTMLMediaElement::paused() const {
   return m_paused;
 }
 
 double HTMLMediaElement::defaultPlaybackRate() const {
   return m_defaultPlaybackRate;
 }
 
@@ skipping 1000 matching lines @@
       }
       // Queue a task to fire a simple event named ended at the media element.
       scheduleEvent(EventTypeNames::ended);
     }
   }
   updatePlayState();
 }
 
 void HTMLMediaElement::durationChanged() {
   BLINK_MEDIA_LOG << "durationChanged(" << (void*)this << ")";
+
+  // durationChanged() triggered by media player.

[wolenetz, 2017/01/09 22:19:34]

nit: s/() trig/() is trig/ ?

[chcunningham, 2017/01/10 18:49:29]

Done.

+  CHECK(m_webMediaPlayer);
+  double newDuration = m_webMediaPlayer->duration();
+
   // If the duration is changed such that the *current playback position* ends
   // up being greater than the time of the end of the media resource, then the
   // user agent must also seek to the time of the end of the media resource.
-  durationChanged(duration(), currentPlaybackPosition() > duration());
+  durationChanged(newDuration, currentPlaybackPosition() > newDuration);
 }
 
 void HTMLMediaElement::durationChanged(double duration, bool requestSeek) {

[wolenetz, 2017/01/09 22:19:34]

aside: the .h comment for this method indicates it's for MSE, but it looks like
it's at least in the common path for any durationChange coming from
webMediaPlayer now. Are we certain that no non-MSE media player ever calls
durationChanged? This is just a drive-by and not critical to this particular CL.

[chcunningham, 2017/01/10 18:49:29]

The only external caller is MSE's durationChangeAlgorithm

   BLINK_MEDIA_LOG << "durationChanged(" << (void*)this << ", " << duration
                   << ", " << boolString(requestSeek) << ")";
 
   // Abort if duration unchanged.
   if (m_duration == duration)
     return;
 
   BLINK_MEDIA_LOG << "durationChanged(" << (void*)this << ") : " << m_duration
                   << " -> " << duration;
   m_duration = duration;
@@ skipping 1042 matching lines @@
       kMostlyFillViewportBecomeStableSeconds, BLINK_FROM_HERE);
 }
 
 void HTMLMediaElement::viewportFillDebouncerTimerFired(TimerBase*) {
   m_mostlyFillingViewport = true;
   if (m_webMediaPlayer)
     m_webMediaPlayer->becameDominantVisibleContent(m_mostlyFillingViewport);
 }
 
 }  // namespace blink