d-gamedev-team/gfm

SDL Audio support

Closed this issue · 29 comments

clinei commented

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.

p0nce commented

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.
clinei commented

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.

p0nce commented

I'll review in ~4 hours, this will take some time.

clinei commented

Of course, this is far from done, anyway.

p0nce commented

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 @propertykeywords. 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.

clinei commented

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.

p0nce commented

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.

clinei commented

replaced by D slices

Do you mean ranges or slices?

p0nce commented

I mean slices like eg. short[]. Also called dynamic arrays (dynamic arrays are slices)

p0nce commented

What will you do regarding mixing? Wrapping SDL_mixer or continue your implementation?

clinei commented

What will you do regarding mixing?

Wrapping SDL_mixer.

p0nce commented

Great! Looking forward to it.

clinei commented

One last question (for now):
Can I move all variable declarations to the top of the class?

p0nce commented

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)

clinei commented

if it ever helps

Every little bit helps :)

clinei commented

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.

p0nce commented

string is OK

p0nce commented

Nice. I think the local imports can be transformed into a top-level one:
import derelict.sdl2.sdl;

clinei commented

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?

clinei commented

I'll wrap SDL_mixer next, hopefully in the coming week.

clinei commented

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

p0nce commented

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.

clinei commented

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.

p0nce commented

Do you have any email? (found it)

clinei commented

email

tanel58 at hotmail dewt com

clinei commented

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].

p0nce commented

Yes that's exactly how you turn a pointer into a slice.

p0nce commented

Closing this item since SDL_mixer was wrapped by @Yoplitein