Bug 1245463: [MSE] P3. When abort() is called, wait until the current appendBuffer completes. r=gerald

The W3C spec indicates that while everything in MSE is asynchronous, the abort() command is to interrupt the current segment parser loop and have the reset parser loop synchronously completes the frames present in the input buffer.
This causes a fundamental issue that abort() will never result in a deterministic outcome as the segment parser loop may be in different condition.

We used to really attempt to abort the current operation, however there could have been a race in the order in which tasks were queued. As such, we now simply wait for the current appendBuffer to complete.

This also simplifies the code greatly, as we don't need to worry about pending concurrent appendBuffer.

The actually happens to be similar to the Chromium behavior.

Similar to bug 1239983, we strongly assert should a segment parser loop be running when it must have completed.

MozReview-Commit-ID: 9772PLQEozf

dom/media/mediasource/SourceBuffer.cpp

@@ -39,27 +39,6 @@ using media::TimeUnit;
 
 namespace dom {
 
-class BufferAppendRunnable : public nsRunnable {
-public:
-  BufferAppendRunnable(SourceBuffer* aSourceBuffer,
-                       uint32_t aUpdateID)
-  : mSourceBuffer(aSourceBuffer)
-  , mUpdateID(aUpdateID)
-  {
-  }
-
-  NS_IMETHOD Run() override final {
-
-    mSourceBuffer->BufferAppend(mUpdateID);
-
-    return NS_OK;
-  }
-
-private:
-  RefPtr<SourceBuffer> mSourceBuffer;
-  uint32_t mUpdateID;
-};
-
 void
 SourceBuffer::SetMode(SourceBufferAppendMode aMode, ErrorResult& aRv)
 {
@@ -226,10 +205,13 @@ void
 SourceBuffer::AbortBufferAppend()
 {
   if (mUpdating) {
-    mPendingAppend.DisconnectIfExists();
+    if (mPendingAppend.Exists()) {
-    // TODO: Abort stream append loop algorithms.
+      mPendingAppend.Disconnect();
-    // cancel any pending buffer append.
+      mContentManager->AbortAppendData();
-    mContentManager->AbortAppendData();
+      // Some data may have been added by the Segment Parser Loop.
+      // Check if we need to update the duration.
+      CheckEndTime();
+    }
     AbortUpdating();
   }
 }
@@ -309,7 +291,6 @@ SourceBuffer::SourceBuffer(MediaSource* aMediaSource, const nsACString& aType)
   , mMediaSource(aMediaSource)
   , mUpdating(false)
   , mActive(false)
-  , mUpdateID(0)
   , mType(aType)
 {
   MOZ_ASSERT(NS_IsMainThread());
@@ -381,7 +362,6 @@ SourceBuffer::StartUpdating()
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!mUpdating);
   mUpdating = true;
-  mUpdateID++;
   QueueAsyncSimpleEvent("updatestart");
 }
 
@@ -390,12 +370,8 @@ SourceBuffer::StopUpdating()
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (!mUpdating) {
-    // The buffer append algorithm has been interrupted by abort().
+    // The buffer append or range removal algorithm  has been interrupted by
-    //
+    // abort().
-    // If the sequence appendBuffer(), abort(), appendBuffer() occurs before
-    // the first StopUpdating() runnable runs, then a second StopUpdating()
-    // runnable will be scheduled, but still only one (the first) will queue
-    // events.
     return;
   }
   mUpdating = false;
@@ -407,7 +383,6 @@ void
 SourceBuffer::AbortUpdating()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(mUpdating);
   mUpdating = false;
   QueueAsyncSimpleEvent("abort");
   QueueAsyncSimpleEvent("updateend");
@@ -438,23 +413,13 @@ SourceBuffer::AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aR
 
   StartUpdating();
 
-  nsCOMPtr<nsIRunnable> task = new BufferAppendRunnable(this, mUpdateID);
+  BufferAppend();
-  NS_DispatchToMainThread(task);
 }
 
 void
-SourceBuffer::BufferAppend(uint32_t aUpdateID)
+SourceBuffer::BufferAppend()
 {
-  if (!mUpdating || aUpdateID != mUpdateID) {
+  MOZ_ASSERT(mUpdating);
-    // The buffer append algorithm has been interrupted by abort().
-    //
-    // If the sequence appendBuffer(), abort(), appendBuffer() occurs before
-    // the first StopUpdating() runnable runs, then a second StopUpdating()
-    // runnable will be scheduled, but still only one (the first) will queue
-    // events.
-    return;
-  }
-
   MOZ_ASSERT(mMediaSource);
   MOZ_ASSERT(!mPendingAppend.Exists());
 
@@ -467,11 +432,8 @@ SourceBuffer::BufferAppend(uint32_t aUpdateID)
 void
 SourceBuffer::AppendDataCompletedWithSuccess(bool aHasActiveTracks)
 {
+  MOZ_ASSERT(mUpdating);
   mPendingAppend.Complete();
-  if (!mUpdating) {
-    // The buffer append algorithm has been interrupted by abort().
-    return;
-  }
 
   if (aHasActiveTracks) {
     if (!mActive) {
@@ -494,7 +456,9 @@ SourceBuffer::AppendDataCompletedWithSuccess(bool aHasActiveTracks)
 void
 SourceBuffer::AppendDataErrored(nsresult aError)
 {
+  MOZ_ASSERT(mUpdating);
   mPendingAppend.Complete();
+
   switch (aError) {
     case NS_ERROR_ABORT:
       // Nothing further to do as the trackbuffer has been shutdown.
@@ -510,10 +474,7 @@ void
 SourceBuffer::AppendError(bool aDecoderError)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  if (!mUpdating) {
+
-    // The buffer append algorithm has been interrupted by abort().
-    return;
-  }
   mContentManager->ResetParserState();
 
   mUpdating = false;

dom/media/mediasource/SourceBuffer.h

@@ -238,7 +238,7 @@ private:
 
   // Shared implementation of AppendBuffer overloads.
   void AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv);
-  void BufferAppend(uint32_t aAppendID);
+  void BufferAppend();
 
   // Implement the "Append Error Algorithm".
   // Will call endOfStream() with "decode" error if aDecodeError is true.
@@ -266,11 +266,6 @@ private:
 
   mozilla::Atomic<bool> mActive;
 
-  // Each time mUpdating is set to true, mUpdateID will be incremented.
-  // This allows for a queued AppendData task to identify if it was earlier
-  // aborted and another AppendData queued.
-  uint32_t mUpdateID;
-
   MozPromiseRequestHolder<SourceBufferContentManager::AppendPromise> mPendingAppend;
   const nsCString mType;
 

dom/media/mediasource/SourceBufferContentManager.h

@@ -108,10 +108,6 @@ public:
   virtual void RestartGroupStartTimestamp() {}
   virtual media::TimeUnit GroupEndTimestamp() = 0;
 
-#if defined(DEBUG)
-  virtual void Dump(const char* aPath) { }
-#endif
-
 protected:
   virtual ~SourceBufferContentManager() { }
 };

dom/media/mediasource/TrackBuffersManager.cpp

@@ -96,15 +96,14 @@ TrackBuffersManager::TrackBuffersManager(dom::SourceBufferAttributes* aAttribute
   , mType(aType)
   , mParser(ContainerParser::CreateForMIMEType(aType))
   , mProcessedInput(0)
-  , mAppendRunning(false)
   , mTaskQueue(aParentDecoder->GetDemuxer()->GetTaskQueue())
   , mSourceBufferAttributes(aAttributes)
   , mParentDecoder(new nsMainThreadPtrHolder<MediaSourceDecoder>(aParentDecoder, false /* strict */))
-  , mAbort(false)
   , mEvictionThreshold(Preferences::GetUint("media.mediasource.eviction_threshold",
                                             100 * (1 << 20)))
   , mEvictionOccurred(false)
   , mMonitor("TrackBuffersManager")
+  , mAppendRunning(false)
 {
   MOZ_ASSERT(NS_IsMainThread(), "Must be instanciated on the main thread");
 }
@@ -135,7 +134,6 @@ TrackBuffersManager::AppendIncomingBuffer(IncomingBuffer aData)
 {
   MOZ_ASSERT(OnTaskQueue());
   mIncomingBuffers.AppendElement(aData);
-  mAbort = false;
 }
 
 RefPtr<TrackBuffersManager::AppendPromise>
@@ -144,32 +142,33 @@ TrackBuffersManager::BufferAppend()
   MOZ_ASSERT(NS_IsMainThread());
   MSE_DEBUG("");
 
+  mAppendRunning = true;
   return InvokeAsync(GetTaskQueue(), this,
                      __func__, &TrackBuffersManager::InitSegmentParserLoop);
 }
 
-// Abort any pending AppendData.
+// The MSE spec requires that we abort the current SegmentParserLoop
-// We don't really care about really aborting our inner loop as by spec the
+// which is then followed by a call to ResetParserState.
-// process is happening asynchronously, as such where and when we would abort is
+// However due to our asynchronous design this causes inherent difficulities.
-// non-deterministic. The SourceBuffer also makes sure BufferAppend
+// As the spec behaviour is non deterministic anyway, we instead wait until the
-// isn't called should the appendBuffer be immediately aborted.
+// current AppendData has completed its run.
-// We do however want to ensure that no new task will be dispatched on our task
-// queue and only let the current one finish its job. For this we set mAbort
-// to true.
 void
 TrackBuffersManager::AbortAppendData()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MSE_DEBUG("");
 
-  mAbort = true;
+  MonitorAutoLock mon(mMonitor);
+  while (mAppendRunning) {
+    mon.Wait();
+  }
 }
 
 void
 TrackBuffersManager::ResetParserState()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(!mAppendRunning, "AbortAppendData must have been called");
+  MOZ_RELEASE_ASSERT(!mAppendRunning, "Append is running, abort must have been called");
   MSE_DEBUG("");
 
   // 1. If the append state equals PARSING_MEDIA_SEGMENT and the input buffer contains some complete coded frames, then run the coded frame processing algorithm until all of these complete coded frames have been processed.
@@ -302,20 +301,6 @@ TrackBuffersManager::Detach()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MSE_DEBUG("");
-
-  // Abort pending operations if any.
-  AbortAppendData();
-
-  RefPtr<TrackBuffersManager> self = this;
-  nsCOMPtr<nsIRunnable> task =
-    NS_NewRunnableFunction([self] () {
-      // Clear our sourcebuffer
-      self->CodedFrameRemoval(TimeInterval(TimeUnit::FromSeconds(0),
-                                           TimeUnit::FromInfinity()));
-      self->mProcessingPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
-      self->mAppendPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
-    });
-  GetTaskQueue()->Dispatch(task.forget());
 }
 
 #if defined(DEBUG)
@@ -348,7 +333,7 @@ void
 TrackBuffersManager::CompleteResetParserState()
 {
   MOZ_ASSERT(OnTaskQueue());
-  MOZ_ASSERT(!mAppendRunning);
+  MOZ_RELEASE_ASSERT(!mAppendRunning);
   MSE_DEBUG("");
 
   for (auto& track : GetTracksList()) {
@@ -586,8 +571,9 @@ RefPtr<TrackBuffersManager::AppendPromise>
 TrackBuffersManager::InitSegmentParserLoop()
 {
   MOZ_ASSERT(OnTaskQueue());
+  MOZ_RELEASE_ASSERT(mAppendPromise.IsEmpty());
+  MSE_DEBUG("");
 
-  MOZ_ASSERT(mAppendPromise.IsEmpty() && !mAppendRunning);
   RefPtr<AppendPromise> p = mAppendPromise.Ensure(__func__);
 
   AppendIncomingBuffers();
@@ -621,6 +607,7 @@ void
 TrackBuffersManager::SegmentParserLoop()
 {
   MOZ_ASSERT(OnTaskQueue());
+
   while (true) {
     // 1. If the input buffer is empty, then jump to the need more data step below.
     if (!mInputBuffer || mInputBuffer->IsEmpty()) {
@@ -724,7 +711,7 @@ TrackBuffersManager::SegmentParserLoop()
           ->Then(GetTaskQueue(), __func__,
                  [self] (bool aNeedMoreData) {
                    self->mProcessingRequest.Complete();
-                   if (aNeedMoreData || self->mAbort) {
+                   if (aNeedMoreData) {
                      self->NeedMoreData();
                    } else {
                      self->ScheduleSegmentParserLoop();
@@ -743,19 +730,23 @@ void
 TrackBuffersManager::NeedMoreData()
 {
   MSE_DEBUG("");
-  if (!mAbort) {
+  RestoreCachedVariables();
-    RestoreCachedVariables();
-  }
-  mAppendRunning = false;
   mAppendPromise.ResolveIfExists(mActiveTrack, __func__);
+  // Wake-up any pending Abort()
+  MonitorAutoLock mon(mMonitor);
+  mAppendRunning = false;
+  mon.NotifyAll();
 }
 
 void
 TrackBuffersManager::RejectAppend(nsresult aRejectValue, const char* aName)
 {
   MSE_DEBUG("rv=%d", aRejectValue);
-  mAppendRunning = false;
   mAppendPromise.RejectIfExists(aRejectValue, aName);
+  // Wake-up any pending Abort()
+  MonitorAutoLock mon(mMonitor);
+  mAppendRunning = false;
+  mon.NotifyAll();
 }
 
 void
@@ -831,12 +822,7 @@ void
 TrackBuffersManager::OnDemuxerResetDone(nsresult)
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort));
   mDemuxerInitRequest.Complete();
-  if (mAbort) {
-    RejectAppend(NS_ERROR_ABORT, __func__);
-    return;
-  }
   // mInputDemuxer shouldn't have been destroyed while a demuxer init/reset
   // request was being processed. See bug 1239983.
   MOZ_DIAGNOSTIC_ASSERT(mInputDemuxer);
@@ -908,13 +894,8 @@ void
 TrackBuffersManager::OnDemuxerInitDone(nsresult)
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort));
   mDemuxerInitRequest.Complete();
 
-  if (mAbort) {
-    RejectAppend(NS_ERROR_ABORT, __func__);
-    return;
-  }
   // mInputDemuxer shouldn't have been destroyed while a demuxer init/reset
   // request was being processed. See bug 1239983.
   MOZ_DIAGNOSTIC_ASSERT(mInputDemuxer);
@@ -1174,9 +1155,8 @@ TrackBuffersManager::OnDemuxFailed(TrackType aTrack,
                                    DemuxerFailureReason aFailure)
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("Failed to demux %s, failure:%d mAbort:%d",
+  MSE_DEBUG("Failed to demux %s, failure:%d",
-            aTrack == TrackType::kVideoTrack ? "video" : "audio",
+            aTrack == TrackType::kVideoTrack ? "video" : "audio", aFailure);
-            aFailure, static_cast<bool>(mAbort));
   switch (aFailure) {
     case DemuxerFailureReason::END_OF_STREAM:
     case DemuxerFailureReason::WAITING_FOR_DATA:
@@ -1203,15 +1183,10 @@ void
 TrackBuffersManager::DoDemuxVideo()
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort));
   if (!HasVideo()) {
     DoDemuxAudio();
     return;
   }
-  if (mAbort) {
-    RejectProcessing(NS_ERROR_ABORT, __func__);
-    return;
-  }
   mVideoTracks.mDemuxRequest.Begin(mVideoTracks.mDemuxer->GetSamples(-1)
                              ->Then(GetTaskQueue(), __func__, this,
                                     &TrackBuffersManager::OnVideoDemuxCompleted,
@@ -1232,15 +1207,10 @@ void
 TrackBuffersManager::DoDemuxAudio()
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort));
   if (!HasAudio()) {
     CompleteCodedFrameProcessing();
     return;
   }
-  if (mAbort) {
-    RejectProcessing(NS_ERROR_ABORT, __func__);
-    return;
-  }
   mAudioTracks.mDemuxRequest.Begin(mAudioTracks.mDemuxer->GetSamples(-1)
                              ->Then(GetTaskQueue(), __func__, this,
                                     &TrackBuffersManager::OnAudioDemuxCompleted,
@@ -1261,7 +1231,6 @@ void
 TrackBuffersManager::CompleteCodedFrameProcessing()
 {
   MOZ_ASSERT(OnTaskQueue());
-  MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort));
 
   // 1. For each coded frame in the media segment run the following steps:
   // Coded Frame Processing steps 1.1 to 1.21.
@@ -1332,22 +1301,12 @@ TrackBuffersManager::CompleteCodedFrameProcessing()
 void
 TrackBuffersManager::RejectProcessing(nsresult aRejectValue, const char* aName)
 {
-  if (mAbort) {
-    // mAppendPromise will be resolved immediately upon mProcessingPromise
-    // completing.
-    mAppendRunning = false;
-  }
   mProcessingPromise.RejectIfExists(aRejectValue, __func__);
 }
 
 void
 TrackBuffersManager::ResolveProcessing(bool aResolveValue, const char* aName)
 {
-  if (mAbort) {
-    // mAppendPromise will be resolved immediately upon mProcessingPromise
-    // completing.
-    mAppendRunning = false;
-  }
   mProcessingPromise.ResolveIfExists(aResolveValue, __func__);
 }
 

dom/media/mediasource/TrackBuffersManager.h

@@ -310,10 +310,6 @@ private:
   MozPromiseHolder<CodedFrameProcessingPromise> mProcessingPromise;
 
   MozPromiseHolder<AppendPromise> mAppendPromise;
-  // Set to true while SegmentParserLoop is running. This is used for diagnostic
-  // purposes only. We can't rely on mAppendPromise to be empty as it is only
-  // cleared in a follow up task.
-  bool mAppendRunning;
 
   // Trackbuffers definition.
   nsTArray<TrackData*> GetTracksList();
@@ -349,8 +345,6 @@ private:
   RefPtr<dom::SourceBufferAttributes> mSourceBufferAttributes;
   nsMainThreadPtrHandle<MediaSourceDecoder> mParentDecoder;
 
-  // Set to true if abort was called.
-  Atomic<bool> mAbort;
   // Set to true if mediasource state changed to ended.
   Atomic<bool> mEnded;
 
@@ -360,7 +354,10 @@ private:
   Atomic<bool> mEvictionOccurred;
 
   // Monitor to protect following objects accessed across multipple threads.
+  // mMonitor is also notified if the value of mAppendRunning becomes false.
   mutable Monitor mMonitor;
+  // Set to true while SegmentParserLoop is running.
+  Atomic<bool> mAppendRunning;
   // Stable audio and video track time ranges.
   media::TimeIntervals mVideoBufferedRanges;
   media::TimeIntervals mAudioBufferedRanges;