Closed Bug 1415556 Opened 7 years ago Closed 7 years ago

Clarify MediaStreamGraph code thread usage

Categories

(Core :: Audio/Video: MediaStreamGraph, enhancement, P3)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 1 obsolete file)

Bug 1415556 - P1. Add precision on thread access with some members.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P2. Make member const.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P3. clearly mark functions' thread use.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P4. Make members atomics.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P5. Use helper to set mDriver.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P6. Ensure mLifecycleState member is always accessed safely.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P7. Remove unecessary locking. .
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P8. Assert on proper access of mStreams member. .
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P9. Remove unused member.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P10. Make functions const where needed.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1415556 - P11. Remove unecessary locking.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Understanding the MediaStreamGraph and how it's used across threads is rather complicated, which isn't helped with the code current structure. Additionally, some members are accessed via different threads, either make their access thread-safe, or document on why they can be used that way.
Interestingly, what prompted me to create this bug, turned out to have been found by :karlt in bug 1408276
See Also: → 1408276
Component: WebRTC: Audio/Video → Audio/Video: MediaStreamGraph
Rank: 25
Priority: -- → P3
Blocks: 1410829
Version: 55 Branch → Trunk
Attachment #8927425 - Attachment is obsolete: true
Attachment #8927425 - Flags: review?(padenot)
Blocks: 1416495
Comment on attachment 8927420 [details] Bug 1415556 - P2. Make member const. https://reviewboard.mozilla.org/r/198722/#review204186
Attachment #8927420 - Flags: review?(padenot) → review+
Comment on attachment 8927419 [details] Bug 1415556 - P1. Add precision on thread access with some members. https://reviewboard.mozilla.org/r/198720/#review204122 ::: dom/media/MediaStreamGraphImpl.h:571 (Diff revision 1) > /** > * Graphs own owning references to their driver, until shutdown. When a driver > * switch occur, previous driver is either deleted, or it's ownership is > * passed to a event that will take care of the asynchronous cleanup, as > * audio stream can take some time to shut down. > + * Only set on the main thread under mMonitor. This comment is incorrect, see [0]. [0]: https://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraphImpl.h#480
Attachment #8927419 - Flags: review?(padenot)
Comment on attachment 8927419 [details] Bug 1415556 - P1. Add precision on thread access with some members. https://reviewboard.mozilla.org/r/198720/#review204438 (changing to r- for consistency)
Attachment #8927419 - Flags: review-
Comment on attachment 8927421 [details] Bug 1415556 - P3. clearly mark functions' thread use. https://reviewboard.mozilla.org/r/198724/#review204188 ::: dom/media/MediaStreamGraph.cpp:69 (Diff revision 2) > */ > static nsDataHashtable<nsUint32HashKey, MediaStreamGraphImpl*> gGraphs; > > MediaStreamGraphImpl::~MediaStreamGraphImpl() > { > - NS_ASSERTION(IsEmpty(), > + NS_ASSERTION(mStreams.IsEmpty() && mSuspendedStreams.IsEmpty(), How about turning this one into a fatal MOZ_ASSERT ? It's quite a strong invariant, if this does not hold, there is clearly an issue somewhere. ::: dom/media/MediaStreamGraph.cpp:180 (Diff revision 2) > void > MediaStreamGraphImpl::ExtractPendingInput(SourceMediaStream* aStream, > GraphTime aDesiredUpToTime, > bool* aEnsureNextIteration) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); It is an error to call this when the graph is not running, so, MOZ_ASSERT(OnGraphThread()). ::: dom/media/MediaStreamGraph.cpp:345 (Diff revision 2) > } > > void > MediaStreamGraphImpl::UpdateCurrentTimeForStreams(GraphTime aPrevCurrentTime) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Ditto. ::: dom/media/MediaStreamGraph.cpp:1152 (Diff revision 2) > +MediaStreamGraph::OnGraphThread() const > +{ > + // we're on the right thread (and calling CurrentDriver() is safe), > + MediaStreamGraphImpl const * graph = > + static_cast<MediaStreamGraphImpl const *>(this); > + // if all the safety checks fail, assert we own the monitor I don't understand the part about "assert we own the monitor", what do you mean? ::: dom/media/MediaStreamGraph.cpp:1244 (Diff revision 2) > > void > MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(uint32_t aStreamIndex, > TrackRate aSampleRate) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same. ::: dom/media/MediaStreamGraph.cpp:1271 (Diff revision 2) > } > > bool > MediaStreamGraphImpl::AllFinishedStreamsNotified() > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same. ::: dom/media/MediaStreamGraph.cpp:1297 (Diff revision 2) > } > > void > MediaStreamGraphImpl::RunMessagesInQueue() > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same. ::: dom/media/MediaStreamGraph.cpp:1314 (Diff revision 2) > } > > void > MediaStreamGraphImpl::UpdateGraph(GraphTime aEndBlockingDecisions) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same. ::: dom/media/MediaStreamGraph.cpp:1375 (Diff revision 2) > } > > void > MediaStreamGraphImpl::Process() > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same. ::: dom/media/MediaStreamGraph.cpp:1457 (Diff revision 2) > } > > bool > MediaStreamGraphImpl::UpdateMainThreadState() > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); Same (this is going to be bitrot by karlt's patches, or the opposite). ::: dom/media/MediaStreamGraph.cpp:1484 (Diff revision 2) > } > > bool > MediaStreamGraphImpl::OneIteration(GraphTime aStateEnd) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); MOZ_ASSERT(OnGraphThread()); ::: dom/media/MediaStreamGraph.cpp:1514 (Diff revision 2) > } > > void > MediaStreamGraphImpl::ApplyStreamUpdate(StreamUpdate* aUpdate) > { > + MOZ_ASSERT(NS_IsMainThread()); Yes. Didn't we decide to stop using NS_*MainThread functions in favor of using mMainThread members and the like? ::: dom/media/MediaStreamGraph.cpp:1890 (Diff revision 2) > } > > void > MediaStreamGraphImpl::EnsureStableStateEventPosted() > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); MOZ_ASSERT(OnGraphThread()); ::: dom/media/MediaStreamGraph.cpp:3918 (Diff revision 2) > } > > void > MediaStreamGraphImpl::IncrementSuspendCount(MediaStream* aStream) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); MOZ_ASSERT(OnGraphThread()); ::: dom/media/MediaStreamGraph.cpp:3931 (Diff revision 2) > } > > void > MediaStreamGraphImpl::DecrementSuspendCount(MediaStream* aStream) > { > + MOZ_ASSERT(OnGraphThreadOrNotRunning()); MOZ_ASSERT(OnGraphThread());
Attachment #8927421 - Flags: review?(padenot) → review-
Comment on attachment 8927422 [details] Bug 1415556 - P4. Make members atomics. https://reviewboard.mozilla.org/r/198726/#review204446 A bit torn about this one. Little changes like this have bitten us in the past, and the code is safe as it is (although I agree you patch makes it way more readable). Let's take this patch, but monitor [0] for regressions. [0]: https://arewefastyet.com/#machine=31&view=single&suite=webaudio
Comment on attachment 8927423 [details] Bug 1415556 - P5. Use helper to set mDriver. https://reviewboard.mozilla.org/r/198728/#review204448
Attachment #8927423 - Flags: review?(padenot) → review+
Comment on attachment 8927424 [details] Bug 1415556 - P6. Ensure mLifecycleState member is always accessed safely. https://reviewboard.mozilla.org/r/198730/#review204450 ::: dom/media/MediaStreamGraph.cpp:1793 (Diff revision 2) > // it to process those messages. Don't do this if we're in a forced > // shutdown or it's a non-realtime graph that has already terminated > // processing. > - if (mLifecycleState == LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP && > + if (LifecycleStateRef() == LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP && > mRealtime && !mForceShutDown) { > - mLifecycleState = LIFECYCLE_RUNNING; > + LifecycleStateRef() = LIFECYCLE_RUNNING; This looks weird for some reason, but clearly having the assert is better.
Attachment #8927424 - Flags: review?(padenot) → review+
Comment on attachment 8927426 [details] Bug 1415556 - P7. Remove unecessary locking. . https://reviewboard.mozilla.org/r/198734/#review204452
Attachment #8927426 - Flags: review?(padenot) → review+
Comment on attachment 8927427 [details] Bug 1415556 - P8. Assert on proper access of mStreams member. . https://reviewboard.mozilla.org/r/198736/#review204454
Attachment #8927427 - Flags: review?(padenot) → review+
Comment on attachment 8927428 [details] Bug 1415556 - P9. Remove unused member. https://reviewboard.mozilla.org/r/198738/#review204456
Attachment #8927428 - Flags: review?(padenot) → review+
Comment on attachment 8927429 [details] Bug 1415556 - P10. Make functions const where needed. https://reviewboard.mozilla.org/r/198740/#review204458
Attachment #8927429 - Flags: review?(padenot) → review+
Comment on attachment 8927556 [details] Bug 1415556 - P11. Remove unecessary locking. https://reviewboard.mozilla.org/r/198856/#review204464 I wonder why this lock was here, I can't see why, even going back to the revision this was introduced.
Attachment #8927556 - Flags: review?(padenot) → review+
Comment on attachment 8927422 [details] Bug 1415556 - P4. Make members atomics. https://reviewboard.mozilla.org/r/198726/#review204466
Attachment #8927422 - Flags: review?(padenot) → review+
Comment on attachment 8927419 [details] Bug 1415556 - P1. Add precision on thread access with some members. https://reviewboard.mozilla.org/r/198720/#review204122 > This comment is incorrect, see [0]. > > [0]: https://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraphImpl.h#480 indeed, it's a mess :) it's access on the main thread without monitor during shutdown. But on both the main thread and the graph thread under monitor. It's use appears to be very dangerous to me. I will add an extra set of change to ensure mDriver is always safely accessed.
Comment on attachment 8927424 [details] Bug 1415556 - P6. Ensure mLifecycleState member is always accessed safely. https://reviewboard.mozilla.org/r/198730/#review204450 > This looks weird for some reason, but clearly having the assert is better. If you have a better name for it... This is similar on how the Maybe<T> is used: RefOr(blah)
Comment on attachment 8927421 [details] Bug 1415556 - P3. clearly mark functions' thread use. https://reviewboard.mozilla.org/r/198724/#review204188 > How about turning this one into a fatal MOZ_ASSERT ? It's quite a strong invariant, if this does not hold, there is clearly an issue somewhere. there's a few use of NS_ASSERTION(OnMainThread) I can change those to MOZ_ASSERT. It makes more sense to me. A software failure to what would otherwise be a data race is a bad idea. > I don't understand the part about "assert we own the monitor", what do you mean? This is the same comment as used in bug 1299172 (I only copied the code from OnGraphThreadOrNotRunning However you're right, there's not even a check for the monitor here (nor is there in OnGraphThreadOrNotRunning()) so it should be removed > Yes. Didn't we decide to stop using NS_*MainThread functions in favor of using mMainThread members and the like? not for assertion, it's retrieving the main thread value that got changed in bug 1364821, where you no longer use main thread's AbstractThread::GetCurrent() despite, there's plenty of existing assertion in this code using NS_IsMainThread() so this would be out of scope.
Comment on attachment 8927419 [details] Bug 1415556 - P1. Add precision on thread access with some members. https://reviewboard.mozilla.org/r/198720/#review204516
Attachment #8927419 - Flags: review?(padenot) → review+
Comment on attachment 8927421 [details] Bug 1415556 - P3. clearly mark functions' thread use. https://reviewboard.mozilla.org/r/198724/#review204518
Attachment #8927421 - Flags: review?(padenot) → review+
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbe88ff91a34 P1. Add precision on thread access with some members. r=padenot https://hg.mozilla.org/integration/autoland/rev/44b20d160436 P2. Make member const. r=padenot https://hg.mozilla.org/integration/autoland/rev/cfc4191ade58 P3. clearly mark functions' thread use. r=padenot https://hg.mozilla.org/integration/autoland/rev/e5e283e7055f P4. Make members atomics. r=padenot https://hg.mozilla.org/integration/autoland/rev/da737c91bb30 P5. Use helper to set mDriver. r=padenot https://hg.mozilla.org/integration/autoland/rev/67a7bd7c1ecf P6. Ensure mLifecycleState member is always accessed safely. r=padenot https://hg.mozilla.org/integration/autoland/rev/329102ab6f3c P7. Remove unecessary locking. r=padenot. https://hg.mozilla.org/integration/autoland/rev/14eb28ed51c3 P8. Assert on proper access of mStreams member. r=padenot. https://hg.mozilla.org/integration/autoland/rev/99281de6fbc7 P9. Remove unused member. r=padenot https://hg.mozilla.org/integration/autoland/rev/9738a5e24b15 P10. Make functions const where needed. r=padenot https://hg.mozilla.org/integration/autoland/rev/6b9dc67f97be P11. Remove unecessary locking. r=padenot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: