1259274 - Handle sourcebuffer user actions in its own task queue

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

In bug 1258410, we have some very weird stuff happening. We've papered around the issue.

One of the core problem with MSE is that it requires most functions to be asyncronous ; yet it has some synchronous parts (such as sourcebuffer.abort).

Action that can be aborted are:
- appendBuffer
- range removal

We currently queue any new action to the TrackBuffersManager task queue sequentially.

I suggest that instead we have a new specialised task queue that keeps for each task the relevant JS context required.

When abort is called, we could then easily skip on the task we don't care running or run them while presenting a JS context as if the new task had already completed (even though it didn't yet).

So will avoid any races on how tasks are being queued by the sourcebuffer or the trackbuffersmanager each keeping their own queue and the trackbuffersmanager processing the sourcebuffer tasks sequentially and only once it has done all it had to do (no task can then be inserted in the middle messing the order of things)

Comment 1

[Jean-Yves Avenard [:jya]]

Setting as P1, because currently MSE causes its fair share of crashes

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P1. Remove unnecessary abstraction layer. r?gerald

We now longer require an abstraction layer with the TrackBuffersManager now that the old MSE has been removed.

MozReview-Commit-ID: 3uEejohvFQD

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P2. Remove unused code path. r?gerald

MozReview-Commit-ID: FHj3u1WL1ul

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r?gerald

MozReview-Commit-ID: 1U8r82kTR0t

Comment 5

[Jean-Yves Avenard [:jya]]

WIP. Uploading here so I can continue working while away.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P1. Remove unnecessary abstraction layer. r?gerald

We now longer require an abstraction layer with the TrackBuffersManager now that the old MSE has been removed.

MozReview-Commit-ID: 3uEejohvFQD

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P2. Remove unused code path. r?gerald

MozReview-Commit-ID: FHj3u1WL1ul

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r?gerald

MozReview-Commit-ID: 1U8r82kTR0t

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P4. Make AbstractThread AddRef/Release methods virtual. r?cpearce

MozReview-Commit-ID: HNIMv45PgRN

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: P5. Add AutoTaskQueue convenience class. r?cpearce

MozReview-Commit-ID: 9JR9mZZuP4w

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P6. Use new AutoTaskQueue with MSE objects. r?gerald

It was possible for a TrackBuffersManager to have pending task currently running while the MediaSourceDemuxer was shutting down the task queue. This would cause an assertion upon resolution of the promise attempting to schedule a new runnable while the task queue is shutdown.

MozReview-Commit-ID: IzPh2OdGbvN

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259274: [MSE] P1. Remove unnecessary abstraction layer. r=gerald

We now longer require an abstraction layer with the TrackBuffersManager now that the old MSE has been removed.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r=gerald

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

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r=gerald

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

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r=gerald

Just like TaskQueue, but doesn't require to be shutdown.

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

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r=gerald

It was possible for a TrackBuffersManager to have pending tasks currently running while the MediaSourceDemuxer was shutting down the task queue. This would cause an assertion upon resolution of the promise attempting to schedule a new runnable as the task queue was now shutdown.

The AutoTaskQueue only shuts down once it's no longer used.

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

Comment 17

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c749de522f19 for media mochitest

and
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b068a4adc69 for web-platform-tests

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735068 [details]
MozReview Request: Bug 1259274: [MSE] P1. Remove unnecessary abstraction layer. r=gerald

https://reviewboard.mozilla.org/r/42571/#review39111

r+ with mini-nits:

::: dom/media/mediasource/SourceBuffer.h:255
(Diff revision 1)
>    void AppendDataCompletedWithSuccess(bool aHasActiveTracks);
>    void AppendDataErrored(nsresult aError);
>  
>    RefPtr<MediaSource> mMediaSource;
>  
> -  RefPtr<SourceBufferContentManager> mContentManager;
> +  RefPtr<TrackBuffersManager> mContentManager;

Now that SourceBufferContentManager is no more, is 'mContentManager' still an appropriate and meaningful name? (No need to change if it's fine, just checking...)

::: dom/media/mediasource/TrackBuffersManager.h:70
(Diff revision 1)
>    bool AppendData(MediaByteBuffer* aData,
> -                  media::TimeUnit aTimestampOffset) override;
> +                  media::TimeUnit aTimestampOffset);
>  
> -  RefPtr<AppendPromise> BufferAppend() override;
> +  RefPtr<AppendPromise> BufferAppend();
>  
> -  void AbortAppendData() override;
> +  void AbortAppendData();
>  
> -  void ResetParserState() override;
> +  void ResetParserState();
>  
>    RefPtr<RangeRemovalPromise> RangeRemoval(media::TimeUnit aStart,
> -                                             media::TimeUnit aEnd) override;
> +                                             media::TimeUnit aEnd);
>  
>    EvictDataResult
>    EvictData(media::TimeUnit aPlaybackTime,
>              int64_t aThresholdReduct,
> -            media::TimeUnit* aBufferStartTime) override;
> +            media::TimeUnit* aBufferStartTime);
>  
> -  void EvictBefore(media::TimeUnit aTime) override;
> +  void EvictBefore(media::TimeUnit aTime);
>  
> -  media::TimeIntervals Buffered() override;
> +  media::TimeIntervals Buffered();
>  
> -  int64_t GetSize() override;
> +  int64_t GetSize();
>  
> -  void Ended() override;
> +  void Ended();
>  
> -  void Detach() override;
> +  void Detach();

Please import the comments from SourceBufferContentManager.h, for all the methods you used to override.

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735069 [details]
MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r=gerald

https://reviewboard.mozilla.org/r/42573/#review39113

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735070 [details]
MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r=gerald

https://reviewboard.mozilla.org/r/42575/#review39117

r+ once the 2 issues are addressed; other nits as you please.

::: dom/media/mediasource/SourceBufferAttributes.h:37
(Diff revision 1)
> +  SourceBufferAttributes(const SourceBufferAttributes& aOther)
> +  {
> +    *this = aOther;
> +  }

Wouldn't '= default' work here? (Especially since operator= is '= default' as well)

::: dom/media/mediasource/SourceBufferTask.h:13
(Diff revision 1)
> +#define MOZILLA_SOURCEBUFFERTASK_H_
> +
> +#include "mozilla/MozPromise.h"
> +#include "mozilla/Pair.h"
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/UniquePtr.h"

No need for UniquePtr.h in this file.

::: dom/media/mediasource/SourceBufferTask.h:65
(Diff revision 1)
> +};
> +
> +class AbortTask : public SourceBufferTask {
> +public:
> +  static const Type sType = Type::Abort;
> +  Type GetType() const { return Type::Abort; }

Missing 'override'.

::: dom/media/mediasource/TrackBuffersManager.h:50
(Diff revision 1)
> +  {
> +    MonitorAutoLock mon(mMonitor);
> +    if (!mQueue.Length()) {
> +      return nullptr;
> +    }
> +    RefPtr<SourceBufferTask> task = mQueue[0];

You could do Move(mQueue[0]) to remove a pair of AddRef/Release.
Or use the null-RefPtr-swap idiom.

::: dom/media/mediasource/TrackBuffersManager.h:87
(Diff revision 1)
> -  bool AppendData(MediaByteBuffer* aData,
> -                  media::TimeUnit aTimestampOffset);
> +  RefPtr<AppendPromise> AppendData(MediaByteBuffer* aData,
> +                                   SourceBufferAttributes aAttributes);

I think you could pass aAttributes by const-reference to AppendData.
(But not to RunAppendData below, because of the InvokeAsyncCallback.)

::: dom/media/mediasource/TrackBuffersManager.h:89
(Diff revision 1)
> -
> -  RefPtr<AppendPromise> BufferAppend();
> +  RefPtr<AppendPromise>
> +  RunAppendData(RefPtr<MediaByteBuffer> aData,
> +                SourceBufferAttributes aAttributes);

RunAppendData should be private.

::: dom/media/mediasource/TrackBuffersManager.h:359
(Diff revision 1)
>  
> +  // SourceBuffer Queues and running context.
> +  SourceBufferTaskQueue mQueue;
> +  void ProcessTasks();
> +  void CancelAllTasks();
> +  // Set if the TrackBufferesManager is currently processing a task.

'TrackBufferesManager' -> 'TrackBuffersManager' (remove spurious 'e').

::: dom/media/mediasource/TrackBuffersManager.h:366
(Diff revision 1)
> +  RefPtr<SourceBufferTask> mCurrentTask;
> +  // Current SourceBuffer state for ongoing task.
> +  // Its content is returned to the SourceBuffer once the AppendBufferTask has
> +  // completed.
> +  UniquePtr<SourceBufferAttributes> mSourceBufferAttributes;
> +  // The current sourcebuffer append window. It's content is equivalent to

"It's" -> "Its".

::: dom/media/mediasource/TrackBuffersManager.h:366
(Diff revision 1)
> +  // The current sourcebuffer append window. It's content is equivalent to
> +  // mSourceBufferAttributes.mAppendWindowStart/End
>    media::TimeInterval mAppendWindow;

If mAppendWindow is always equivalent to mSourceBufferAttributes.mAppendWindowStart/End, couldn't we just get rid of it, and use the latter in the one spot where it's used?
But if there is a good reason for keeping it, please add that reason to the comment.

::: dom/media/mediasource/TrackBuffersManager.cpp:173
(Diff revision 1)
> +        } else if (!mInputBuffer->AppendElements(*task->As<AppendBufferTask>()->mBuffer, fallible)) {
> +          RejectAppend(NS_ERROR_OUT_OF_MEMORY, __func__);
> +        }

RejectAppend clears a few things and comes back to ProcessTasks. So I believe you should "return;" from here in this case, instead of continuing with this task.

::: dom/media/mediasource/TrackBuffersManager.cpp:242
(Diff revision 1)
> +  }
>  }
>  
>  // The MSE spec requires that we abort the current SegmentParserLoop
>  // which is then followed by a call to ResetParserState.
>  // However due to our asynchronous design this causes inherent difficulities.

"difficulities" -> "difficulties"

::: dom/media/mediasource/TrackBuffersManager.cpp:243
(Diff revision 1)
>  // As the spec behaviour is non deterministic anyway, we instead wait until the
>  // current AppendData has completed its run.
>  void
>  TrackBuffersManager::AbortAppendData()

No more waiting here, the comment should be updated.

::: dom/media/mediasource/TrackBuffersManager.cpp:758
(Diff revision 1)
> -    mon.NotifyAll();
>    }
> -  mAppendPromise.ResolveIfExists(mActiveTrack, __func__);
> +  MOZ_DIAGNOSTIC_ASSERT(mCurrentTask && mCurrentTask->GetType() == SourceBufferTask::Type::AppendBuffer);
> +  MOZ_DIAGNOSTIC_ASSERT(mSourceBufferAttributes);
> +
> +  mCurrentTask->As<AppendBufferTask>()->mPromise.Resolve(SourceBufferTask::AppendBufferResult(mActiveTrack, *mSourceBufferAttributes), __func__);

Please break long line.

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735072 [details]
MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r=gerald

https://reviewboard.mozilla.org/r/42579/#review39119

r+ if previous patch is accepted.

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8735071 [details]
MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r=gerald

https://reviewboard.mozilla.org/r/42577/#review39121

(jya asked me to review, instead of cpearce)

::: dom/media/mediasource/AutoTaskQueue.h:36
(Diff revision 1)
> +                DispatchReason aReason = NormalDispatch) override
> +  {
> +    mTaskQueue->Dispatch(Move(aRunnable), aFailureHandling, aReason);
> +  }
> +
> +  // Blocks until all task finish executing.

"task" -> "tasks"

Comment 23

[Jean-Yves Avenard [:jya]]

I'm just keeping consistency with the original TaskQueue.h!!!!

Exactly the same text.

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735068 [details]
MozReview Request: Bug 1259274: [MSE] P1. Remove unnecessary abstraction layer. r=gerald

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

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735069 [details]
MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r=gerald

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

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735070 [details]
MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r=gerald

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

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735071 [details]
MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r=gerald

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

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735072 [details]
MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r=gerald

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735070 [details]
MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r=gerald

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

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735071 [details]
MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r=gerald

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

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8735072 [details]
MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r=gerald

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

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d26a81fecde9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8498ca83977b
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c24e7caa30
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a3df878ca6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0996ba84787e

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d26a81fecde9
https://hg.mozilla.org/mozilla-central/rev/8498ca83977b
https://hg.mozilla.org/mozilla-central/rev/99c24e7caa30
https://hg.mozilla.org/mozilla-central/rev/75a3df878ca6
https://hg.mozilla.org/mozilla-central/rev/0996ba84787e