1507607 - Have ThreadedDriver use a TaskQueue/SharedThreadPool

Status: NEW

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

Description

[Jean-Yves Avenard [:jya]]

the ThreadedDriver class creates a single nsIThread that will be shutdown/restarted when needed.

As such it also has to manage the lifetime of this nsIThread.

We could instead have all ThreadedDriver use a common SharedThreadPool via a TaskQueue.

The design and code flow would be simpler and there would be no need to worry thread lifetime management.

Comment 1

[Karl Tomlinson (:karlt)]

Please don't do this.
This is contrary to the plans of bug 1473469, on which we are depending for bug 1476514.

Comment 2

[Jean-Yves Avenard [:jya]]

(In reply to Karl Tomlinson (:karlt) from comment #1)
> Please don't do this.
> This is contrary to the plans of bug 1473469, on which we are depending for
> bug 1476514.

I don't see how doing this change will go against bug 1473469.

Quite the opposite.

Once this is done, configure the shared thread pool to be made of a single thread, and every ThreadedDriver will now run on the same thread.

Comment 3

[Karl Tomlinson (:karlt)]

I assume you are proposing an infinite idleThreadTimeout, so that the same thread stays alive for re-use.  If there is only ever the same thread, then I don't see the advantage of a SharedThreadPool over just using one thread.

The intention of bug 1507607 is to use a single thread for each MSG, which would mean the same thread is used both with the new AudioIPC API and when cubeb is not involved.  That corresponds to both drivers, but we may no longer have separate drivers.

Having a single thread across several MSGs is not compatible with the API proposed in bug 1473469, which is expecting one thread per MSG.  There may be alternative options, but multiplexing multiple streams and callback/sample rates onto one thread would be a significant change in approach, and it is not clear to me that we want all MSGs/streams to be rendering sequentially on the same thread.

Does NS_DispatchToCurrentThread()/GetCurrentThreadEventTarget() do the right thing with nsThreadPool and SharedThreadPool?

With the changes for bug 1473469, the shutdown and restart will not be required in the situations where we currently switch drivers.  Is that the difficulty that you'd like to address?

Is there an immediate problem which is motivating the proposal in this bug?
If you are able to describe that, please, then perhaps we can come up with an intermediate solution that will not need to be reverted for bug 1473469.

Comment 4

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

(In reply to Karl Tomlinson (:karlt) from comment #3)
> I assume you are proposing an infinite idleThreadTimeout, so that the same
> thread stays alive for re-use.  If there is only ever the same thread, then
> I don't see the advantage of a SharedThreadPool over just using one thread.
> 
> The intention of bug 1507607 is to use a single thread for each MSG, which
> would mean the same thread is used both with the new AudioIPC API and when
> cubeb is not involved.  That corresponds to both drivers, but we may no
> longer have separate drivers.
> 
> Having a single thread across several MSGs is not compatible with the API
> proposed in bug 1473469, which is expecting one thread per MSG.  There may
> be alternative options, but multiplexing multiple streams and
> callback/sample rates onto one thread would be a significant change in
> approach, and it is not clear to me that we want all MSGs/streams to be
> rendering sequentially on the same thread.
> 
> Does NS_DispatchToCurrentThread()/GetCurrentThreadEventTarget() do the right
> thing with nsThreadPool and SharedThreadPool?
> 
> With the changes for bug 1473469, the shutdown and restart will not be
> required in the situations where we currently switch drivers.  Is that the
> difficulty that you'd like to address?
> 
> Is there an immediate problem which is motivating the proposal in this bug?
> If you are able to describe that, please, then perhaps we can come up with
> an intermediate solution that will not need to be reverted for bug 1473469.

The immediate problem is my patches for bug 1423241 which are causing a stack overflow on android when running the test_gUM_cubebDisabled tests, because starting cubeb inherently fails so we fall back to a threaded driver, but since there's still audio we switch back to cubeb, and end up flip-flopping between the two drivers until there's no more audio track.

That together with the event loop spinning in nsThread::Shutdown causes the stack overflow, and the patch here prevents the latetr. See it on try for more: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=211984537&revision=bcd7b9f8a618e8f55c124efe64ad9e4da2850436 (both live_backing.log and moz.log are of interest to understand what's going on)

I haven't looked too hard at what in my patches is triggering this. It's possible I can find something to bring us back to the status quo.

Comment 5

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

(In reply to Andreas Pehrson [:pehrsons] from comment #4)
> I haven't looked too hard at what in my patches is triggering this. It's
> possible I can find something to bring us back to the status quo.

I have looked harder now. The triggering patch removed some of the stream-finishing we used to do. That meant that streams were still present in the graph after all their tracks had finished.

In the case of the immediate problem there was a single stream with a single *ended* track remaining, forcing the graph to use an AudioCallbackDriver because of the AudioTrackPresent logic [1].

With this logic adapted to ignore ended tracks we are passing on Android again [2].


[1] https://searchfox.org/mozilla-central/rev/5117a4c4e29fcf80a627fecf899a62f117368abf/dom/media/MediaStreamGraph.cpp#350
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=193dd4357839d23d008bbecfa966b3f49a38eeef