[FR] Fixing audio rate correction on video freq adjust

[vanfanel]

Is your feature request related to a problem? Please describe.
Since "FPS Adjust" causes audio problems because of the lack of proper audio samplerate adaptation, I am trying the other way around: when the emulated Amiga changes to a different video mode (as in

amiberry/src/custom.cpp

Line 7176 in efebb3f

write_log(_T("%s mode%s%s V=%.4fHz H=%0.4fHz (%dx%d+%d) IDX=%d (%s) D=%d RTG=%d/%d\n"),

) the host refresh rate should be changed.

I already have wlroots-based code ready that changes the videomode on wlroots-based Wayland compositors, but now, what's a good place to detect internal emulated Amiga video mode changes in https://github.com/BlitterStudio/amiberry/blob/preview/src/osdep/amiberry_gfx.cpp? (Specially refresh rate changes)

Or maybe no function in https://github.com/BlitterStudio/amiberry/blob/preview/src/osdep/amiberry_gfx.cpp is run when internal Amiga videomode changes are made?

[midwan]

That's probably not a good idea. There is no guarantee the new refresh rate will be supported by the monitor, it may end up just showing an "out of sync" screen, with no easy way to go back.

[vanfanel]

Ah yes, I know what you mean. But for example I have several monitors around that can do near-60Hz modes with no problems at all.
In fact near-60Hz modes are WAY more compatible among modern displays than plain standard 50.000000Hz PAL.
And after all, it would be an option. Even being an option, do you consider it a "dangerous" one, enough to not merge such a PR if I made it? (Of course I will respect your decision in any case, no hard feelings or anything! :D )

Also, the MiSTer does it all the time with the low-latency video sync option, and nobody seems to complain: if it doesn't work ("out of sync!") simply don't use it.

[midwan]

Usually most monitors will handle 60Hz, but going down to 50Hz is not supported on all of them.
If we implement what I understand you're suggesting, it would set the most common screenmodes (PAL) problematic to a large percentage of users, who may not have a 50Hz-capable monitor.

I think it would be best to fix the resampling aspect instead, which can't cause any such issues. Don't you agree?

[vanfanel]

@midwan Ah, yes, people would use a PAL Amiga as default, and would most certainly go into black-screen land automatically. You are right. I am so used to having monitors supporting everything...

Yes, fixing the resampling would be the desired solution. Let's work on that.

Do you have an idea on what the problem is?
I don't remember how audio works in SDL2: I recall (correct me if I'm wrong) that a callback function needs to be registered, which would "consume" audio by being fired periodically when a FIFO vector runs out of samples. Am I right?
If so, where is that done in the code?

[midwan]

SDL2 handles audio in two ways:

• An audio Callback, which runs in the background
• or Pushing data in intervals manually to it

Both are supported in Amiberry, with the options Push/Pull. The audio buffer used is also configurable, and it works best if you use a small buffer with Push, but a setting of around 6 with Pull.

The SDL2 wiki has a lot of details, starting with this: https://wiki.libsdl.org/SDL2/SDL_OpenAudioDevice

All of the code in Amiberry is here: https://github.com/BlitterStudio/amiberry/blob/master/src/sounddep/sound.cpp
I've tried to keep most of the functions very similar to those of WinUAE, to allow merges of updates from there. But of course, WinUAE does not use SDL2, so a lot of things are different (or even unsupported).

[vanfanel]

Ok, I can see that the SDL2 audio callback function is only used when in "pull mode", right?
As in "pull mode" is what I was used to work with when doing SDL2 audio.

How often is the audio callback function called?
Am I right in that it's called when the SDL2-side audio buffer is depleted?

[midwan]

@vanfanel Yes, that's correct. In the other cases, there are places in the code where it will "push" audio chunks accordingly. The handling all happens inside this file, in any case. The distinction is Push or Pull, which is also saved as a setting in the config file.

I only implemented the call to the resampling calculations when using the Push (no callback) option, which works best with a small buffer size. I did a test with setting it to "1" for example, and it seemed to work for me.

[vanfanel]

@midwan Ah! I tried the PUSH mode with latency set to "1", and indeed there are no audio defects, but audio is terribly lagged behind video.
You can see it on Lemmings, where clicking on a "task" the audio effect is instantaneous on a real Amiga or when using PULL mode, but is very delayed on Amiberry with PUSH.
So it seems that either PUSH is not a solution, or there is some problem with it.

[vanfanel]

@midwan Also tried PUSH with latency set to "minimum", and audio latency is still very noticeable.

[vanfanel]

I've also been trying to decrease sd->sndbufsize in open_audio_sdl2() to something like 0x40 (=64) with no noticeable changes.
What's the correct way to push audio more often??

[midwan]

The smaller the buffer, the faster the device needs to be in order to keep up. I've had no trouble with a Pi5, for instance.
Have you tried increasing it a bit?

[vanfanel]

@midwan Increasing sd->sndbufsize in open_audio_sdl2() doesn't have any noticeable effect on the audio delay when using PUSH.

What I need to know is: What determines how often finish_sound_buffer_sdl2_push() is called?

[midwan]

@vanfanel you don't have to increase it in the code, that's what the slider does... Trying with a setting of 2 or 3 for instance, will use a larger buffer. It depends on how fast your device is, but like I said, something along the lines of a Pi4 or Pi5 should be enough to handle most cases with a setting of 1.

The code pushes audio as part of each frame drawn, in custom.cpp.

[vanfanel]

@midwan We are not understanding each other :P
My system is a 12th gen i5, with only ALSA audio.
I can set the audio buffer to "1" and there is zero audio distortion. But the audio is delayed: it takes like half-a-second more to come out of the speakers than when using PULL. That is the problem: audio delay. It's not because of the lack of CPU power at all.

A smaller audio buffer should provide less audio delay. But, using PUSH, it does not.

[midwan]

@vanfanel
Ah, sorry for the confusion. I'll need to run some tests here later on, to see if I can recreate this exact scenario. I'm assuming you have it set to 60Hz, Vsync enabled, anything else? Can you post a .uae config with the specific settings you're using, so I can get as close to that as possible? I can set the monitor refresh rate accordingly first.

[vanfanel]

Here's my config:
Amiga 1200.uae.txt

I use 60Hz (NTSC Amiga), low-latency Vsync enabled.

Also, you said this:
The code pushes audio as part of each frame drawn, in custom.cpp.
Then, could it be that, only when using PUSH, audio latency is related to video latency?

[midwan]

@vanfanel
Low-latency modes are not implemented, I'm not even sure that's supported through SDL2. You should use one of the normal Vsync modes, instead. Not sure if that will make a difference in your case yet.

Both Push and Pull functions will get new audio chunks through the same mechanism in custom.cpp, the difference is how they do that: With Push, we call SDL_QueueAudio, with Pull, there's a memcpy operation, and the callback does the rest.

[vanfanel]

@midwan I recently implemented low-latency video on SDL2+Wayland:
libsdl-org/SDL@9e6b8d5
...But that is controlled via an env variable called SDL_VIDEO_DOUBLE_BUFFER (which I also implemented for the KMS/DRM backend years ago).
So I guess the low-latency vsync doesn't make much sense on the menu on GNU/Linux. It should be a no-op, but I will try normal vsync later and report back.

[midwan]

I can recreate the issue, only seems to happen with Push mode. Testing on a previous release which didn't have the docorrections piece added in there, doesn't show this problem. So, it's a bug that I introduced when trying to resample the audio (probably did it incorrectly somewhere).

[midwan]

I've just reverted that commit, at least until we have a proper implementation that doesn't break things.