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

Redesign audio core to return a locked AudioPlayer object instead of having a multitude of helper functions #2344

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Mar 2, 2024

The "audio core" system was implemented in 3.6.0, as a part of moving from Allegro 4 towards SDL-based utilities and a more convenient audio thread.

Although the original idea of "audio core" interface was to have everything behind the set of functions, in practice it proved to be rather annoying, as we had to have a duplicate function per each "audio player" action. This creates a mostly redundant layer of complexity.
But besides that, there's a technical issue: each "audio core" function's call locks the mutex, and sometimes we need to do several things at once. This means either call several functions with a lock/unlock pair in a succession, or write more cumbersome helper functions that do a number of things under one lock/unlock pair. If calling several functions, the state of the audio player may actually change in between. (In fact, it may even get disposed in between, although this probably never happened in practice because of how our synchronization is organized.)

The new design is inspired by the older "Channel Lock" existing in versions 3.5.0-3.5.1 (e.g. see this code), and follows this outline:

  1. AudioCoreSlot is picked out to a separate code unit and renamed to AudioPlayer.
  2. Majority of audio core's playback control and state reading functions are REMOVED.
  3. Instead, there's ONE function called audio_core_get_player() that returns a AudioPlayer's pointer wrapped into a AudioPlayerLock class, which holds a mutex lock, and unlocks on destruction. AudioPlayerLock class lets user to work with AudioPlayer, doing any manipulation necessary, and unlocks the provided mutex as soon as it gets out of scope.

@ivan-mogilko ivan-mogilko added context: audio context: code fixing/improving existing code: refactor, optimise, tidy... labels Mar 2, 2024
@ivan-mogilko ivan-mogilko force-pushed the 362--audiocoreexposeobject branch from 1e2d005 to 1e4fb99 Compare March 2, 2024 09:02
I am not sure why is it there, and whether this is actually needed. Possibly it was to make volume levels roughly equivalent to Allegro implementation?
This makes things bit inconsistent, and also not clear at which stage in all our audio interfaces this should be applied.
@ivan-mogilko ivan-mogilko force-pushed the 362--audiocoreexposeobject branch from 1e4fb99 to 5807291 Compare March 2, 2024 09:06
@ericoporto
Copy link
Member

ericoporto commented Mar 2, 2024

Hey, this looks similar to the video player implementation in #2338 but ported to audio. Is this the idea? Can the new video player internals also be backported even if keeping the script API the same as is now in 3.6.x?

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Mar 2, 2024

Hey, this looks similar to the video player implementation in #2338 but ported to audio. Is this the idea?

No, not at all. There have already been "audio player", only called differently (AudioCoreSlot). I only renamed it.
On contrary, in #2338 I was redoing video to look like the audio system; and I'd like to do changes similar to this PR for video too, either in #2338 or after that.

The idea is that currently each action on audio player has to come through extra layer of functions, like:

audio_core_slot_pause(slot_);
audio_core_slot_seek_ms(slot_, (float)pos_ms);
float posms_f;
audio_core_slot_get_play_state(slot_, posms_f);

each of these functions inside do 2 things:

  • lock the mutex
  • call the real method from "audio player".

This PR replaces this with direct access to "audio player":

player = audio_core_get_player(slot_);
player->Pause();
player->Seek((float)pos_ms);
float posms_f = player->GetPositionMs();

This solves 2 issues: removes extra functions in between (now it looks like a object-oriented code too), and reduces number of mutex locks/unlocks.

Can the new video player internals also be backported even if keeping the script API the same as is now in 3.6.x?

Maybe some things could be backported; but I would not like to backport whole "video core" thing, because there may be only one active player at a time with old script API. But I guess it's worth adding a video thread there, for performance,

@ivan-mogilko ivan-mogilko force-pushed the 362--audiocoreexposeobject branch 2 times, most recently from 060851f to c818b98 Compare March 4, 2024 12:55
Although the original idea of "audio core" interface was to have everything behind the set of functions, in practice it proved to be rather annoying, as we had to have a duplicate function per each "audio player" action. This increases amount of necessary code and creates a mostly redundant layer of complexity.
Besides that, there's a technical issue: each "audio core" function's call locks the mutex, and sometimes we need to do several things at once. This means either call several functions with a lock/unlock pair in a succession, or write more cumbersome helper functions that do a number of things under one lock/unlock pair. If calling several functions, the state of the audio player may actually change in between. (In fact, it may even get disposed in between, although this probably never happened in practice because of how our synchronization is organized.)

The new design follows this outline:
1. AudioCoreSlot is picked out to a separate code unit and renamed to AudioPlayer.
2. Majority of audio core's playback control and state reading functions are REMOVED.
3. Instead, there's ONE function called audio_core_get_player() that returns a AudioPlayer's pointer wrapped into a AudioPlayerLock class, which holds a mutex lock, and unlocks on destruction. AudioPlayerLock class lets user to work with AudioPlayer, doing any manipulation necessary, and unlocks the provided mutex as soon as it gets out of scope.
@ivan-mogilko ivan-mogilko force-pushed the 362--audiocoreexposeobject branch from c818b98 to ac509b8 Compare March 4, 2024 21:42
@ivan-mogilko ivan-mogilko merged commit a87e840 into adventuregamestudio:master Mar 5, 2024
20 checks passed
@ivan-mogilko ivan-mogilko deleted the 362--audiocoreexposeobject branch March 5, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: audio context: code fixing/improving existing code: refactor, optimise, tidy...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants