Port to FAudio

[N00byKing]

Changes:

I was browsing the repo and saw #829, with the note that porting to FAudio should be "really easy" for someone else to do.
Answer is no it wasn't (tbf I never touched any audio stuff ever), but I tried nonetheless.

This should be functional, but buggy. I'm opening the PR to see if I should continue or just stop.
Thanks for looking at my awful code

CI is failing because I used a submodule for FAudio. I can either make CI clone recursive or just copy folder contents, will wait for feedback on that.
Had to increase C Standard Version due to stb_vorbis (90 -> 99). If someone has suggestions on how to avoid that please share.

Most Bugs revolve around tabbing in and out of the game. Sometimes there is no Resume() generated for some reason I think. For example, tabbing out during the intro sequence causes the game to stay mute until you do it again during gameplay. I think it might be because of FAudio_StopEngine() in Pause(), but as that was written in the comment I let it be.

Legal Stuff:

By submitting this pull request, I confirm that...

• My changes may be used in a future commercial release of VVVVVV
• I will be credited in a CONTRIBUTORS file and the "GitHub Friends"
  section of the credits for all of said releases, but will NOT be compensated
  for these changes

[flibitijibibo]

This actually doesn't look too far off what I expected, so well done!

If it's any help, you can reference the stb_vorbis copy from the FAudio repo - I was actually planning on statically linking FAudio into the build so it will add to the CMake file more but in exchange it will build less overall.

I would say keep going with this, you're on the right track as far as I can tell.

[InfoTeddy]

It was brought up on the VVVVVV Discord server that this PR might remove support for loop points, which existing custom levels use. I haven't tested it and have no idea if this PR supports loop points, but I'm just saying this so we can pay attention to it.

[flibitijibibo]

I don't think it does, but it's not hard to pull from the Vorbis comments and apply to the decode/seek routine.

[N00byKing]

ootl, sorry, what are loop points?

[InfoTeddy]

Loop points are special markers in audio files that let people have an "intro" segment before a segment that actually loops. The example I can think of off the top of my head is Slider from Super Mario 64, where the start of the track has this little blurb before it jumps straight into a looping segment, and that start blurb is never repeated again.

[flibitijibibo]

Short version: They're notes in the file's comments that tell you two things:

• Loop start, for example if a song has an intro a loop can seek not to 0 but to an offset after the intro
• Loop end, for example if a song has an outro the decoder can stop at this offset and loop back to the loop start

[N00byKing]

Thanks! I'll see to adding those.

[N00byKing]

FAudio is now compiled statically.
I have not yet made use of the stb_vorbis copy in FAudio due linking errors; It seems that FAudio disables stb_vorbis_get_samples_short_interleaved at certain points at least. The thing is, even disabling XNA Song the linking errors stayed, so I'll return to that Issue later.

[flibitijibibo]

FAudio is now compiled statically. I have not yet made use of the stb_vorbis copy in FAudio due linking errors; It seems that FAudio disables stb_vorbis_get_samples_short_interleaved at certain points at least. The thing is, even disabling XNA Song the linking errors stayed, so I'll return to that Issue later.

Sounds about right - you can get a minor performance improvement by using float PCM as the wave format rather than s16, since the internal mixer format is float anyway.

[N00byKing]

Oh, I didnt know that. Well, probably should've guessed as much if the define is called integer conversion lol. Switched it over, and found a very, very hacky way to reuse FAudio's copy of stb_vorbis.

[Daaaav]

About loop points, this is how the current SDL_mixer supports them (I had to research this myself for Ved):

• Loop points are Vorbis comments, LOOPSTART in combination with either LOOPLENGTH or LOOPEND
• These names are case-insensitive, and there can optionally be a _ or - after LOOP (LOOP_START for example)
• Values can be either sample numbers (most common), or human-readable HH:MM:SS.mmm times, which can be shortened as long as there is at least one colon in the string (0:20, :20.0, etc)

It seems that stb_vorbis does support Vorbis comments (comment_list_length/comment_list) and that FAudio supports looping by sample numbers (LoopBegin/LoopLength in FAudioBuffer) so hopefully all that's needed is to lookup and parse the comments, and then pass the values to FAudio!

[N00byKing]

I've added support for loop points. Tested it with Maps "Glory Glory" and "Maximally Misleading Miserable Misadventure". Both maps however only use the loop begin parameter (MMMM uses loopbegin/looplength as well, but runs up until the end). If someone knows of a map using it audibly please send a link!
The comment parsing was lifted straight from SDL_mixer with minimal adjustment.

P.S: If you want an earbleed experience try listening an allocated but unwritten buffer

[Daaaav]

The loop points seem to work nicely to me! Here's a level with my test cases: loop_testcases.zip

(I spent minutes pouring a lot of musical skill into this song.)

