1459255 - Update thread locking model during GraphDriver switching.

Status: RESOLVED WONTFIX

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

Description

[Alex Chronopoulos [:achronop]]

The following methods are used for switching a GrpahDriver in MSG:

GraphDriver::SetNextDriver
GraphDriver::NextDriver
GraphDruver::SetPreviousDriver
GraphDriver::PreviousDriver
GraphDriver::SetGrpahTime

All of them currently assert that the MSG monitor is locked [1]. However, this is not always necessary. They are executed either in audio thread or when the thread is not running. In MSG this is verified by [2]. This bug adds more of those asserts in methods used for switching. In addition, it removes the monitor asserts [1] when a shared variable is accessed exclusively under that condition. Finally, it removes the monitor wherever is possible.

[1] https://searchfox.org/mozilla-central/source/xpcom/threads/BlockingResourceBase.cpp#399
[2] https://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.h#1413

Comment 1

[Jean-Yves Avenard [:jya]]

With our past track record in regards to thread safety in this code, we shouldn't be removing such assertion, even if they appear unnecessary.

Comment 2

[Alex Chronopoulos [:achronop]]

I am planning to use atomic for the shared members. Is that your concern or you are worrying about something else?

Comment 3

[Jean-Yves Avenard [:jya]]

Atomics can't always replace monitors/mutexes

If multiple members are modified under a monitor then you can't use atomics there. I'd like to review those changes thank you. 
Or better, leave it as is while this is all being reworked under the new architecture.

Comment 4

[Alex Chronopoulos [:achronop]]

I am aware of this. You can review it. In any case just adding the asserts will help in readability. Thanks.

Comment 5

[Alex Chronopoulos [:achronop]]

First part, adding the asserts, will be handled in Bug 1460346.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:padenot, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Paul Adenot (:padenot)]

Unnecessary now.