Closed Bug 976037 Opened 6 years ago Closed 6 years ago

Implement eviction algorithm for MediaSourceExtensions

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: owpard, Assigned: owpard)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

The media source extensions implementation currently does not evict any data. All data is held in memory and the method for appending new data results in copying and moving memory.

The Media Source Extensions spec lists a coded frame eviction algorithm. Something like this should be implemented.
Blocks: MSE, 881512
Attached patch Implment an eviction algorithm (obsolete) — Splinter Review
This implements an eviction algorithm. It modifies the existing system so that a SourceBufferResource has a queue containing the data that is appended to it. 

The queue holds instances of ResourceItem which is an array of the bytes. Appending data to the SourceBufferResource pushes this onto the queue. As items are played from the queue. Data is evicted once it reaches a size threshold. This pops the items off the front of the queue and deletes it. If an eviction happens then the MediaSource that effectively owns the SourceBufferResource is notified (done in SourceBuffer::AppendData) which then requests all SourceBuffers to evict data up to approximately the same timepoint.

The timestamps used to evict are estimated using a rough guess. Hopefully improvements to this can be done in a further patch if needed.

There is currently no 'buffer full' flag maintained. It's assumed that data can be added always. Eviction does not remove any data before the current playback point. Since data is kept in memory we should refine in a future patch (or this one if the reviewer wants) more stringent buffer management rules. I'm unsure what good rules are here.

The demos on the following page work with this patch: http://cd.pn/mse/

I tested 'basic demo', 'dash player' and 'youtube player' with the 'feelings2' mpd. I was unable to test the youtube player with multiple streams as it requires being able to reset the stream which is not implemented.
Attachment #8380605 - Flags: review?(kinetik)
Comment on attachment 8380605 [details] [diff] [review]
Implment an eviction algorithm

Review of attachment 8380605 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: content/media/mediasource/MediaSource.h
@@ +84,5 @@
>    {
>      return mDecoder;
>    }
>  
> +  // Called by SourceBuffer's to notify this MediaSource that data has

No apostrophe.

::: content/media/mediasource/MediaSourceDecoder.h
@@ +78,5 @@
>    {
>      return mAudioReader;
>    }
>  
> +  double GetDuration();

Move this up to below GetSeekable and add MOZ_OVERRIDE.

::: content/media/mediasource/SourceBuffer.cpp
@@ +98,5 @@
>  
> +int64_t
> +SubBufferDecoder::ConvertToByteOffset(double aTime)
> +{
> +  // Uses a conversion based on (aTime/duration) * Length.  For the

Lower case l in length.

@@ +106,5 @@
> +  double duration = mParentDecoder->GetDuration();
> +  if (duration <= 0.0 || IsNaN(duration)) {
> +    return -1;
> +  }
> +  int64_t length = GetResource()->GetLength();

MOZ_ASSERT length is > 0

@@ +382,5 @@
> +    double s = ranges->Start(i, rv);
> +    double e = ranges->End(i, rv);
> +    startTime = (startTime < 0.0) ? s : (startTime < s ? startTime : s);
> +    endTime = endTime > e ? endTime : e;
> +  }

You could call ranges->Normalize(), then use ranges->Start(0) and ranges->End(ranges->Length() - 1) for start and end times.

::: content/media/mediasource/SourceBufferList.cpp
@@ +103,5 @@
>    }
>  }
>  
>  void
> +SourceBufferList::Evict(double aStart, double aEnd) {

Brace on new line
Attachment #8380605 - Flags: review?(kinetik) → review+
Attached patch Implement and eviction algorithm (obsolete) — Splinter Review
Updates review comments ready for landing.
Attachment #8380605 - Attachment is obsolete: true
Attachment #8381739 - Flags: review+
Attached patch Implement an eviction algorithm (obsolete) — Splinter Review
Minor changes to include a couple of header files to fix non-unified builds picked up by try. r+ carried forward.
Attachment #8381739 - Attachment is obsolete: true
Attachment #8382525 - Flags: review+
Assignee: nobody → cajbir
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
There was a test failure in the original patch. This fixes that. I'll attach an interdiff shortly.
Attachment #8382525 - Attachment is obsolete: true
Attachment #8383398 - Flags: review?(kinetik)
This is the interdiff between attachment 8383398 [details] [diff] [review] and the original patch that was backed out. The issue was that 'GetDuration' on the MediaSourceDecoder is expected to return the duration as computed by the Decoder, whereas the internal calculations for eviction need to use the MediaSource view of the duration.
Attachment #8383398 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/9b295844b85b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.