When setting OSS property, like format, number of channels or sample rate, kernel can set the property to a supported one instead of requested one. Check the property value after setting it to make sure it was not changed.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Check sample rate and format after ioctl call. While here, print sample size and number of channels of configured device.
Comment Actions
We can have this check for SNDCTL_DSP_CHANNELS as well. Also the commit title and message have to be adapted.
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 |
This wasn't addressed. | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 | But the code calls for errx in case the values are not equal, so what would be the point? | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 | Good catch. This should just be a printf to stderr. We do not need to fail because of this IMHO. | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 | Yes we do want to fail, because the rest of the code assumes int samples, which might not be what ioctl has put into the format. Same for channel number. Each case can produce read/write of memory pass the buffer limit. At least I thought that's what we're preventing here. While different sample rate won't do anything so drastic as format and channels will, it will still make the rest of the code believe in that it's operating under wrong rate and produce weird sound. As this is an example, I don't expect real world code to just break like this, but we're ending the program early to make the code minimal. | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 |
From what I see this seems to be the case only in simple.c where the buffer which stores the buffer after it's split into channels, is defined as int32_t *. However, that's a problem with simple.c. We should be flexible and not assume a specific data type. We already have config->sample_size and a void *buf, so apart from simple.c, we do not need to worry about this. IMHO we should modify the channels buffer in simple.c to be uint8_t * instead. | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 | But than channels wouldn't represent the channels, so what would be the point? The channels exist to show how to handle interleaved format and get per-channel samples. Having channels as uint8_t makes no sense. | |
| share/examples/sound/oss.h | ||
|---|---|---|
| 121 | I would suggest the failure be in simple.c, rather than fail for every use-case, and have a warnx() here only. After all, this is the only case where we want to make sure the format is exactly what we asked. | |
Comment Actions
Warn only if sample format/rate/channels were not set to desired values, but fail only in simple.c if the format is not the requested one.
Comment Actions
LGTM. Please take a look at the inline comments.
| share/examples/sound/oss.h | ||
|---|---|---|
| 128 | I think this is a bit confusing (and probably wrong since we are editing something that should be returned by OSS). We could store the number of channels in a struct variable. But this can be done in a follow-up patch. | |
| share/examples/sound/simple.c | ||
| 32 | Not needed. It's included by oss.h. | |
| share/examples/sound/simple.c | ||
|---|---|---|
| 32 | It sneaked up again on me, so I spent some time figuring out what's going on. It turns out my editor's automation does that when I use some symbol like AFMT_S32_NE and there's no proper include for it. It doesn't do deep check I guess. Anyway, I'll try to disable that specific feature of my editor, but in any case I know what to look for now. Sorry for the noise! | |