sound: Unit test the pcm sample read and write macros.
Needs ReviewPublic
Actions

Authored by dev_submerge.ch on Mon, Jan 6, 12:41 AM.

Details

Reviewers

christos
markj
emaste

Summary

Main goal is to have a unit test, with sample test data that is verified
against the current macro implementation of pcm sample read and write
functions. With a test in place, we can proceed on a planned refactoring
of the sample read and write code, and confidently check the new code
for regressions.
Implementation of the unit test itself has to avoid any cast or
conversion affected by endianness, to make the tests compatible with
all machine architectures.

Preliminary version for discussion and testing, still missing some more
exotic sample formats.

MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 61514
Build 58398: arc lint + arc unit

Event Timeline

dev_submerge.ch created this revision.Mon, Jan 6, 12:41 AM

Herald added a subscriber: imp. · View Herald TranscriptMon, Jan 6, 12:41 AM

dev_submerge.ch requested review of this revision.Mon, Jan 6, 12:41 AM

Harbormaster completed remote builds in B61514: Diff 148799.Mon, Jan 6, 12:42 AM

Christos, I hope it's ok for me to "hijack" your refactoring efforts with this unit test, but I think it's the proper way forward with D47932. If this test succeeds now on all supported architectures, I'll be much more confident about the correctness of the test and test data. We can then adapt it to test your refactored pcm_sample_read() and pcm_sample_write() functions.

I intentionally left out the 32bit calculation case when SND_PCM_64 is not set, the PCM_FXSHIFT shift is rather trivial and I suspect it's currently broken.

The test is not complete yet, but I suppose we can already use it to work out some of the problems in D47932.

This broadly looks good to me. I'd suggest having a comment somewhere which explains at a higher level what the tests are testing.

tests/sys/sound/pcm_read_write.c
23	Can this be `const`?
62
117

AFMT_AC3 is a NO-OP, so we can either not test it at all, or check if it's 0 for now. For AFMT_MU_LAW and AFMT_A_LAW we will have to make use of the tables defined in pcm/g711.h, which is probably not very helpful, since we'll essentially duplicate the sound(4) code here.

tests/sys/sound/pcm_read_write.c
19	Why not use `AFMT_BIT(test->format) / 8` directly in the `memcpy()` instead? I think it is simple enough that it doesn't really obfuscate the logic, and, although not related to the test, this would also make sure `AFMT_BIT()` works correctly, as a side effect.
200	Why `0x66`?

christos added a child revision: D47932: sound: Refactor the format conversion framework.Tue, Jan 7, 5:20 PM

christos added inline comments.Tue, Jan 7, 5:27 PM

tests/sys/sound/pcm_read_write.c
206	I think it'd be helpful to print the format here and in the other `ATF_CHECK_MSG()`s as well.

christos mentioned this in D47932: sound: Refactor the format conversion framework.Tue, Jan 7, 5:49 PM

christos added inline comments.Tue, Jan 7, 5:49 PM

tests/sys/sound/pcm_read_write.c
21	I would probably rename this to `expect_norm` and `shifted` to `expect_shifted`. Or something along those lines. `value` sounds a bit too vague IMHO.