This currently shifts 32bit samples out of the 32bit range in case SND_PCM_64 is not set. sample << PCM_FXSHIFT happens twice. The root cause is in D47932 though.

sys/dev/sound/pcm/pcm.h
316	To match how `pcm_clamp()` is used in the macros, and make it 64bit in case `SND_PCM_64` is set (yes, bad type name).
333–335	This doesn't make sense to check for a 32bit signed int (`typedef int32_t intpcm_t;`). Also it's not what the original clamp checks if `SND_PCM_64` is not set.
339	I think this code should never be executed anyway. Means we can return 0, but please put an assertion here.

christos marked 2 inline comments as done.Sun, Jan 12, 8:10 PM

christos added inline comments.

sys/dev/sound/pcm/pcm.h
333–335	I did fix the clamp checks, but I'm not sure what you mean by "to check for a 32bit signed int".

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

sys/dev/sound/pcm/pcm.h
333–335	That was a tautology, you can forget about it if you fix the clamp checks and the type of the sample argument. It was just the first thing to cross my mind, that a 32bit signed integer cannot exceed 32bit signed max and min values, and thus the check doesn't make sense.

Address comments.

christos marked 2 inline comments as done.Sun, Jan 12, 11:14 PM

Harbormaster completed remote builds in B61685: Diff 149161.Sun, Jan 12, 11:15 PM

christos added a parent revision: D48077: include: add a userland version of __assert_unreachable.Sun, Jan 12, 11:15 PM

christos removed a parent revision: D48077: include: add a userland version of __assert_unreachable.

Reflect D47932 changes.

Harbormaster completed remote builds in B61756: Diff 149330.Thu, Jan 16, 1:19 PM

Reflect changes in D47932.

Harbormaster completed remote builds in B62144: Diff 150192.Thu, Jan 30, 3:37 PM

christos added a child revision: D48764: sound: Retire FEEDEQ_CLAMP().Fri, Jan 31, 3:03 PM

christos added a child revision: D48765: sound: Retire Z_CLAMP().

I have a proposal to simplify this code, may be worth considering. The general idea would be to

Use normalized, full 32bit magnitude values for calculation.
In feeder_mixer.c, for 32bit calculation, check potential overflow before doing the addition, substitute PCM_S32_MIN or PCM_S32_MAX if it would overflow.
In feeder_volume.c, always use 64bit multiplication but make it clear to the compiler that the volume value is 16bit.

For the mixer this just takes a bit more logic on 32bit architectures, and for the volume multiplying a 16bit sample with a 16bit volume would now be two multiplication ops. On the positive side, we can always maintain the full 32bit resolution, and get rid of the whole *_calc() sample handling.

Does that make sense?

sys/dev/sound/pcm/pcm.h
333–335	This is still incorrect in case of 32bit calculations. The macro checks for 24bit min and max values in that case, and we're missing the shift back to 32bit magnitude before writing the sample value. But there may be a better solution, see my general comment.