SDL Audio support
Closed this issue · 29 comments
I feel like GFM, being a library to ease game / multimedia application creation, should provide OOP wrappers to ease audio playback.
I intend to contribute that, but I need at least one question answered first.
Are there any guidelines the implementation should follow?
e.g. how separate components are opened/closed, how memory is handled etc.
Thanks for getting in touch before-hand!
You can find inspiration in the SDL_mixer wrapper for a SDL satellite library, or any wrapped SDL object for vanilla SDL.
The guidelines are more or less the following:
- the wrapper does almost nothing. We don't create abstractions that aren't very necessary. An exception is eg
SDL2Keyboard
and this was probably a mistake. At one point the events were passed to windows using virtual functions, but this was worse than using SDL event polling directly, so I removed it. - every error code is checked and turned into an Exception.
- GFM try to follow Phobos style guide: https://dlang.org/dstyle.html
- I often use classes for resources, they have then to follow the "GC-proof resource class idiom" http://p0nce.github.io/d-idioms/#GC-proof-resource-class
- an implicit design rule is that each resource has its wrapper class.
Progress so far: https://github.com/clinei/gfm/tree/861cbeb35fdb1550a0541cd4db5d7991f3321277
Note that I'm going to flesh out the API before making it GC-proof.
Should there be a Mixer
class that makes using AudioChunk
with AudioDevice
easier?
I could remove the mixing capability and rename it to Player
, if not.
I'll review in ~4 hours, this will take some time.
Of course, this is far from done, anyway.
First, thank you for asking for input way before commiting too much energy into a design.
General:
- do not define D counterparts to SDL constant:
For example use the existing and documented SDL_AUDIO_STOPPED, SDL_AUDIO_PLAYING and SDL_AUDIO_PAUSED. The problem with defining enum counterparts to C enum is:- something more to learn for the user
- the original documentation speaks with original names
- new names for same things add very little value
- consequently:
- SDL2AudioDeviceType must die
- SDL2PauseState must die and be replaced with a bool
- SDL2AudioDeviceStatus must die. You get the picture :)
- don't use Uint8 / Uint32 but the corresponding D types
Rationale: those types are always a defined size anywhere SDL is used, and also D types.
So there is no need to use these names.
About SDL2AudioDevice:
- no need for name() and type() getters. Please remove _name and _type fields too. They will be added only if needed.
- remove all
@property
keywords. This is a deprecated keyword with a sad story that will be deprecated soon. - please merge open() and constructor. This is very important, this bring the guarantee that once built a resource correspond to something.
close()
can simply be renamed to~this()
then.- the SDL2AudioDevice.open function should look like SDL_OpenAudioDevice and take name and type in its signature.
- no logic in SDL2AudioDevice.play() and SDL2AudioDevice.pause(). Wrapper functions should be able to use the documentation of SDL. You should just call the SDL function everytime, it doesn't add much value to have a different behaviour.
- please add DDoc comments linking to the original documentation. You can see how it's done in other SDL2 wrappers like SDL2Renderer
- please remove opened() getter . If it's not opened, we should have thrown in the constructor and would never arrive anywhere it's used. Since you will have merged constructor and open() anyway.
Other than that I like it :)
About SDL2AudioSettings:
- It must disappear too I'm sorry. Just pass a SDL_AudioSpec.
It's OK to pass SDL_AudioSpec around since it hasn't associated functions in SDL, and it's not a resource.
About SDL2AudioMixer:
This is recreating the work in SDL_mixer whatever we do.
I'd much, much prefer if you wrap SDL_mixer instead like SDL_ttf was instead of rewriting it (I thought a SDL_mixer wrapper was already in but it's not). Sorry if it sounds harsh, but the philosophy of GFM is often not to do anything more than the underlying library. The problem is that any divergence has to be documented.
I've made remarks anyway but be aware this probably won't be merged because of the above reasoning unless you can change my mind:
- you rewrite the settings.callback member, something you could do is save the previous member value and call it in your callback.
- because SDL2AudioDevice is now always opened, no need to open it in SDL2AudioSettings.play()
- make your callback @nogc is possible
- don't log messages in the audio callback, since throwing allocates with
new
- SDL2AudioSettings constructor should not take settings, those settings were passed to SDL2AudioDevice constructor already. It shouldn't have a _settings field.
Unless it replaces SDL2AudioDevice completely. But as it is now the redundancy of settings is odd. - mixInserts() doesn't track the current time
It should mix as much as the callback gets passed a long buffer, not as much as the arrays in inserts can fit in master!
What you should do is for insert to keep track of where they are in playback,
Then an API to add an insert would be playSound(audio[], format) and that would add a sound to be played back to a fixed number of "channels". A channel would be ( current sample to play + current offset + desired volume). The problem with an unbounded list is that you cannot remove an item of a list from within a @nogc callback.
About SDL2AudioChunk:
SDL2AudioChunk is too much like a D slice, it must die and be replaced by D slices. Unless it gets WAV loading capabilities and also holds a format. I think it shouldn't hold a volume.
The master buffer should just be a slice too.
Thanks for the extensive reply :)
I'll follow the guidelines in totality (including mixing).
If a convention guide for contributors doesn't exist yet, generalizing and putting the information on this issue somewhere accessible would help future contributors write conformant code sooner.
If a convention guide for contributors doesn't exist yet, generalizing and putting the information on this issue somewhere accessible would help future contributors write conformant code sooner.
Absolutely, sorry that it wasn't existing. This PR allowed me to verbalize some of it.
replaced by D slices
Do you mean ranges or slices?
I mean slices like eg. short[]
. Also called dynamic arrays (dynamic arrays are slices)
What will you do regarding mixing? Wrapping SDL_mixer or continue your implementation?
What will you do regarding mixing?
Wrapping SDL_mixer.
Great! Looking forward to it.
One last question (for now):
Can I move all variable declarations to the top of the class?
Sure, wherever you want.
I have some horrible D1 code that wraps SDL_mixer here: https://github.com/p0nce/Vibrant/blob/master/source/common2/sdl/mixer.d :| if it ever helps. (edit: oh god it's embarrassing code)
if it ever helps
Every little bit helps :)
the SDL2AudioDevice.open function should look like SDL_OpenAudioDevice
Does that mean the name should be passed as const(char)*
?
Passing it as string
or const(char)[]
and doing toStringz
inside AudioDevice.this
might be more user-friendly.
string is OK
Small update: https://github.com/clinei/gfm/blob/ca65cfd5d951b3136397d69ea6f5c3fe7a46036d/sdl2/gfm/sdl2/audio.d
Cut actual code down to ~60 lines.
Nice. I think the local imports can be transformed into a top-level one:
import derelict.sdl2.sdl;
Yeah, having the imports local doesn't make much sense, in this case.
Moving them top-level.
P.S. Can imports like toStringz
remain local?
I'll wrap SDL_mixer next, hopefully in the coming week.
Actually, I want to mix audio myself.
Since SDL_mixer seems to do it for you and GFM is too lightweight to implement it directly, I'll create a separate library that depends on GFM.
I plan on eventually creating a DAW with D.
Would there be any interest from you or the D community in that regard?
EDIT: made a post in General
about it: http://forum.dlang.org/post/lbhdvnhhygfeqfvqefgs@forum.dlang.org
A DAW in D would certainly be interesting. But I'm doing plugins the whole time, I can't commit to another project. DAW seem a tought market to be in.
I made a start of a VST host class here: https://github.com/p0nce/dplug/blob/master/host/dplug/host/vst.d
Example of usage: https://github.com/p0nce/dplug/blob/master/tools/process/source/main.d#L140
Lack lots of features but could help you a bit.
I made a start of a VST host
Yeah, I plan on integrating and contributing to dplug.
The shared library support/docs of D could be a bit better, but it will take a long time for that to be needed.
Do you have any email? (found it)
tanel58 at hotmail dewt com
I'm trying to implement loading WAV into a D array, however I ran into a problem.
SDL_LoadWAV
expects a pointer to the first element of a ubyte
array and a pointer to a separate length property.
Is there any way to make the aforementioned function fill the buffer cleanly and without multiple calls?
EDIT: Nevermind, found a way using pointer[0..len]
.
Yes that's exactly how you turn a pointer into a slice.
Closing this item since SDL_mixer was wrapped by @Yoplitein