1286810 - [MSE] Disable abort() when range removal is in progress

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

As per https://github.com/w3c/media-source/issues/19, https://github.com/w3c/media-source/issues/20 and https://github.com/w3c/media-source/issues/26

abort() should throw an error if the range removal algorithm is pending.

Other changes include:
- Require remove() for all Range Removals
- Error on Duration Changes that need remove first
- Error on abort() during Range Removals

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P1. Error on abort() during Range Removals.

As per https://github.com/w3c/media-source/issues/26

Review commit: https://reviewboard.mozilla.org/r/64706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64706/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P2. Update duration change as per new MSE spec.

See w3c/MSE Issue 19, 20 & 26.

Changing the duration now can never call the range removal algorithm. An explicit call to remove must be used for range removal.
This change performs the following:
- Require remove() for all Range Removals
- Error on Duration Changes that need remove first

Review commit: https://reviewboard.mozilla.org/r/64708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64708/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P3. Update mochitests to reflect new duration behavior. .

update/updateend is no longer fired when changing the duration. Also update the test to use promises, it makes things more readable

Review commit: https://reviewboard.mozilla.org/r/64710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64710/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P4. Mochitest for new duration behavior.

Review commit: https://reviewboard.mozilla.org/r/64712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64712/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P5. Update webref MSE tests according to updated spec.

See w3c/MSE Issue 19, 20 & 26.

Changing the duration now can never call the range removal algorithm. An explicit call to remove must be used for range removal.
This spec change performed the following:
- Require remove() for all Range Removals
- Error on Duration Changes that need remove first
- Error on abort() during Range Removals

Review commit: https://reviewboard.mozilla.org/r/64714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64714/

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1286810: [MSE] P6. Add webref test ensuring new spec compliance.

Review commit: https://reviewboard.mozilla.org/r/64716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64716/

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771616 [details]
Bug 1286810: [MSE] P4. Mochitest for new duration behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64712/diff/1-2/

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771617 [details]
Bug 1286810: [MSE] P5. Update webref MSE tests according to updated spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64714/diff/1-2/

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771618 [details]
Bug 1286810: [MSE] P6. Add webref test ensuring new spec compliance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64716/diff/1-2/

Comment 10

[Gerald Squelart (he/him)]

Comment on attachment 8771613 [details]
Bug 1286810: [MSE] P1. Error on abort() during Range Removals.

https://reviewboard.mozilla.org/r/64706/#review61746

r+ as it works, but please consider making it simpler (unless there is a future reason for using a promise) :

::: dom/media/mediasource/SourceBuffer.cpp:255
(Diff revision 1)
> +  mPendingRemoval.Begin(
> -  mTrackBuffersManager->RangeRemoval(TimeUnit::FromSeconds(aStart),
> +    mTrackBuffersManager->RangeRemoval(TimeUnit::FromSeconds(aStart),
> -                                     TimeUnit::FromSeconds(aEnd))
> +                                       TimeUnit::FromSeconds(aEnd))
> -    ->Then(AbstractThread::MainThread(), __func__,
> +      ->Then(AbstractThread::MainThread(), __func__,
> -           [self] (bool) { self->StopUpdating(); },
> -           []() { MOZ_ASSERT(false); });
> +             [self] (bool) {
> +               self->mPendingRemoval.Complete();
> +               self->StopUpdating();
> +             },
> +             []() { MOZ_ASSERT(false); }));

Does 'mPendingRemoval' have to be a promise? It seems a bit heavy-handed when it could probably just be a bool.
Simpler code:
- mPendingRemoval.Begin -> mPendingRemoval = true
- mPendingRemoval.Complete() -> mPendingRemoval = false;
- if (mPendingRemoval.Exists()) at line 195 -> if (mPendingRemoval)
- And of course initialize it to false.

Comment 11

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/64706/#review61746

> Does 'mPendingRemoval' have to be a promise? It seems a bit heavy-handed when it could probably just be a bool.
> Simpler code:
> - mPendingRemoval.Begin -> mPendingRemoval = true
> - mPendingRemoval.Complete() -> mPendingRemoval = false;
> - if (mPendingRemoval.Exists()) at line 195 -> if (mPendingRemoval)
> - And of course initialize it to false.

What do you mean using a promise? it was already using a promise, I just happened to store it in a promise holder so I can check its status elsewhere.

How is that any more complicated?

we're just checking if the promise is pending. Far easier than having to deal with booleans IMHO, and there's no need for default value as it's automatic.

Comment 12

[Gerald Squelart (he/him)]

Comment on attachment 8771614 [details]
Bug 1286810: [MSE] P2. Update duration change as per new MSE spec.

https://reviewboard.mozilla.org/r/64708/#review61752

r+ with comment nit:

::: dom/media/mediasource/TrackBuffersManager.h:284
(Diff revision 1)
> +    // Highest presentation timestamp in track buffer. Write protected by
> +    // global monitor. Only ever written on taskqueue.

The explanation of the protection confuses me.
If I understand what you've done correctly, I'd explain it that way: "Protected by global monitor, except when reading on the task queue as it is only written there."

Comment 13

[Gerald Squelart (he/him)]

Comment on attachment 8771615 [details]
Bug 1286810: [MSE] P3. Update mochitests to reflect new duration behavior. .

https://reviewboard.mozilla.org/r/64710/#review61754

Comment 14

[Gerald Squelart (he/him)]

Comment on attachment 8771616 [details]
Bug 1286810: [MSE] P4. Mochitest for new duration behavior.

https://reviewboard.mozilla.org/r/64712/#review61768

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771613 [details]
Bug 1286810: [MSE] P1. Error on abort() during Range Removals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64706/diff/1-2/

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771614 [details]
Bug 1286810: [MSE] P2. Update duration change as per new MSE spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64708/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771615 [details]
Bug 1286810: [MSE] P3. Update mochitests to reflect new duration behavior. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64710/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771616 [details]
Bug 1286810: [MSE] P4. Mochitest for new duration behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64712/diff/2-3/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771617 [details]
Bug 1286810: [MSE] P5. Update webref MSE tests according to updated spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64714/diff/2-3/

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771618 [details]
Bug 1286810: [MSE] P6. Add webref test ensuring new spec compliance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64716/diff/2-3/

Comment 21

[James Graham [:jgraham]]

Comment on attachment 8771618 [details]
Bug 1286810: [MSE] P6. Add webref test ensuring new spec compliance.

These changes appear to use the testharness in the correct way, but I am not familiar with the MSE spec, so if you have a reviewer who is, I suggest that they review these tests.

Comment 22

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/64714/#review61866

::: testing/web-platform/tests/media-source/mediasource-duration.html:48
(Diff revision 3)
>  
> -                      test.expectEvent(mediaElement, 'seeking', 'Seeking to truncated duration');
> -
>                        assert_false(sourceBuffer.updating, 'sourceBuffer.updating');
>  
> -                      mediaSource.duration = truncatedDuration;
> +                      sourceBuffer.remove(truncatedDuration, Infinity);;

;;

Comment 23

[Gerald Squelart (he/him)]

Comment on attachment 8771617 [details]
Bug 1286810: [MSE] P5. Update webref MSE tests according to updated spec.

https://reviewboard.mozilla.org/r/64714/#review61880

r=gerald
You should add "f=jgraham" to the commit description.

Comment 24

[Gerald Squelart (he/him)]

Comment on attachment 8771618 [details]
Bug 1286810: [MSE] P6. Add webref test ensuring new spec compliance.

https://reviewboard.mozilla.org/r/64716/#review61884

r=gerald
You should add "f=jgraham" to the commit description.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65c0c29e778
[MSE] P1. Error on abort() during Range Removals. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/54291dfe3bcd
[MSE] P2. Update duration change as per new MSE spec. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c06c6fb41b
[MSE] P3. Update mochitests to reflect new duration behavior. r=gerald.
https://hg.mozilla.org/integration/mozilla-inbound/rev/af10601ac03f
[MSE] P4. Mochitest for new duration behavior. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cdbb75807fe
[MSE] P5. Update webref MSE tests according to updated spec. r=gerald,f=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/9afc48ae820f
[MSE] P6. Add webref test ensuring new spec compliance. r=gerald,f=jgraham

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d65c0c29e778
https://hg.mozilla.org/mozilla-central/rev/54291dfe3bcd
https://hg.mozilla.org/mozilla-central/rev/66c06c6fb41b
https://hg.mozilla.org/mozilla-central/rev/af10601ac03f
https://hg.mozilla.org/mozilla-central/rev/6cdbb75807fe
https://hg.mozilla.org/mozilla-central/rev/9afc48ae820f