1178938 - Redesign iterative decoding to remove the dependency on the decoder monitor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eb99719ea2a

Looks green.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce a new mechanism for iterative work and prototype it with RawReader. v1

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. :-)

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Copy MediaDecoderReader::DecodeToFirstVideoData into OggReader and label the APIs as explicitly sync. v1

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.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make MediaDecoderReader::DecodeToFirstVideoData async. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Mirror shutdown-ness from the MDSM to the MD. v1

Comment 6

[Jean-Yves Avenard [:jya]]

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<..> ?

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

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

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 10

[Jean-Yves Avenard [:jya]]

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)

Comment 11

[Bobby Holley (:bholley)]

(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.

Comment 12

[Bobby Holley (:bholley)]

(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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a585d0f6ee12
https://hg.mozilla.org/integration/mozilla-inbound/rev/221ca3aab842
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d9a47dfefd
https://hg.mozilla.org/integration/mozilla-inbound/rev/4272c902d349

Comment 14

[Daniel Holbert [:dholbert]]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a71b167f2a

Comment 16

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

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97a71b167f2a