Closed Bug 1280613 Opened 8 years ago Closed 6 years ago

[MSE] Have appendBuffer and remove return a promise

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Bug 1280613 - P3. Mochitests.
59 bytes, text/x-review-board-request
bryce
: review+
Details
Bug 1280613 - P1. Add experimental SourceBuffer.appendBufferAsync.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1280613 - P2. Add experimental SourceBuffer.removeAsync.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Implementation details are described upstream at https://github.com/w3c/media-source/issues/100

appendBuffer/remove would now return a promise that would be resolved/rejects upon termination of the respective algorithm.
Mass change P2 -> P3
Priority: P2 → P3
Comment on attachment 8972916 [details]
Bug 1280613 - P1. Add experimental SourceBuffer.appendBufferAsync.

https://reviewboard.mozilla.org/r/241474/#review247338
Attachment #8972916 - Flags: review?(bzbarsky)
Comment on attachment 8972917 [details]
Bug 1280613 - P2. Add experimental SourceBuffer.removeAsync.

https://reviewboard.mozilla.org/r/241476/#review247340
Attachment #8972917 - Flags: review?(bzbarsky)
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review247748

Mostly nits, but the calls to `setupTest` need to be changed so we're waiting on the pref change promise.

::: dom/media/mediasource/test/mediasource.js:129
(Diff revision 2)
> +    var afterBuffered = timeRangeToString(sb.buffered);
> +    info(`SourceBuffer buffered ranges grew from ${beforeBuffered} to ${afterBuffered}`);
> +  });
> +}
> +
> +function fetchAndLoadAsync(sb, prefix, chunks, suffix) {

Nit: This function could benefit from async await in terms of brevity (and clarity, IMO) if so desired. E.g., I believe the following is functionally equiavalent (let is used so that `chunk` doesn't shadow in the second loop):
```
async function fetchAndLoadAsync(sb, prefix, chunks, suffix) {
  // Fetch the buffers in parallel.
  var buffers = {};
  var fetches = [];
  for (let chunk of chunks) {
    fetches.push(fetchWithXHR(prefix + chunk + suffix).then(response => buffers[chunk] = response));
  }

  // Load them in series, as required per spec.
  await Promise.all(fetches);
  for (let chunk of chunks) {
    await loadSegmentAsync(sb, buffers[chunk]);
  }
}
```

::: dom/media/mediasource/test/mediasource.js:135
(Diff revision 2)
> +
> +  // Fetch the buffers in parallel.
> +  var buffers = {};
> +  var fetches = [];
> +  for (var chunk of chunks) {
> +    fetches.push(fetchWithXHR(prefix + chunk + suffix).then(((c, x) => buffers[c] = x).bind(null, chunk)));

Nit: Is there a reason to use `bind` here rather than `fetches.push(fetchWithXHR(prefix + chunk + suffix).then(response => buffers[chunk] = response));`?

::: dom/media/mediasource/test/mediasource.js:137
(Diff revision 2)
> +  var buffers = {};
> +  var fetches = [];
> +  for (var chunk of chunks) {
> +    fetches.push(fetchWithXHR(prefix + chunk + suffix).then(((c, x) => buffers[c] = x).bind(null, chunk)));
> +  }
> +  

nit: whitespace

::: dom/media/mediasource/test/test_ExperimentalAsync.html:14
(Diff revision 2)
> +<body>
> +<pre id="test"><script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});

Nit: mix of single and double quotes. Prefer double.

::: dom/media/mediasource/test/test_ExperimentalAsync.html:16
(Diff revision 2)
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});
> +}
> +setupTest();

This call will immediately return with a promise which we're not handling. It's equivalent to `SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});`

