Closed
Bug 1274189
Opened 8 years ago
Closed 8 years ago
Remove use of FlushableTaskQueue::Flush() from OpusDataDecoder
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54782/
Attachment #8755744 -
Flags: review?(jyavenard)
Attachment #8755745 -
Flags: review?(jyavenard)
Attachment #8755746 -
Flags: review?(jyavenard)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54784/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54786/
Updated•8 years ago
|
Attachment #8755744 -
Flags: review?(jyavenard) → review+
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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.
Attachment #8755745 -
Flags: review?(jyavenard) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8755746 [details]
MozReview Request: Bug 1274189. Part 3 - remove use of FlushableTaskQueue. r=jya
https://reviewboard.mozilla.org/r/54786/#review51474
Attachment #8755746 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 8•8 years ago
|
||
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?
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
(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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f128105bf879
https://hg.mozilla.org/mozilla-central/rev/d22455a5638f
https://hg.mozilla.org/mozilla-central/rev/33feb325df68
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•