RustAudio / rodio

Rust audio playback library
Apache License 2.0
1.66k stars 214 forks source link

Fix channel count upscaling #559

Closed ideka closed 3 months ago

ideka commented 3 months ago

This changes channel upscaling so all input samples are spread out across the output channels, instead of repeating the last sample over and over. This approach should yield more accurate results. Fixes #558.

dvdsk commented 3 months ago

It seems like there is no standard order for 5.1 and 7.1 surround sound instead it differs across file formats and can even be configurable. For example see channel assignment for flac. That is gonna make correct channel conversion quite challenging. As it might need work on the decoders to unify how they provide channels. Symphonia does at least provide a consistent order it seems: symphonia docs.

It makes perfect sense to turn mono to stereo by assigning both speakers the same sample. Scaling stereo to 5.1 and 7.1 should probably be done by recognizing which of the output channels is the left and which the right channel and sending the rest zero's/nothing.

Now back to this PR and stereo to 5.1/7.1 up-scaling :) From this cpal PR it seems cpal uses the WAVEFORMATEXTENSIBLE order (see winapi docs) just like symphonia does. That means you should be able to figure out which is the front left and front right channel for any device. Then we can have rodio send stereo to only those channels.

Does that sound good to you?

ideka commented 3 months ago

So it can be safely assumed that the first 18 channels will always be in the WAVEFORMATEXTENSIBLE order? Like in this table here?

So the way it should be is...

For channels from 9 to 18 it'd get more complicated because they don't alternate left/right anymore, but the idea would be the same. Match left with left, right with right, center with center.

This kinda begs the question though. As you said, when going from mono to stereo it makes sense to duplicate the sample, but should that be done as well when going higher? maybe for 2 to 8 for example, this makes more sense? [1, 2, 0, 0, 0, 0, 0, 0, 3, 4, 0, 0, 0, 0, 0, 0]

Let me know what you think.

dvdsk commented 3 months ago

So it can be safely assumed that the first 18 channels will always be in the WAVEFORMATEXTENSIBLE order? Like in this table here?

As far as I can deduce from the cpal source thats correct. I would say anything beyond 8 channels it too nice for rodio right now (though that's not my call to make).

Match left with left, right with right, center with center.

That would ensure audio to most speaker. Though that is not what (a quick google shows) is recommended. The tldr being:

While you can attempt to stream music in surround sound, this will often just make the audio louder without improving clarity, and often results in sound distortion.

I think we can safely generalize that to: do not use more channels then the source is offers as that will decrease clarity. With an exception for going from mono to stereo (it is quite annoying to have sound only left or right).

This kinda begs the question though. As you said, when going from mono to stereo it makes sense to duplicate the sample, but should that be done as well when going higher? maybe for 2 to 8 for example, this makes more sense? [1, 2, 0, 0, 0, 0, 0, 0, 3, 4, 0, 0, 0, 0, 0, 0]

So yes given the source above to me that makes more sense. Just for clarity that would correspond to this table:

7.1 speaker position interleaved stereo sample numb non interleaved stereo sample
SPEAKER_FRONT_LEFT 1 sample 1 left channel
SPEAKER_FRONT_RIGHT 2 sample 1 right channel
SPEAKER_FRONT_CENTER 0 silent
SPEAKER_LOW_FREQUENCY 0 silent
SPEAKER_BACK_LEFT 0 silent
SPEAKER_BACK_RIGHT 0 silent
SPEAKER_FRONT_LEFT_OF_CENTER 0 silent
SPEAKER_FRONT_RIGHT_OF_CENTER 0 silent
SPEAKER_FRONT_LEFT 3 sample 2 left channel
SPEAKER_FRONT_RIGHT 4 sample 2 right channel
SPEAKER_FRONT_CENTER 0 silent
SPEAKER_LOW_FREQUENCY 0 silent
SPEAKER_BACK_LEFT 0 silent
SPEAKER_BACK_RIGHT 0 silent
SPEAKER_FRONT_LEFT_OF_CENTER 0 silent
SPEAKER_FRONT_RIGHT_OF_CENTER 0 silent
dvdsk commented 3 months ago

oh and while your at would you mind doing a tiny refactor :)

At line 74 (right now) I think this:

if self.next_output_sample_pos == self.to {
    self.next_output_sample_pos -= self.to;

would be clearer like this

if self.next_output_sample_pos == self.to {
    self.next_output_sample_pos = 0;

and still work right? Would you agree?

xMAC94x commented 3 months ago

thanks for seeing a fix here :) Greetings from the veloren team

dvdsk commented 3 months ago

Since this is a non trivial PR and touches an essential part of rodio I've asked @est32 to take a look too. As soon as that's done we are ready to merge! :tada:

Thanks a lot for adding proper multi channel support @ideka to rodio! As you've seen its already appreciated by the community!