sound: Implement AFMT_FLOAT support

[christos]

Summary

Even though the OSS manual [1] advises against using AFMT_FLOAT, there
are applications that expect the sound driver to support it, and might
not work properly without it.

This patch adds AFMT_F32_LE|BE (as well as AFMT_FLOAT for OSS
compatibility) in sys/soundcard.h and implements AFMT_F32_LE|BE <->
AFMT_S32_LE|BE conversion functions. As a result, applications can
write/read floats to/from sound(4), but internally, because sound(4)
works with integers, we convert floating point samples to integer ones,
before doing any processing.

[1] http://manuals.opensound.com/developer/AFMT_FLOAT.html

PR: 157050, 184380, 264973, 280612, 281390
Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Test Plan

Apply D47639 to test sound.

[christos]

Rebase on top of D48421.

[christos]

Use __DECONST to silence const warning in pcm_sample_read(). A bit ugly...

[christos]

• Fix bugs.
• Write unit tests.
• Re-arrange fpu_kern(9) calls.

There are a few sound tests I haven't done yet, but I am posting as a WIP in
case someone wants to leave a comment.

[christos]

sys/dev/sound/pcm/pcm.h

228

Fixed.

[christos]

Fix big endian regressions introduced in last diff.

I tried recording and playback with real audio (both LE and BE) and things seem
to work fine.

@dev_submerge.ch The big endian tests fail, because on a little endian machine,
converting 0x3f000000 (0.5) to 0x0000003f will result in the float value being
0, so the pcm_sample_* functions return will return wrong values. For
example, pcm_sample_read() reads the input as float (i.e 0x3f000000 ->
0.5), then converts it to big endian (0x3f000000 -> 0x0000003f -> 0), and
lastly converts it to S32 (0 * PCM_S32_MAX = 0), in which case the return
value will be 0, hence the test failure. I am not sure yet how we should adapt
the test program to accomodate floating-point big endian tests.

[dev_submerge.ch]

In D47638#1111768, @christos wrote:

@dev_submerge.ch The big endian tests fail, because on a little endian machine,
converting 0x3f000000 (0.5) to 0x0000003f will result in the float value being
0, so the pcm_sample_* functions return will return wrong values. For
example, pcm_sample_read() reads the input as float (i.e 0x3f000000 ->
0.5), then converts it to big endian (0x3f000000 -> 0x0000003f -> 0), and
lastly converts it to S32 (0 * PCM_S32_MAX = 0), in which case the return
value will be 0, hence the test failure. I am not sure yet how we should adapt
the test program to accomodate floating-point big endian tests.

Well, you do have the test data backwards for the big-endian floats? Just fill in the byte array in big-endian order and check for the correct sample values.

sys/dev/sound/pcm/pcm.h

199–210

Isn't this a bit complicated? Just read it as a 32bit integer from src, reinterpret that as a float, and then calculate the integer value from that float?

[christos]

• Simplify read.
• Make big-endian tests pass. Hopefully it will all work properly on actual big-endian architectures as well.

[christos]

@Alexander88207_protonmail.com I remember you tested the patch when it was still a draft. Can you re-test please?

[Alexander88207_protonmail.com]

Thanks for the ping, sure!

Can you please update your patch so i can apply it cleanly?

error: patch failed: sys/dev/sound/pcm/pcm.h:124
error: sys/dev/sound/pcm/pcm.h: patch does not apply

Thanks.

[christos]

Reflect minor changes in D47932 so that patch can apply cleanly.

[christos]

@Alexander88207_protonmail.com now it should apply fine. Do not forget to apply D47932 and D48421 first.

[Alexander88207_protonmail.com]

[Creating objdir /usr/obj/usr/src/amd64.amd64/sys/GENERIC/modules/usr/src/sys/modules/sgx...]
--- machine ---
--- feeder_eq.o ---
In file included from /usr/src/sys/dev/sound/pcm/feeder_eq.c:43:
/usr/src/sys/dev/sound/pcm/pcm.h:124:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
--- modules-all ---
machine -> /usr/src/sys/amd64/include
--- feeder_eq.o ---
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:229:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  229 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:256:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  256 | static __always_inline __inline void
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:361:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  361 | static __always_inline __inline void
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:370:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  370 | static __always_inline __inline void
      |                        ^
