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

acm_open_decoder(): Support force_chans = -1 for "always trust header" #3

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

DanielGibson
Copy link
Contributor

@DanielGibson DanielGibson commented Apr 26, 2024

Descent3 has both mono and stereo ACM files, and their headers appear to specify to correct number of channels.

Support that with force_chans = -1.

While at it, I added some documentation.

@markokr
Copy link
Owner

markokr commented Apr 26, 2024

In hindsight I think having such override in decode.c is simply wrong. Forcing channels should be purely decision of caller, like acmtool does with command line flags. There should be no extra flag needed to accept channel info from file.

Please just remove the condition to override file channels.

@DanielGibson
Copy link
Contributor Author

If what the comment in the code says is true, and it's only plain ACM files that get this wrong (but not always), while WAVC files always get it right, then the decision is harder to make for the caller, as they don't have the information about what kind of ACM file it is, at least not without parsing the header themselves or opening once to figure out the type and then (possibly) again with force_chans = 2..

@markokr
Copy link
Owner

markokr commented Apr 26, 2024

Well, there are no new ACM files created and for specific game the user/emulator will know what files are correct or not - or whether all of them are OK.

I remember that it was systematic problem with specific games like iwd/planescape, but there is no way to encode such knowledge to decode.c, thus it is much better if such override happens somewhere such context is known.

@DanielGibson
Copy link
Contributor Author

Assuming that there are games that use both plain ACMs and WAVC ACMs, deciding this on the client-side seems like a PITA.

Maybe it should be switched and force_chans = 0 should mean "trust the header" and -1 should mean "trust the header if it's WAVC, assume it's stereo for plain ACM", but it seems to me like having both modes is useful.

@markokr
Copy link
Owner

markokr commented Apr 26, 2024

Digged through some archives - iwd has both wavc and bad-1-chan acm, so yeah such mode could be useful. So activating the workaround via force_chans=-1 looks fine to me.

@DanielGibson
Copy link
Contributor Author

Ok, I'll change the code (and docs) accordingly tomorrow :)

force_chans = 0 used to assume that plain ACM files are stereo (even if
their header says mono), as apparently that's the case for the files
from some games - but not all, Descent3 has both mono and stereo ACM
files, and their headers specify the correct number of channels, so
this quirk turns out to be a bad default, especially when there is no
way to tell libacm to trust the header.

Now force_chans = 0 assumes that the header is always correct, and the
old quirk mode (that assumes that plain ACM files are always stereo,
 but the headers of "WAVC" ACM files are always correct)
can be enabled with force_chans = -1.
mostly the parts that weren't obvious to me from the
function/argument names

including the new behavior for force_chans
@DanielGibson DanielGibson force-pushed the force-trust-header-chans branch from 981616a to b3afa7d Compare April 27, 2024 16:20
@DanielGibson
Copy link
Contributor Author

Ok, I made these changes - now force_chans = 0 always trusts the header and force_chans = -1 enables the old workaround.

Additional benefit: If other quirk modes come up in the future (who knows in what ways ACM files of other games are buggy..), they could be added with force_chans = -2, force_chans = -3 etc

@markokr markokr merged commit 08298ee into markokr:master Apr 27, 2024
@markokr
Copy link
Owner

markokr commented Apr 27, 2024

Merged, thanks!

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.

2 participants