Closed
Bug 1178938
Opened 9 years ago
Closed 9 years ago
Redesign iterative decoding to remove the dependency on the decoder monitor
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(4 files)
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
A common pattern in the legacy decoders is to decode frames in a |while| loop, and, after every frame, do a cross-thread read to determine whether we've shut down.
This is a big mess for the the new threading model. I have some patches to make this kind of iterative decoding asynchronous, which solves most of the use cases. The OGG decoder looks like a particular PITA to make asynchronous, so I have a hacky, narrowly-scoped monitor that should probably be good enough for this situation.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bobbyholley
Assignee | ||
Comment 2•9 years ago
|
||
The current model can hog the task queue indefinitely, and relies on synchronously
reading cross-thread state in order to detect shutdown conditions. In order to stop
doing that, we need a different model. Thankfully, MediaPromises/closures make this
a lot more concise than it could be.
jya: A new tool for your toolbox. :-)
Attachment #8628397 -
Flags: review?(jwwang)
Attachment #8628397 -
Flags: feedback?(jyavenard)
Assignee | ||
Comment 3•9 years ago
|
||
This is in preparation for making MDR::DecodeToFirstVideoData async. The ogg
seeking code is totally insane, so we'll do something special and hacky to solve
this problem there.
Attachment #8628398 -
Flags: review?(jwwang)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8628399 -
Flags: review?(jwwang)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8628401 -
Flags: review?(jwwang)
Comment 6•9 years ago
|
||
Comment on attachment 8628397 [details] [diff] [review]
Part 1 - Introduce a new mechanism for iterative work and prototype it with RawReader. v1
Review of attachment 8628397 [details] [diff] [review]:
-----------------------------------------------------------------
Could we use this to simulate synchronicity on the main thread (thinking of a particular use in MSE)?
How would that work with threads where sync dispatch may be used? as the task will not be run until the current task completes.
::: dom/media/raw/RawReader.cpp
@@ +272,2 @@
>
> + return p.forget();
why return an already_Addrefed when the return type is nsRefPtr<..> ?
Attachment #8628397 -
Flags: feedback?(jyavenard)
Comment 7•9 years ago
|
||
Comment on attachment 8628397 [details] [diff] [review]
Part 1 - Introduce a new mechanism for iterative work and prototype it with RawReader. v1
Review of attachment 8628397 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/VideoUtils.h
@@ +285,5 @@
> +
> + // Iteratively invokes aWork until aCondition returns true, or aWork returns false.
> + // Use this rather than a while loop to avoid bogarting the task queue.
> + template<class Work, class Condition>
> + nsRefPtr<GenericPromise> InvokeUntil(Work aWork, Condition aCondition)
Indentation should be consistent with that of class AutoSetOnScopeExit.
::: dom/media/raw/RawReader.cpp
@@ +252,5 @@
> + nsRefPtr<SeekPromise::Private> p = new SeekPromise::Private(__func__);
> + nsRefPtr<RawReader> self = this;
> + InvokeUntil([self] () {
> + MOZ_ASSERT(self->OnTaskQueue());
> + NS_ENSURE_TRUE(!self->mShutdown, false);
Where is mShutdown defined? Do you mean self->mDecoder->IsShutdown()?
@@ +266,5 @@
> }
> + p->Resolve(aTime, __func__);
> + }, [self, p, frame] {
> + self->mCurrentFrame = frame;
> + p->Reject(NS_ERROR_FAILURE, __func__);
Don't we need to clear the queue when it fails?
@@ +272,2 @@
>
> + return p.forget();
already_Addrefed is implicitly convertible to nsRefPtr.
Attachment #8628397 -
Flags: review?(jwwang) → review+
Updated•9 years ago
|
Attachment #8628398 -
Flags: review?(jwwang) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8628397 [details] [diff] [review]
Part 1 - Introduce a new mechanism for iterative work and prototype it with RawReader. v1
Review of attachment 8628397 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/raw/RawReader.cpp
@@ +250,5 @@
>
> mVideoQueue.Reset();
> + nsRefPtr<SeekPromise::Private> p = new SeekPromise::Private(__func__);
> + nsRefPtr<RawReader> self = this;
> + InvokeUntil([self] () {
Will compiler infer the return type of the closure automatically?
Comment 9•9 years ago
|
||
Comment on attachment 8628399 [details] [diff] [review]
Part 3 - Make MediaDecoderReader::DecodeToFirstVideoData async. v1
Review of attachment 8628399 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaDecoderReader.cpp
@@ +170,5 @@
> + })->Then(TaskQueue(), __func__, [self, p] () {
> + p->Resolve(self->VideoQueue().PeekFront(), __func__);
> + }, [p] () {
> + // We don't have a way to differentiate EOS, error, and shutdown here. :-(
> + p->Reject(END_OF_STREAM, __func__);
We should be able to generalize InvokeUntil() to allow Work to return an error code instead of a bool. Then p can reject with the error code passed from Work.
Attachment #8628399 -
Flags: review?(jwwang) → review+
Updated•9 years ago
|
Attachment #8628401 -
Flags: review?(jwwang) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8628397 [details] [diff] [review]
Part 1 - Introduce a new mechanism for iterative work and prototype it with RawReader. v1
Review of attachment 8628397 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/raw/RawReader.cpp
@@ +272,2 @@
>
> + return p.forget();
sure, but there's no point returning an already_addRefed when we already have a nsRefPtr, only to convert it back to nsRefPtr.
return p; is just as good (and less confusing)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Could we use this to simulate synchronicity on the main thread (thinking of
> a particular use in MSE)?
I'm not quite sure what you mean here.
> How would that work with threads where sync dispatch may be used? as the
> task will not be run until the current task completes.
Like almost anything promise-based, this is incompatible with sync dispatch.
> why return an already_Addrefed when the return type is nsRefPtr<..> ?
Because nsRefPtr<Derived> is not implicitly convertible to nsRefPtr<Base>, but already_AddRefed<Derived> is.
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #7)
> Indentation should be consistent with that of class AutoSetOnScopeExit.
Fixed, though the existing style doesn't match Gecko style.
> Where is mShutdown defined?
In MediaDecoderReader.h.
> Do you mean self->mDecoder->IsShutdown()?
Definitely not - that's the cross-thread read this bug is trying to eliminate.
> Don't we need to clear the queue when it fails?
I guess? The existing code doesn't do that, but it seems sensible - I've added it, hoping it doesn't blow up.
> Will compiler infer the return type of the closure automatically?
Yes, according to Ehsan and Try.
> We should be able to generalize InvokeUntil() to allow Work to return an error code
> instead of a bool. Then p can reject with the error code passed from Work.
Do you mean an nsresult? The problem is that the error code we need here is a NotDecodedReason, which doesn't have a non-error value. We could templatize on error code type T and have Work return Maybe<T>, which would do the trick I think. That's kind of involved though, so I'd rather leave it for a followup.
Comment 13•9 years ago
|
||
Comment 14•9 years ago
|
||
The last commit here incidentally caused some "-Winconsistent-missing-override" build warnings, because it added some correctly-annotated 'override' function declarations alongside some not-yet-annotated methods. (which made the class "inconsistent", hence "-Winconsistent-missing-override")
I'm pushing a followup (posted here via pulsebot shortly) to add that keyword to the not-yet-annotated methods, to fix this issue, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a585d0f6ee12
https://hg.mozilla.org/mozilla-central/rev/221ca3aab842
https://hg.mozilla.org/mozilla-central/rev/44d9a47dfefd
https://hg.mozilla.org/mozilla-central/rev/4272c902d349
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 17•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•