Bug 1411504. P7 - don't change mChannelOffset in MediaCache::Update(). draft
authorJW Wang <jwwang@mozilla.com>
Wed, 25 Oct 2017 09:37:58 +0800
changeset 687249 c7d38ffe73d4984a64c8739ec8d64d3373b33fed
parent 687248 5c5b610efb91b2446dbaf45a328d28bbea5fcd03
child 687250 fc4e976431be6f65f78ffd9d78815935eac499ef
push id86457
push userjwwang@mozilla.com
push dateFri, 27 Oct 2017 02:39:04 +0000
bugs1411504
milestone58.0a1
Bug 1411504. P7 - don't change mChannelOffset in MediaCache::Update(). This is a fix to P3. Since seek is performed asynchronously by CacheClientSeek(), it is possible for OnStopRequest() to come before Seek(). Changing mChannelOffset will cause MediaCacheStream::NotifyDataEnded() to update mStreamLength incorrectly. mChannelOffset should only be changed in response to channel activities such as NotifyDataStarted() and NotifyDataReceived(). However, if MediaCache::Update() calls CacheClientSeek() without updating mChannelOffset, next Update() might make a wrong decision and call CacheClientSeek() again which is bad. So we add a member mSeekTarget to track if there is a pending seek on which the stream reading decisions will be made. MozReview-Commit-ID: VWP0vdlEYM
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -1313,47 +1313,53 @@ MediaCache::Update()
     actions.AppendElement(StreamAction{});
 
     MediaCacheStream* stream = mStreams[i];
     if (stream->mClosed) {
       LOG("Stream %p closed", stream);
       continue;
     }
 
+    // We make decisions based on mSeekTarget when there is a pending seek.
+    // Otherwise we will keep issuing seek requests until mChannelOffset
+    // is changed by NotifyDataStarted() which is bad.
+    int64_t channelOffset =
+      stream->mSeekTarget != -1 ? stream->mSeekTarget : stream->mChannelOffset;
+
     // Figure out where we should be reading from. It's the first
     // uncached byte after the current mStreamOffset.
     int64_t dataOffset = stream->GetCachedDataEndInternal(stream->mStreamOffset);
     MOZ_ASSERT(dataOffset >= 0);
 
     // Compute where we'd actually seek to to read at readOffset
     int64_t desiredOffset = dataOffset;
     if (stream->mIsTransportSeekable) {
-      if (desiredOffset > stream->mChannelOffset &&
-          desiredOffset <= stream->mChannelOffset + SEEK_VS_READ_THRESHOLD) {
+      if (desiredOffset > channelOffset &&
+          desiredOffset <= channelOffset + SEEK_VS_READ_THRESHOLD) {
         // Assume it's more efficient to just keep reading up to the
         // desired position instead of trying to seek
-        desiredOffset = stream->mChannelOffset;
+        desiredOffset = channelOffset;
       }
     } else {
       // We can't seek directly to the desired offset...
-      if (stream->mChannelOffset > desiredOffset) {
+      if (channelOffset > desiredOffset) {
         // Reading forward won't get us anywhere, we need to go backwards.
         // Seek back to 0 (the client will reopen the stream) and then
         // read forward.
         NS_WARNING("Can't seek backwards, so seeking to 0");
         desiredOffset = 0;
         // Flush cached blocks out, since if this is a live stream
         // the cached data may be completely different next time we
         // read it. We have to assume that live streams don't
         // advertise themselves as being seekable...
         ReleaseStreamBlocks(stream);
       } else {
         // otherwise reading forward is looking good, so just stay where we
         // are and don't trigger a channel seek!
-        desiredOffset = stream->mChannelOffset;
+        desiredOffset = channelOffset;
       }
     }
 
     // Figure out if we should be reading data now or not. It's amazing
     // how complex this is, but each decision is simple enough.
     bool enableReading;
     if (stream->mStreamLength >= 0 && dataOffset >= stream->mStreamLength) {
       // We want data at the end of the stream, where there's nothing to
@@ -1361,18 +1367,18 @@ MediaCache::Update()
       // might create a new channel and seek unnecessarily (and incorrectly,
       // since HTTP doesn't allow seeking to the actual EOF), and we don't want
       // to suspend if we're not suspended and already reading at the end of
       // the stream, since there just might be more data than the server
       // advertised with Content-Length, and we may as well keep reading.
       // But we don't want to seek to the end of the stream if we're not
       // already there.
       LOG("Stream %p at end of stream", stream);
-      enableReading = !stream->mCacheSuspended &&
-        stream->mStreamLength == stream->mChannelOffset;
+      enableReading =
+        !stream->mCacheSuspended && stream->mStreamLength == channelOffset;
     } else if (desiredOffset < stream->mStreamOffset) {
       // We're reading to try to catch up to where the current stream
       // reader wants to be. Better not stop.
       LOG("Stream %p catching up", stream);
       enableReading = true;
     } else if (desiredOffset < stream->mStreamOffset + BLOCK_SIZE) {
       // The stream reader is waiting for us, or nearly so. Better feed it.
       LOG("Stream %p feeding reader", stream);
@@ -1420,46 +1426,44 @@ MediaCache::Update()
       }
     }
 
     if (enableReading) {
       for (uint32_t j = 0; j < i; ++j) {
         MediaCacheStream* other = mStreams[j];
         if (other->mResourceID == stream->mResourceID && !other->mClosed &&
             !other->mClient->IsSuspended() &&
-            OffsetToBlockIndexUnchecked(other->mChannelOffset) ==
+            OffsetToBlockIndexUnchecked(other->mSeekTarget != -1
+                                          ? other->mSeekTarget
+                                          : other->mChannelOffset) ==
               OffsetToBlockIndexUnchecked(desiredOffset)) {
           // This block is already going to be read by the other stream.
           // So don't try to read it from this stream as well.
           enableReading = false;
           LOG("Stream %p waiting on same block (%" PRId32 ") from stream %p",
               stream,
               OffsetToBlockIndexUnchecked(desiredOffset),
               other);
           break;
         }
       }
     }
 
-    if (stream->mChannelOffset != desiredOffset && enableReading) {
+    if (channelOffset != desiredOffset && enableReading) {
       // We need to seek now.
       NS_ASSERTION(stream->mIsTransportSeekable || desiredOffset == 0,
                    "Trying to seek in a non-seekable stream!");
       // Round seek offset down to the start of the block. This is essential
       // because we don't want to think we have part of a block already
       // in mPartialBlockBuffer.
-      stream->mChannelOffset =
+      stream->mSeekTarget =
         OffsetToBlockIndexUnchecked(desiredOffset) * BLOCK_SIZE;
       actions[i].mTag = StreamAction::SEEK;
       actions[i].mResume = stream->mCacheSuspended;
-      actions[i].mSeekTarget = stream->mChannelOffset;
-      // mChannelOffset is updated to a new position. We don't want data from
-      // the old channel to be written to the wrong position. 0 is a sentinel
-      // value which will not match any ID passed to NotifyDataReceived().
-      stream->mLoadID = 0;
+      actions[i].mSeekTarget = stream->mSeekTarget;
     } else if (enableReading && stream->mCacheSuspended) {
       actions[i].mTag = StreamAction::RESUME;
     } else if (!enableReading && !stream->mCacheSuspended) {
       actions[i].mTag = StreamAction::SUSPEND;
     }
   }
 #ifdef DEBUG
   mInUpdate = false;
@@ -1915,33 +1919,37 @@ MediaCacheStream::NotifyDataStarted(uint
                                     int64_t aOffset,
                                     bool aSeekable)
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   MOZ_ASSERT(aLoadID > 0);
   LOG("Stream %p DataStarted: %" PRId64 " aLoadID=%u", this, aOffset, aLoadID);
 
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
-  NS_WARNING_ASSERTION(aOffset == mChannelOffset,
+  NS_WARNING_ASSERTION(aOffset == mSeekTarget || aOffset == mChannelOffset,
                        "Server is giving us unexpected offset");
   MOZ_ASSERT(aOffset >= 0);
   mChannelOffset = aOffset;
   if (mStreamLength >= 0) {
     // If we started reading at a certain offset, then for sure
     // the stream is at least that long.
     mStreamLength = std::max(mStreamLength, mChannelOffset);
   }
   mLoadID = aLoadID;
 
   MOZ_ASSERT(aOffset == 0 || aSeekable,
              "channel offset must be zero when we become non-seekable");
   mIsTransportSeekable = aSeekable;
   // Queue an Update since we may change our strategy for dealing
   // with this stream
   mMediaCache->QueueUpdate();
+
+  // Reset mSeekTarget since the seek is completed so MediaCache::Update() will
+  // make decisions based on mChannelOffset instead of mSeekTarget.
+  mSeekTarget = -1;
 }
 
 void
 MediaCacheStream::UpdatePrincipal(nsIPrincipal* aPrincipal)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
   while (MediaCacheStream* stream = iter.Next()) {
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -496,16 +496,18 @@ private:
   nsresult          mNotifyDataEndedStatus;
   // The last reported read mode
   ReadMode          mCurrentMode;
   // True if some data in mPartialBlockBuffer has been read as metadata
   bool              mMetadataInPartialBlockBuffer;
   // The load ID of the current channel. Used to check whether the data is
   // coming from an old channel and should be discarded.
   uint32_t mLoadID = 0;
+  // The seek target initiated by MediaCache. -1 if no seek is going on.
+  int64_t mSeekTarget = -1;
 
   bool mThrottleReadahead = false;
 
   // Data received for the block containing mChannelOffset. Data needs
   // to wait here so we can write back a complete block. The first
   // mChannelOffset%BLOCK_SIZE bytes have been filled in with good data,
   // the rest are garbage.
   // Heap allocate this buffer since the exact power-of-2 will cause allocation