Closed Bug 1250934 Opened 4 years ago Closed 4 years ago

Resolve driver switching issues with full-duplex audio in MediaStreamGraph

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(9 files)

58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
7.00 KB, patch
padenot
: review+
Details | Diff | Splinter Review
An earlier landing of pref-on for Linux full-duplex showed there were some race conditions on automated tests (that wouldn't repro locally).
Comment on attachment 8723086 [details]
MozReview Request: Bug 1250934: Update some MediaStreamGraph logging r?padenot

https://reviewboard.mozilla.org/r/36339/#review32957
Attachment #8723086 - Flags: review?(padenot) → review+
Comment on attachment 8723085 [details]
MozReview Request: Bug 1250934: Block GraphDriver switching if audio input is enabled (full-duplex) r?padenot

https://reviewboard.mozilla.org/r/36337/#review32955

Let's say we can land this, but I'd really like to know why we aren't getting and audio track here.

::: dom/media/MediaStreamGraph.cpp:375
(Diff revision 1)
> +  // should not allow a switch back to a SystemClockDriver

Do we know what creates the audio track when calling gUM? Maybe the bug is there and not here ?
Attachment #8723085 - Flags: review?(padenot) → review+
Comment on attachment 8723084 [details]
MozReview Request: Bug 1250934: Don't reopen input AudioCallbackDrivers on a second use r?padenot

https://reviewboard.mozilla.org/r/36335/#review32953

r+, but see comment.

::: dom/media/MediaStreamGraph.cpp:1000
(Diff revision 1)
> +      NS_ASSERTION(false, "Can't open cubeb inputs in shutdown");

What about MSG revival ?
Attachment #8723084 - Flags: review?(padenot) → review+
Attachment #8723083 - Flags: review?(padenot) → review+
Comment on attachment 8723083 [details]
MozReview Request: Bug 1250934: Factor out AudioTracksPresent() in MediaStreamGraph, and add checks of 'pending' tracks r?padenot

https://reviewboard.mozilla.org/r/36333/#review32951

::: dom/media/MediaStreamGraphImpl.h:337
(Diff revision 1)
> +   * Determine if we have any audio tracks, or are about to add any audiotracks.

s/audiotracks/audio tracks/
Attachment #8723082 - Flags: review?(padenot) → review+
Comment on attachment 8723082 [details]
MozReview Request: Bug 1250934: Don't allow switching to a clock driver when we already have a switch pending r?padenot

https://reviewboard.mozilla.org/r/36331/#review32959
Comment on attachment 8723087 [details]
MozReview Request: Bug 1250934: Make tests use the correct audio frequency when fake devices are used r?drno

https://reviewboard.mozilla.org/r/36341/#review32961

LGTM
Attachment #8723087 - Flags: review?(drno) → review+
Comment on attachment 8723408 [details]
MozReview Request: Bug 1250934: Only dig out cubeb deviceIDs immediately before use since they can be freed r?padenot

https://reviewboard.mozilla.org/r/36541/#review33161
Attachment #8723408 - Flags: review?(padenot) → review+
Comment on attachment 8723082 [details]
MozReview Request: Bug 1250934: Don't allow switching to a clock driver when we already have a switch pending r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36331/diff/1-2/
Comment on attachment 8723083 [details]
MozReview Request: Bug 1250934: Factor out AudioTracksPresent() in MediaStreamGraph, and add checks of 'pending' tracks r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36333/diff/1-2/
Comment on attachment 8723084 [details]
MozReview Request: Bug 1250934: Don't reopen input AudioCallbackDrivers on a second use r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36335/diff/1-2/
Comment on attachment 8723085 [details]
MozReview Request: Bug 1250934: Block GraphDriver switching if audio input is enabled (full-duplex) r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36337/diff/1-2/
Comment on attachment 8723086 [details]
MozReview Request: Bug 1250934: Update some MediaStreamGraph logging r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36339/diff/1-2/
Comment on attachment 8723087 [details]
MozReview Request: Bug 1250934: Make tests use the correct audio frequency when fake devices are used r?drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36341/diff/1-2/
Comment on attachment 8723408 [details]
MozReview Request: Bug 1250934: Only dig out cubeb deviceIDs immediately before use since they can be freed r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36541/diff/1-2/
Comment on attachment 8723407 [details]
MozReview Request: Bug 1250934: Split WebRTC MediaEngine shutdown into two stages to allow async shutdown r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36539/diff/1-2/
Attachment #8723407 - Attachment description: MozReview Request: imported patch pref_on_full_duplex → MozReview Request: Bug 1250934: Split WebRTC MediaEngine shutdown into two stages to allow async shutdown r?padenot
Attachment #8723407 - Flags: review?(padenot)
Comment on attachment 8723407 [details]
MozReview Request: Bug 1250934: Split WebRTC MediaEngine shutdown into two stages to allow async shutdown r?padenot

Hit this debugging linux opt M2 failures when running locally - a race where Shutdown() happened before the final AudioCallbackDriver NotifyInput was done, since StopCapture() now is effectively async.  (caused null-deref)

This is not the apparent cause of the M2 failures, though :-(
Comment on attachment 8723407 [details]
MozReview Request: Bug 1250934: Split WebRTC MediaEngine shutdown into two stages to allow async shutdown r?padenot

https://reviewboard.mozilla.org/r/36539/#review33449

::: dom/media/webrtc/MediaEngine.h:92
(Diff revision 2)
> +  virtual void ReleaseInterfaces() = 0;

A couple comments on those couldn't hurt, this is getting hairy.
Attachment #8723407 - Flags: review?(padenot) → review+
Depends on: 1253333
This replaces the 'split-shutdown' version with a lock.  Now that I've fixed some other issues, I'll figure out if we can go with the first or with this one.  (This one should be very safe).  This could be done with Weakptrs as well I think, if weakptrs are threadsafe.
Attachment #8726290 - Flags: review?(padenot)
Attachment #8726290 - Flags: review?(padenot) → review+
Depends on: 1255217
Depends on: 1256555
Depends on: 1263251
You need to log in before you can comment on or make changes to this bug.