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

[WIP / RFC] New audio backend API #79

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

Conversation

bsmiles32
Copy link
Member

This PR introduces a new "audio backend API" which should replace (when mature) the existing audio plugin mecanism.

Frontends that want to use this API should call the CoreSetAudioInterfaceBackend function (before starting emulation)
with a valid pointer to a m64_audio_backend structure.
See documentation for explanation of this structure and intended usage.

For now, usage of the API is optionnal, and therefore frontends are not required to use it.
In this case the core will use the audio plugin to emit sound.
However we encourage frontends writers to consider this new API over the audio plugin mecanism.

Feedback
@richard42 :
How should I reflect version change in the core (ie which number to bump if any) ?
In case I need to change this API later (for instance when we implement a fully accurate the AI) what would be the best course of action ?
Is my documentation good enough ? I'm not a native english speaker, so if you find mistakes please let me know so that I correct them.

@ frontend writers :
Would this API be convenient to work with ?

@littleguy77
Copy link
Member

Quick question - is it possible to use the new backend API while still using ui-console? I'm guessing sooner or later mupen64plus-ae would stop using ui-console, but just wondering if there are easy ways to test in the transition.

@bsmiles32
Copy link
Member Author

For now ui console is not updated to use this new API, but I'm working on it :)

@@ -439,7 +440,7 @@ SOURCE = \
$(SRCDIR)/pi/pi_controller.c \
$(SRCDIR)/pi/sram.c \
$(SRCDIR)/plugin/emulate_game_controller_via_input_plugin.c \
$(SRCDIR)/plugin/emulate_speaker_via_audio_plugin.c \
$(SRCDIR)/plugin/audio_backend_compat.c \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it silly to have a distinct folder plugin and backend? As the new backend approach is supposed to take away the concept of plugin, I just ask.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the audio_backend_compat in the plugin folder because it is related to plugins, and will also go away when plugins are dropped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Sorry for the bad idea. ^^'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about asking questions even if you think they are "bad idea".
I'm am not perfect and I make mistake very often, so I'm glad someone look at my work and question it. That way if I have overlooked something I can correct it. Otherwise if it's not a mistake on my part, I can justify why I did it that way, so it helps other to understand my work.

@bsmiles32
Copy link
Member Author

I was thinking about how I woud handle other backends (game_controller input, rumble, ...) and I think it is not good if I have to add a specific library function for each different backends, even more if they all just take a pointer to a backend as parameter.
So maybe I should think a ittle bit more general and do the API like this :

CoreSetBackend(enum backend_type type, void* backend_ptr);

enum backend_type
{
BACKEND_AI_AUDIO,
BACKEND_P1_GAME_CONTROLLER,
BACKEND_P1_RUMBLE,
BACKEND_P2_GAME_CONTROLLER,
BACKEND_P2_RUMBLE,
...
}

For BACKEND_AI_AUDIO, the void* would be expected to be a pointer to a struct m64p_audio_backend, for BACKEND_Px_GAME_CONTROLLER it would be struct m64p_gc_backend, and so on.

This way, I can add later other backends easily : just add a m64p_xxxx_backend struct and a BACKEND_XXXX entry in the backend_type enum.

Plus for frontends it would be easier if we present them a unified way of setting their backends.

What do you think ?

Edit: It seems that given the prototype of CoreSetBackend, I could just extend the CoreDoCommand API with new commands (SET_AI_AUDIO_BACKEND, SET_P1_GAME_CONTROLLER, ...) instead of adding another function.

@littleguy77
Copy link
Member

Obviously not as typesafe, but much easier to maintain/expand during all the refactorings. Sounds good to me. We can always make typesafe CoreSetFooBackend() later once the dust settles, if there ever becomes a need.

Interesting that you allow different backends for each player's controller, but now that I think of it you might as well. Cool...

@littleguy77
Copy link
Member

Quick question - is it possible to use the new backend API while still using ui-console? I'm guessing sooner or later mupen64plus-ae would stop using ui-console, but just wondering if there are easy ways to test in the transition.

For now ui console is not updated to use this new API, but I'm working on it :)

Come to think of it, I might as well start implementing a native front-end for mupen64plus-ae now. I would basically implement it from the ground up with barebones capability (no cheats, no fancy options, etc.) to test the new backend APIs as they come.

@bsmiles32 @richard42 Rather than (or in addition to?) tracking the core API evolution in mupen64plus-ui-console, perhaps we should setup a skeleton front-end in a new repository (say mupen64plus-ui-dummy, -ui-template, -ui-example, ui-v3api, or whatever), which contains just a bare-bones, stubbed out implementation of a new front-end. It would start mostly as method stubs and pseudo-code comments. It doesn't need to have makefiles or even build. As the new API emerges, it can start to appear in the example ui in concrete form. Give @bsmiles32 direct write access to the repository, so he can update it immediately when he has new PR's in mupen64plus-core (to provide more context for the PR). Downstream/front-end devs can submit PR's and issues directly to the skeleton project and it would be a good way to consolidate (and trace) the design discussion and decisions (I fear a lot of this useful discussion will be forgotten after the PR's close). We could also use the wiki to document things as they solidify, without polluting the core's wiki. Thoughts?

@inactive123
Copy link
Contributor

The libretro port now uses this new audio interface.

libretro/mupen64plus-libretro@f000eeb

Nice thing about this is that I now no longer have to go through the Zilmar audio plugin function pointers, so this new audio API aligns pretty well with what is needed for making the libretro port more upstream friendly.

I made a custom audio backend for libretro and moved all of the audio code that libretro needs (including the audio resamplers) to mupen64plus-core/src/plugin/audio_libretro. We can later look at starting to move parts of libretro's code upstream piece by piece.

@bsmiles32
Copy link
Member Author

@twinaphex : The porting you did is good, but I would have put the libretro audio backend in the libretro folder instead of core/plugins.

edit: You can also get rid of:
set_audio_format_via_audio_plugin, push_audio_samples_viaaudio_plugin in the libretro audio backend.

|'''<tt>backend</tt>''' Pointer to a structure (defined in [[Mupen64Plus v2.0 headers#m64p_types.h|m64p_types.h]]) containing pointers to the Front-end's audio backend functions.
|-
|Requirements
|The Mupen64Plus library must already be intialized before calling this function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialized (intialized)

@Nebuleon
Copy link
Contributor

In the documentation, the Usage cell's formatting is pretty bad, and the Prototype cell is merged with other markup. See http://www.mediawiki.org/wiki/Help:Tables for more on how to format MediaWiki tables.

@richard42
Copy link
Member

@bsmiles32 : I would bump the minor number of the FRONTEND_API_VERSION returned by the core's PluginGetVersion function. Also, you can add a version number to the m64p_audio_backend struct. This will allow future changes.

@bsmiles32
Copy link
Member Author

Okay, I fixed some documentation errors. Thanks for the corrections :)
Reminder for the future merger : please squash "fixup" commits before merging (or ask me to fixup/rebase stuff before merging).

@richard42 : for frontend API version you mean 2.1.2 or 2.2.0 ?
I find redundant the need to put a version inside audio_backend struct, because we already have a version in the frontend API which audio backend is part of. Plus if we go that route, we would have to do the same for all other future backends (game_controller, rumble, ...). If we rely on the frontend API version only, we don't have to version each backends separately.

In an earlier comment I proposed a more generic backend setup approach which would be suitable to use for the game_controller and rumble backends for instance. I either proposed :

  • to generalize backend setup with a CoreSetBackend(enum backend_type, void* backend_ptr) function
  • or to extend the CoreDoCommand with new commands (SET_AI_AUDIO_BACKEND, ...) [I kind of prefer that option personally]

What's your opinion on that ?

@richard42
Copy link
Member

The frontend api version number could go up to either 2.1.2 or 2.2.0; it's a matter of personal choice. You are right that it is redundant to have a version number in the audio_backend struct, but there is an advantage to having more granular versioning in this way. As an example, the function pointer struct for the video extension API contains a "number of functions" parameter which serves this same purpose. Putting a version number in this struct allows the code in the core and frontend which handle this audio backend feature to deal with compatibility using only this single struct; they don't need access to the API version info which may be in a different part of the code. Also, it would be easier in the future to extend the api by bumping up the version number in the struct, rather than bumping up the API version number and dealing with everything that is required for that. In the end it will work either way; it's sort of a matter of personal preference, but I think it would be less work overall to put a version # in the struct.

Regarding the more generic backend setup, I think either option is fine. I kind of prefer the 2nd option (through CoreDoCommand) as well

@bsmiles32
Copy link
Member Author

updated:

  • uses the DoCoreCommand function instead of a specific function
  • bumped frontend version number
  • added a version check at backend setting
  • updated documentation accordingly
  • splitted the original commit into 1 for m64p_audio_backend introduction, and one for interfacing it with the frontend API.

@loganmc10
Copy link
Member

@bsmiles32 What ever happened with this?

@ghost
Copy link

ghost commented Jan 26, 2019

What advantages does this API offers over the current one if there are any? Does it implies at performance improvements? if so, I'll be supporting this API, otherwise if it brings performance decrease instead, I won't be supporting this API. (If it doesn't brings any performance changes, I am fine too).

@richard42
Copy link
Member

The API change for the audio plugin/backend should not have a performance impact. The primary advantages of the new API will be: better integration with the front-end (for non-SDL platforms), and proper integration with the speed throttling mechanism for smoother frame rates and no audio drop-outs.

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.

None yet

8 participants