sound: Refactor the format conversion framework

[christos]

Summary

Merge the PCM_READ|WRITE_* macros defined in pcm/pcm.h, as well as the
intpcm_read|write_* macros defined in pcm/feeder_format.c, into six
inline functions: pcm_sample_read|write[_norm|calc](). The absence of
macro magic makes the code significantly easier to read, use and modify.

Since these functions take the input/output format as a parameter, get
rid of the read() and write() function pointers defined in struct
feed_format_info, as well as the feeder_format_read|write_op()
functions, and use the new read/write functions directly.

Sponsored by: The FreeBSD Fondation
MFC after: 1 week

Test Plan

Apply D48330 before this patch and run the tests.

[christos]

• Remove unit test in favor of D48330. Work will continue there. Depend on it.
• Make shift functions inline, instead of macros.
• Mark read/write functions as __unused to suppress compiler warnings about unused shift functions in various files.
• Fix regressions with unsigned format conversion. Now passes D48330 tests.
• Modify D48330 to work with the refactor.

BTW, I hope you all had a pleasant start into 2025!

Happy new year! :-)

[dev_submerge.ch]

There's actually three different sample value types involved here:

1. Original magnitude of the sample format (_PCM_READ_*() and _PCM_WRITE_*()).
2. Original magnitude, shift to 24 bit for 32 bit processing (PCM_READ_*() and PCM_WRITE_*()).
3. Normalized to 32 bit magnitude (intpcm_read_*() and intpcm_write_*()).

The refactoring is currently missing the first, I suggest that pcm_sample_read() and pcm_sample_write() should implement that instead of the second (see inline comments). We can handle the second type in a separate function. The third type then becomes pcm_sample_read_shift() and pcm_sample_write_shift().

sys/dev/sound/pcm/pcm.h

231–234

This should be factored out into a separate function.

251–254

This should be factored out into a separate function.

[christos]

This comment was removed by christos.

[christos]

This comment was removed by christos.

[christos]

This comment was removed by christos.

[christos]

Scratch that. I remembered the reason for the shift down to 24-bits is that if we are on a 32-bit machine, then can save some space this way and avoid overflows...

[christos]

sys/dev/sound/pcm/pcm.h

251–254

The PCM_WRITE_* are unused, so I think we can get away with not implementing this. No?

[christos]

• Depend on D48077. The test won't compile without this patch.
• Introduce pcm_sample_read_24bit() and use in place of PCM_READ_*. I did not implement pcm_sample_write_24bit() respectively, since PCM_WRITE_* was never used.

[dev_submerge.ch]

sys/dev/sound/pcm/pcm.h

75–77

I don't think we can remove this (yet)?

251–254

It's not strictly necessary to implement it, but it's only unused because the original code includes the shift back to 32bit in the clamp macro which is ... at least non-obvious. If we want to untangle that convolution (I would), there will be a need to do the shift to 32bit outside of the clamp function.

My idea was actually to have this 32bit <-> 24bit transformation (or no-op) in a separate function without calling pcm_sample_write() or pcm_sample_read(), but never mind. What is important is to make it obvious what sample magnitude each feeder code is dealing with.

[dev_submerge.ch]

sys/dev/sound/pcm/pcm.h

151

As originally suggested by @markj in D48330.

227

233

[christos]

Address comments.

[christos]

sys/dev/sound/pcm/pcm.h

75–77

This was a leftover from an experimental change. The diff is fixed already, but I didn't want to keep spamming everyone with more updates. :-)

251–254

I will implement the function, but for now I will keep it unused, since it'd require to change the logic of the clamp function as well in order to use pcm_sample_write_24bit() and not do the shift in the clamp function. I think it's better to implement this in a followup patch to not over-complicate this one.

[christos]

Use pcm_sample_write_24bit() instead of calling PCM_CLAMP_* and then
passing the result to pcm_sample_write(). I said I would do it in a separate
patch, but I think it's not that bad to have it here.

[christos]

sys/dev/sound/pcm/feeder_mixer.c

69

We are discussing this in D48394 as well, but we lose precision here, and in the other call of pcm_sample_write_24bit(). If this function is called with a 32-bit format, then the type assigned to z will be int64_t if SND_PCM_64 is set. pcm_sample_write_*() take the sample as intpcm_t, aka int32_t.

I think we need to address this intpcm_t thing ASAP. It's quite confusing.

[christos]

Include assert.h if we're not in the kernel, and also set v to in
pcm_sample_read() default case, so that the compiler doesn't complain
when building the tests.

[christos]

• Rename *_shift to *_norm and *_24bit to *_calc, similar to D48330.
• Adapt D48330. Tests pass.

[christos]

@dev_submerge.ch Both 32 and 64 bit tests pass normally (with the fix mentioned in D48330). Are we done with this one?

[dev_submerge.ch]

In D47932#1110250, @christos wrote:

@dev_submerge.ch Both 32 and 64 bit tests pass normally (with the fix mentioned in D48330). Are we done with this one?

Nope. First we need to know whether the unit test is correct and passes on all architectures, AFAICT we only have little endian results yet.

[dev_submerge.ch]

sys/dev/sound/pcm/feeder_volume.c

68–72

Is there a reason you did omit the x = PCM_CLAMP_##SIGN##BIT(v); here? We might run into overflows without it, no?

[christos]

sys/dev/sound/pcm/feeder_volume.c

68–72

I might be wrong, but I used the _calc variant intentionally, so that we can also clamp 32-bit samples when SND_PCM_64 is not set. Since we are always working with int32_ts essentially, I'm not sure how we'd end up with an overflow in the <32-bit cases.

[dev_submerge.ch]

sys/dev/sound/pcm/feeder_volume.c

68–72

The _calc variants do not clamp. You need an extra function for that. As far as I understand, FEEDVOLUME_CALC##BIT(x, vol[matrix[i]]) may boost the volume and thus generate values that overflow the bit resolution of the sample format here. If we just write values as is, we'll get truncated noise samples instead of just clipping with clamped values.

[christos]

Address comments.

[christos]

I think with the recent fixes to the tests, and since all comments here have been addressed, we could go ahead with this?

[markj]

sys/dev/sound/pcm/pcm.h

150

What's with the __unused annotations? Is it to quiet warnings about unused functions when this header is included? I would expect __always_inline to be sufficient.

[christos]

sys/dev/sound/pcm/pcm.h

150

Indeed I added __unused to silence warnings; just __always_inline does not seem to be enough.

In file included from /mnt/src/sys/dev/sound/pcm/feeder_rate.c:58:
/mnt/src/sys/dev/sound/pcm/pcm.h:225:1: warning: unused function 'pcm_sample_read_norm' [-Wunused-function]
  225 | pcm_sample_read_norm(const uint8_t *src, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:234:1: warning: unused function 'pcm_sample_read_calc' [-Wunused-function]
  234 | pcm_sample_read_calc(const uint8_t *src, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:366:1: warning: unused function 'pcm_sample_write_norm' [-Wunused-function]
  366 | pcm_sample_write_norm(uint8_t *dst, intpcm_t v, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:375:1: warning: unused function 'pcm_sample_write_calc' [-Wunused-function]
  375 | pcm_sample_write_calc(uint8_t *dst, intpcm_t v, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~~
4 warnings generated.

That's just a sample. There are more warnings.

[markj]

sys/dev/sound/pcm/pcm.h

150

What happens if you add __inline after __always_inline?

[christos]

sys/dev/sound/pcm/pcm.h

150

That works. Should we go with this instead of __unused?

[markj]

sys/dev/sound/pcm/pcm.h

150

Yes please.

[christos]

Use __inline instead __unused to silence warnings.

[christos]

Remove __always_inline. Related discussion in D47638.

[dev_submerge.ch]

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

[christos]

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

[dev_submerge.ch]

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation. Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

[markj]

In D47932#1118466, @dev_submerge.ch wrote:

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation.

My complaint about the original patch is that the __unused function is misleading, as the functions are not unused. It's very common for us to define static functions in header files, and they're virtually all defined __inline precisely to suppress warnings about unused functions, rather than out of any particular concern about controlling whether or not they do get inlined. So, yes, this is an abuse of the specifier, but all else being equal I'd rather handle that problem consistently within the tree.

Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

In the absence of a good solution that's known to work with both clang and gcc, I withdraw my objection to using __always_inline+__unused, but I'm still a bit skeptical that that's a good idea. A compiler might well warn about not being able to honour __always_inline, and if pcm.h is ever installed to /usr/include (I assumed it already was, but apparently not), we won't control the compiler, so might easily see new problems.

Another solution would be to replace the __inline annotations with PCM_INLINE, and add #define PCM_INLINE __always_inline in compilation units where you really care about it, and let it default to __inline otherwise. But, since I'm not contributing to this area, I won't insist on anything in particular.

[dev_submerge.ch]

In D47932#1118481, @markj wrote:

In D47932#1118466, @dev_submerge.ch wrote:

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation.

My complaint about the original patch is that the __unused function is misleading, as the functions are not unused. It's very common for us to define static functions in header files, and they're virtually all defined __inline precisely to suppress warnings about unused functions, rather than out of any particular concern about controlling whether or not they do get inlined. So, yes, this is an abuse of the specifier, but all else being equal I'd rather handle that problem consistently within the tree.

Ok, I see now where this is coming from. But since that abuse of __inline does not work together with the always inline that we want here, to me it seems like we're fixing the wrong end.

Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

In the absence of a good solution that's known to work with both clang and gcc, I withdraw my objection to using __always_inline+__unused, but I'm still a bit skeptical that that's a good idea. A compiler might well warn about not being able to honour __always_inline, and if pcm.h is ever installed to /usr/include (I assumed it already was, but apparently not), we won't control the compiler, so might easily see new problems.

Another solution would be to replace the __inline annotations with PCM_INLINE, and add #define PCM_INLINE __always_inline in compilation units where you really care about it, and let it default to __inline otherwise. But, since I'm not contributing to this area, I won't insist on anything in particular.

The alternatives I see are

• Avoid the definition of unused functions, through separate headers for example. I suspect we'll reduce the number of different read / write functions anyway.
• Make these functions extern instead of static, implement them in one translation unit.
• Different workarounds with compiler directions or flags.

Unless we're pressed to, I would really want to instruct the compiler to inline these functions whenever possible.

[christos]

In D47932#1118510, @dev_submerge.ch wrote:

The alternatives I see are

• Avoid the definition of unused functions, through separate headers for example. I suspect we'll reduce the number of different read / write functions anyway.
• Make these functions extern instead of static, implement them in one translation unit.
• Different workarounds with compiler directions or flags.

Unless we're pressed to, I would really want to instruct the compiler to inline these functions whenever possible.

I am voting for the second approach, seems like the most straight-forward solution.

[christos]

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

[christos]

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

[dev_submerge.ch]

In D47932#1120121, @christos wrote:

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

It looks like you moved the (one and only) implementation of the pcm_*() functions into feeder_format.c? That's not what I meant, this would prevent any inlining, completely. For a simpler solution, why don't you start with the previous approach where you stumbled upon the "unused" warnings, move the pcm_*() functions into separate headers (one for each pair), and then just include where we really use them?
I think that would even make good sense of the "unused" warnings as they are intended to work.

Also I'm sorry I can't contribute more often currently, I just started a new job and my spare time is really spread thin these days.

[christos]

In D47932#1120403, @dev_submerge.ch wrote:

In D47932#1120121, @christos wrote:

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

It looks like you moved the (one and only) implementation of the pcm_*() functions into feeder_format.c? That's not what I meant, this would prevent any inlining, completely.

I know. I only posted this as a WIP initially.

For a simpler solution, why don't you start with the previous approach where you stumbled upon the "unused" warnings, move the pcm_*() functions into separate headers (one for each pair), and then just include where we really use them?
I think that would even make good sense of the "unused" warnings as they are intended to work.

The only reason I didn't do this already is because of tidiness, since we'll introduce 2 more headers for 4 functions, although I'm not sure whether that's a good enough reason not to do it. Personally I wouldn't mind leaving them all in pcm.h with __unused, but if it's best to avoid it, then we can look into this approach. @markj what do you think?

Also I'm sorry I can't contribute more often currently, I just started a new job and my spare time is really spread thin these days.

No worries, I appreciate that you give part of your free time to review all my never-ending patches. Good luck with your new job. :-)

[christos]

I think we should just go ahead with using __always_inline __unused for now.

[christos]

Return to __always_inline __unused. I think it is better to leave it like
that for now and avoid complicating the code base further.