If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bad assumption about Command Message Queues in MSG when starting a AudioCallbackDriver

RESOLVED FIXED

Status

()

Core
Audio/Video: MediaStreamGraph
P1
normal
Rank:
15
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Loading this URL in a Debug build usually (on my linux64 inbound builds) crashes with an assertion in MediaStreamGraphImpl::SwapMessageQueues() -- 
    MOZ_ASSERT(mFrontMessageQueue.IsEmpty());

The message in the queue is an AddDirectTrackListener.  The cause is that the graph started as a timer-driven driver, and while running the last (and maybe the first) iteration of the timer driver, it processed the mBackMessageQueue by moving it to mFrontMessageQueue (as it normally does at the end of an iteration).  The mCurrentTaskMessageQueue held a single message as well, the graph startup message; this gets moved to mBackMessageQueue.

Then it swapped to an AudioCallbackDriver.  on the first DataCallback from that it checks if it's the first callback, and if so runs the code after GraphDriver.cpp:917, which calls SwapMessageQueues if there is a message in mBackMessageQueue (which there is).  And we assert.  

I caught it in RR, and traced it down.  The attached patch should fix it.
(Assignee)

Comment 1

9 months ago
Created attachment 8824322 [details] [diff] [review]
Don't swap MSG command queues on startup if there are unprocessed commands still in the front queue
Attachment #8824322 - Flags: review?(padenot)
(Assignee)

Updated

9 months ago
Rank: 15
Dan, this is a fallout from 1228226. I'm not too happy about adding more and more special casing. How about we back out 1228226 and try to find an other solution ?
Flags: needinfo?(dminor)

Comment 3

9 months ago
(In reply to Paul Adenot (:padenot) from comment #2)
> Dan, this is a fallout from 1228226. I'm not too happy about adding more and
> more special casing. How about we back out 1228226 and try to find an other
> solution ?

That is fine by me, although I'm guessing the only alternative to special casing is a rewrite of the switching logic between GraphDrivers to simplify it. That would be nice to have anyway.
Flags: needinfo?(dminor)
(Assignee)

Updated

9 months ago
Blocks: 1228226
Fixed by backing out bug 1228226.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
(In reply to Paul Adenot (:padenot) from comment #2)
> Dan, this is a fallout from 1228226.

I assume the effect of https://hg.mozilla.org/mozilla-central/rev/2553c3b5ba89 here was that it prevented mStateComputedTime from advancing before the SwapMessageQueues() at the end of OneIteration(), and so the code in AudioCallbackDriver::DataCallback() to detect a graph start for the special first SwapMessageQueues() thought that the graph was starting, when in fact it had already started.

https://hg.mozilla.org/mozilla-central/file/2553c3b5ba89/dom/media/GraphDriver.cpp#l883
Yes, that's correct.
Clearing NI.

Updated

4 months ago
Attachment #8824322 - Flags: review?(padenot)
You need to log in before you can comment on or make changes to this bug.