Closed Bug 1259274 Opened 8 years ago Closed 8 years ago

Handle sourcebuffer user actions in its own task queue

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 3 obsolete files)

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
mozbugz
: review+
Details
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
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)
Setting as P1, because currently MSE causes its fair share of crashes
Priority: -- → P1
We now longer require an abstraction layer with the TrackBuffersManager now that the old MSE has been removed.

MozReview-Commit-ID: 3uEejohvFQD
MozReview-Commit-ID: FHj3u1WL1ul
MozReview-Commit-ID: 1U8r82kTR0t
WIP. Uploading here so I can continue working while away.
We now longer require an abstraction layer with the TrackBuffersManager now that the old MSE has been removed.

MozReview-Commit-ID: 3uEejohvFQD
Attachment #8734580 - Attachment is obsolete: true
Attachment #8734581 - Attachment is obsolete: true
Attachment #8734582 - Attachment is obsolete: true
MozReview-Commit-ID: FHj3u1WL1ul
MozReview-Commit-ID: HNIMv45PgRN
MozReview-Commit-ID: 9JR9mZZuP4w
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
Depends on: 1259706
No longer depends on: 1259706
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)
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+
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"
I'm just keeping consistency with the original TaskQueue.h!!!!

Exactly the same text.
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
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 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 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 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 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 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 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/
Depends on: 1262396
You need to log in before you can comment on or make changes to this bug.