Closed Bug 1397941 Opened 7 years ago Closed 7 years ago

Incorrect labeling in DecodedStream.cpp

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
I found this bug during some testing. The problem is that we can have calls like this:

1. DecodedStream::Start()
2. DecodedStream::Stop()
3. DecodedStream::Start()

These happen on some media thread, but they post runnables to the main thread. Start() posts its runnable using SystemGroup. Stop() posts its runnable using mAbstractMainThread. We don't guarantee any ordering between how these are scheduled. So the DestroyData runnable posted in step 2 might actually run after the SystemGroup runnable posted in step 3.

If this happens, then we'll call OutputStreamManager::Connect in step 3 and assert because we're already connected. This is a bit similar to bug 1256520.

Ideally we would use mAbstractMainThread to post the sync runnable in steps 1 and 3. However, we can't do that because we can't use tail dispatch for sync runnables, and mAbstractMainThread uses tail dispatch.

Another option is to use SystemGroup when posting the DestroyData runnable in step 2. This seems to work as far as I can tell. However, I'm a little worried that there might be other problems where this SystemGroup runnable runs before some mAbstractMainThread runnable that was posted earlier.

Since I don't know this code well at all, I took a more conservative path and left the DestroyData runnable unlabeled. This way, it will run in the order it was posted to the main thread, regardless of which queue other runnables are posted to. I checked the ranking of common runnables, and I don't even see it there. So I don't think this will have much effect on performance.

JW, it's up to you whether we should leave the runnable unlabeled, or use SystemGroup to label it. Or we could do something else I haven't thought of.
Attachment #8905703 - Flags: review?(jwwang)
Consider the following flow (with your changes):
1. SetPlaying()
   -> UpdateStreamSuspended()
   -> mAbstractMainThread->Dispatch(r1)
2. Stop()
   DestroyData()
   -> NS_DispatchToMainThread(r2)

Is it possible for r2 to run before r1?
No, r2 will never run before r1. Any unlabeled runnable:
  - will run after all runnables (labeled or unlabeled) that were dispatched before it and
  - will run before all runnables (labeled or unlabeled) that were dispatched after it.

I guess this does mean that using SystemGroup for DestroyData won't work though.
Thanks for the explanation.
Attachment #8905703 - Flags: review?(jwwang) → review+
Component: Audio/Video → Audio/Video: Playback
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f0ff3546ee
Fix DecodedStream labeling bug (r=jwwang)
https://hg.mozilla.org/mozilla-central/rev/f3f0ff3546ee
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.