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

In-game Radio chat sounds cut off #180

Closed
4 of 20 tasks
tnmeyer opened this issue Apr 25, 2024 · 14 comments · Fixed by #191
Closed
4 of 20 tasks

In-game Radio chat sounds cut off #180

tnmeyer opened this issue Apr 25, 2024 · 14 comments · Fixed by #191
Labels
bug Something isn't working
Milestone

Comments

@tnmeyer
Copy link

tnmeyer commented Apr 25, 2024

Build Version

47ff6e5

Operating System Environment

  • Microsoft Windows (32-bit)
  • Microsoft Windows (64-bit)
  • Mac OS X
  • Linux (specify distribution and version below)

CPU Environment

  • x86 (32-bit Intel/AMD)
  • x86_64 (64-bit Intel/AMD)
  • ARM (32-bit)
  • ARM64 (64-bit; sometimes called AArch64)
  • Other (RISC V, PPC...)

Game Modes Affected

  • Single player
  • Anarchy
  • Hyper-Anarchy
  • Robo-Anarchy
  • Team Anarchy
  • Capture the Flag
  • Bounty
  • Entropy
  • Hoard
  • Monsterball
  • Cooperative

Game Environment

Arch Linux x86_64
X11, NVidia driver 550.76
Descent3 compiled from source as 32-bit or 64-bit
gcc version 13.2.1 20230801 (GCC)
Game files from european (german) retail release, Windows version, patched to v1.4

Description

In-game radio speech messages (at the beginning of missions) is cut off after a few seconds.

Regression Status

This bug was introduced when new libacm code was imported. Reverting to old code (commit f6f3c33 and libacm related follow-up commits) restore correct sound output.

Steps to Reproduce

  1. Start Descent 3 with no options
  2. Start a new game, i.e. Mission 1
  3. Let the In-game intro cutscene play
  4. Radio message starts to play, is cut off after ~5 secs
@tnmeyer tnmeyer added the bug Something isn't working label Apr 25, 2024
@jcoby
Copy link
Contributor

jcoby commented Apr 25, 2024

Happens on M-series mac as well. SE: Data is NULL is shown on the console when it happens.

dd_lnxsound/mixer.cpp:460 and dd_sndlib/Ds3dlib.cpp:1084 both print that message.

@JeodC
Copy link
Collaborator

JeodC commented Apr 25, 2024

Related PRs: #135 and possibly #158

@DAK404
Copy link

DAK404 commented Apr 25, 2024

Hello! I face a potentially similar issue where the SFX for completing objectives and robot noises are "laggy".

I have used the build artifact from https://github.com/DescentDevelopers/Descent3/actions/runs/8836170706
Platform: Windows.

Here is the issue in action: https://drive.google.com/file/d/17ofkyK12CWtHAWf724wmiBCAzLxFB838/view?usp=sharing

(GitHub wont let me attach videos or files above 10MB therefore the GDrive link)

@InsanityBringer
Copy link
Contributor

InsanityBringer commented Apr 26, 2024

I spent a lot of time debugging this, and so far the conclusion I can tell is that old libacm thinks the sound is a single channel, while the new libacm seems to think it is two channels, which ends up halving the amount of samples that end up being played.

I'm not sure why they diverge yet, trying to dig deeper.

There's even a comment in adecode.cpp about it

// TODO: the old libacm.cpp was more optimistic about the numbers of channel
// from the file header
//       than libacm's decode.c, which assumes that channels are always >= 2
//       (unless it's WAVC instead of "plain ACM"), i.e. that a file header
//       specifying 1 is wrong. If it turns out that we really have ACM files
//       with just one channel (not unusual for ingame sounds?), we might have
//       to either patch acm_open_decoder() or somehow detect the number of
//       channels here and set force_channels accordingly

whoever wrote that, looks like it's going to have to be addressed

@DanielGibson
Copy link
Contributor

DanielGibson commented Apr 26, 2024

I wrote that.

In libacm/decode.,c try to comment out 814 and 815 (else if (!acm->wavc_file && acm->info.channels < 2) acm->info.channels = 2;)

If that indeed fixes the issue, it probably makes sense to send a patch to upstream libacm.
As they will have their reasons for treating files as stereo even if the header suggests they have one channel (presumably some game had ACM files where stereo files had the wrong channel value in the header), it might be necessary to make it a compile option that we can activate (and other users of libacm don't), like

	if (force_chans > 0)
		acm->info.channels = force_chans;
#ifndef ACM_TRUST_HEADER_CHANNELS
	else if (!acm->wavc_file && acm->info.channels < 2)
		acm->info.channels = 2;
#endif

UPDATE: possibly nicer solution: give int force_chans = -1 the meaning of "use whatever the file header says":

	if (force_chans > 0)
		acm->info.channels = force_chans;
	else if (force_chans != -1 && !acm->wavc_file && acm->info.channels < 2)
		acm->info.channels = 2;

and then just set force_channels = -1; in libacm/adecode.cpp

@DAK404
Copy link

DAK404 commented Apr 26, 2024

If necessary, I am ready to test any changes to check if the issue is fixed or not 😄

@jcoby
Copy link
Contributor

jcoby commented Apr 26, 2024

I wrote that.

In libacm/decode.,c try to comment out 814 and 815 (else if (!acm->wavc_file && acm->info.channels < 2) acm->info.channels = 2;)

This change fixes the problem for me.

As they will have their reasons for treating files as stereo even if the header suggests they have one channel (presumably some game had ACM files where stereo files had the wrong channel value in the header)

That does seem to be the case. See: markokr/libacm@31e3bc9 and markokr/libacm@28c8b45

UPDATE: possibly nicer solution: give int force_chans = -1 the meaning of "use whatever the file header says":

I like this. However, do we have any stereo files? If not we could set force_chans to 1 and not need to patch or fork the lib.

@JeodC JeodC added this to the 1.5 Stable milestone Apr 26, 2024
@tnmeyer
Copy link
Author

tnmeyer commented Apr 26, 2024

This change fixes the problem for me.

Indeed, this works for me as well.
In the end, I changed it to

else if (!acm->wavc_file && (acm->info.channels < 1 || acm->info.channels > 2))
          acm->info.channels = 2;

to catch any cases with bogus channel values in the header (should not occur).

I like this. However, do we have any stereo files? If not we could set force_chans to 1 and not need to patch or fork the lib.

The main menu background music is a "interplayacm 2ch 22050Hz" file, but all the speech files seem to be mono.

@JeodC
Copy link
Collaborator

JeodC commented Apr 26, 2024

Futureproofing is preferred I'd think. Prior descent ports have higher quality sound mods.

@jcoby
Copy link
Contributor

jcoby commented Apr 26, 2024

i poked around in the ffmpeg code and they give up if channels <= 0: FFmpeg/FFmpeg@5540d6c

i suppose there could be other code upstream of that to validate the header but i'm not sure it matters for our purposes since we seem to be able to trust the header.

@DanielGibson
Copy link
Contributor

DanielGibson commented Apr 26, 2024

i poked around in the ffmpeg code and they give up if channels <= 0:

<= 0, yes, but the case that libacm handles is mostly treating == 1 as stereo, I think

I'll try to get a patch into libacm to support -1 for "really do what the header says"

@DanielGibson
Copy link
Contributor

Prior descent ports have higher quality sound mods.

Note that mono vs stereo is not primarily a quality issue.

Only sounds that are non-positional should be stereo, i.e. ones that are played at the players position no matter of their orientation. This is mostly music and movies, maybe sound effects of your own weapons and such.

But normal ingame sound-effects must be mono, because they have a single sound source (an enemy, the position where a rocket explodes, etc) that will be mixed into the stereo (or even surround) audio stream according to its position.

@DanielGibson
Copy link
Contributor

DanielGibson commented Apr 26, 2024

I'll try to get a patch into libacm to support -1 for "really do what the header says"

The PR is here: markokr/libacm#3

@DanielGibson DanielGibson mentioned this issue Apr 28, 2024
13 tasks
@DanielGibson
Copy link
Contributor

For the sake of completeness, my PR has been merged into libacm, now force_channels = 0 means "trust the file header" and the old quirk of assuming plain ACM is always stereo is used when force_channels = -1 is set.

libacm/ in this repo has already been updated with the latest upstream libacm code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants