Fix random audio dropouts and corruption of audio data (2/2)

[floele]

(Draft PR since I'm trying to get a few more positive test results before merging).

Description

1) libobs: Fix corruption of audio data in audio_input_buf

Previously, source->next_audio_ts_min was incremented by conv_frames_to_time(sample_rate, in.frames) in each call of source_output_audio_data(), so only by the duration of the audio data which is basically a fixed value (if in.frames doesn't vary).

However, this does not take the incoming audio timestamps into account, which also include execution time. In ProcessCaptureData() for example os_gettime_ns() is used to generate the audio timestamp if device timestamps are off so it's unavoidable that the incoming timestamps are always a little later than the raw duration of the audio data. This also applies if device timestamps are enabled though, at least when recording from desktop audio (Windows 10) which I have used to verify the issue.
This adds up, because in.timestamp is being set to source->next_audio_ts_min before calculating the next timestamp source->next_audio_ts_min = in.timestamp + conv_frames_to_time(sample_rate, in.frames).

If you watch the values over time, you can see the timestamps deviating like this (extract of a few thousand iterations):

next_audio_ts_min: 366688637418000, actual timestamp: 366688637507100, diff: 89100
next_audio_ts_min: 366690227418000, actual timestamp: 366690227453000, diff: 350000
next_audio_ts_min: 366697247418000, actual timestamp: 366697247892200, diff: 474200
next_audio_ts_min: 366702747418000, actual timestamp: 366702748130500, diff: 712500
next_audio_ts_min: 366705197418000, actual timestamp: 366705198281800, diff: 863800
next_audio_ts_min: 366708257418000, actual timestamp: 366708258445200, diff: 1027200

So the incoming audio timestamps gradually deviate further from the expected next_audio_ts_min timestamp, eventually exceeding TS_SMOOTHING_THRESHOLD and then causing a call to source_output_audio_place() which either overwrites existing audio data and causes a glitch or puts data beyond the end of the buffer, resizing it and inserting a gap and thus causing an audio dropout.

Now next_audio_ts_min is incremented based on the current incoming audio timestamp to prevent this issue.

For testing purposes, this issue can be detected by looking for "frames: %lu, size: %lu, placement: %lu, base_ts: %llu, ts: %llu" debug messages (with DEBUG_AUDIO enabled).

2) libobs: Fix audio dropouts in input_and_output()

If the audio buffer is not currently sufficiently filled (at least AUDIO_OUTPUT_FRAMES), process_audio_source_tick() does not put any data into audio_output_buf. This situation is also taken into consideration in discard_audio() which will emit a debug message "can't discard, data still pending" (assuming DEBUG_AUDIO == 1) in this situation.

However, input_and_output() still executes do_audio_output(), taking exactly AUDIO_OUTPUT_FRAMES from the buffer, causing an audio dropout.

The change now returns false in audio_callback() if any audio source is still pending, so that further processing in input_and_output() is prevented.

For testing purposes, this issue can be detected by looking for "can't discard, data still pending" debug messages (with DEBUG_AUDIO enabled).

Motivation and Context

These two fixes solve the issues discussed in #4600.

In short, audio recorded or streamed by OBS contains random short dropouts or corruption, independently of settings and environment, though frequency may vary. The frequency of the issue occuring ranges from every couple of minutes to maybe once or twice per hour based on my observations and the reports in the mentioned issue.

How Has This Been Tested?

For my tests I use a test tone generator to record continuous uniform audio data with OBS (default settings, Windows 10). I then check the resulting audio stream for dropouts. For testing, I usually record 1 to 2 h of audio data.

I tried to make the changes so that the overall behaviour of the code changes as little as possible.

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.

[RytoEX]

Please wrap your commit message bodies at 72 characters, per our Commit Guidelines in our Contributing Guidelines.

In the future, please include "Fixes #<Issue Number>" in your PR description to automatically link the PR to whatever GitHub Issue(s) it may fix. I have manually linked this to Issue #4600. See these for more information:

• https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue
• https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

Generally, it's a good idea to keep your branch up-to-date with the branch that you're submitting it to by rebasing before submitting it. Please rebase this when you're able to do so.

[floele]

Branch now rebased and commit messages wrapped.

BTW, where would I find the compiled packages that include these changes?

[dodgepong]

If you click the "CI Multiplatform Build" section of our actions page, you can find the latest build from your pull request there with artifacts at the bottom of the page.

Example: https://github.com/obsproject/obs-studio/actions/runs/1878408431

[floele]

Additional changes I have been thinking about:

• Do we need to add comments to the code explaining the situation? (if at all, probably only for 86d03b5)
• Should we add additional output messages for the log file to keep track of audio issue occurrences? We could add one for audio_pending = true; for example. However, these are probably very common and might not be worthy of a log entry.

[norihiro]

I tested with my test audio generator (https://github.com/norihiro/obs-asynchronous-audio-source/) and seeing ~20ms silences like waveform below.

Note that above test audio might make something different from the actual issue.

[floele]

Note that above test audio might make something different from the actual issue.

Yes, that is possible. As far as I can see, there isn't really anything wrong with the timestamps when these issues occur, so I would not consider the audio source being the source of the problem.

I had someone make a 9 h test recording without issues a few days ago who previously reproduced the problem reliably, so I think the problem is fixed. Still concerned about breaking anything else with these changes though.

[norihiro]

Reading the code, I'd like to raise possible concerns.

• audio_time is now inconsistent with samples.
• The audio_callback function is expected to be called every 1024 audio sample time (~21 ms for 48kHz) according to the document. This change will break the behavior when audio_callback returns false.
• The loop always waits audio_wait_time so that there is a possibility audio_time is incremented twice.
  If the function input_and_output returns false when the loop is about to happen twice,
  audio_time won't reach cur_time and will cause high-CPU usage until new audio data arrives.

[floele]

* `audio_time` is now inconsistent with `samples`.

I don't think I understand, can you give me an example?

* The `audio_callback` function is expected to be called every 1024 audio sample time (~21 ms for 48kHz) according to [the document](https://obsproject.com/docs/backend-design.html?highlight=audio_callback). This change will break the behavior when `audio_callback` returns `false`.

That is true and I was a bit concerned about that initially, however, os_sleep_ms() is not guaranteed to sleep exactly 21 ms anyway. So if a lack of accuracy was a problem, it should already have been noticed.

* The loop always waits `audio_wait_time` so that there is a possibility `audio_time` is incremented twice.
  If the function `input_and_output` returns `false` when the loop is about to happen twice,
  `audio_time` won't reach `cur_time` and will cause high-CPU usage until new audio data arrives.

Good point, should we just break; the inner loop in that case? Or rather add an additional sleep(), maybe with a smaller duration than in the main loop?

[norihiro]

> * `audio_time` is now inconsistent with `samples`. I don't think I understand, can you give me an example?

Prior to this change, `samples` and `audio_time` are updated at the same time. With this change, only `audio_time` gets updated if `input_and_output` returns false.

> * The `audio_callback` function is expected to be called every 1024 audio sample time (~21 ms for 48kHz) according to [the document](https://obsproject.com/docs/backend-design.html?highlight=audio_callback). This change will break the behavior when `audio_callback` returns `false`. That is true and I was a bit concerned about that initially, however, `os_sleep_ms()` is not guaranteed to sleep *exactly* 21 ms anyway. So if a lack of accuracy was a problem, it should already have been noticed.

The prior code below was calling `input_and_output` at exactly `rate/1024` times per second on average. Even though `os_sleep_ms` is not accurate, the control `while (audio_time <= cur_time)` adjusted the time to call `input_and_output`. ``` while (os_event_try(audio->stop_event) == EAGAIN) { uint64_t cur_time; os_sleep_ms(audio_wait_time); cur_time = os_gettime_ns(); while (audio_time <= cur_time) { samples += AUDIO_OUTPUT_FRAMES; audio_time = start_time + audio_frames_to_ns(rate, samples); input_and_output(audio, audio_time, prev_time); prev_time = audio_time; } } ``` So, my understanding is that the code is equivalent to the code below except less overhead of `os_sleepto_ns`. ``` while (os_event_try(audio->stop_event) == EAGAIN) { os_sleepto_ns(audio_time); samples += AUDIO_OUTPUT_FRAMES; audio_time = start_time + audio_frames_to_ns(rate, samples); input_and_output(audio, audio_time, prev_time); prev_time = audio_time; } ```

> * The loop always waits `audio_wait_time` so that there is a possibility `audio_time` is incremented twice. > If the function `input_and_output` returns `false` when the loop is about to happen twice, > `audio_time` won't reach `cur_time` and will cause high-CPU usage until new audio data arrives. Good point, should we just `break;` the inner loop in that case? Or rather add an additional `sleep()`, maybe with a smaller duration than in the main loop?

I think the problem is how to handle audio when the audio source sends less samples or more samples than the output requires. Unfortunately I don't have a clear vision on how to fix in libobs so far. I'd like to leave further discussion for the initial author of the function, maybe Jim.

[floele]

Prior to this change, samples and audio_time are updated at the same time. With this change, only audio_time gets updated if input_and_output returns false.

OK I see what you mean now. I could change that, but if we're still discussing the general approach to problem, it probably makes sense to wait with further adjustments until we have decided?

Another approach I could imagine is to not use a constant audio_size in audio_callback but instead adjust to what is actually on the buffer. However, since the "insufficient audio data on buffer" issue has already been anticipated in two other methods it seemed reasonable to me to just go along with that.

[floele]

Also one more option: Since we are dealing with two separate issues here, we could also split the PR and merge the first commit if that one is undisputed.

[WizardCM]

Yep that sounds like a logical next step.

[floele]

OK, did that now. First part is #6133
I could remove the first commit from this PR, however, then we would not have a build with both fixes available. Maybe I should wait with doing that until the first PR has been merged?

[plektron89]

Hey there good ppl!

Did this issue get resolved yet? I'm currently recording musical live jams with OBS and get these random 20ms silences like described in this issue every now and then.

[floele]

@plektron89 Not to my knowledge. We had to stop testing because OBS wasn't in a stable state at that point. Probably makes sense to look into that again now to check if at least one of my two proposed fixes helps.

[dojima]

@plektron89
I'm using norihiro's excellent asynchronous audio filter to solve the issue for now. It works wonderfully. Just ensure to have it applied as a filter to every audio source in your scene, even ones that may be muted or otherwise not output to a track.

[plektron89]

thank you both for the quick reply!

@dojima Do you also do musical live streams or recordings with this? I mean sure its one sample being added or removed but when I think about what effort goes into getting the "best" or "truest" sound.. I don't know if I can live with that... :'D I may need to give it a try but I guess I'd rather take the hassle and record into logic at the same time..

[dojima]

@plektron89
I make recordings, but not of the musical variety. Your own ears are the best test, but what I would leave you with is this post by norihiro: #6351 (comment)

Looking at the behavior after 300 seconds have passed, I think the compensation is stable enough. Cumulative probability of the compensation value is shown in the figure below. Its standard deviation is 0.097 S/s. 99% points are within 0.50 S/s range. The minimum is -4.86 S/s, the maximum is -4.19 S/s. Giving a 442 Hz tone, pitch variation will be between 441.9966 Hz and 442.0027 Hz. I don't think even well-trained musicians can notice.

I'm no expert, but I tend to agree that no one could ever possibly notice.

[plektron89]

@dojima Ok I tried it out but I still get those silences.. if anything I think it got more frequent.. :/ I'm on a Mac using the replay buffer. Maybe that's an issue? Or maybe its Ladiocast which I need for routing... gotta try recording that output directly to logic. Hmmm really is a bummer for me.. other than that I really enjoy OBS so far. Hopefully I'll find a solution soon.

[PatTheMav]

I don't fully understand the purpose of these changes here:

• At first the thread waits the normal amount of time and then calls input_and_output to check if the expected number of samples were provided by the audio source
• If that is the case, the timestamps are updated and the thread continues as before
• If that is not the case, the thread waits for half its usual cycle, then increments wait_cycles (should be 1 before the next iteration)
• On the next iteration, the thread waits for half the cycle again, then decrements wait_cycles again (it should be 0 then).

As such it only seems to wait for half a circle to increment and decrement a single variable that otherwise has no impact on the code executed in the loop. Or am I missing something?

[PatTheMav]

I'm not fully clear on which specific issue this PR (and its companion) are trying to fix or if it's just adding code that attempts to "hide" an underlying issue with sources not being able to keep up with OBS timing demands?

I left a comment on part 1 (which scope I understand) but I'm not sure about what behaviour this PR tries to fix? If the device is not providing enough audio samples in time for OBS' audio renderer, there have to be cuts in the audio stream. But maybe I'm missing something here.

[floele]

Hi @PatTheMav ,
my changes were meant to fix the issue though unfortunately the test results were not positive.

I personally believe that the audio devices are not to blame here as recording the same audio source using different software works perfectly fine without glitches. There should in general be sufficient audio samples and it just seems that temporary fluctuations of available audio samples is not handled well in OBS (which my code attempted to change). Additional to a second issue that is basically programmed to fail even if sufficient samples are always available (first commit).

But I'm not an expert in this kind of applications and especially not familiar with the OBS codebase, so I lack some background information and may not entirely understand why it's happening. I can only say that it does happen, and any solution is fine for me. And it's been two years by now so whatever temporary deeper understanding I got while working on this is now certainly gone.

So at this point I assume the most I can do is provide the observations and expectations. And I'm happy to do testing as well. But fixing it properly should probably be left to the project maintainers or whoever knows what they're doing.

[PatTheMav]

I personally believe that the audio devices are not to blame here as recording the same audio source using different software works perfectly fine without glitches. There should in general be sufficient audio samples and it just seems that temporary fluctuations of available audio samples is not handled well in OBS (which my code attempted to change). Additional to a second issue that is basically programmed to fail even if sufficient samples are always available (first commit).

The main issue is that recording is an asynchronous operation, so even if buffers aren't filled in time, the writing of the recording can stall until enough data is available (the provider of data is effectively acting as the clock for the process). But OBS has an equally strong focus on live-streaming which doesn't allow for stalls (data has to arrive in time).

If a device can't fill a buffer in time, OBS still has to produce a stream packet in time and if there's not enough audio data, that packet will then have "empty" data.

One could argue that this system is ill-suited for recordings, but for better or worse OBS doesn't treat both scenarios much differently in its core architecture.

Are there sure-fire steps to reproduce this issue? In my tests I wasn't able to reproduce your numbers because all my audio devices filled the samples in much less time than OBS allows so the timestamps always matched.

[floele]

Yes it is reliably reproducible on machines where it happens. See the linked issue for how I did it. It doesn't even involve an external audio device.

[tt2468]

While chasing a different audio bug, I found that the changes in this file appear to negatively affect A-V sync for some jittery sources. Due to the nature of how difficult it is to reproduce this bug, I am unable to practically investigate any further.