TerryCavanagh / VVVVVV

The source code to VVVVVV! http://thelettervsixtim.es/
Other
7k stars 559 forks source link

Mute/low volume doesn't always affect first fragments of sounds #880

Closed Daaaav closed 2 years ago

Daaaav commented 2 years ago

This seems to be an FAudio regression, since it doesn't happen on a8feba029f157a2f009fe712b8cf988cd6571bf7 but it does on f877eb3b562ca10b433c6cee94e84a9116da7c95, as well as master.

When all sound is muted with M, or when the volume is set low, you may notice small fragments of sound effects still being played at regular volume. The effect is inconsistent (for example, it happened only once with the menu squeak in the first video).

(Make sure to unmute both videos)

https://user-images.githubusercontent.com/44736680/168695640-35dc96a5-4d3e-4bd7-bfdf-e1bc64fac325.mp4

https://user-images.githubusercontent.com/44736680/168696274-b2e3d733-906c-47ce-aef6-b1b6a2782563.mp4

flibitijibibo commented 2 years ago

Shouldn't be too hard to fix, either the volume can be set before playing/queuing or we can use operation sets to make it so call order doesn't matter (as much).

N00byKing commented 2 years ago

Just saw this Issue and tried reproducing it, but I wasn't able to... Does this fix it?

InfoTeddy commented 2 years ago

Does this fix it?

This commit introduces a regression where music never fades in when it starts playing. It's noticeable as early as the title screen - when pressing ACTION you can hear Presenting VVVVVV start immediately, instead of slowly fading in like it usually does.

Daaaav commented 2 years ago

It's still happening on that commit, and as Misa said, music doesn't fade in or out anymore (but maybe that was intentional just to test if it would fix the issue).

N00byKing commented 2 years ago

Dang. I know about the fade in thing (was testing something else), but hoped that the volume bug might go. Guess I'll try reproducing it again

Daaaav commented 2 years ago

I can get it to happen on both Linux and Windows. I can't try it on macOS unless I have a build with proper dependencies. (I can't get the CI build from https://github.com/InfoTeddy/VVVVVV/actions/runs/2368400505 to work even after fiddling with it, and I really want to keep my macOS free from "developer" customizations like system-wide-installed SDL or Homebrew to keep it a representative testing ground.)

flibitijibibo commented 2 years ago

The bug is probably timing-focused, which suggests that the source isn't given the volume data before it plays. This is definitely possible if you, for example, call Play() and then SetVolume(), because the OS audio system can trigger a mix in between those two (no matter how unlikely) and therefore it will be somewhat nondeterministic when tested across different hardware. I wouldn't change the call order necessarily, unless it's something easy like putting a SetVolume before Play, instead you can use the operationset parameter combined with FAudio_CommitOperationSet to ensure that all the voice calls happen atomically, avoiding machine-specific races.

InfoTeddy commented 2 years ago

This is definitely possible if you, for example, call Play() and then SetVolume(),

Yeah, that might be what's happening here. musicclass::playef calls SoundTrack::Play, but if the sound effect isn't volume 0 at that point then it will only be set to 0 when musicclass::updatemutestate gets called later in the frame (which calls SoundTrack::SetVolume).

N00byKing commented 2 years ago

Would probably be possible to just call updatemutestate from within playef then right?

InfoTeddy commented 2 years ago

Well if what Ethan is saying is true, then that won't fix it. Instead playef (or SoundTrack::Play) should use an operation set to make sure the calls happen atomically.

flibitijibibo commented 2 years ago

It depends on timing - if updatemutestate is feasible between CreateSourceVoice and Play then it would work without the batching system, but without looking it up in the source I don't know if that would have side effects.

N00byKing commented 2 years ago

Thing is, the batching system is hard to do cleanly i think. It would either require restructuring SoundTrack (it doesnt know current volume) or make musicclass do it itself, destroying the abstraction layer. Maybe I'm not imaginative enough lol. Anyway, maybe its worth trying just adding the updatemutestate to playef. If someone who does have the issue can test this that'd be great. I removed the broken stuff from earlier as well

Daaaav commented 2 years ago

If someone who does have the issue can test this that'd be great.

Still not fixed with that commit.

N00byKing commented 2 years ago

But now it totally works right? Right?

Daaaav commented 2 years ago

Yep, I can't reproduce it anymore with that! The only problem is that pressing M doesn't mute sound effects that are already playing.

N00byKing commented 2 years ago

oopsies. I'll fix that and PR! Thanks for testing my guy