Details

Reviewers

markj
emaste
dev_submerge.ch

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 61755
Build 58639: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Notice that in the test code I left some comments, namely:

Since the results are hardcoded, I'm not sure whether this will work on big-endian machines.
I left out tests (for now) for when SND_PCM_64 is not set.
I'm not sure whether we need to add tests for AFMT_MU_LAW and AFMT_A_LAW. See how pcm/pcm.h handles these formats.

dev_submerge.ch added inline comments.Dec 31 2024, 2:30 AM

sys/dev/sound/pcm/pcm.h
327–333	Why macros instead of inline functions?
tests/sys/sound/fmtconv.c
68 ↗	(On Diff #148558)	Shouldn't this value be negative? Same for the following unsigned sample formats?
158–162 ↗	(On Diff #148558)	That wasn't the idea, it can't work that way. You have to define the format independent of the machine's endianness, here that would be `AFMT_U32_LE`. We want to test that reading a little endian U32 sample of bytes `{ 0x81, 0x82, 0x83, 0x84 }` always results in the corresponding integer value of `0x04838281`. That should be true on any machine, regardless of it's endianness. For the same reason, the result of the write functions has to be a byte array. We want to test that writing a value of `0x04838281` as a little endian U32 sample results in a byte array of `{ 0x81, 0x82, 0x83, 0x84 }`, regardless of the machine's endianness. Don't compare integer values here, compare the resulting byte arrays. Then our test checks the right thing, without any endianness of the machine running it involved. Only the implementation of the read and write function has to care about endianness. Our test just checks the correctness of the results. Also you may want to create separate tables for read and write test data. I think you can then fit each test case on one line, tidying up the whole thing.
190 ↗	(On Diff #148558)	I'd prefer to avoid the undefined behavior of shifting a negative signed integer here, for the sake of testing. Can we add the shifted value to the test data? Alternatively we could reformulate the calculation without undefined behavior.

Please note that I'm working on a unit test which exercises the original sample read / write macros, to be committed before this refactoring. This way we can verify the test procedure and test data with the original code, then adapt it to the refactored code. It's still WIP, but I can already tell that we definitely have some regressions here.

Christos, please have a close look at the precise definition of arithmetic operators in C, regarding integer promotion. There's some subtle nuances about conversion order that affect your code.

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

dev_submerge.ch mentioned this in D48330: sound: Unit test the pcm sample read and write macros.Jan 6 2025, 1:21 AM

christos added a parent revision: D48330: sound: Unit test the pcm sample read and write macros.Jan 7 2025, 5:20 PM

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! :-)

Harbormaster completed remote builds in B61561: Diff 148897.Jan 7 2025, 5:49 PM

christos edited the summary of this revision. (Show Details)Jan 7 2025, 5:51 PM

christos edited the test plan for this revision. (Show Details)

christos added a child revision: D48421: sound: Turn PCM_CLAMP_* macros into a function.Jan 10 2025, 6:36 PM

dev_submerge.ch mentioned this in D48421: sound: Turn PCM_CLAMP_* macros into a function.Jan 12 2025, 1:36 AM

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

Original magnitude of the sample format (_PCM_READ_*() and _PCM_WRITE_*()).
Original magnitude, shift to 24 bit for 32 bit processing (PCM_READ_*() and PCM_WRITE_*()).
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
227–230	This should be factored out into a separate function.
247–250	This should be factored out into a separate function.

christos added a comment.Jan 12 2025, 8:31 PM

This comment was removed by christos.

christos added a comment.Jan 12 2025, 8:38 PM

This comment was removed by christos.

christos added a comment.Jan 12 2025, 8:46 PM

This comment was removed by 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 marked 2 inline comments as done.Jan 12 2025, 9:39 PM

christos added inline comments.

sys/dev/sound/pcm/pcm.h
247–250	The `PCM_WRITE_*` are unused, so I think we can get away with not implementing this. No?

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.

Harbormaster completed remote builds in B61683: Diff 149158.Jan 12 2025, 9:50 PM

christos added a parent revision: D48077: include: add a userland version of __assert_unreachable.Jan 12 2025, 9:51 PM

christos mentioned this in D48394: sound: Simplify pcm/feeder_mixer.c.Jan 12 2025, 9:54 PM

dev_submerge.ch added inline comments.Jan 12 2025, 11:43 PM

sys/dev/sound/pcm/pcm.h
75–77	I don't think we can remove this (yet)?
247–250	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 added inline comments.Jan 13 2025, 12:00 AM

sys/dev/sound/pcm/pcm.h
147	As originally suggested by @markj in D48330.
223
229

christos edited the summary of this revision. (Show Details)Jan 13 2025, 12:19 AM

christos edited the test plan for this revision. (Show Details)

Address comments.

Harbormaster completed remote builds in B61686: Diff 149162.Jan 13 2025, 12:20 AM

christos added inline comments.Jan 13 2025, 12:20 AM

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. :-)
247–250	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.

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.

Harbormaster completed remote builds in B61755: Diff 149329.Jan 16 2025, 1:13 PM

christos added inline comments.Jan 16 2025, 1:16 PM

sys/dev/sound/pcm/feeder_mixer.c
68	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.

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.

Harbormaster completed remote builds in B61758: Diff 149332.Jan 16 2025, 1:33 PM

christos edited the summary of this revision. (Show Details)Jan 21 2025, 11:30 AM

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

Harbormaster completed remote builds in B61859: Diff 149652.Jan 21 2025, 11:31 AM

christos mentioned this in D48036: sound: Remove feed_matrix_apply_generic().Jan 21 2025, 11:38 AM

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

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 added inline comments.Jan 26 2025, 5:48 PM

sys/dev/sound/pcm/feeder_volume.c
68–71	Is there a reason you did omit the `x = PCM_CLAMP_##SIGN##BIT(v);` here? We might run into overflows without it, no?

christos added inline comments.Jan 27 2025, 11:18 AM

sys/dev/sound/pcm/feeder_volume.c
68–71	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_t`s essentially, I'm not sure how we'd end up with an overflow in the <32-bit cases.

dev_submerge.ch added inline comments.Mon, Jan 27, 4:07 PM

sys/dev/sound/pcm/feeder_volume.c
68–71	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.

Address comments.

Harbormaster completed remote builds in B62143: Diff 150191.Thu, Jan 30, 3:32 PM

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

markj added inline comments.Tue, Feb 11, 3:12 PM

sys/dev/sound/pcm/pcm.h
146	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 added inline comments.Tue, Feb 11, 3:17 PM

sys/dev/sound/pcm/pcm.h

146

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 added inline comments.Tue, Feb 11, 3:20 PM

sys/dev/sound/pcm/pcm.h
146	What happens if you add `__inline` after `__always_inline`?

christos added inline comments.Tue, Feb 11, 3:24 PM

sys/dev/sound/pcm/pcm.h
146	That works. Should we go with this instead of `__unused`?

markj added inline comments.Tue, Feb 11, 3:25 PM

sys/dev/sound/pcm/pcm.h
146	Yes please.

Use __inline instead __unused to silence warnings.

Harbormaster completed remote builds in B62371: Diff 150846.Tue, Feb 11, 3:27 PM

Remove __always_inline. Related discussion in D47638.

Harbormaster completed remote builds in B62444: Diff 151036.Fri, Feb 14, 3:55 PM

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.

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.

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.

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.

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.

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.

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

Harbormaster completed remote builds in B62576: Diff 151314.Fri, Feb 21, 9:01 PM

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.

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.

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 removed parent revisions: D48077: include: add a userland version of __assert_unreachable, D48330: sound: Unit test the pcm sample read and write macros.Tue, Feb 25, 12:11 PM

christos added a child revision: D48032: sound: Remove macro magic from pcm/feeder_eq.c.Tue, Feb 25, 12:33 PM

sound: Refactor the format conversion framework
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 149329

sys/dev/sound/pcm/feeder.h

sys/dev/sound/pcm/feeder_eq.c

sys/dev/sound/pcm/feeder_format.c

sys/dev/sound/pcm/feeder_matrix.c

sys/dev/sound/pcm/feeder_mixer.c

sys/dev/sound/pcm/feeder_rate.c

sys/dev/sound/pcm/feeder_volume.c

sys/dev/sound/pcm/pcm.h

sys/dev/sound/pcm/sndstat.c

sys/dev/sound/pcm/sound.h

tests/sys/sound/pcm_read_write.c

sound: Refactor the format conversion frameworkNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 149329

sys/dev/sound/pcm/feeder.h

sys/dev/sound/pcm/feeder_eq.c

sys/dev/sound/pcm/feeder_format.c

sys/dev/sound/pcm/feeder_matrix.c

sys/dev/sound/pcm/feeder_mixer.c

sys/dev/sound/pcm/feeder_rate.c

sys/dev/sound/pcm/feeder_volume.c

sys/dev/sound/pcm/pcm.h

sys/dev/sound/pcm/sndstat.c

sys/dev/sound/pcm/sound.h

tests/sys/sound/pcm_read_write.c

sound: Refactor the format conversion framework
Needs ReviewPublic
Actions

Revision Contents
Changeset List