Anyway, all songs should sound the same - there's a 2 seconds drum intro, a 2 seconds looping part with piano, and (in most variants) a 2 seconds dissonant mess to indicate if the loop failed.

[N00byKing]

Nice! Thanks for the testcases!
I'll see if I can get the issues with tabbing out of the game fixed next (as long as there isn't another vorbis feature I don't know of yet)

[N00byKing]

I mean, it works when just looping over the voice like when setting volume instead of using StopEngine.
Things left to do are see if the volume scale is "about right" (Currently: Middle is FAudio default, max is 2*default), and see if everything works. As I'm not encountering bugs atm, marking ready for review

[InfoTeddy]

Quick style pass, plus some other stuff I could spot just by looking at the diff.

I haven't tested this yet, but other than what I've pointed out it seems good so far.

[N00byKing]

Fixed all but unresolved

[N00byKing]

Also, regarding the use of submodules: Yay or nay for this codebase?

[InfoTeddy]

I don't know why you called it janky, the code seems fine to me.

Still got some other stuff though.

---

Additionally, -Wextra reports that a couple parameters in FAudio's stb_vorbis.h are unused (step2_flag in do_floor, left_end in vorbis_decode_packet_rest). Not sure if we should PR them to FAudio or to STB, @flibitijibibo?

Also, Valgrind still reports that the RW on line 635 is getting leaked.

[flibitijibibo]

I don't know why you called it janky, the code seems fine to me.

Still got some other stuff though.

Additionally, -Wextra reports that a couple parameters in FAudio's stb_vorbis.h are unused (step2_flag in do_floor, left_end in vorbis_decode_packet_rest). Not sure if we should PR them to FAudio or to STB, @flibitijibibo?

Also, Valgrind still reports that the RW on line 635 is getting leaked.

For stb I would check FNA-XNA/FAudio#260 first, then if we want we can file reports with Sean (but these warnings may be based on the build optimization and thus may not be fixable).

[InfoTeddy]

Almost there, just need to fix the segfault.

[InfoTeddy]

If MusicTrack's constructor fails to create the Vorbis handle, then it leaks memory. Valgrind complains whenever I load my level, Maximally Misleading Miserable Misadventure, which nulls out unused music blobs in vvvvvvmusic.vvv to save on filesize.

[InfoTeddy]

Ok hang on, my change doesn't work because we are mixing declarations and code instead of having all declarations at the top of the block.

[InfoTeddy]

These need to be changed as well...

[InfoTeddy]

By the way, this isn't the end of it. The game has floating-point exceptions / invalid frees with my other level, which has a completely blank name.

[InfoTeddy]

For reference:

• https://cdn.discordapp.com/attachments/308802659643817984/956660135898529882/2a50310f42b1781c.zip
  (rename to " .zip" - with a space in front - else the level won't be loaded)
• https://cdn.discordapp.com/attachments/308802659643817984/956660236943491112/maximally_misleading_miserable_misadventure_v1.1.zip

[N00byKing]

I don't get the Issues with Maximally Misleading Miserable Misadventure when just adding a SDL_RWclose(rw); in the abort case. Added it, please retest

[InfoTeddy]

I don't get the Issues with Maximally Misleading Miserable Misadventure when just adding a SDL_RWclose(rw); in the abort case. Added it, please retest

Yes, that works, but it's duplicating code. I'd prefer to use the goto so I know for sure there's always one code path taken when exiting the function.

[N00byKing]

Fair, but I don't really like goto's either xD... Why not just add nullchecks to Dispose then and call that when failing initialization?

[InfoTeddy]

There's nothing wrong with using gotos lol. Also that doesn't work because rw is a local variable passed in to the constructor and not a member of MusicTrack.

If you don't want to do the goto, fine, but that's more work for me to do after merging this in.

[N00byKing]

I've no issues adding it here, if you think it's better. Maintainer gets last say after all

[N00byKing]

Where do the Issues occur on the secound level?

[N00byKing]

Nvm think I found them. Should be fixed.
Tbh I am just learning using valgrind after you mentioned using it. Useful (But very slow ;-;)!

[InfoTeddy]

(But very slow ;-;)!

This game isn't so bad under Valgrind if you have a decent CPU like mine (an i7-9700).

---

The " " level ends up with a floating-point exception if attempting to play a nulled sound, and Valgrind complains about numerous reads of uninitialized member variables when that happens. I think the best solution to this is to zero each SoundTrack on construction (memsetting this is okay in C++ as long as you don't use virtual functions, and we don't, and no one likes virtual functions anyway), and to add a valid attribute, and make sure we don't play it if valid is false. I also want to do the same for every MusicTrack as well.

Also some slight other nits than that. But other than that, I think this will be ready to merge!

[N00byKing]

Adressed feedback