Closed Bug 1270350 Opened 4 years ago Closed 4 years ago

Use SyncRunnable to save some code for MediaDataDecoder subclasses

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

https://hg.mozilla.org/mozilla-central/file/369a5ee3a2880a4a98df3a00bf3db8d8f36b181b/dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp#l123

MonitorAutoLock mon(mMonitor);
mTaskQueue->Dispatch(runnable.forget());
while (mIsFlushing) {
  mon.Wait();
}

This code pattern can be replaced by SyncRunnable::DispatchToThread() and we don't need the |mMonitor| member any more.
Blocks: 1270039
Depends on: 1269963
Attachment #8748991 - Flags: review?(jyavenard) → review+
Comment on attachment 8748991 [details]
MozReview Request: Bug 1270350 - per comment 0, use SyncRunnable to repalce the boilerplate code. r=jya.

https://reviewboard.mozilla.org/r/50675/#review47411

very nice....

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:193
(Diff revision 1)
>  }
>  
>  void
>  WMFMediaDataDecoder::ProcessDrain()
>  {
> -  bool isFlushing;
> +  if (!mIsFlushing && mMFTManager) {

Seeing that Flush() is synchronous, I don't really see the point on why we would need to test if we're flushing in Processdrain. mIsFlushing should always be false there.

Could simply assert there.
https://reviewboard.mozilla.org/r/50675/#review47417

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:193
(Diff revision 1)
>  }
>  
>  void
>  WMFMediaDataDecoder::ProcessDrain()
>  {
> -  bool isFlushing;
> +  if (!mIsFlushing && mMFTManager) {

When Flush() is called on the reader thread, it is possible ProcessDrain() is ongoing on the task queue thread at the same time. There is no guarantee ProcessDrain() should always observe |mIsFlushing == false| on the task queue thread.
(In reply to JW Wang [:jwwang] from comment #4)
> https://reviewboard.mozilla.org/r/50675/#review47417
> 
> ::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:193
> (Diff revision 1)
> >  }
> >  
> >  void
> >  WMFMediaDataDecoder::ProcessDrain()
> >  {
> > -  bool isFlushing;
> > +  if (!mIsFlushing && mMFTManager) {
> 
> When Flush() is called on the reader thread, it is possible ProcessDrain()
> is ongoing on the task queue thread at the same time. There is no guarantee

no this is a scenario that is not possible to encounter.
Flush() is only ever called once a drain has completed (and DrainComplete() has been called).

And even if there was a drain going, all decoded frames output after Flush() have been called are automatically dropped anyway (as MediaFormatReader::mVideo.mDecodingRequested is false) so it doesn't matter if drain dropped the frames rather than outputting them (there's a reason why none of the other decoder check a flush flag when it's draining)
The reason is much deeper than I thought. I will file new bug to describe the rationale for the assertion.
https://hg.mozilla.org/mozilla-central/rev/534314f239e0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Approval Request Comment
[Feature/regressing bug #]:none
[User impact if declined]: Per bug 1274498 comment 4, we want to uplift this bug to beta (48) to see if crash can be reduced. Note this bug must be uplifted after bug 1269963.
[Describe test coverage new/current, TreeHerder]: Tested on Central and TreeHerder.
[Risks and why]: Low. The change has been running on Central for weeks without any regression.
[String/UUID change made/needed]: none
Attachment #8764141 - Flags: review+
Attachment #8764141 - Flags: approval-mozilla-beta?
Comment on attachment 8764141 [details] [diff] [review]
1270350_uplift_beta_48.patch

We want to fix crashes!
taking it in 48 beta 3
Attachment #8764141 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.