PulseAudio: Remove get_latency() caching

[Birdulon]

Removes the caching on AudioDriverPulseAudio::get_latency() and slightly renames variable name palat to pa_lat to slightly improve readability.

Rationale

As mentioned over at #38215 (comment) the API call pa_stream_get_latency returns different values at different times. Being able to sample it only once per program execution is a severe handicap when doing any profiling or using it for things such as latency compensation in rhythm games, as it can be wildly out if sampled under some load conditions.

As a somewhat hyperbolic analogy - what if you could only measure frame time once per program execution? What impact would that have on debugging and optimising your game?
Personally, I was about ready to tear apart my initialization routines when I noticed that my output latency number went from ~50ms to ~30ms on runs where I had breakpoints in initialization. Had I not investigated the engine source code I would have wasted considerable time chasing a dead end because of this cached value under the impression that Pulse was giving me conservative settings under certain start conditions!

Knock-on effects

Most of my testing has been on the 3.2 branch (Birdulon/godot/PAlatency) and all I've done on master is verify that it compiles and prints sensible values in a minimal project as I can't port my actual game to master due to unimplemented mesh rendering etc. (made this branch entirely because the default PR text told me to!)
In 3.2 I've observed no degraded performance with the Debugger's Monitors showing Audio Output Latency continuously during gameplay, however I noticed small bits of lag when I also had a label calling it 60 times a second in addition to the monitor. The documentation might need a small note to reflect that AudioServer.get_output_latency() and by extension Performance.get_monitor(Performance.AUDIO_OUTPUT_LATENCY) are potentially costly calls that should be used sparingly.
In master none of the performance monitors seem to draw for me, but I confirmed behavior with a label changing in _process.
I stress again that it should be preferable for this function to return what it says it does, even at cost, even if approximate, rather than lie to the programmer for the sake of saving cycles.

[Calinou]

The documentation might need a small note to reflect that AudioServer.get_output_latency() and by extension Performance.get_monitor(Performance.AUDIO_OUTPUT_LATENCY) are potentially costly calls that should be used sparingly.

Could you do that in this pull request (in the same commit)? See this page for instructions on updating the class reference.

[Birdulon]

Thanks for that, for some reason I'd assumed it would be a separate issue in the godot-docs repo! Added.

[Birdulon]

Is there anything else that should be addressed?

[akien-mga]

Is there anything else that should be addressed?

The PR is fine but it needs to be reviewed by a contributor who knows about PulseAudio and/or general constraints on what can and can't be done in AudioDriver (likely @reduz for the latter).

[ellenhp]

I like this change, but could we get some performance numbers? How long does this hold the global audio server lock for? Because people will 100% call this every frame and expect it to work. If it takes a millisecond or something I can live with that, but if it's holding the global audio server lock for 10 milliseconds that's not okay because it'll most likely cause buffer underruns.

Also do you have an idea of how much variation there is in the values returned by this function? If it's +/- 5% or something I'm not really convinced it's necessary to remove caching, but if there are latency spikes occasionally this might be a good change to merge.

[Birdulon]

I like this change, but could we get some performance numbers? How long does this hold the global audio server lock for? Because people will 100% call this every frame and expect it to work. If it takes a millisecond or something I can live with that, but if it's holding the global audio server lock for 10 milliseconds that's not okay because it'll most likely cause buffer underruns.

I haven't run numbers on it, but I was able to play my rhythm game with the performance monitor charting it constantly on the 3.2 PR, without any noticeable audio issues.

Also do you have an idea of how much variation there is in the values returned by this function? If it's +/- 5% or something I'm not really convinced it's necessary to remove caching, but if there are latency spikes occasionally this might be a good change to merge.

As mentioned in the linked comment, it can be pretty severe. I've even seen it go over 100ms during initialisation, likely tied to whatever is happening on the frame it is called.

[ellenhp]

I don't really think that ripple is worth tracking perfectly, especially because I suspect it's caused by the time to the next mix being different each time get_latency is called. That said, the spike there could cause serious problems. What if the value that we cache in the audio driver is way higher than the average latency because we got unlucky and cached an abnormally high value? We'd just have to wait until the game restarts I guess? Not good.

One thing I'd like to ask you to change is to continue caching the result of the call how we were before, but to clear the cache every time audio is mixed. That way we can continue to track the overall latency trend and give fresh data back, while providing an upper bound on the number of times we do this per frame. Latency isn't likely to change a huge amount between mixes, since unless I'm misunderstanding what it measures, it can't change any faster than time passes.

Also: Make sure to only ever read from or write to the cached value while the global audio server lock is held. We have enough intermittent threading issues as it is, I don't want to introduce more.

Thank you for looking into this by the way! The audio drivers are not my area of expertise so I appreciate people taking the time to contribute patches back to them.

---

edit: This has a double-benefit of giving rhythm game developers extremely fast tracking of latency information since they almost always turn the audio latency wayyy down, so the interval between audio mixes is extremely small. Let me know if you think it's a problem, but unless I'm missing something it seems like a nicer solution that completely eliminating latency caching.

[Crystalwarrior]

Any updates on this? We ran into this issue with just trying to replicate Ace Attorney blips which we expected to be extremely basic but is giving us hell. The reference video we're using for replicating that is https://www.youtube.com/watch?v=Min0hkwO43g
And in Godot the audio playback ended up being completely random and hard to figure out the culprit other than determining it must be audio buffer and inconsistent latency. We also encountered the issue of being unable to use the provided functions to introduce delay which further makes it impossible to develop any kind of sound-precision on even the basic audio presentation level.

[Calinou]

@Crystalwarrior Please don't copy-paste comments across issues, as this makes the discussion more difficult to track for others.

I recommend testing this pull request locally and making it sure it actually fixes the issue for you.

[YuriSizov]

Seems like this is generally positively received, so would be nice to finish it (rebase, review, approve) for 4.1 :)

[ellenhp]

Code looks good and I do think this is a good idea because it gives users more flexibility to design their own latency compensation systems, which is something we don't currently support.

[ellenhp]

I will note that caching and clearing the cache every mix is still something that I think could be an improvement here because it would be nice if users could call this several times back-to-back without consequences, instead of having to call it once and cache it themselves. Not sure how beneficial that would be and it's a fair amount of extra work. It should be fine to ship this without that.

[akien-mga]

Looks fine to me, but needs a rebase and a fix to the documentation.

[akien-mga]

Sorry I should have mentioned this earlier too, could you squash the commits together? The doc change is directly required by the removal of caching so it makes sense to so both in the same commit.

[Birdulon]

Squashed

[Birdulon]

scons target=editor tests=yes dev_build=yes scu_build=yes precision=double use_asan=yes use_ubsan=yes linker=gold succeeded on my machine (linux x86-64), is there an intermittent build failure on CI?

[akien-mga]

It fails after the build when running a test project. That's a known intermittent issue indeed. I restarted the CI job.

[akien-mga]

Thanks! And apologizes for the huge delay reviewing and merging this. Thanks for sticking around :)