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

Basic FUNCTIONAL XMA2 support #247

Merged
merged 8 commits into from
Mar 18, 2021
Merged

Basic FUNCTIONAL XMA2 support #247

merged 8 commits into from
Mar 18, 2021

Conversation

0x0ade
Copy link
Contributor

@0x0ade 0x0ade commented Mar 17, 2021

Compared to #66, this is no longer "initial" and it's no longer taking stupid shortcuts.

*breathes in*

  • gst-libav ships with avdec_xma2 (citation needed). While we could wait for every distro under the sun to expose proper sink caps, I've taken the creative liberty to add an optional autoplug-factories handler with some dynamic mimetype shenanigans for that specific factory.
  • I've added FAudioXMA2WaveFormat as FFmpeg currently accepts (but doesn't fully respect) XMA2 extra data.
    • XMA2 wave data no longer gets packed into FAudioWaveFormatExtensible. Goodbye, broken shortcuts!
    • To handle the multitude of wave formats more easily, I've added FAudioAnyWaveFormat as an union of all wave formats.
  • The C# bindings for FAudio_CreateSourceVoice and FAudioSourceVoice_SubmitSourceBuffer have been modified to take multiple wave formats and FAudioBufferWMA into account. Given how both are passed by reference and not by value anyway, I'm expecting things to end up fine. Please let me know if I'm overlooking something though, no matter how obvious.
  • FACTWaveBank_Prepare reconstructs as much of the XMA2 extra data as necessary and documents all the quirks I've encountered for future reference. Maybe a future replacement decoder will verify the accuracy of this.
  • The XMA2 seek table uses sample indices, unlike the WMA decoded packet cumulative bytes table. FACT converts the seek table to the byte index format that everything else expects.
  • nBlockAlign wasn't enough to feed the decoder with properly sized blocks (in part due to its 16-bit size limitation) - the GStreamer decoder now uses dwBytesPerBlock (32-bit) when necessary.
    • ... and who knows if XMA2 really "is quite similar to WMA Pro" in that aspect...
  • The XMA2 data end can be misaligned to the block size while still being aligned to the packet size (2kB). FFmpeg standalone doesn't care about this (it always uses the 2kB packet size), but we must handle this properly.
    • Speaking of misaligned ends: FAudio_GSTREAMER_FillConvertCache happily read beyond the amount of encoded bytes into unknown territory. This PR changes that to feed undersized end buffers properly, but I don't know if every format expects that. Checking for the format type and handling it accordingly (return early? pad?) is out of the scope of this PR though.
  • I've encountered a deadlock or two with the GStreamer decoder along the way, mainly "we need more samples, let's retry infinitely no matter if we're re-decoding the same block or if we're clearly out of data." As with the C# bindings, please let me know of any side effects which I'm not aware of.
  • FACTWaveBank_GetWaveProperties now asserts when encountering XMA2 because I've yet to encounter a game that calls it and who knows what games expect? If required, I'll follow up with an approximate implementation.
  • prolly some other things ive forgotten abt idk

*breathes out*

Some things are handled outside of this PR, namely a FACT big endian variation table fix (thanks for merging that ahead of time!) and a FNA PR which I'll follow up with soon.

By the way, this finally fixes all XMA2 bugs related to #95 which I've encountered.

@0x0ade
Copy link
Contributor Author

0x0ade commented Mar 17, 2021

With all that being said: this still isn't perfect.

Any idea as to what could be causing that weird filter-like sound?

Copy link
Member

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Functionally I'll trust that what you have works. A lot of the notes just concern compatibility with existing data/applications and some style stuff here and there.

For WMA however I'll have to defer to someone else - once everything else is finished I'll CC them in so they can check it out.

csharp/FAudio.cs Outdated Show resolved Hide resolved
include/FAudio.h Outdated Show resolved Hide resolved
include/FAudio.h Outdated Show resolved Hide resolved
include/FAudio.h Outdated Show resolved Hide resolved
src/FACT.c Outdated Show resolved Hide resolved
src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
src/FAudio_gstreamer.c Show resolved Hide resolved
src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
Copy link
Member

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Pass 2, pretty close to ready at this point.

csharp/FAudio.cs Outdated Show resolved Hide resolved
include/FAudio.h Outdated Show resolved Hide resolved
src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
Copy link
Member

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Latest looks good! Going to CC @aeikum to make sure we didn't screw up the WMA support.

@aeikum
Copy link
Collaborator

aeikum commented Mar 18, 2021

Thanks for checking. Unfortunately 810ffd4 crashes both Banished and Castle Crashers for me, so looks like something is going wrong :( I've attached a log here, but it doesn't look super informative to me. The crash is on line 1669.

steam-204360.txt

@0x0ade
Copy link
Contributor Author

0x0ade commented Mar 18, 2021

In my attempt to decouple XMA2 decoding from FAudioBufferWMA, I mistakenly moved an access to it from FAudio_INTERNAL_DecodeGSTREAMER to FAudio_GSTREAMER_init, which is callable before that buffer was available. I've now changed DecodeGSTREAMER's behavior to either use a value supplied by init or to fall back to bufferWMA, fixing a crash with a game I was able to test this with locally. Hopefully this also fixes the crashes you were experiencing.

As for the block != gstreamer->curBlock change - it was added during development with another change that prevented certain deadlocks (which I'll attach a comment to shortly). Should I revert those changes?

src/FAudio_gstreamer.c Outdated Show resolved Hide resolved
@0x0ade
Copy link
Contributor Author

0x0ade commented Mar 18, 2021

@aeikum can you please re-review the PR with the latest two commits? 😅

@aeikum
Copy link
Collaborator

aeikum commented Mar 18, 2021

@0x0ade Confirmed it's no longer crashing, and audio is functional, with 31ae786.

@flibitijibibo
Copy link
Member

Awesome, thanks to both of you for checking this out!

@flibitijibibo flibitijibibo merged commit fcac4a3 into FNA-XNA:master Mar 18, 2021
@0x0ade 0x0ade deleted the xma2 branch March 21, 2021 17:19
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

3 participants