1274189 - Remove use of FlushableTaskQueue::Flush() from OpusDataDecoder

Status: RESOLVED FIXED

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

Comment 1

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

review: https://reviewboard.mozilla.org/r/53876
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b9cd9e378c3

Comment 2

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

Attachment: MozReview Request: Bug 1274189. Part 1 - rename some functions to be consistent with other MediaDataDecoder sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/54782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54782/

Comment 3

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

Attachment: MozReview Request: Bug 1274189. Part 2 - remove use of FlushableTaskQueue::Flush(). r=jya.

Review commit: https://reviewboard.mozilla.org/r/54784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54784/

Comment 4

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

Attachment: MozReview Request: Bug 1274189. Part 3 - remove use of FlushableTaskQueue. r=jya

Review commit: https://reviewboard.mozilla.org/r/54786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54786/

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755744 [details]
MozReview Request: Bug 1274189. Part 1 - rename some functions to be consistent with other MediaDataDecoder sub-classes. r=jya.

https://reviewboard.mozilla.org/r/54782/#review51470

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755745 [details]
MozReview Request: Bug 1274189. Part 2 - remove use of FlushableTaskQueue::Flush(). r=jya.

https://reviewboard.mozilla.org/r/54784/#review51472

::: dom/media/platforms/agnostic/OpusDecoder.cpp:323
(Diff revision 1)
>  nsresult
>  OpusDataDecoder::Flush()
>  {
> -  mTaskQueue->Flush();
> -  if (mOpusDecoder) {
> +  mIsFlushing = true;
> +  nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableFunction([this] () {
> +    MOZ_ASSERT(mOpusDecoder);

do not assert. While it's very unlikely, it is possible for mOpusDecoder to be false if initialisation failed.

Flush() is called when the MediaFormatReader shutdown, in which case you would hit the assertion whe initialisation failed.

I guess you could simply return Flush() early if mOpusDecoder is null.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755746 [details]
MozReview Request: Bug 1274189. Part 3 - remove use of FlushableTaskQueue. r=jya

https://reviewboard.mozilla.org/r/54786/#review51474

Comment 8

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

https://reviewboard.mozilla.org/r/54784/#review51472

> do not assert. While it's very unlikely, it is possible for mOpusDecoder to be false if initialisation failed.
> 
> Flush() is called when the MediaFormatReader shutdown, in which case you would hit the assertion whe initialisation failed.
> 
> I guess you could simply return Flush() early if mOpusDecoder is null.

I will remove the assertion. It is usually a bad idea to continue to use an abject after Init() fails. However, since Init() is resolved (or rejected) asynchronously, chances are we call Shutdown() before the promise is resolved, right?

Comment 9

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

https://reviewboard.mozilla.org/r/54784/#review51472

> I will remove the assertion. It is usually a bad idea to continue to use an abject after Init() fails. However, since Init() is resolved (or rejected) asynchronously, chances are we call Shutdown() before the promise is resolved, right?

Oops! I mean to return realy when mOpusDecoder is null.

Comment 10

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

Comment on attachment 8755745 [details]
MozReview Request: Bug 1274189. Part 2 - remove use of FlushableTaskQueue::Flush(). r=jya.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54784/diff/1-2/

Comment 11

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

Comment on attachment 8755746 [details]
MozReview Request: Bug 1274189. Part 3 - remove use of FlushableTaskQueue. r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54786/diff/1-2/

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #8)
> https://reviewboard.mozilla.org/r/54784/#review51472
> 
> > do not assert. While it's very unlikely, it is possible for mOpusDecoder to be false if initialisation failed.
> > 
> > Flush() is called when the MediaFormatReader shutdown, in which case you would hit the assertion whe initialisation failed.
> > 
> > I guess you could simply return Flush() early if mOpusDecoder is null.
> 
> I will remove the assertion. It is usually a bad idea to continue to use an
> abject after Init() fails. However, since Init() is resolved (or rejected)
> asynchronously, chances are we call Shutdown() before the promise is
> resolved, right?

actually, upon checking; the decoder is no longer being flushed if init failed...

so it should be fine with the assert. still doing nothing looks better to me

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f128105bf879
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22455a5638f
https://hg.mozilla.org/integration/mozilla-inbound/rev/33feb325df68

Comment 14

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

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> actually, upon checking; the decoder is no longer being flushed if init
> failed...
> 
> so it should be fine with the assert. still doing nothing looks better to me

I see. Thanks for the update and review!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f128105bf879
https://hg.mozilla.org/mozilla-central/rev/d22455a5638f
https://hg.mozilla.org/mozilla-central/rev/33feb325df68