Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pulse: fill audio monitor buffer more aggressively #4908

Merged
merged 1 commit into from Nov 4, 2021

Conversation

kkartaltepe
Copy link
Collaborator

Description

Previously we would wait for pulse to attempt to read from the monitor
source and obs buffered at least 5ms of audio data before we tried to
fill the buffer. In some cases this resulted in consistently triggering
underruns in pulse.

Instead we try to fill the buffer immediately as obs outputs audio data
and while the pa buffer is not full. We also stop trying to grow the
buffer to prevent underruns after we reach 1s of latency.

Motivation and Context

Attempting to fix #3525

How Has This Been Tested?

Adding a media source with an mp3 gave consistent underrun's with the old code. After these changes there are no more under runs reported from the pulse log or obs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Linux Affects Linux labels Jun 16, 2021
@kkartaltepe kkartaltepe added the Seeking Testers Build artifacts on CI label Jun 18, 2021
@kkartaltepe
Copy link
Collaborator Author

Right now this logs whenever the pulse monitor underflows. This occurs probably more often then people think but generally isnt an issue (when audio sources are muted/end, similar underruns are reported in things like firefox and mpv for me) but I left the logging in. It may be better to turn it off unless we reach the 1s latency threshold.

@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 19, 2021

After testing this, I found that the latency doesn't increase over time, but there is a noticeable initial latency when first enabling monitoring. After a restart of OBS, there is no noticeable latency.

However, changing from "monitoring disabled" to "monitoring and output" sometimes crashes OBS with this Assertion 'data' failed at ../src/pulse/stream.c:1485, function pa_stream_write_ext_free(). Aborting. (logs)

Side note, it doesn't fix #4759.

@kkartaltepe
Copy link
Collaborator Author

post a log from when you trigger #4759 with this PR please. And if you could describe precisely the input you are using to cause this, preferably share it if its an audio file or something. I can not replicate #4759 on my setup with PR on my side.

@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 19, 2021

post a log from when you trigger #4759 with this PR please.

Log. Nothing suspicious, and Pipewire doesn't report anything either where it used to report underruns. The issue might be in Pipewire, I haven't tried this PR with PulseAudio yet.

And if you could describe precisely the input you are using to cause this, preferably share it if its an audio file or something. I can not replicate #4759 on my setup with PR on my side.

It happens with either a PulseAudio input source, a media source or a Jack input source.

When OBS crashes, this appears in Pipewire logs:

juin 19 20:15:31 fedora pipewire[1801]: 34 events suppressed
juin 19 20:15:31 fedora pipewire[1801]: (Client d'entrée JACK-92) client too slow! rate:1024/48000 pos:84469760 status:awake
juin 19 20:15:32 fedora pipewire[1801]: 3 events suppressed

@kkartaltepe
Copy link
Collaborator Author

kkartaltepe commented Jun 19, 2021

Yes i can replicate the crash, but i was more concerned with the crackling. You should be observing the pipewire-pulse logs not the pipewire logs for events related to over and underrun that might be causing cracking. In this case it seems that pulseaudio is not reporting under runs to OBS so something else is causing your crackling.

Please provide an example audio file that you are using in media source if you can.

@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 19, 2021

You should be observing the pipewire-pulse logs not the pipewire logs for events related to over and underrun that might be causing cracking.

I am observing the pipewire-pulse logs as I am using this command: journalctl --user -u pipewire --user -u pipewire-media-session --user -u pipewire-pulse -f.

Please provide an example audio file that you are using in media source if you can.

Here you go: valse-gymnopedie-by-kevin-macleod-from-filmmusic-io.zip

@kkartaltepe
Copy link
Collaborator Author

So you see no under/over runs in any logs but still hear crackling? I have tried with your example file and dont hear any cracking or when adding other media sources and jack sources and monitoring them (4 monitors running).

@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 19, 2021

No, nothing in the logs. The crackling happens when you lower the output volume and gets worse as you go down.

Here's a capture (beware, loud audio):

capture.mp4

@kkartaltepe
Copy link
Collaborator Author

kkartaltepe commented Jun 19, 2021

Right that doesnt occur for me on the old or the new code, Im not sure what the source of that particular issue is for you but since you dont see underruns or overruns its unrelated to this code I suppose.

I'd ask you replicate that effect on pulse audio and if it replicates we can reopen your bug otherwise file a bug against pipewire for that one.

@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 19, 2021

It works as expected with PulseAudio. I had already opened an issue against Pipewire: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/1238

@kkartaltepe
Copy link
Collaborator Author

Updated the write checks and it no longer crashes on my machine, thanks for finding that!

Previously we would wait for pulse to attempt to read from the monitor
source and obs buffered at least 5ms of audio data before we tried to
fill the buffer. In some cases this resulted in consistently triggering
underruns in pulse.

Instead we try to fill the buffer immediately as obs outputs audio data
and while the pa buffer is not full. We also stop trying to grow the
buffer to prevent underruns after we reach 1s of latency.
@rmnvgr
Copy link
Contributor

rmnvgr commented Jun 20, 2021

I can confirm that it doesn't crash anymore on my side either, and the initial latency when first enabling monitoring disappeared.

@ogmkp
Copy link

ogmkp commented Jul 11, 2021

The patch #4908 is good for me, no lag for hours on Debian 10 but only for input device, no change for medias.

@TwoClocks
Copy link

I tried out this branch. It did improve the increasing lag over time but did not remove it.

The stock version 27 of obs, the audio would fall behind 2-3 seconds every 30 min.

This PR improved those times to 2-3 seconds over 4 hours. So much better.

No other issues so far (only been using for a day).

How I measured :
1 - Started the virtual camera, and reset the audio (by disabling, then re-enabling monitoring).
2- Went to the lobby screen for a google meet call. There is an audiometer in the corner of the video image.
3 - time delay between clapping and audiometer moving.
4 - come back 4 hours later - retime.

kinda janky. If anyone has a better measurement methodology I'd be happy to try it.

@TheMuso
Copy link
Contributor

TheMuso commented Sep 20, 2021

Prior to applying this PR, I experienced stuttering when attempting to play a FLAC file either with the media source, or a VLC source, Fedora 34, Pipewire 0.3.36, OBS master branch. I also experienced stuttering on OBS studio 27.0.1 from RPM fusion. This PR completely clears up stuttering output for me.

I have not tested any capture scenarios.

@WizardCM WizardCM added this to PRs Pending Review in OBS Studio 27.2 via automation Oct 7, 2021
@Zulleyy3
Copy link
Contributor

I tried the drop-in replacement of Pulseaudio with Pipewire (version: 0.3.38) on Arch Linux and it resulted in monitored audio being broken completely (crackling audio, cutting out audio and so on).

Applying this PR fixed the issues when the audio source volume was at 0,0db, but introduced the same issue that rmnvgr had: #4908 (comment)

@Zulleyy3
Copy link
Contributor

After going back to PulseAudio with PR applied (in a local portable build) and without the PR applied (a package from AUR), I can reproduce #4908 (comment) with PulseAudio as well.

@kkartaltepe
Copy link
Collaborator Author

Audio at lower volumes being wrong appears to be fixed by #5400 , if you could try that and this commit.

@norihiro
Copy link
Contributor

norihiro commented Oct 11, 2021

Audio at lower volumes being wrong appears to be fixed by #5400 , if you could try that and this commit.

According to Zulleyy3 at a discussion on discord, the audio volume bug is fixed on their host.

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took some time to properly understand the PA API, and the differences between this branch and main, and AFAIU this is correct. Bonus points for getting rid of a lock + unlock 🙂

Tested on PulseAudio and PipeWire, both are working as expected. So 👍 from my side. Thanks!

@GeorgesStavracas GeorgesStavracas moved this from PRs Pending Review to PRs Pending Merge (Reviewed) in OBS Studio 27.2 Nov 4, 2021
@jp9000 jp9000 merged commit 5142a76 into obsproject:master Nov 4, 2021
OBS Studio 27.2 automation moved this from PRs Pending Merge (Reviewed) to Completed Nov 4, 2021
@RytoEX RytoEX added this to the OBS Studio 27.2 milestone Nov 4, 2021
@cameron-doyle
Copy link

Does this fix the issue across all platforms, or just Linux? Sorry for the stupid question but I'm not familiar with this codebase.

@norihiro
Copy link
Contributor

norihiro commented Nov 6, 2021

Does this fix the issue across all platforms, or just Linux? Sorry for the stupid question but I'm not familiar with this codebase.

Hi @teaStudios,
This fix applies only Linux.
Windows and macOS won't be affected. If you have a problem on audio monitor, see https://obsproject.com/help for FAQ and support.

@cameron-doyle
Copy link

Hi @norihiro
Thanks for the info. I'm not going to post anything further about it. I've found a workaround thanks to:
#3525 (comment)

I've seen posts about this exact bug on windows as far back as 2016 which is unfortunate.
Anyway thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Linux Affects Linux Seeking Testers Build artifacts on CI
Projects
OBS Studio 27.2
  
Completed
Development

Successfully merging this pull request may close these issues.

[Linux] Big latency when monitoring auxiliary audio device