sfxmgr: Add functions for vol, pan, freq, loop

[skmp]

This adds snd_sfx_play_chn_ex and snd_sfx_play_ex witch take an extra parameter for loop control, and also adds snd_sfx_volume, snd_sfx_pan, snd_sfx_freq to modify an active sfx.

[andressbarajas]

Looks good. There have been few mentions of looping for sfx but people mostly want to be able to have longer playing sound effects. As for the API breaking, usually what happens is we create a new function and add *_ex to it. So it might be a good idea to keep the current API and add int snd_sfx_play_ex(sfxhnd_t idx, int loop, int vol, int pan); instead. My two cents.

EDIT: Looks like you updated while I commenting to.

[skmp]

Looks good. There have been few mentions of looping for sfx but people mostly want to be able to have longer playing sound effects. As for the API breaking, usually what happens is we create a new function and add *_ex to it. So it might be a good idea to keep the current API and add int snd_sfx_play_ex(sfxhnd_t idx, int loop, int vol, int pan); instead. My two cents.

I just pushed that excact change.

I'll add longer SFX and other stuff for GTA3, so hopefully more PRs to come!

[skmp]

@andressbarajas do I have to add the symbols to exports-naomi.txt and exports-pristine.txt? The other functions are there.

[DC-SWAT]

@andressbarajas do I have to add the symbols to exports-naomi.txt and exports-pristine.txt? The other functions are there.

This is needed for loadable modules (.klf). So yes, it is advisable to do this.

[skmp]

This is needed for loadable modules (.klf). So yes, it is advisable to do this.

Added them, and also modified the description. This is ready for re-review from my side.

[QuzarDC]

Looks great, just have a few open-ended questions especially related to how we might expand the API. None of them are meant to be declarative, but want to understand the use cases better and see if there's a better way to set up the expanded API:

• It seems that the API added works to add setters for the audio_aica_ch_update group of commands from aica_comm.h. Is that something that might bear mention for the documentation? Perhaps someone more familiar with doxygen might have a way to link them together. Not sure what kind of mechanisms would make sense to use to do that (or if it'd even be useful for the docs).
• Is there any reason though that someone would want to change playback frequency on-the-fly? It seems to me that a likely use case would be to play an sfx at a different rate from the start rather than during playback (in order to vary the effect). To that end, would it make sense to add a freq/rate override param to the *_ex functions?
• Along those same lines, then might it not make sense to just expose the whole set of mutable members of aica_channel_t to be adjusted at play? So loop, loopstart, loopend, freq, vol, and pan. If so, then that could be a named type that could be passed in to the _ex functions rather than having the pile of individual parameters. Something like snd_sfx_play_chn_ex(int chn, sfxhnd_t idx, aica_chan_opts_t *opts).
• Since the update command can take freq, vol, and pan all at once, might it make sense to have the three setters backed by a single generic for the update command snd_sfx_update maybe? That way if you want to change vol+pan it doesn't need to be two independent commands being sent. That depends significantly on the use cases though, and if it would ever make sense to be setting more than one of the options at once.
• Considering the previous two together, then you could have the update command take that whole same aica_chan_opts_t and with some updates to the aica's process_chn have the ability to fully control looping on the fly as well as making a lot of the rest a bit cleaner.

Like I had started with, mostly just ruminating on the topic, and would invite feedback or related discussion. If none is forthcoming then this should be fine as-is.

[DC-SWAT]

@QuzarDC

• Is there any reason though that someone would want to change playback frequency on-the-fly? It seems to me that a likely use case would be to play an sfx at a different rate from the start rather than during playback (in order to vary the effect). To that end, would it make sense to add a freq/rate override param to the *_ex functions?

I can confirm that changing the frequency on-the-fly is used in games. For example, to change the frequency of the sound of the engine rotation. Also, the volume and panning can be changed on-the-fly.

[andressbarajas]

Like I had started with, mostly just ruminating on the topic, and would invite feedback or related discussion. If none is forthcoming then this should be fine as-is.

I like this idea. The less commands we have to send to the AICA the better. Here is what I came up with:

#define AICA_CHAN_OPTS_INITIALIZER   { -1, -1, -1 }

typedef struct aica_chan_opts {
    int32_t freq;
    int32_t vol;
    int32_t pan;
} aica_chan_opts_t;

void snd_sfx_update(int chn, aica_chan_opts_t *opts) {
    AICA_CMDSTR_CHANNEL(tmp, cmd, chan);

    cmd->cmd = AICA_CMD_CHAN;
    cmd->timestamp = 0;
    cmd->size = AICA_CMDSTR_CHANNEL_SIZE;
    cmd->cmd_id = chn;
    chan->cmd = AICA_CH_CMD_UPDATE;

    if(opts->freq >= 0) {
        chan->cmd |= AICA_CH_UPDATE_SET_FREQ;
        chan->freq = opts->freq;
    }

    if(opts->vol >= 0) {
    	chan->cmd |= AICA_CH_UPDATE_SET_VOL;
    	chan->vol = opts->vol;
    }

    if(opts->pan >= 0) {
    	chan->cmd |= AICA_CH_UPDATE_SET_PAN;
    	chan->pan = opts->pan;
    }

    snd_sh4_to_aica(tmp, cmd->size);
}

[DC-SWAT]

@andressbarajas

Like I had started with, mostly just ruminating on the topic, and would invite feedback or related discussion. If none is forthcoming then this should be fine as-is.

I like this idea. The less commands we have to send to the AICA the better. Here is what I came up with:

Unfortunately, changes to these parameters most often occur separately in different cases.

[andressbarajas]

Unfortunately, changes to these parameters most often occur separately in different cases.

Okay. Just thought I'd throw that out there since our AICA driver supports it: https://github.com/KallistiOS/KallistiOS/blob/ee50f748af060646ff9d9fddaf02690f596475b6/kernel/arch/dreamcast/sound/arm/main.c#L88

[DC-SWAT]

Unfortunately, changes to these parameters most often occur separately in different cases.

Okay. Just thought I'd throw that out there since our AICA driver supports it:

https://github.com/KallistiOS/KallistiOS/blob/ee50f748af060646ff9d9fddaf02690f596475b6/kernel/arch/dreamcast/sound/arm/main.c#L88

I think this function will not be superfluous. In general, it can replace the rest and use the necessary arguments. These are purely stylistic nuances, since the functionality is needed separately. Although I do not exclude that there may be examples of using several parameters at the same time.

[QuzarDC]

@skmp @DC-SWAT - I believe the update in #903 would satisfy the same use cases here. Could you please take a look?

[DC-SWAT]

@skmp @DC-SWAT - I believe the update in #903 would satisfy the same use cases here. Could you please take a look?

Yes, I think this PR can be closed.