Closed Bug 1844181 Opened 3 years ago Closed 3 years ago

Poor quality in WebRTC audio capture on Linux

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
Firefox 115
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
120 Branch
Tracking Flags:
Tracking Status
firefox120 --- fixed

People

(Reporter: jim3692, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

Attachments

(48 external files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/115.0

Steps to reproduce:

We are developing this extension, that allows screensharing with audio on Linux: https://github.com/IceDBorn/pipewire-screenaudio

Actual results:

When getUserMedia creates a new Pipewire Node, it's locked to F32LE format. We suspect that this causes audio pitching in calls, mainly when music is present. When the audio transmitted only contains voices, or sound effects, the quality is perfect. We checked the format on Chromium and it is set to S16LE. They also have different latency values. Firefox has 1200/48000 whereas Chromium has 480/48000.

Expected results:

Both browsers should have transmitted audio with similar quality, while Chromium has crystal clear audio, even with music, Firefox has pitching issues.

The only place I have seen a discussion related to this topic is this issue on Github: https://github.com/edisionnano/Screenshare-with-audio-on-Discord-with-Linux/issues/12

The Bugbug bot thinks this bug should belong to the 'Core::WebRTC: Audio/Video' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core

We are developing this extension, that allows screensharing with audio on Linux ...

When getUserMedia creates a new Pipewire Node, it's locked to F32LE format. We suspect that this causes audio pitching in calls, mainly when music is present.

Do you mean getDisplayMedia? getUserMedia is for camera and microphone.

Blocks: 1685232
Severity: -- → S3
Flags: needinfo?(jim3692)
Priority: -- → P3

He meant getDisplayMedia, I'm another developer of the project and I can confirm by looking at the source code.

Flags: needinfo?(jib)
Flags: needinfo?(jib)
Flags: needinfo?(jib)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #3)

We are developing this extension, that allows screensharing with audio on Linux ...

When getUserMedia creates a new Pipewire Node, it's locked to F32LE format. We suspect that this causes audio pitching in calls, mainly when music is present.

Do you mean getDisplayMedia? getUserMedia is for camera and microphone.

Sorry for the confusion. We override the getDisplayMedia method in order to inject the audio from the pipewire-screenaudio node. To do that, we use getUserMedia to find that node.

Flags: needinfo?(jim3692)
Flags: needinfo?(jib) → needinfo?(padenot)

Can someone get us ways to reproduce or audio examples of this happening? Should we install the extension or something?

Flags: needinfo?(padenot) → needinfo?(jim3692)

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

Can someone get us ways to reproduce or audio examples of this happening? Should we install the extension or something?

I have samples from when the bug was discovered (plus samples of the original music for comparison). https://mega.nz/file/dK4mXQqL#quxhbB_sS6ukf5Qqyi6iGHobcCepKg2ycBYxV7RDthE

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

Can someone get us ways to reproduce or audio examples of this happening? Should we install the extension or something?

You will need to install the extension from Addons, and optimally give it access to all websites, in case the call is in an iframe: https://addons.mozilla.org/en-US/firefox/addon/pipewire-screenaudio/

For its native part, which controls Pipewire you can either:

  1. Install our AUR package on Arch: https://aur.archlinux.org/packages/pipewire-screenaudio
    And choose the audio source to stream from within the extension

  2. Or you can manually create the capturing node with this command:
    pw-cli create-node adapter "{ factory.name=support.null-audio-sink node.name=pipewire-screenaudio media.class=Audio/Source/Virtual object.linger=1 audio.position=[FL,FR] }"
    And connect the inputs of the " Pipewire Screenaudio" node to a media player using Helvum

After selecting or connecting your media, you can start your screen share.

You can try using Spotify for the test. We saw that setting its quality to High, made the pitching much more noticeable than when it was set to Normal. But you can also try YouTube videos with and without music.

Flags: needinfo?(jim3692)

This is a bug in our drift compensator. An easier way to repro:

Run Helvum, make a new device like instructed:

pw-cli create-node adapter "{ factory.name=support.null-audio-sink node.name=pipewire-screenaudio media.class=Audio/Source/Virtual object.linger=1 audio.position=[FL,FR] }"

Play something in a media player, connect it to this new device using Helvum.

Open:
https://paul.cx/public/testing-multi-gum.html

Open a regular mic, and then open the new device, that should play what the media player plays.

Essentially what happens is that the device buffer size isn't the same as the graph, and everything breaks.

Assignee: nobody → padenot
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Is there a workaround we can use on our extension until this bug gets fixed?

Flags: needinfo?(padenot)

(In reply to github.envenomed from comment #11)

Is there a workaround we can use on our extension until this bug gets fixed?

I'm not sure. Opening the mic in second instead of first should do the trick, but maybe it's not practical. We'll get this fixed.

Flags: needinfo?(padenot)
No longer blocks: 1685232

Opening the mic in second instead of first should do the trick

What do you mean by that?

Flags: needinfo?(padenot)

Do the capture first, then the microphone.

But we have new code for this that is getting ready, there should be something to try in a few days.

Flags: needinfo?(padenot)

Do the capture first, then the microphone.

But we have new code for this that is getting ready, there should be something to try in a few days.

Assignee: padenot → apehrson

Watching a stream using Firefox 117.0b3 seems to fix the issue. We tried watching the stream with Firefox ESR and the audio drift bug was present. Did you fix the bug in 117?

Flags: needinfo?(apehrson)

No. Maybe you end up with different latencies on 117?

This try build is of my WIP work. You could try the linux x64 shippable build to see how it fares.

Flags: needinfo?(apehrson)

It works, the audio pitch is gone!

Flags: needinfo?(apehrson)

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:pehrsons, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Flags: needinfo?(cchang)
Flags: needinfo?(apehrson)
Flags: needinfo?(apehrson)

Enables differentiating audio callback sources when tracing a two-mic use case
together with https://github.com/padenot/fx-profiler-audio-cb/pull/3.

This will help keep things together when in a later patch we add a plotting
script and instructions for using it.

The pre-buffer has the same size as the target buffer size we aim towards
keeping constant. If the first input segment is large the total amount buffered
is significantly higher than the target buffer size, which leads to us applying
the minimum drift correction (0.9) to shrink it back to the target buffer size
eventually. This can be avoided by accounting for the first input segment when
adding pre-buffering silence.

In order to do the correction calculations on fresh data.

This guarantees that our measurement of the current buffering level is fairly
accurate, since the reading of the input buffering level is recent.

This is especially important in situations where the input stream latency
(callback buffer size) is significantly higher than the output stream latency
which drives the drift correction.

These help verify the functionality of the drift correction. Especially useful
together with the data logging and plotting, see comments on the test cases.

PID-controllers are a proven way to control a system by only measuring an
indirect output of them.

This PID-controller is tuned by hand by looking at step responses
(see TestDriftController.cpp) and applying, in order, the P, D and I terms.

The tuning was done on top of later patches in this bug, so may not be ideal
when looking at this patch in isolation.

This can be handy when AudioDriftCorrection resamples the first time.

In order to calculate how many frames to prepend in order to get the buffer to
contain exactly the number of desired pre-buffer frames after resampling, so
the DriftController starts out with an error = 0.

This allows the drift correction code to start out with an error of 0, avoiding
starting out with a significant correction.

This lets AudioDriftCorrection handle situations where it's about to overflow
the buffer. Be it because of unexpectedly high drift, very high input latency or
an increased target buffering level.

This patch allows setting the desired buffering level on AudioDriftCorrection
dynamically. If the AudioResampler's pre-buffer has not yet been set, it will
be populated to match the new desired buffering level on the next Resample().

Note that this patch forwards the latency from CubebInputStream which can be
queried prior to starting, and we are therefore guaranteed to be able to set it
on the drift corrector prior to the first input callback.

CrossGraphPort is more complex with an entire MediaTrackGraph and its
GraphDriver behind it, and is left out at this time.

This patch handles an underrun to the drift correction input buffer by:

  • Resetting the pre-buffer in the AudioResampler, so on the next Resample(),
    it will prepend silence to match the desired buffering level.
  • Doubling the desired buffering level, since apparently it was not enough.

Overall this will make the perceived discontinuity longer than if we just output
the underrun duration worth of silence. There should also be fewer
discontinuities because of this, likely just one.

This is so that the integral term does not grow unboundedly while the control
signal is hitting its cap, as it would lead to increasing overshoot and
instability.

Having a hysteresis threshold on the error signal significantly reduces the
frequency at which we reconfigure the AudioResampler as a response to changes
in the control signal.

For allowing rounding upwards.

For safer conversions between time bases, of which drift control has plenty.

This prevents iterating MediaTrackGraph 0 frames.

CrossGraphTransmitter does not have an mSegment so its duration is meaningless.

There are not many statements identifying fallback wrappers in MediaTrackGraph:5
logs. Log the owning AudioCallbackDriver pointer instead.

Manual mode means it is up to the test to manually call the DataCallback.
With manual mode there will be no background thread spawned to run callbacks.
This can help make unittests that use MockCubeb more deterministic.

When a sudden (one high sample to a zero sample) discontinuity is fed to the
speex resampler, this will typically be smoothed out over multiple samples in
the output. Before this patch AudioVerifier only cares about the difference
between two adjacent samples when counting discontinuity.

This patch makes AudioVerifier account for several discontinuous samples in a
single discontinuity. I.e., once a discontinuity is detected, AudioVerifier
waits for a handful of samples to pass or for the discontinuity to go away,
whichever comes first, before being eligible for detecting another
discontinuity.

This also makes testMonoToStereoInput more similar to testAudioCorrection, since
they are essentially two variants of the same test, and fixes a test expectation.

It must not store more data than was consumed by the resampler in the tail
buffer.

See Also: → 1404967

This is ready, except for a data race found by tsan (roughly 20% failure rate) that we don't yet understand.

Flags: needinfo?(cchang)
See Also: → 1670633

Without this, the necessary synchronization must be provided externally.

This fixes the memory order in the following case of changing producer thread:

  • Thread A does SPSCQueue::Enqueue
    • non-atomic write into the ring buffer, at memory location X
    • mWriteIndex.load(relaxed)
    • mWriteIndex.store(release)
  • Producer thread is switched to B, no external memory order synchronization is
    provided, but thread B is guaranteed to run after thread A has finished its
    Enqueue task.
  • Thread B does SPSCQueue::Enqueue
    • mWriteIndex.load(relaxed)
    • mWriteIndex.store(release)
  • Thread C does SPSCQueue::Dequeue
    • mWriteIndex.load(acquire)
    • non-atomic read from the ring buffer, at memory location X

In this scenario, there is no memory synchronization between threads A and B,
and therefore the non-atomic read on C is a data race, and flagged as such by
TSAN.

A similar scenario can be applied to changing the consumer thread, if first A
enqueues, then B dequeues, then C dequeues. However, since Dequeue doesn't
necessarily (MoveOrCopy) do non-atomic writes to the ring buffer, and more
importantly, since Enqueue doesn't do non-atomic reads from the ring buffer,
this is less of a problem.

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f3d52a166497 Add gtest for transmitter block size that is larger than the receiver block size. r=padenot https://hg.mozilla.org/integration/autoland/rev/07f593daa504 Add gtests for block sizes that are larger than the desired buffer size. r=padenot https://hg.mozilla.org/integration/autoland/rev/b3ab4f7079a6 Add gtest for changing transmitter block size dynamically. r=padenot https://hg.mozilla.org/integration/autoland/rev/3995617f816a Allow to differentiate different sources of audio callbacks when tracing. r=padenot https://hg.mozilla.org/integration/autoland/rev/c1ab79a9ac72 Trace audio callbacks in AudioInputSource. r=padenot https://hg.mozilla.org/integration/autoland/rev/30f59a0c95f2 Move drift control classes to a dedicated dir. r=padenot https://hg.mozilla.org/integration/autoland/rev/0bce9535589c Split driftcontrol files into .h and .cpp, dedicated per-class. r=padenot https://hg.mozilla.org/integration/autoland/rev/6f25e0f2a000 Add support for plotting data from ClockDrift. r=padenot https://hg.mozilla.org/integration/autoland/rev/dfc424bf89e4 Add WavDumper to DynamicResampler as a diagnosis tool. r=padenot https://hg.mozilla.org/integration/autoland/rev/d5b79e973899 In DynamicResampler include first audio input segment in pre-buffer. r=padenot https://hg.mozilla.org/integration/autoland/rev/42f435429f25 Allow to drain DynamicResampler of all remaining input. r=padenot https://hg.mozilla.org/integration/autoland/rev/6197b1172cdc Update clock states prior to calculating correction. r=padenot https://hg.mozilla.org/integration/autoland/rev/1538a5bea488 Calculate drift correction on a callback where there's input data present. r=padenot https://hg.mozilla.org/integration/autoland/rev/60880eedb9de Add some step response gtests for AudioDriftCorrection. r=padenot https://hg.mozilla.org/integration/autoland/rev/39e5ea5b7c9a Undef some media LOG macros properly. r=padenot https://hg.mozilla.org/integration/autoland/rev/ed07e8de5754 Use a logging macro in ClockDrift. r=padenot https://hg.mozilla.org/integration/autoland/rev/541cfe3da283 Measure and plot input and output latency. r=padenot https://hg.mozilla.org/integration/autoland/rev/907c917b52fe Use a PID-controller to correct drift. r=padenot https://hg.mozilla.org/integration/autoland/rev/c4f57f87e34d Implement AudioRingBuffer::PrependSilence. r=padenot https://hg.mozilla.org/integration/autoland/rev/15af59e306c6 Prepend silence to the pre-buffer on first resample. r=padenot https://hg.mozilla.org/integration/autoland/rev/68d05ed50d8a Make it possible to resize AudioRingBuffer on the fly. r=padenot https://hg.mozilla.org/integration/autoland/rev/d448783c33e4 Allocate DynamicResampler's internal in-buffer dynamically. r=padenot https://hg.mozilla.org/integration/autoland/rev/5cdcf42ebc69 Drive drift corrections by target clock only. r=padenot https://hg.mozilla.org/integration/autoland/rev/89c0108a7c3a Track/plot/test AudioDriftCorrection's current buffer size. r=padenot https://hg.mozilla.org/integration/autoland/rev/2e257850dc13 Increase drift correction's desired buffering level dynamically. r=padenot https://hg.mozilla.org/integration/autoland/rev/1e1f3831e261 Add memory synchronization to SPSCQueue's thread id reset methods. r=decoder,padenot,handyman https://hg.mozilla.org/integration/autoland/rev/d0497551c605 Seed the desired buffering level with input stream latency. r=padenot https://hg.mozilla.org/integration/autoland/rev/f4195ac81452 Handle drift correction underruns. r=padenot https://hg.mozilla.org/integration/autoland/rev/8484b3a81291 Impose a cap on the integral term's steps. r=padenot https://hg.mozilla.org/integration/autoland/rev/b06a6636ac90 Apply hysteresis to the drift controller. r=padenot https://hg.mozilla.org/integration/autoland/rev/7b797b4dd354 Remove comments about threading from AudioDriftCorrection since it's single threaded. r=padenot https://hg.mozilla.org/integration/autoland/rev/fbbfe1de3c65 Add a CeilingPolicy to TimeUnit. r=padenot https://hg.mozilla.org/integration/autoland/rev/25757a9dd4ca Use TimeUnit across drift control classes. r=padenot https://hg.mozilla.org/integration/autoland/rev/1aa8fb4ee69c Remove commented-out variables from TestAudioResampler.cpp. r=padenot https://hg.mozilla.org/integration/autoland/rev/706c4902fd36 Fix an assertion in AudioBufferUtils. r=padenot https://hg.mozilla.org/integration/autoland/rev/5c577d284800 Fix a log message in CrossGraphTransmitter. r=padenot https://hg.mozilla.org/integration/autoland/rev/e0ab2775ca26 Improve some fallback driver log messages. r=padenot https://hg.mozilla.org/integration/autoland/rev/333b7208d6d7 In MockCubeb add a manual mode. r=padenot https://hg.mozilla.org/integration/autoland/rev/e2e5c96f32e4 In AudioVerifier allow a discontinuity to stretch across multiple samples. r=padenot https://hg.mozilla.org/integration/autoland/rev/630a12c1de0e Use MockCubeb's manual mode and tighten expectations in TestCrossGraphPort. r=padenot https://hg.mozilla.org/integration/autoland/rev/18c8184118db Improve comparison of arrays in TestAudioInputSource. r=padenot https://hg.mozilla.org/integration/autoland/rev/da112e1234e5 Reduce the desired buffering level automatically. r=padenot https://hg.mozilla.org/integration/autoland/rev/dd7b8d38431e Fix channel asserts in DynamicResampler. r=padenot https://hg.mozilla.org/integration/autoland/rev/f103c687dfe1 Remove helper for rudimentary logging of audio frames in TestAudioDriftCorrection. r=padenot https://hg.mozilla.org/integration/autoland/rev/61d2120b3d48 Make testAudioCorrection and testMonoToStereoInput more accurate on amount of drift generated. r=padenot https://hg.mozilla.org/integration/autoland/rev/c86d8d07927a Fix DynamicResampler's handling of channel count changes. r=padenot https://hg.mozilla.org/integration/autoland/rev/e7b747380a92 Merge testMonoToStereoInput into testAudioCorrection. r=padenot https://hg.mozilla.org/integration/autoland/rev/c406e2e8f277 Remove the media.clockdrift.buffering pref. r=padenot

https://hg.mozilla.org/mozilla-central/rev/f3d52a166497
https://hg.mozilla.org/mozilla-central/rev/07f593daa504
https://hg.mozilla.org/mozilla-central/rev/b3ab4f7079a6
https://hg.mozilla.org/mozilla-central/rev/3995617f816a
https://hg.mozilla.org/mozilla-central/rev/c1ab79a9ac72
https://hg.mozilla.org/mozilla-central/rev/30f59a0c95f2
https://hg.mozilla.org/mozilla-central/rev/0bce9535589c
https://hg.mozilla.org/mozilla-central/rev/6f25e0f2a000
https://hg.mozilla.org/mozilla-central/rev/dfc424bf89e4
https://hg.mozilla.org/mozilla-central/rev/d5b79e973899
https://hg.mozilla.org/mozilla-central/rev/42f435429f25
https://hg.mozilla.org/mozilla-central/rev/6197b1172cdc
https://hg.mozilla.org/mozilla-central/rev/1538a5bea488
https://hg.mozilla.org/mozilla-central/rev/60880eedb9de
https://hg.mozilla.org/mozilla-central/rev/39e5ea5b7c9a
https://hg.mozilla.org/mozilla-central/rev/ed07e8de5754
https://hg.mozilla.org/mozilla-central/rev/541cfe3da283
https://hg.mozilla.org/mozilla-central/rev/907c917b52fe
https://hg.mozilla.org/mozilla-central/rev/c4f57f87e34d
https://hg.mozilla.org/mozilla-central/rev/15af59e306c6
https://hg.mozilla.org/mozilla-central/rev/68d05ed50d8a
https://hg.mozilla.org/mozilla-central/rev/d448783c33e4
https://hg.mozilla.org/mozilla-central/rev/5cdcf42ebc69
https://hg.mozilla.org/mozilla-central/rev/89c0108a7c3a
https://hg.mozilla.org/mozilla-central/rev/2e257850dc13
https://hg.mozilla.org/mozilla-central/rev/1e1f3831e261
https://hg.mozilla.org/mozilla-central/rev/d0497551c605
https://hg.mozilla.org/mozilla-central/rev/f4195ac81452
https://hg.mozilla.org/mozilla-central/rev/8484b3a81291
https://hg.mozilla.org/mozilla-central/rev/b06a6636ac90
https://hg.mozilla.org/mozilla-central/rev/7b797b4dd354
https://hg.mozilla.org/mozilla-central/rev/fbbfe1de3c65
https://hg.mozilla.org/mozilla-central/rev/25757a9dd4ca
https://hg.mozilla.org/mozilla-central/rev/1aa8fb4ee69c
https://hg.mozilla.org/mozilla-central/rev/706c4902fd36
https://hg.mozilla.org/mozilla-central/rev/5c577d284800
https://hg.mozilla.org/mozilla-central/rev/e0ab2775ca26
https://hg.mozilla.org/mozilla-central/rev/333b7208d6d7
https://hg.mozilla.org/mozilla-central/rev/e2e5c96f32e4
https://hg.mozilla.org/mozilla-central/rev/630a12c1de0e
https://hg.mozilla.org/mozilla-central/rev/18c8184118db
https://hg.mozilla.org/mozilla-central/rev/da112e1234e5
https://hg.mozilla.org/mozilla-central/rev/dd7b8d38431e
https://hg.mozilla.org/mozilla-central/rev/f103c687dfe1
https://hg.mozilla.org/mozilla-central/rev/61d2120b3d48
https://hg.mozilla.org/mozilla-central/rev/c86d8d07927a
https://hg.mozilla.org/mozilla-central/rev/e7b747380a92
https://hg.mozilla.org/mozilla-central/rev/c406e2e8f277

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox120: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch

Reporter, do you mind testing your add-on again in a Firefox Nightly? Please make sure that it's up-to-date since this just got merged, it's only on the very last version.

This is set to release to the general public in December 5th.

Flags: needinfo?(jim3692)

Hello, we have tested with Firefox Nightly and it looks like it is fixed.
Thank you very much for your time and support.

Flags: needinfo?(jim3692)
See Also: → 1859201
Regressions: 1860681
Blocks: 1890467
Attachment #9346243 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: