1664115 - Regression caused by Bug 1647717

Status: NEW

(Core :: Audio/Video, defect, P5)

Description

[Ka Ho Ng]

User Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

Occasional to frequent tab crashes after the introduction of Bug 1647717.

Actual results:

Bug 1647717 was intended to allow profiling tracking audio threads launched by libcubeb.

This seemed to introduce a regression in recent version of Firefox. My attempts to workaround the problem is in https://reviews.freebsd.org/D26303 (I was working on FreeBSD OSS backend for Firefox and libcubeb)

The change in this ticket tries to profile the audio threads created by libcubeb. However, for some of the backends at least, such as sndio and sun and even pulse in my test setup,
the code here would not unregister the NativeAudioCallback from audio thread registry whenever the audio stream is paused. When that happens,
the RegisteredThread would remain in CorePS::RegisteredThreads. After that, if another thread is created, and reusing the same thread ID, it will
reuse the existing RegisteredThread without initializing the TLSRegisteredThread pointer to the existing RegisteredThread. This later will fail the
following lines in profiler_unregister_thread():

MOZ_RELEASE_ASSERT(registeredThread ==
TLSRegisteredThread::RegisteredThread(lock));

The patch failed to consider the situation of pausing of audio threads, or if error happens in the IO path that is outside of data_callback(). It seems to me that the patch also places some seemingly unrealistic assumption on usage of audio threads (Did libcubeb restricts the audio backend to only call data_callback in the same worker even if the audio stream didn't stop?). I think it is better to allow launching a callback when the audio threads are to be stopped. This may require either libcubeb having a new API for such purpose , or at least receiving notification somewhere in AudioStream.

The patch should either be reverted, or redrafted to allow safe registration/de-registration of audio threads by Firefox.

Comment 1

[Ka Ho Ng]

Currently please ignore the "pulse" backend for the above description, but only consider sndio backend and potentially sun backend (due to the same pattern of thread creation as sndio backend).

Comment 2

[Paul Adenot (:padenot)]

Thanks. Do you know if FreeBSD uses audio remoting (media.cubeb.sandbox set to true) ?

I assume not because otherwise the thread would be called AudioIPC with a number, and not NativeAudioCallback, but are there plans to enable it?

Comment 3

[Ka Ho Ng]

(In reply to Paul Adenot (:padenot) from comment #2)

Thanks. Do you know if FreeBSD uses audio remoting (media.cubeb.sandbox set to true) ?

I assume not because otherwise the thread would be called AudioIPC with a number, and not NativeAudioCallback, but are there plans to enable it?

media.cubeb.sandbox by default is false on FreeBSD

Comment 4

[Jon Bauman [:jbauman:]]

padenot, do you have a preference for priority/severity on this one?

Comment 5

[Paul Adenot (:padenot)]

(In reply to Ka Ho Ng from comment #0)

It seems to me that the patch also places some seemingly unrealistic assumption on usage of audio threads (Did libcubeb restricts the audio backend to only call data_callback in the same worker even if the audio stream didn't stop?).

This is how all mainstream audio stacks work in practice. Having different threads servicing the audio callback is a problem, quite a lot of data structures and other algorithms used in real-time audio programming only work if the threads don't change. Changing the thread on pause/play is fine, however, this is a bug in AudioStream.cpp, that I will address.

Comment 6

[Ka Ho Ng]

(In reply to Paul Adenot (:padenot) from comment #5)

(In reply to Ka Ho Ng from comment #0)

It seems to me that the patch also places some seemingly unrealistic assumption on usage of audio threads (Did libcubeb restricts the audio backend to only call data_callback in the same worker even if the audio stream didn't stop?).

This is how all mainstream audio stacks work in practice. Having different threads servicing the audio callback is a problem, quite a lot of data structures and other algorithms used in real-time audio programming only work if the threads don't change. Changing the thread on pause/play is fine, however, this is a bug in AudioStream.cpp, that I will address.

That makes sense. So the only remaining part would be how we are going to make sure that we won't lose track when either the stream is paused, or when error happens in the audio thread. By the way, do you think we need to do something similar in GraphDriver.cpp as the current way it tracks audio threads created by cubeb look similar to AudioStream.cpp

Comment 7

[Paul Adenot (:padenot)]

Indeed, those are the exact two ways that a Firefox process will do audio. AudioStream.cpp is for higher latency audio (lower CPU usage and resilience favored over latency), and GraphDriver.cpp is for low-latency interactive audio (games, music, communication, etc.), there are no other paths.

Comment 8

[Paul Adenot (:padenot)]

Where do we stand with this, in relation to the now merged OSS backend, that has received modification since this was opened ?

Comment 9

[Ka Ho Ng]

(In reply to Paul Adenot (:padenot) from comment #8)

Where do we stand with this, in relation to the now merged OSS backend, that has received modification since this was opened ?

The issues mentioned still applies to cubeb sndio, which spawns a new thread everytime when a stream is started. For OSS since the thread is only spawned for the first stream starting attempt this bug should be circumvented.

Comment 10

[Paul Adenot (:padenot)]

Do you have a patch on Firefox downstream that you're using? I could maybe finish it up and land it?

Comment 11

[Ka Ho Ng]

(In reply to Paul Adenot (:padenot) from comment #10)

Do you have a patch on Firefox downstream that you're using? I could maybe finish it up and land it?

https://reviews.freebsd.org/file/data/qt7outh55g5jygib6yni/PHID-FILE-7c3zfh5v7qyhgnpog45w/D26303.vson.id76591.diff

My original attempt to solve this problem was to unregister the thread when state change occurs. However, it wasn't correct to assume that thread ID of the state callback being the same as data callback. So instead I reverted the profiler changes related to audio code that manifested this regression.