--- modules-all ---
--- x86 ---
x86 -> /usr/src/sys/x86/include
--- i386 ---
i386 -> /usr/src/sys/i386/include
--- feeder_eq.o ---
6 errors generated.
--- modules-all ---
--- opt_hwpmc_hooks.h ---
ln -sf /usr/obj/usr/src/amd64.amd64/sys/GENERIC/opt_hwpmc_hooks.h opt_hwpmc_hooks.h
--- opt_kstack_pages.h ---
--- all_subdir_sge ---
ctfconvert -L VERSION -g if_sge.o
--- all_subdir_sgx ---
ln -sf /usr/obj/usr/src/amd64.amd64/sys/GENERIC/opt_kstack_pages.h opt_kstack_pages.h
--- feeder_eq.o ---
*** [feeder_eq.o] Error code 1

make[2]: stopped in /usr/obj/usr/src/amd64.amd64/sys/GENERIC
--- modules-all ---
*** [modules-all] Error code 6

make[2]: stopped in /usr/obj/usr/src/amd64.amd64/sys/GENERIC
--- hdaa.o ---
ctfconvert -L VERSION -g hdaa.o
2 errors

[christos]

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

[markj]

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

[christos]

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

[christos]

In D47638#1117358, @christos wrote:

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

It compiles fine.

[markj]

In D47638#1117374, @christos wrote:

In D47638#1117358, @christos wrote:

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

It compiles fine.

I think that's reasonable.

[christos]

Reflect changes in D47932.

[christos]

@Alexander88207_protonmail.com could you apply D47932, D48421 and D47638 again and see if everything works fine?

[christos]

Reflect changes in D47932.

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

Rounding toward zero introduces a crossover distortion bias. Although quantization error itself is unavoidable here, we shouldn't do that.
lrintf or roundf or equivalent is needed.

Tested values -> results -> errors
{F111904629}

Error with INTPCM_T(fv * (float)PCM_S32_MAX);
{F111904009}

Error with INTPCM_T(roundf(fv * (float)PCM_S32_MAX));
{F111904015}

Test programs
{F111903997}
{F111904003}

[christos]

sys/dev/sound/pcm/pcm.h

207

That's very useful information, which I admittedly didn't take into consideration. However, I cannot access those files. Can you share them somehow else?

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

https://gist.github.com/therontarigo/7c7f642d7baa4c053285f6eaf4be2f1f

[christos]

sys/dev/sound/pcm/pcm.h

207

Thank you very much for this. :-)

Updating the diff in a moment.

[christos]

sys/dev/sound/pcm/pcm.h

207

Hmm, while we can call roundf() in userland, it cannot be used in the kernel, which where this code is executed in real-world scenarios.

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

Considering that use of AFMT_FLOAT is discouraged in new programs and that the motivation for this review is to support certain closed libraries (please correct me if I am mistaken), is it reasonable to only support this on CPUs with SSE2, which introduced the efficient rounding instructions?

Otherwise, I can provide a slower though still reasonably efficient non-FPU conversion routine. Even if fpu_kern_enter()/fpu_kern_leave() is moved outside the per-sample loop, avoiding FPU may still be a win depending on write size.

[christos]

sys/dev/sound/pcm/pcm.h

207

Feel free to share the non-FPU solution and we can definitely discuss it. I have no objection towards not using fpu_kern(9), in fact, in the crude benchmarks I've posted above you can see that using it makes things around 10 times more expensive for each sample.

[darkvincentdude_yahoo.com]

It looks like the patch fails to apply against main:

$ git apply D47638.diff
error: patch failed: sys/dev/sound/pcm/feeder_mixer.c:73
error: sys/dev/sound/pcm/feeder_mixer.c: patch does not apply

[christos]

Rebase.

[christos]

In D47638#1125021, @darkvincentdude_yahoo.com wrote:

It looks like the patch fails to apply against main:

$ git apply D47638.diff
error: patch failed: sys/dev/sound/pcm/feeder_mixer.c:73
error: sys/dev/sound/pcm/feeder_mixer.c: patch does not apply

I forgot to rebase it since I committed some patches. Try again. :-)

[darkvincentdude_yahoo.com]

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

[christos]

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

[darkvincentdude_yahoo.com]

In D47638#1125224, @christos wrote:

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

Well, audio works correctly in the game if I choose PulseAudio or Alsa as the Wine sound backends (via winetricks) in the game. So it's only by using the OSS audio backend that sound is all static. Other games with fixed audio (with the patch) are using the OSS backend.

[christos]

In D47638#1125676, @darkvincentdude_yahoo.com wrote:

In D47638#1125224, @christos wrote:

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

Well, audio works correctly in the game if I choose PulseAudio or Alsa as the Wine sound backends (via winetricks) in the game. So it's only by using the OSS audio backend that sound is all static. Other games with fixed audio (with the patch) are using the OSS backend.

That could also be a fault in Cyberpunks OSS code though. But you could try setting hw.snd.verbose=2 and seeing if there any sound(4) warnings.

[jrtc27]

sys/dev/sound/pcm/pcm.h

204

Not all architectures support FPU use in the kernel.

[christos]

sys/dev/sound/pcm/pcm.h

204

According to the man page:

The fpu_kern is currently implemented only for the i386, amd64, arm64,
and powerpc architectures.

And after a quick grep for ^fpu_kern_enter():

sys/i386/i386/npx.c:fpu_kern_enter(struct thread *td, struct fpu_kern_ctx *ctx, u_int flags)
sys/amd64/amd64/fpu.c:fpu_kern_enter(struct thread *td, struct fpu_kern_ctx *ctx, u_int flags)
sys/powerpc/powerpc/fpu.c:fpu_kern_enter(struct thread *td, struct fpu_kern_ctx *ctx, u_int flags)
sys/arm64/arm64/vfp.c:fpu_kern_enter(struct thread *td, struct fpu_kern_ctx *ctx, u_int flags)
sys/arm/arm/vfp.c:fpu_kern_enter(struct thread *td, struct fpu_kern_ctx *ctx, u_int flags)

So it seems like all the main architecture are supported. Which ones are not, except riscv? I guess we could add and #ifdef to support AFMT_FLOAT only for supported architectures.

[christos]

sys/dev/sound/pcm/pcm.h

204

Also, I have no objections towards a non-FPU solution to this, but I think it's better to (for now) push the patch as-is so that some ports that need this patch can work again ASAP.

[emaste]

sys/dev/sound/pcm/pcm.h

204

At a minimum we can't break building kernels on other architectures

[christos]

sys/dev/sound/pcm/pcm.h

204

I agree. We can go ahead with an #ifdef until a non-FPU solution is implemented.

[christos]

Use IEEE754 instead of the FPU, since some platforms do not support this. The
code is based on Ariff's AFMT_FLOAT draft [1]. Unit and audio tests pass
normally. That should also give us a performance boost.

[1] https://people.freebsd.org/~ariff/utils/ieee754.c

[emaste]

Use IEEE754 instead of the FPU

IEEE754 is the representation for floating point numbers, also used by the FPU and it's not something we're using "instead of" the FPU. Maybe "integer fixed point conversion"

sys/dev/sound/pcm/pcm.h

131–132

s could be bool
and maybe expand it to sign?

209

with s being bool this can just be s = v & 0x80000000U

[christos]

In D47638#1129320, @emaste wrote:

Use IEEE754 instead of the FPU

IEEE754 is the representation for floating point numbers, also used by the FPU and it's not something we're using "instead of" the FPU. Maybe "integer fixed point conversion"

Yeah, that was bad wording. It'd be more clear to say "instead of fpu_kern(9)".

sys/dev/sound/pcm/pcm.h

131–132

I agree with changing to bool. As for expanding the name, I don't have a strong objection, but I just kept it like this for uniformity with the other variables. After all the meaning of this variable is quite clear from the code IMHO.

[christos]

Address Ed's comments.

[emaste]

OK. Please include a reference to the source of the float conversion routines in the commit message.