Handle sourcebuffer user actions in its own task queue

RESOLVED FIXED in Firefox 48

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(11 attachments, 3 obsolete attachments)

18.57 KB, patch
Details | Diff | Splinter Review
9.37 KB, patch
Details | Diff | Splinter Review
64.04 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
Details | Diff | Splinter Review
4.13 KB, patch
Details | Diff | Splinter Review
5.20 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
(Assignee)

Description

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

Comment 1

3 years ago
Setting as P1, because currently MSE causes its fair share of crashes
Priority: -- → P1
(Assignee)

Comment 2

3 years ago
Created attachment 8734580 [details] [diff] [review]
[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
(Assignee)

Comment 3

3 years ago
Created attachment 8734581 [details] [diff] [review]
[MSE] P2. Remove unused code path. r?gerald

MozReview-Commit-ID: FHj3u1WL1ul
(Assignee)

Comment 4

3 years ago
Created attachment 8734582 [details] [diff] [review]
[MSE] P3. Refactor handling of tasks so they only ever run concurrently. r?gerald

MozReview-Commit-ID: 1U8r82kTR0t
(Assignee)

Comment 5

3 years ago
WIP. Uploading here so I can continue working while away.
(Assignee)

Comment 6

3 years ago
Created attachment 8735036 [details] [diff] [review]
[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
(Assignee)

Updated

3 years ago
Attachment #8734580 - Attachment is obsolete: true
Attachment #8734581 - Attachment is obsolete: true
Attachment #8734582 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8735037 [details] [diff] [review]
[MSE] P2. Remove unused code path. r?gerald

MozReview-Commit-ID: FHj3u1WL1ul
(Assignee)

Comment 8

3 years ago
Created attachment 8735038 [details] [diff] [review]
[MSE] P3. Refactor handling of tasks so they only ever run concurrently. r?gerald

MozReview-Commit-ID: 1U8r82kTR0t
(Assignee)

Comment 9

3 years ago
Created attachment 8735039 [details] [diff] [review]
P4. Make AbstractThread AddRef/Release methods virtual. r?cpearce

MozReview-Commit-ID: HNIMv45PgRN
(Assignee)

Comment 10

3 years ago
Created attachment 8735040 [details] [diff] [review]
P5. Add AutoTaskQueue convenience class. r?cpearce

MozReview-Commit-ID: 9JR9mZZuP4w
(Assignee)

Comment 11

3 years ago
Created attachment 8735041 [details] [diff] [review]
[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
(Assignee)

Updated

3 years ago
Depends on: 1259706
(Assignee)

Updated

3 years ago
No longer depends on: 1259706
(Assignee)

Comment 12

3 years ago
Created attachment 8735068 [details]
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/
Attachment #8735068 - Flags: review?(gsquelart)
Attachment #8735069 - Flags: review?(gsquelart)
Attachment #8735070 - Flags: review?(gsquelart)
Attachment #8735071 - Flags: review?(cpearce)
Attachment #8735072 - Flags: review?(gsquelart)
(Assignee)

Comment 13

3 years ago
Created attachment 8735069 [details]
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/
(Assignee)

Comment 14

3 years ago
Created attachment 8735070 [details]
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/
(Assignee)

Comment 15

3 years ago
Created attachment 8735071 [details]
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/
(Assignee)

Comment 16

3 years ago
Created attachment 8735072 [details]
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 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.
Attachment #8735068 - Flags: review?(gsquelart) → review+
Comment on attachment 8735069 [details]
MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r=gerald

https://reviewboard.mozilla.org/r/42573/#review39113
Attachment #8735069 - Flags: review?(gsquelart) → review+
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.
Attachment #8735070 - Flags: review?(gsquelart) → review+
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.
Attachment #8735072 - Flags: review?(gsquelart) → review+
Attachment #8735071 - Flags: review+
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"
Attachment #8735071 - Flags: review?(cpearce)
(Assignee)

Comment 23

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

Exactly the same text.
(Assignee)

Comment 24

3 years ago
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/
Attachment #8735068 - Attachment description: MozReview Request: Bug 1259274: [MSE] P1. Remove unnecessary abstraction layer. r?gerald → MozReview Request: Bug 1259274: [MSE] P1. Remove unnecessary abstraction layer. r=gerald
Attachment #8735069 - Attachment description: MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r?gerald → MozReview Request: Bug 1259274: [MSE] P2. Remove unused code path. r=gerald
Attachment #8735070 - Attachment description: MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r?gerald → MozReview Request: Bug 1259274: [MSE] P3. Refactor handling of tasks so they only ever run concurrently. r=gerald
Attachment #8735071 - Attachment description: MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r?cpearce → MozReview Request: Bug 1259274: [MSE] P4. Add AutoTaskQueue convenience class. r=gerald
Attachment #8735072 - Attachment description: MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r?gerald → MozReview Request: Bug 1259274: [MSE] P5. Use new AutoTaskQueue with MSE objects. r=gerald
(Assignee)

Comment 25

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

Comment 26

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

Comment 27

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

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

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

Updated

3 years ago
Depends on: 1262396
You need to log in before you can comment on or make changes to this bug.