Closed Bug 1462990 Opened 6 years ago Closed 6 years ago

Use async/await in mediasource mochitests

Categories

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

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

Tracking

()

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

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(2 files)

Bug 1462990 - Use async/await in mediasource/test/mediasource.js
59 bytes, text/x-review-board-request
bryce
: review+
Details
Bug 1462990 - Use async/await in mediasource mochitests
59 bytes, text/x-review-board-request
bryce
: review+
Details 
Update the mediasource mochitests to use async/await.
Attachment #8979106 - Flags: feedback?(jyavenard)
Attachment #8979107 - Flags: feedback?(jyavenard)
Comment on attachment 8979107 [details]
Bug 1462990 - Use async/await in mediasource mochitests

https://reviewboard.mozilla.org/r/245350/#review251594

LGTM. Minor style nits. I prefer event handlers to some of the inline async decl and calls for erros, change or leave as you feel appropriate. Both changesets could be merged as they both end up touching mediasource.js.

::: dom/media/mediasource/test/test_AVC3_mp4.html:19
(Diff revision 2)
>  
> -runWithMSE(function(ms, el) {
> +runWithMSE(async (ms, el) => {
>  
> -  once(ms, "sourceopen").then(function() {
> +  await once(ms, "sourceopen");
> -    ok(true, "Receive a sourceopen event");
> +  ok(true, "Receive a sourceopen event");
>      const videosb = ms.addSourceBuffer("video/mp4");

Indentation

::: dom/media/mediasource/test/test_BufferingWait.html:17
(Diff revision 2)
>  
> -let receivedSourceOpen = false;
> -runWithMSE(function(ms, v) {
> +runWithMSE(async (ms, v) => {
> +  await once(ms, "sourceopen");
> -  ms.addEventListener("sourceopen", function() {
> -    ok(true, "Receive a sourceopen event");
> +  ok(true, "Receive a sourceopen event");
> -    ok(!receivedSourceOpen, "Should only receive one sourceopen for this test");
> +  (async () => {

I wonder if this is more readable than using a listener? I think those familiar with aysnc will be fine, but having to read to the right/lower side to understand this is being invoked immediately may throw some. We also retain the use of event handlers for other error handling such as sb several lines down. Leave or change as you see fit (along with other files with the same pattern).

::: dom/media/mediasource/test/test_BufferingWait.html:40
(Diff revision 2)
> -        const promise = waitUntilTime(v, .767 - 0.04);
> -        info("Playing video. It should play for a bit, then fire 'waiting'");
> +  info("Playing video. It should play for a bit, then fire 'waiting'");
> -        v.play();
> +  v.play();
> -        return promise;
> -      }).then(function() {
> -        window.firstStop = Date.now();
> +  await waitUntilTime(v, .767 - 0.04);
> +  const firstStop = Date.now();
> +  const p = loadSegment(sb, new Uint8Array(arrayBuffer, 46712, 67833 - 46712)); // no await (on purpose?)

I believe we could wait on this promise, and that the tests original intention is that the wait below will cover that anyway -- I believe the segment load here will enable us to reach the time waited on below. I personally prefer waiting on both the load and the wait until time. It would be nice if the test had a few more comments about the ranges and literals floating about (I suspect 0.04 is being used as an epsilon, but who knows?), however that is outside the scope of these patches.

::: dom/media/mediasource/test/test_BufferingWait.html:46
(Diff revision 2)
> -      }).then(function() {
> -        const waitDuration = (Date.now() - window.firstStop) / 1000;
> -        ok(waitDuration < 15, "Should not spend an inordinate amount of time buffering: " + waitDuration);
> +  ok(waitDuration < 15, "Should not spend an inordinate amount of time buffering: " + waitDuration);
> +  await p;
> -        SimpleTest.finish();
> +  SimpleTest.finish();
> -        /* If we allow the rest of the stream to be played, we get stuck at
> +  /* If we allow the rest of the stream to be played, we get stuck at

We can probably bring this code back in, but out of scope for this bug unless we want to drive by it.

::: dom/media/mediasource/test/test_BufferingWait_mp4.html:39
(Diff revision 2)
> -      const promise = waitUntilTime(v, 1.57895);
> -      info("Playing video. It should play for a bit, then fire 'waiting'");
> +  info("Playing video. It should play for a bit, then fire 'waiting'");
> -      v.play();
> +  v.play();
> -      return promise;
> -    }).then(function() {
> -      window.firstStop = Date.now();
> +  await waitUntilTime(v, 1.57895);
> +  const firstStop = Date.now();
> +  const p = fetchAndLoad(sb, "bipbop/bipbop", [ "3" ], ".m4s"); // no await (on purpose?)

As with similar case I think we can wait here.

::: dom/media/mediasource/test/test_DrainOnMissingData_mp4.html:33
(Diff revision 2)
> -      return once(el, "waiting");
> +  await once(el, "waiting");
> -    }).then(function() {
> -      info("got waiting");
> +  info("got waiting");
> -      info("Loading more data");
> +  info("Loading more data");
> -      // Waiting will be fired on the last frame +- 40ms.
> +  // Waiting will be fired on the last frame +- 40ms.
>        isfuzzy(el.currentTime, videosb.buffered.end(0) - 1 / 30,

Indentation.

::: dom/media/mediasource/test/test_DurationChange.html:64
(Diff revision 2)
> -            is(e.name, "InvalidStateError", "Error is InvalidStateError");
> -            error = true;
> -          }
> -          ok(error, "got an error");
> -          error = false;
> -          try {
> +             "InvalidStateError");
> +  is(v.duration, sb.buffered.end(0),
> +     "duration is the highest end time reported by the buffered attribute ");
> +  await once(sb, "updateend");
> +  ms.endOfStream();
> +  await once(ms, "sourceended");

Have we changed the order here? In the original test this listener was installed much earlier, right? I think this shouldn't matter since the line above will result in the event being fired once we're waiting, but want to confirm.

::: dom/media/mediasource/test/test_Eviction_mp4.html:33
(Diff revision 2)
> -    audiosb.mode = "sequence";
> +  audiosb.mode = "sequence";
> -    fetchAndLoad(audiosb, "bipbop/bipbop_audio", [ "init" ], ".mp4")
> -    .then(function() {
> -      fetchWithXHR("bipbop/bipbop_audio1.m4s", function(audioBuffer) {
> -        fillUpSourceBuffer(audiosb,
> -          function() { // doAppendDataFunc
> +  await fetchAndLoad(audiosb, "bipbop/bipbop_audio", [ "init" ], ".mp4");
> +  const audioBuffer = await fetchWithXHR("bipbop/bipbop_audio1.m4s");
> +
> +  await must_reject(async () => {
> +      // We are appending data repeatedly in sequence mode, there should be no gaps.

Indented 1 level too far.

::: dom/media/mediasource/test/test_Eviction_mp4.html:36
(Diff revision 2)
> -      fetchWithXHR("bipbop/bipbop_audio1.m4s", function(audioBuffer) {
> -        fillUpSourceBuffer(audiosb,
> -          function() { // doAppendDataFunc
> -            audiosb.appendBuffer(audioBuffer);
> +
> +  await must_reject(async () => {
> +      // We are appending data repeatedly in sequence mode, there should be no gaps.
> +      while (true) {
> +        ok(audiosb.buffered.length <= 1, "there should be no gap in buffered ranges.");
> +        await audiosb.appendBuffer(audioBuffer);

I don't think we need to await here.

::: dom/media/mediasource/test/test_MediaSource_memory_reporting.html:33
(Diff revision 2)
>  
> -    let amount = 0;
> -    let resourcePathSeen = false;
> +  let amount;
> +  const handleReport = (aProcess, aPath, aKind, aUnits, aAmount) => {
> -    const handleReport = function(aProcess, aPath, aKind, aUnits, aAmount, aDesc) {
> -      if (aPath == "explicit/media/resources") {
> +    if (aPath == "explicit/media/resources") {
> -        resourcePathSeen = true;
> +      amount = (amount || 0) + aAmount;

It saves a line of code to combine `resourcePathSeen` and `amount` here as it took me a moment to decode the meanings for amount here and the `(amount || 0)`, but I the previous more explicit style. Change or leave as you see fit.

::: dom/media/mediasource/test/test_MediaSource_mp4.html:92
(Diff revision 2)
>      loadedmetadataCount++;
>    });
>  
>    v.addEventListener("ended", function() {
>      // The bipbop video doesn't start at 0. The old MSE code adjust the
> -    // timestamps and ignore the audio track. The new one doesn't.
> +    // timestamps and ignore the audio track. The new one doesn"t.

' -> " shouldn't change in comment. Let me know if the linter did this automatically, because it shouldn't and I can raise it with the devtools team.

::: dom/media/mediasource/test/test_SplitAppendDelay_mp4.html:40
(Diff revision 2)
>      });
>    });
>  
>    v.addEventListener("ended", function() {
>      // The bipbop video doesn't start at 0. The old MSE code adjust the
> -    // timestamps and ignore the audio track. The new one doesn't.
> +    // timestamps and ignore the audio track. The new one doesn"t.

' -> " in comment.

::: dom/media/mediasource/test/test_SplitAppend_mp4.html:37
(Diff revision 2)
>      });
>    });
>  
>    v.addEventListener("ended", function() {
>      // The bipbop video doesn't start at 0. The old MSE code adjust the
> -    // timestamps and ignore the audio track. The new one doesn't.
> +    // timestamps and ignore the audio track. The new one doesn"t.

' -> " in comment.

::: dom/media/mediasource/test/test_Threshold_mp4.html:29
(Diff revision 2)
> -    .then(fetchAndLoad.bind(null, videosb, "bipbop/bipbop_video", range(1, 5), ".m4s"))
> +  await fetchAndLoad(videosb, "bipbop/bipbop_video", range(1, 5), ".m4s");
> -    .then(function() {
> -      // We will insert a gap of threshold
> +  // We will insert a gap of threshold
> -      videosb.timestampOffset = threshold;
> +  videosb.timestampOffset = threshold;
> -      return fetchAndLoad(videosb, "bipbop/bipbop_video", range(5, 9), ".m4s");
> -    }).then(function() {
> +  await fetchAndLoad(videosb, "bipbop/bipbop_video", range(5, 9), ".m4s");
> +  // HTMLMediaElement fires "waiting" if somebody invokes |play()| before the MDSM

Possibly unwanted ' -> " in all 3 lines of comment.
Attachment #8979107 - Flags: review?(bvandyk) → review+
Comment on attachment 8979106 [details]
Bug 1462990 - Use async/await in mediasource/test/mediasource.js

https://reviewboard.mozilla.org/r/245348/#review252502
Attachment #8979106 - Flags: review?(bvandyk) → review+
Comment on attachment 8979106 [details]
Bug 1462990 - Use async/await in mediasource/test/mediasource.js

https://reviewboard.mozilla.org/r/245348/#review252640

::: dom/media/mediasource/test/mediasource.js:20
(Diff revision 2)
> -  function bootstrapTest() {
> +  await once(window, "load");
> +  await SpecialPowers.pushPrefEnv({"set": gMSETestPrefs});
> +
> -    const ms = new MediaSource();
> +  const ms = new MediaSource();
>  
> -    const el = document.createElement("video");
> +  let el = document.createElement("video");

this looks out of scope to me, espeically considering why const was used instead of let

::: dom/media/mediasource/test/mediasource.js:31
(Diff revision 2)
> -      el.remove();
> +    el.remove();
> -      el.removeAttribute("src");
> +    el.removeAttribute("src");
> -      el.load();
> +    el.load();
> -    });
> +  });
> -
> -    testFunction(ms, el);
> +  try {
> +    await testFunction(ms, el);

this assumes that testFunction return a promise, which isn't the case for any of the current test.

::: dom/media/mediasource/test/mediasource.js:39
(Diff revision 2)
> -    SpecialPowers.pushPrefEnv({"set": gMSETestPrefs}, bootstrapTest);
> -  });
>  }
>  
> -function fetchWithXHR(uri, onLoadFunction) {
> -  const p = new Promise(function(resolve, reject) {
> +async function fetchWithXHR(uri, onLoadFunction) {
> +  let result = await new Promise(resolve => {

ditto const vs let

::: dom/media/mediasource/test/mediasource.js:51
(Diff revision 2)
>      });
>      xhr.send();
>    });
>  
>    if (onLoadFunction) {
> -    p.then(onLoadFunction);
> +    result = await onLoadFunction(result);

same deal, this now requires onLoadFunction to return a promise.

that sounds counter intuitive to me seeing how fetchWithXHR is called.

it also no longer returns a promise, which will require to embed all methods...

::: dom/media/mediasource/test/mediasource.js:53
(Diff revision 2)
>    });
>  
>    if (onLoadFunction) {
> -    p.then(onLoadFunction);
> +    result = await onLoadFunction(result);
>    }
> -
> +  return result;

you no longer return a promise, and this code now means all calls to FetchXHR now run sequentially while they would have run in parallel before, allowing to quickly load all segments at once

::: dom/media/mediasource/test/mediasource.js:67
(Diff revision 2)
> -    target.addEventListener(name, function() {
> -      resolve();
> -    }, {once: true});
> -  });
>    if (cb) {
> -    p.then(cb);
> +    result = await cb();

this isn't equivalent to what was there before.

you now wait for cb() to complete, while before it returned a promise

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

so fetchWithXHR no longer return a promise, will run sequentially

And yet we do Promise.all on that array of value
Attachment #8979106 - Flags: review-
Comment on attachment 8979106 [details]
Bug 1462990 - Use async/await in mediasource/test/mediasource.js

https://reviewboard.mozilla.org/r/245348/#review252640

> this assumes that testFunction return a promise, which isn't the case for any of the current test.

It does allow for future tests to do so, and with the next changeset a number will now be async. It will also remain compatible with non-async funcitons.

> same deal, this now requires onLoadFunction to return a promise.
> 
> that sounds counter intuitive to me seeing how fetchWithXHR is called.
> 
> it also no longer returns a promise, which will require to embed all methods...

The function should still immediately retun a promise when called, but will resolve it when the final `return result;` is hit. It's a bit different in the reading, but I believe this is functionally equivalent. onLoadFunction doesn't have to return a promise, I believe this approach should be flexible for if it is thenable or not (as handled by await).

> you no longer return a promise, and this code now means all calls to FetchXHR now run sequentially while they would have run in parallel before, allowing to quickly load all segments at once

I think this should still work correctly. Here's a codepen showing how new uses should still work in parallel https://codepen.io/SingingTree/pen/Bxgywm?editors=1011

> this isn't equivalent to what was there before.
> 
> you now wait for cb() to complete, while before it returned a promise

The function should still immediately retun a promise when called, but will resolve it when the final `return result;` is hit. It's a bit different in the reading, but I believe this is functionally equivalent.
Comment on attachment 8979106 [details]
Bug 1462990 - Use async/await in mediasource/test/mediasource.js

https://reviewboard.mozilla.org/r/245348/#review252640

> this looks out of scope to me, espeically considering why const was used instead of let

Looks like I only ran lint on the second patch, where this got fixed. I'll rebase those changes here.

> It does allow for future tests to do so, and with the next changeset a number will now be async. It will also remain compatible with non-async funcitons.

It doesn't assume `testFunction` returns a promise, since `await undefined` is perfectly fine, and merely pushes the remainder of the function onto a micro-task.

> ditto const vs let

Can't use `const` here since `result` is conditionally modified further down.

> The function should still immediately retun a promise when called, but will resolve it when the final `return result;` is hit. It's a bit different in the reading, but I believe this is functionally equivalent. onLoadFunction doesn't have to return a promise, I believe this approach should be flexible for if it is thenable or not (as handled by await).

This should be equivalent. E.g. `await 1; foo()` is the same as `Promise.resolve(3).then(foo)`.

> I think this should still work correctly. Here's a codepen showing how new uses should still work in parallel https://codepen.io/SingingTree/pen/Bxgywm?editors=1011

`result` is never a promise here, always the resolved value. It's there only to support the `cb` callback feature, which is a poor pattern, since we want to return the callback's result in case it returned something, and the response from fetch in the case where no callback was provided.

We should rip this out now that none of the tests rely on it anymore.

> The function should still immediately retun a promise when called, but will resolve it when the final `return result;` is hit. It's a bit different in the reading, but I believe this is functionally equivalent.

Pretty sure it's equivalent. `async` functions describe the promise chain, so from the perspective of a caller they "return" at the first `await`. Like a generator function.

> so fetchWithXHR no longer return a promise, will run sequentially
> 
> And yet we do Promise.all on that array of value

All `async` functions return a promise. Type `(async () => {})()` into console and you get
`Promise { <state>: "fulfilled", <value>: undefined }`
Comment on attachment 8979106 [details]
Bug 1462990 - Use async/await in mediasource/test/mediasource.js

https://reviewboard.mozilla.org/r/245348/#review252746

thanks for those explanations
Attachment #8979106 - Flags: review- → review+
Comment on attachment 8979107 [details]
Bug 1462990 - Use async/await in mediasource mochitests

https://reviewboard.mozilla.org/r/245350/#review251594

The first patch also works with the existing tests.

> Have we changed the order here? In the original test this listener was installed much earlier, right? I think this shouldn't matter since the line above will result in the event being fired once we're waiting, but want to confirm.

Right. The test succeeds on try.

> It saves a line of code to combine `resourcePathSeen` and `amount` here as it took me a moment to decode the meanings for amount here and the `(amount || 0)`, but I the previous more explicit style. Change or leave as you see fit.

I think it's simpler to say "did we get an amount (value 0 or more)?" (or did it remain `undefined`)?

So I'm sticking up for this one. Thanks.

> ' -> " shouldn't change in comment. Let me know if the linter did this automatically, because it shouldn't and I can raise it with the devtools team.

Linter's fine. Just couldn't get it working in time. You don't want to know what I did. :)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5715760325a0a1a980eca4a72d2f573a9294c675 -d 27450c1a3366: rebasing 465254:5715760325a0 "Bug 1462990 - Use async/await in mediasource/test/mediasource.js r=bryce,jya"
rebasing 465255:483ec12e0c36 "Bug 1462990 - Use async/await in mediasource mochitests r=bryce" (tip)
merging dom/media/mediasource/test/test_ChangeWhileWaitingOnMissingData_mp4.html
warning: conflicts while merging dom/media/mediasource/test/test_ChangeWhileWaitingOnMissingData_mp4.html! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9d31a050a67
Use async/await in mediasource/test/mediasource.js r=bryce,jya
https://hg.mozilla.org/integration/autoland/rev/5f5b118631b0
Use async/await in mediasource mochitests r=bryce
https://hg.mozilla.org/mozilla-central/rev/e9d31a050a67
https://hg.mozilla.org/mozilla-central/rev/5f5b118631b0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: