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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

3 years ago
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
Assignee

Updated

3 years ago
Blocks: MSE
Assignee

Comment 1

3 years ago
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/
Attachment #8771613 - Flags: review?(gsquelart)
Attachment #8771614 - Flags: review?(gsquelart)
Attachment #8771615 - Flags: review?(gsquelart)
Attachment #8771616 - Flags: review?(gsquelart)
Attachment #8771617 - Flags: review?(james)
Attachment #8771618 - Flags: review?(james)
Assignee

Comment 2

3 years ago
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/
Assignee

Comment 3

3 years ago
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/
Assignee

Comment 5

3 years ago
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/
Assignee

Comment 7

3 years ago
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/
Assignee

Comment 8

3 years ago
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/
Assignee

Comment 9

3 years ago
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 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.
Attachment #8771613 - Flags: review?(gsquelart) → review+
Assignee

Comment 11

3 years ago
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 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."
Attachment #8771614 - Flags: review?(gsquelart) → review+
Comment on attachment 8771615 [details]
Bug 1286810: [MSE] P3. Update mochitests to reflect new duration behavior. .

https://reviewboard.mozilla.org/r/64710/#review61754
Attachment #8771615 - Flags: review?(gsquelart) → review+
Comment on attachment 8771616 [details]
Bug 1286810: [MSE] P4. Mochitest for new duration behavior.

https://reviewboard.mozilla.org/r/64712/#review61768
Attachment #8771616 - Flags: review?(gsquelart) → review+
Assignee

Comment 15

3 years ago
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/
Assignee

Comment 16

3 years ago
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/
Assignee

Comment 17

3 years ago
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/
Assignee

Comment 18

3 years ago
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/
Assignee

Comment 19

3 years ago
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/
Assignee

Comment 20

3 years ago
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 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.
Attachment #8771618 - Flags: review?(james) → feedback+
Attachment #8771617 - Flags: review?(james) → feedback+
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);;

;;
Assignee

Updated

3 years ago
Attachment #8771617 - Flags: feedback+ → review?(gsquelart)
Attachment #8771618 - Flags: feedback+ → review?(gsquelart)
Attachment #8771617 - Flags: review?(gsquelart) → review+
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 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.
Attachment #8771618 - Flags: review?(gsquelart) → review+

Comment 25

3 years ago
Pushed by jyavenard@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.