Closed Bug 1178938 Opened 5 years ago Closed 5 years ago

Redesign iterative decoding to remove the dependency on the decoder monitor

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files)

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: nobody → bobbyholley
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)
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)
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 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+
Attachment #8628398 - Flags: review?(jwwang) → review+
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 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+
Attachment #8628401 - Flags: review?(jwwang) → review+
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)
(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.
(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.
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.
You need to log in before you can comment on or make changes to this bug.