SourceBuffer eviction/TrackBuffer coalescing improvements

RESOLVED FIXED in mozilla36

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kinetik, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → matt.woodrow
(Assignee)

Comment 1

3 years ago
(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.
(Assignee)

Comment 2

3 years ago
Created attachment 8513232 [details] [diff] [review]
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.
Attachment #8513232 - Flags: review?(kinetik)
(Assignee)

Comment 3

3 years ago
Created attachment 8513233 [details] [diff] [review]
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).
Attachment #8513233 - Flags: review?(kinetik)
(Assignee)

Comment 4

3 years ago
Created attachment 8513234 [details] [diff] [review]
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8513235 [details] [diff] [review]
Part 4: Discard superceeded data from decoders
(Reporter)

Updated

3 years ago
Attachment #8513232 - Flags: review?(kinetik) → review+
(Reporter)

Updated

3 years ago
Attachment #8513233 - Flags: review?(kinetik) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8514026 [details] [diff] [review]
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.
Attachment #8513233 - Attachment is obsolete: true
Attachment #8514026 - Flags: review?(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?
Attachment #8514026 - Flags: review?(karlt) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8516267 [details] [diff] [review]
Part 2: Discard decoders that don't have any data in them v3
Attachment #8514026 - Attachment is obsolete: true
Attachment #8516267 - Flags: review?(karlt)
Attachment #8516267 - Flags: review?(karlt) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/974bf86f7002
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb67cbef0a07
https://hg.mozilla.org/mozilla-central/rev/974bf86f7002
https://hg.mozilla.org/mozilla-central/rev/bb67cbef0a07
Priority: -- → P1
Can we close this?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 12

3 years ago
Yeah, we probably can.

There's still more things we can do for eviction, but we have other bugs tracking those.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(matt.woodrow)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1092915
You need to log in before you can comment on or make changes to this bug.