1118589 - MSE: SourceBuffer::appendBuffer doesn't fill the buffer asynchronously

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

As per spec:
http://w3c.github.io/media-source/#widl-SourceBuffer-appendBuffer-void-ArrayBufferView-data:

"5. Asynchronously run the buffer append algorithm."

Currently the entire SourceBuffer::appendBuffer is done synchronously. This is causing an issue with bug 1118126.

For example, one problem of not running asynchronously could be:

var ms = new MediaSource();
var sb = new SourceBuffer();

ms.duration = 0; // this queue a secondary task that will change the duration
sb.appendBuffer(data); // this adds data to the source buffer.

Now the queue task that will set the duration is run and (with bug 1118126 fixed) will clear all data in the source buffer found after timestamp 0.

We now have a cleared source buffer.

Either setting the mediasource duration has to be done so that it's not processed asynchronously in a separate task; or make appendBuffer queue the task so it always occur after an earlier operation has completed.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] Run appendBuffer internal's asynchronously

Run appendBuffer's internal asynchronously rather than just the updateend event. I ponder if it would have been better to use NS_NewRunnableMethodWithArg with a nsTArary instead, as the change would have been smaller, but it would have doubled the memory usage (albeit temporarily)

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8545655 [details] [diff] [review]
[MSE] Run appendBuffer internal's asynchronously

I think cajbir is more suited for the review.
Plus I can see some issues with the order of events.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] Run appendBuffer internal's asynchronously

Add check if sourcebuffer was aborted before task got the chance to run

Comment 4

[cajbir (:cajbir)]

Comment on attachment 8545671 [details] [diff] [review]
[MSE] Run appendBuffer internal's asynchronously

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

::: dom/media/mediasource/SourceBuffer.cpp
@@ +39,5 @@
>  namespace mozilla {
>  
>  namespace dom {
>  
> +class AppendDataRunnable : public nsRunnable {

Add comment explaining what this class is for.

@@ +41,5 @@
>  namespace dom {
>  
> +class AppendDataRunnable : public nsRunnable {
> +public:
> +  explicit AppendDataRunnable(SourceBuffer* aSourceBuffer,

'explicit' isn't needed in multi argument constructors and is ignored by the compiler.

@@ +49,5 @@
> +  {
> +    mData.AppendElements(aData, aLength);
> +  }
> +
> +  NS_IMETHOD Run() MOZ_OVERRIDE MOZ_FINAL {

This does a lot of stuff internal to mSourceBuffer. How about move it all to a method of SourceBuffer and just call that method from Run(). Then you don't need to do all the mSourceBuffer-> stuff.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] Run appendBuffer internal's asynchronously

Carrying r+. Move code to SourceBuffer class, and re-base to support timestampOffset

Comment 6

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=762a6f0e0757

Comment 7

[Jean-Yves Avenard [:jya]]

Since bug 1119456, try has been failing test_eme_playback.patch on all non-debug build but windows

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fd45ecd1603

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: MSE: Run appendBuffer internal's asynchronously

Rebase...

Comment 9

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a20b290d8c86

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a20b290d8c86

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8554417 [details] [diff] [review]
MSE: Run appendBuffer internal's asynchronously

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve Flash video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This fixes a spec compliance issue and is MSE-specific. We don't know that it's critical for youtube, but I think the the churn is worthwhile to remove the branch variance.
[String/UUID change made/needed]: None.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47d2ad95b657
https://hg.mozilla.org/releases/mozilla-beta/rev/afc24a951c4e