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

Fix audio stuttering when first playing a sound #885

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

o-p-a
Copy link

@o-p-a o-p-a commented Aug 29, 2024

Have you ever had a strange first sound (such as moving the cursor) after starting up ES?
It might not be rumbling, it might sound fuzzing, etc.
I wanted to get rid of that stuttering.


A bit of research, I found seemed like AudioManager wasn't initialized until the moment just before something was played.
AudioManager is initialized just before the first sound is played. and I assumed this issue was caused by the first sound played in an unstable moment.
So I changed it so that initialization takes place before the first sound is played, before the ES main loop.

@o-p-a
Copy link
Author

o-p-a commented Aug 29, 2024

The sound stuttering was fixed, but something suspicious was noticed in the log es_log.txt.
An error was recorded saying that "audio device is already opened".
Why did this happen when init() should have been called only once?

AudioManager is a singleton class, and we call getInstance() to obtain an instance.
Also, initialize/release of resource (SDL AUDIO) are performed in init()/deinit().
I made the corrections with that understanding, but then I got an error that the device is already opened.

In AudioManager, init() is called by ctor, and deinit() is called by dtor.
In other words, the idea seems to be that the lifespan of AudioManager instance is linked to the initialize/release of resources.
In my first commit, I got an instance with getInstance() and called init(), but when an instance is created with the first getInstance(), init() is also called, so it seems to have been called twice, including the subsequent init().
Similarly, in deinit(), the smart pointer sInstance is cleared at the end of function, so the dtor of the instance it was pointing to is called, and within that, deinit() is called again, so deinit() is also called twice (although this is not clear from the log).

So, I added a check to prevent init and deinit from being called twice.

@o-p-a
Copy link
Author

o-p-a commented Aug 29, 2024

I would like to make commits and comments step by step, but I'm not very familiar with git and it's not going well.

In FileData::launchGame, the AudioManager, VolumeControl, and InputManager are deinit()/init() each time the game is launched.
These should also be init()/deinit() when ES starts/ends.

  • AudioManager is as per the previous commit.
  • Is VolumeControl there any intention behind the getsInstance() is different from AudioManager?
    It's tricky, but it doesn't seem to be causing a memory leak, I won't change it.
    For both AudioManager and VolumeControl, I think it would be fine for sInstance to be a simple pointer, like InputManager...
  • Regarding InputManager, when init() is called for the second time, deinit() is called before init() is processed, but if it is already after init(), there is no need to do anything, so change it to simply return.
    In addJoystickByDeviceIndex(), change it so that it does not add duplicates. (It seems that joysticks already added by init() are notified SDL_JOYDEVICEADDED again.)
    Also, the static member mInstance should be named sInstance.

@o-p-a
Copy link
Author

o-p-a commented Aug 29, 2024

I think there is room for improvement in the VolumeControl.
First, a copy constructor and an assignment operator are defined, but they're never used and the logic doesn't make sense.
Delete them.

Next, originalVolume and internalVolume are assigned but not referenced.
Let's delete this too.

@o-p-a
Copy link
Author

o-p-a commented Aug 29, 2024

Continuing with VolumeControl.

On Linux, debug messages are output during initialization. (with --debug)
On Windows, debug messages are also output.

On Windows (Vista and later), the current volume value is output to the log every time getVolume() is called. Change this to a debug message.

Calling GetVersionEx() to determine the Windows version can be made simpler.

@pjft
Copy link
Collaborator

pjft commented Aug 29, 2024

Hm.

I'm pretty sure that I looked into AudioManager when the Pi5 came out, as I was having some issues there, and after spending some time here my takeaways were "there's a good reason for this to work the way it does" and "I'm not going to touch it". I did not touch VolumeControl, so I cannot comment on that. I do wonder, however, if we do need to touch VolumeControl - is something not working there? I normally try to follow the "Chesterton's fence" thought process, whereby if I don't really understand why something is the way it is, I don't try to change it.

Alas, it was a long time ago, but let me try to shed some more light into some of the things I seem to recall, for you to at least test.

First, SDL Audio isn't great - when something takes over the Audio, it'll exclusively own that resource. This means that, if you don't init/deinit things you cannot have theme audio AND video audio coexisting. Audio used to work differently before the Pi5 (I'm assuming related to the latest Raspberry Pi OS, rather than hardware) and on the Pi5, so any meaningful change here would need to be tested on both Pi3/4 and Pi5. Have you checked if you get audio from videos (VLC and OMX Player if on a Pi3/4) and from a theme with your changes?

Second, even before all these changes, then, I'd challenge some potentially simpler change since, per your comments:

  • the stuttering happens only on the first sound being played, and
  • every single sound playback does, in fact, the same init/deinit

So the init/deinit in itself cannot be the problem. Did you try just doing a fake init/deinit at the beginning of ES seeing if it changes the first playback, which is effectively what you're trying to address, without trying to change the entire audio stack?

Just a thought.

I'll be honest, but unless this is thoroughly tested across multiple setups, I will be very hesitant to merge changes to sensitive code that has been around for the better part of 12 years, without any meaningful issue, as this is an area of the code I have (other than my failed experience) no meaningful knowledge of, and I'd have a hard time making sure it keeps working on all types of OS's it does.

Hope this helps, and do keep us posted.

@o-p-a o-p-a marked this pull request as ready for review August 29, 2024 08:54
@o-p-a
Copy link
Author

o-p-a commented Aug 29, 2024

@pjft Thank you for your quick comment.
It's true that I can only test in a limited environment. (I usually test on Pi5 and Windows 11.)
I would like to test it in many environments, both by myself and with the help of the forum.
There are many other things I would like to try other than this PR.
I would like to respect the comments of maintainers.

@pjft
Copy link
Collaborator

pjft commented Aug 29, 2024

By all means, keep exploring this and do try out the suggestion I made first as well, as well as the edge cases I asked about and report back.

I agree that the forums can be a good source for testing. Thanks for looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants