1062661 - SourceBuffer eviction/TrackBuffer coalescing improvements

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Matthew Gregan [:kinetik]]

Each of these changes can be made in separate bugs, but to make it easier to discuss them I'm filing just one.  I'd like to make the following changes to the eviction and coalescing code:

- The threshold passed into TrackBuffer::Evict by SourceBuffer::AppendData should either be global, apply to the entire MediaSource, or at least apply to the entire SourceBuffer.  The current behavior applies the threshold to each decoder owned by a TrackBuffer.

- Verify that evicting 100% of a decoder's resource works correctly, then detect when this has happened and tear down that decoder's resources.

- Detect when a newer decoder owner by a TrackBuffer has a superset of media for a range of an earlier decoder and evict/remove the earlier decoder.
  - Subtask: also do this for cases where the newer decoder is not a superset and has only partial overlap.

Comment 1

[Matt Woodrow (:mattwoodrow)]

(In reply to Matthew Gregan [:kinetik] from comment #0)

> - The threshold passed into TrackBuffer::Evict by SourceBuffer::AppendData
> should either be global, apply to the entire MediaSource, or at least apply
> to the entire SourceBuffer.  The current behavior applies the threshold to
> each decoder owned by a TrackBuffer.

It appears that we currently apply the threshold to all the decoders owned by the TrackBuffer.

That seems sane, though we probably shouldn't bother kicking off the eviction process until we get a memory pressure event. I'll worry about that in a separate bug.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Fix ResourceQueue::Evict

I believe this was the intended behaviour of this function.

The value computed in TrackBuffer::EvictData, and passed as the aThreshold value for this function is the number of bytes we need to get rid of in order to meet the threshold. 

The existing code only evicted anything if there was more than that amount in the queue. Often this wouldn't be true, so we'd be left with ResourceQueue's with small amounts of data in them that won't ever be evicted.

The new code should evict as much as possible until it gets us under the threshold, or runs out of things to evict.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Discard decoders that don't have any data in them

When combined with part 1, I can see these code paths being hit when changing resolution on the dash test player (http://dash-mse-test.appspot.com/dash-player.html).

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Discard decoders that have been entirely superceeded by another

Not asking for review on this part yet because I haven't find a way to get this to actually happen. Any ideas?

I should also add some tests for TimeRanges::Subtract.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Discard superceeded data from decoders

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Discard decoders that don't have any data in them v2

Karl pointed at that the reader has a raw pointer to the decoder, and it's possible that an in-progress task is holding a reference keeping a reader alive.

This should make sure that any reader tasks are completed before destroying the decoder, and nulls out the pointer so we don't risk use-after-free.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8514026 [details] [diff] [review]
Part 2: Discard decoders that don't have any data in them v2

>-  for (uint32_t i = 0; i < mDecoders.Length(); ++i) {
>+  for (uint32_t i = 0; i < mInitializedDecoders.Length(); ++i) {
>     MSE_DEBUG("TrackBuffer(%p)::EvictData decoder=%u threshold=%u toEvict=%lld",
>               this, i, aThreshold, toEvict);
>-    toEvict -= mDecoders[i]->GetResource()->EvictData(toEvict);
>+    toEvict -= mInitializedDecoders[i]->GetResource()->EvictData(toEvict);

mInitializedDecoders is modified in RegisterDecoder on initialization task
queue, under decoder monitor, so can't be accessed here without holding the
monitor.

mDecoders shouldn't require holding the monitor to iterate, though I guess it
would be necessary to check that it has been initialized before removing it,
which would probably require holding the lock (but then only if it were a
candidate for release).  Also bug 1092915 would need fixing to remove
potential double free risks if using mDecoders.

>+    mDecoder->GetReader()->Shutdown();
>+    mDecoder->GetReader()->ClearDecoder();

Would it be practical to null mDecoder in MediaDecoderReader::Shutdown()
instead of adding a separate method for it?
MP4Reader::Shutdown() would then either need to chain up or also null mDecoder.

>   RefPtr<nsIRunnable> task = new ReleaseDecoderTask(aDecoder);
>+  nsRefPtr<MediaTaskQueue> taskQueue;
>   {
>     ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>-    MOZ_ASSERT(!mInitializedDecoders.Contains(aDecoder));
>     mDecoders.RemoveElement(aDecoder);
>+    if (mInitializedDecoders.RemoveElement(aDecoder)) {
>+      taskQueue = aDecoder->GetReader()->GetTaskQueue();
>+      task = new DelayedDispatchToMainThread(aDecoder);
>+    }
>+
>     if (mCurrentDecoder == aDecoder) {
>       DiscardDecoder();
>     }
>   }
>-  // At this point, task should be holding the only reference to aDecoder.
>-  NS_DispatchToMainThread(task);
>+  // At this point, decoder should be holding the only reference to aDecoder.
>+  if (taskQueue) {
>+    // If we were initialized, post the task via the reader's
>+    // task queue to ensure that the reader isn't in the middle
>+    // of an existing task.
>+    taskQueue->Dispatch(task);
>+  } else {
>+    NS_DispatchToMainThread(task);
>+  }

I wonder whether it would be simpler to always do the DelayedDispatch.  If
not, can you reorder things, please, so that ReleaseDecoderTask is only
created if necessary.  The Task needs to hold a ref before mDecoders releases.

I'm not sure exactly how this will look when bug 1092915 is fixed.

>+  virtual MediaTaskQueue* GetTaskQueue() {
>+    return mTaskQueue;
>+  }

This method doesn't seem to have any overrides.
Would you mind removing "virtual", while you are here, please?

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Discard decoders that don't have any data in them v3

Comment 9

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/974bf86f7002
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb67cbef0a07

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/974bf86f7002
https://hg.mozilla.org/mozilla-central/rev/bb67cbef0a07

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Can we close this?

Comment 12

[Matt Woodrow (:mattwoodrow)]

Yeah, we probably can.

There's still more things we can do for eviction, but we have other bugs tracking those.