libretro-mirrors / libretro-arb

For proposed improvements to libretro API.
8 stars 2 forks source link

Remove the retro_audio_sample_t callback #10

Closed Gu1 closed 9 years ago

Gu1 commented 9 years ago

Currently, a core can pass audio sample to the frontend by two different callbacks:

/* Renders a single audio frame. Should only be used if implementation
* generates a single sample at a time.
* Format is signed 16-bit native endian.
*/
typedef void (*retro_audio_sample_t)(int16_t left, int16_t right);

and

/* Renders multiple audio frames in one go.
*
* One frame is defined as a sample of left and right channels, interleaved.
* I.e. int16_t buf[4] = { l, r, l, r }; would be 2 frames.
* Only one of the audio callbacks must ever be used.
*/
typedef size_t (*retro_audio_sample_batch_t)(const int16_t *data,
      size_t frames);

I propose that the first one be removed from future versions. It is inefficient (generating tens of thousands of function calls per seconds), redundant (since the same behavior can be obtained using the batch callback), and not very flexible since it wouldn't work with anything other than stereo (if support for it was added at some later point).

Alcaro commented 9 years ago

We could create a new function through an env if we want anything other than stereo, but that doesn't invalidate the other arguments. Disruption caused by this will be minimal; as far as I know, everything except some bsnes forks uses the batch callback, and we converted those a few days ago. So yes, this should be removed.

Though I do want the batch callback better documented. Especially the return value; you have to read the source code of something to know what it does, and the limit at which the core has to worry about return!=frames is even more well hidden. (Nor do I know why the target can't just block and consume all samples. The core will probably repeat the call in one form or another until the entire buffer is consumed, anyways.)

Themaister commented 9 years ago

As long as the cores cache up blocks of audio before calling audio_sample_batch, audio_sample_t is considered deprecated.

The problem with having the frontend cache sample_batch_t is causing needless and ugly caching code in two places, but yes, it's an API wart to have two different ways of pushing data. The size_t return on sample_batch_t was a premature optimization to avoid a while loop in frontend to flush out all data. Very few cores actually care about the return value because it usually just works.

Alcaro commented 9 years ago

Reliance on "usually just works" is exactly why Microsoft has to spend so much effort on keeping things working, and "premature optimization" and "to avoid a while loop in the frontend" both sound like legacy cruft we'd rather not keep. There should already be while loops in the audio drivers (otherwise small writes could blow up), I see no reason why we can't reuse them. RetroArch has never cached sample_batch_t. There is a cache for sample_t, but batch_t skips it and jumps directly to the audio driver. (Not sure about the other fronts, but I'd suspect they do the same.) I still want that function to guarantee it'll take everything, and the return value replaced with void. It makes life easier for everyone.