::: dom/media/mediasource/test/test_ExperimentalAsync.html:41
(Diff revision 2)
> +      fillUpSourceBuffer(sourceBuffer, doAppendDataFunc, onCaughtExceptionCallback);
> +    }, (ex) => {
> +      ok(true, "appendBuffer promise got rejected");
> +      onCaughtExceptionCallback(ex);
> +    });
> +  } catch(ex) {

Nit: missing space after catch keywords.

::: dom/media/mediasource/test/test_ExperimentalAsync.html:57
(Diff revision 2)
> +    // Test removeAsync
> +    audiosb.mode = "sequence";
> +    fetchWithXHR('bipbop/bipbop_audioinit.mp4', function(audioInitBuffer) {
> +      return audiosb.appendBufferAsync(audioInitBuffer);
> +    })
> +    .then(() => {

Nit: This would be a lot more readable using async await. At the deepest we're 12 callbacks deep, with our linter allowing for a max of 10.

::: dom/media/mediasource/test/test_ExperimentalAsync.html:99
(Diff revision 2)
> +                             is(el.currentTime, el.duration, "played to the end");
> +                             SimpleTest.finish();
> +                           });
> +                         });
> +                       }, () => {
> +                         ok(false, "shouldn't throw");

Can we change the language here and below to use 'reject' instead of 'throw'?

::: dom/media/mediasource/test/test_FrameSelectionExperimental.html:15
(Diff revision 2)
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});

Nit: mix of single and double quotes. Prefer double.

::: dom/media/mediasource/test/test_FrameSelectionExperimental.html:17
(Diff revision 2)
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});
> +}
> +setupTest();

As with comment in test_ExperimentalAsync.html

::: dom/media/mediasource/test/test_FrameSelectionExperimental.html:28
(Diff revision 2)
> +                   { currentTime: 0, videoWidth: 320, videoHeight: 240 }];
> +    var target;
> +
> +var lowResBuffer;
> +runWithMSE(function (ms, v) {
> +  ms.addEventListener("sourceopen", function () {

Nit: There's a mix of functions with spaces before the parens and without. Prefer without.

::: dom/media/mediasource/test/test_FrameSelectionExperimental.html:49
(Diff revision 2)
> +      // Append initialization segment.
> +      info("Appending low-res init segment");
> +      lowResBuffer = arrayBuffer;
> +      return sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 438));
> +    }).then(function() {
> +      var promises = [];

This var is not used.

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:23
(Diff revision 2)
> +// and that we've seeked to the new end of the media as per W3C spec and
> +// video.currentTime got updated.
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});

Nit: mix of single and double quotes. Prefer double.

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:25
(Diff revision 2)
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});
> +}
> +setupTest();

As with comment in test_ExperimentalAsync.html

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:43
(Diff revision 2)
> +  var v = e.target;
> +  v.removeEventListener("seeked", do_seeked);
> +  var duration = round(v.duration / 3);
> +  is(v._sb.updating, false, "sourcebuffer isn't updating");
> +  v._sb.remove(duration, Infinity).then(function() {
> +    v._ms.duration = duration

Missing semicolon

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:45
(Diff revision 2)
> +  var duration = round(v.duration / 3);
> +  is(v._sb.updating, false, "sourcebuffer isn't updating");
> +  v._sb.remove(duration, Infinity).then(function() {
> +    v._ms.duration = duration
> +    // frames aren't truncated, so duration may be slightly more.
> +    isfuzzy(v.duration, duration, 1/30, "element duration was updated");

Nit: space around division infix operator.

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:56
(Diff revision 2)
> +    // Truncated mediasource duration will cause the video element to seek.
> +    v.addEventListener("seeking", do_seeking);
> +  });
> +}
> +
> +runWithMSE(function (ms, v) {

Nit: There's a mix of functions with spaces before the parens and without. Prefer without.
Attachment #8972915 - Flags: review?(bvandyk) → review-
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review247748

??? that's what await is for, the function is declared async

> Nit: This function could benefit from async await in terms of brevity (and clarity, IMO) if so desired. E.g., I believe the following is functionally equiavalent (let is used so that `chunk` doesn't shadow in the second loop):
> ```
> async function fetchAndLoadAsync(sb, prefix, chunks, suffix) {
>   // Fetch the buffers in parallel.
>   var buffers = {};
>   var fetches = [];
>   for (let chunk of chunks) {
>     fetches.push(fetchWithXHR(prefix + chunk + suffix).then(response => buffers[chunk] = response));
>   }
> 
>   // Load them in series, as required per spec.
>   await Promise.all(fetches);
>   for (let chunk of chunks) {
>     await loadSegmentAsync(sb, buffers[chunk]);
>   }
> }
> ```

its a copy of the existing code, if you want to rewrite the existing code, feel free to take that in another bug

> Nit: Is there a reason to use `bind` here rather than `fetches.push(fetchWithXHR(prefix + chunk + suffix).then(response => buffers[chunk] = response));`?

yes, because you want the chunk variable current state to be bound to that promise, as chunk is changing in each loop.
this is the same as the existing code, just duplicated as the appendBufferAsync returns a promise

> Nit: mix of single and double quotes. Prefer double.

you wrote to use single quote in a previous test i wrote... pick one

> This call will immediately return with a promise which we're not handling. It's equivalent to `SpecialPowers.pushPrefEnv({'set': [['media.mediasource.experimental.enabled', true]]});`

no it will not, the method itself is async and await for the change

> Nit: This would be a lot more readable using async await. At the deepest we're 12 callbacks deep, with our linter allowing for a max of 10.

except that all this code is dependent on the previous callback being called, which cant be made async
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review247748

> its a copy of the existing code, if you want to rewrite the existing code, feel free to take that in another bug

You are free to not change the nits, but I wished to indicate what I felt would be easier for other to read. I do not think existing code need impact our choices going forward in this regard.

> yes, because you want the chunk variable current state to be bound to that promise, as chunk is changing in each loop.
> this is the same as the existing code, just duplicated as the appendBufferAsync returns a promise

I believe the closure would close over the current chunk.

> you wrote to use single quote in a previous test i wrote... pick one

I believe I stated double there also, as specified by our linting rules: https://reviewboard.mozilla.org/r/240788/#review246518

> no it will not, the method itself is async and await for the change

This is not the case, see here: https://codepen.io/SingingTree/pen/JvOwWv?editors=1011

> except that all this code is dependent on the previous callback being called, which cant be made async

aysnc await does exactly that. It is simply syntactic sugar over promises.
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review248058

::: dom/media/mediasource/test/test_TruncatedDurationExperimental.html:22
(Diff revision 2)
> +// We ensure that the buffered range immediately reflect the truncation
> +// and that we've seeked to the new end of the media as per W3C spec and
> +// video.currentTime got updated.
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {

setupTest here is equivalent to:

function setupTest() {
  return new Promise((resolve, reject) => {
    await SpecialPowers...
  });
}

The function passed to "new Promise" does not run to completion synchronously before setupTest() returns.  setupTest() returns a promise and start running the function passed asycnhronously.

So running setupTest() does not wait for the promise to resolve before execution continues. If you want that, you need:

await setupTest();

Or setupTest().then(runWithMSE...)
following further discussion with the chrome team, something that is not 100% backward compatible isn't going to cut it.
so the appendBuffer/remove modification serves no purpose, let's focus on appendBufferAsync/removeAsync instead only
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review247748

> Can we change the language here and below to use 'reject' instead of 'throw'?

good point...
within a await blah.catch(...), throw may be more appropriate no? as await returns an exception if the promise is rejected.
Comment on attachment 8972916 [details]
Bug 1280613 - P1. Add experimental SourceBuffer.appendBufferAsync.

https://reviewboard.mozilla.org/r/241474/#review249248

::: dom/media/mediasource/SourceBuffer.h:14
(Diff revision 3)
>  
>  #include "mozilla/MozPromise.h"
>  #include "MediaContainerType.h"
>  #include "MediaSource.h"
>  #include "js/RootingAPI.h"
> +#include "js/TypeDecls.h"

Why do you need this include?  I don't see a need for it.

::: dom/media/mediasource/SourceBuffer.h:203
(Diff revision 3)
>    RefPtr<TimeRanges> mBuffered;
>  
>    MozPromiseRequestHolder<MediaSource::ActiveCompletionPromise> mCompletionPromise;
> +
> +  // Only used if MSE v2 experimental mode is active.
> +  RefPtr<Promise> mDOMPromise;

Should document what this is used for.

::: dom/media/mediasource/SourceBuffer.cpp:543
(Diff revision 3)
> +already_AddRefed<Promise>
> +SourceBuffer::AppendDataAsync(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIGlobalObject> parentObject =

Presumably the spec should define which global is used for this promise.  We should make sure it defines the "relevant global" and there are WPTs for that.

The globals of "this" and "mMediaSource" are guaranteed to match because SourceBuffer::GetParentObject() returns the mediasource, right?  Worth a comment to that effect here.
Attachment #8972916 - Flags: review?(bzbarsky) → review+
Comment on attachment 8972917 [details]
Bug 1280613 - P2. Add experimental SourceBuffer.removeAsync.

https://reviewboard.mozilla.org/r/241476/#review249286
Attachment #8972917 - Flags: review?(bzbarsky) → review+
Comment on attachment 8972916 [details]
Bug 1280613 - P1. Add experimental SourceBuffer.appendBufferAsync.

https://reviewboard.mozilla.org/r/241474/#review249248

> Presumably the spec should define which global is used for this promise.  We should make sure it defines the "relevant global" and there are WPTs for that.
> 
> The globals of "this" and "mMediaSource" are guaranteed to match because SourceBuffer::GetParentObject() returns the mediasource, right?  Worth a comment to that effect here.

not quite.
the names of GetParentObject() is confusing however.
SourceBuffer::GetParentObject() returns the mediasource object, which is the same of mMediaSource.
But MediaSource::GetParentObject() returns the nsPIDOMWindowInner* which is obtained at construction time via: nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports());

maybe I should rename parentObject into mediaSourceParentObject as it's more explicit.
> not quite.

Why not?  The global of a thing, from the point of view of bindings, is the global of its GetParentObject().  That's the whole point of the GetParentObject() function.
Because for some obscure reason SourceBuffer::GetParentObjecf gets your the media-source the source buffer is attached to, and not the usual GetParentObject
Should probably change that.
There is no "usual GetParentObject".  The only thing the bindings want is that the GetParentObject() chain reaches a global eventually.  Doesn't have to be in one hop.
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review249464

::: dom/media/mediasource/test/test_ExperimentalAsync.html:94
(Diff revisions 2 - 4)
> -                             SimpleTest.finish();
> +          SimpleTest.finish();
> +          throw ex; // ensure we don't fallback on lines below.
> -                           });
> +        });
> +        ok(false, "should have returned an error");
> -                         });
> +      });
> -                       }, () => {
> +      ok(false, "should have returned an error")

Missing semicolon
Attachment #8972915 - Flags: review?(bvandyk) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #28)
> There is no "usual GetParentObject".  The only thing the bindings want is
> that the GetParentObject() chain reaches a global eventually.  Doesn't have
> to be in one hop.

I didn't know this. Thank you
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #28)
> There is no "usual GetParentObject".  The only thing the bindings want is
> that the GetParentObject() chain reaches a global eventually.  Doesn't have
> to be in one hop.

well, i tried again:
-  nsCOMPtr<nsIGlobalObject> parentObject = do_QueryInterface(GetParentObject());  -> doesn't work
+  nsCOMPtr<nsIGlobalObject> parentObject = do_QueryInterface(mMediaSource->GetParentObject()); -> works

that is the mochitest work.
Comment on attachment 8972915 [details]
Bug 1280613 - P3. Mochitests.

https://reviewboard.mozilla.org/r/241472/#review249464

> Missing semicolon

thanks for the review
> well, i tried again:

Well, right, if GetParentObject() doesn't return a global object then you can't QI it to global object...
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5bf2e9ff60a
P1. Add experimental SourceBuffer.appendBufferAsync. r=bz
https://hg.mozilla.org/integration/autoland/rev/bc87e9eef2b8
P2. Add experimental SourceBuffer.removeAsync. r=bz
https://hg.mozilla.org/integration/autoland/rev/38d9226183f0
P3. Mochitests. r=bryce
Doc updates are in progress. I have rough first drafts of the following pages in place:

https://developer.mozilla.org/en-US/docs/Web/API/SourceBuffer/appendBufferAsync
https://developer.mozilla.org/en-US/docs/Web/API/SourceBuffer/removeAsync

I've added these methods to:

https://developer.mozilla.org/en-US/docs/Web/API/SourceBuffer

And these changes are now linked to from https://developer.mozilla.org/en-US/Firefox/Experimental_features.

I've also submitted updates to the browser compatibility database for this.

Remaining to do is to finish up the two reference pages with code snippets and such.
Examples derived from the mochitests have been added to the appendBufferAsync() and removeAsync() pages listed in comment 39. They have not been independently tested because I have no way to do so currently set up here. If someone can look and make sure they aren’t outright wrong, that would be helpful. :)

Assuming that’s good, this documentation is now complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: