Paths

Table of Contentst

sound: Unit test the pcm sample read and write macros
ClosedPublic
Actions

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

Details

Reviewers

christos
markj
emaste

Commits

rG5edda2488cc5: sound: Unit test the pcm sample read and write macros
rG27ef5d48c729: sound: Unit test the pcm sample read and write macros

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 61641
Build 58525: 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
24	Can this be `const`?
63
118

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
20	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.
201	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
207	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
22	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.

christos added inline comments.Tue, Jan 7, 6:58 PM

tests/sys/sound/pcm_read_write.c
22	Scratch that. Probably it'll get more confusing since we're using that value as input in the write test.

christos mentioned this in D47638: sound: Implement AFMT_FLOAT support.Wed, Jan 8, 4:39 PM

Address some of the comments:

Turn test data into a const array.
Read samples from the local buffer, as intended.
Test u-law and A-law sample formats.
Extra test case to check the AFMT_BIT macro for correctness.
Add a high level comment on the scope of the tests.

Harbormaster completed remote builds in B61641: Diff 149040.Fri, Jan 10, 11:20 AM

dev_submerge.ch marked 7 inline comments as done.Fri, Jan 10, 11:57 AM

dev_submerge.ch added inline comments.

tests/sys/sound/pcm_read_write.c
20	We already use `AFMT_BIT()` in the implementation (and the mock helper functions here). An error in that macro could make the tests succeed despite the results being wrong. Also we can now test this macro for correctness, see below.
22	Actually, we should strive for some consistent naming, here in the test and the implementation. I don't have a better name yet for "value" (original sample value, face value), but I would suggest "normalized" for the shifted one (normalized to 32bit magnitude). How about that?
63	This would require changes throughout the current implementation of the `_PCM_READ_*()` macros. While I fully agree that this is a good idea, let's postpone it to when we adapt the test to Christos' refactored implementation (it's not const there yet ATM). Ok for you?
118	Same as above, since const is transitive.
201	Number of the beast - indicates that the implementation is possessed by evil bugs. ;-) Seriously, I just wanted a pattern that stands out with the test values and expected transformations on them. There's other possibilities though. Also in the previous code, I didn't even use the local buffer as intended, fixed now.
207	The format alone isn't unique, and I didn't find a convenient way to print it. Suggestions? As an alternative we could label the rows in the test data in some semi-descriptive way.

markj added inline comments.Fri, Jan 10, 2:30 PM

tests/sys/sound/pcm_read_write.c
63	That's perfectly fine with me. I can't comment much on the meat of the tests so I'm just looking at implementation nits. Thanks for writing these.

christos added inline comments.Fri, Jan 10, 5:55 PM

tests/sys/sound/pcm_read_write.c
22	I think "normalized" is good. I can modify the `pcm_sample_read\|write_shift()` functions to say "normalize" instead of "shift".
207	You mean not unique, as in, you don't know which case exactly it's referring to since we are using each format more than once? If yes, then that shouldn't be a problem because the error message also prints the input/expected output, so we can match the exact test case easily. We could either print the format hex, or make the `snd_afmt2str()` function from `pcm/channel.c` accessible here.

Extend tests and restructure test data:

Label each row of test data explicitly.
Replace normalized test values with non-UB normalize function.
Test read / write macros that use 24 bit samples for 32 bit calculation.
Rename some helper functions for clarity.

Harbormaster completed remote builds in B61787: Diff 149468.Fri, Jan 17, 7:30 PM

Hopefully the last update, sorry for the breakage in the commits based on this. If there's no further issues, we should get this into CI for all supported architectures - then verify that this test passes everywhere before merging the followup commits with the refactorings.

markj accepted this revision.Sat, Jan 18, 3:10 PM

This revision is now accepted and ready to land.Sat, Jan 18, 3:10 PM

christos accepted this revision.Tue, Jan 21, 11:40 AM

christos retitled this revision from sound: Unit test the pcm sample read and write macros. to sound: Unit test the pcm sample read and write macros.Tue, Jan 21, 11:56 AM

Closed by commit rG27ef5d48c729: sound: Unit test the pcm sample read and write macros (authored by dev_submerge.ch, committed by christos). · Explain WhyTue, Jan 21, 12:04 PM

This revision was automatically updated to reflect the committed changes.

christos added a commit: rG27ef5d48c729: sound: Unit test the pcm sample read and write macros.

Pushed a fix for gcc builds: https://cgit.freebsd.org/src/commit/?id=f6631da0b581b28c2bfeea1199b52013bb46aa41

In D48330#1108326, @christos wrote:

Pushed a fix for gcc builds: https://cgit.freebsd.org/src/commit/?id=f6631da0b581b28c2bfeea1199b52013bb46aa41

@christos, thanks - do we have some feedback on running the tests by now? I only know of ci.freebsd.org, and I couldn't find anything about test runs.

In D48330#1108350, @dev_submerge.ch wrote:

In D48330#1108326, @christos wrote:

Pushed a fix for gcc builds: https://cgit.freebsd.org/src/commit/?id=f6631da0b581b28c2bfeea1199b52013bb46aa41

@christos, thanks - do we have some feedback on running the tests by now? I only know of ci.freebsd.org, and I couldn't find anything about test runs.