Create 4 C files to show different polling mechanisms used with sound(4): simple, select, poll, kqueue.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I hope I addressed all the comments except showing example for both polling directions and readme. I think if a person understands polling in one direction, it is easy to extrapolate how to work with two directions. As these are examples, not complete guides, I would argue that this is good enough, especially in kqueue case where both directions would create two events and example would become more complicated. As I do plan to add mmap example once this is merged, that example will have input device and output device so even that configuration will be shown, I just don't think that all examples should split the sound card into two directions. As for the readme, good catch, I didn't think of that, but let's make the code in the examples good and I'll change the readme to correspond to those changes.
Comment Actions
- All relevant comments I left in the kqueue example apply to the rest of the examples, of course.
- It seems like in all examples, apart from simple.c, ibuf and obuf are the same. Couldn't we just remove one and keep a buf only? This way we avoid the memcpy() and the 2nd malloc().
- Regarding the README, I think it'd be even better to move its content into comments inside oss.h in the appropriate places. This would make it easier to understand both what the code does exactly (especially in the fragment setting parts), as well as making sure the README is read, because currently I think it's quite easy to miss it.
| share/examples/sound/oss/kqueue.c | ||
|---|---|---|
| 42 ↗ | (On Diff #165629) | I think this is a bit non-obvious unless you also read what oss_init() does. I suppose we could optionally initialize it, and check against channels == 0 (as opposed to channels == -1) in oss_init(). |
| 44 ↗ | (On Diff #165629) | This gets set in oss_init() so we do not need to specify a value here. |
| 46 ↗ | (On Diff #165629) | It'd make more sense to calculate this value in oss_init(), otherwise this can be quite error prone. It will be more verbose, but we could have a helper function with a switch statement that returns the number of sample bytes/bits depending on the format. We do that already in some places (e.g., virtual_oss). sys/dev/sound/pcm/sound.h includes some helpful macros (e.g., AFMT_BIT()) for doing that automatically, but they are not available in soundcard.h. I might move them over at some point. |
| 47 ↗ | (On Diff #165629) | Ditto with frag comment. |
| 48 ↗ | (On Diff #165629) | Ditto with frag comment. |
| 60 ↗ | (On Diff #165629) | Since we do not use process_audio() anymore, we do not need that. |
| 76 ↗ | (On Diff #165583) | This wasn't addressed. |
| share/examples/sound/oss/oss.h | ||
| 71 ↗ | (On Diff #165629) | |
| 88 ↗ | (On Diff #165629) | This should be errx(), as there is no errno to print. |
| 93 ↗ | (On Diff #165629) | Ditto. |
| 130 ↗ | (On Diff #165629) | Ditto. |
Comment Actions
The directory where these files live is "oss". As everything is about OSS in "sound" directory, should I perhaps rename oss to audio? My reasoning is that there are audio and MIDI examples that we can show, so "oss" is a bit missleading. Or maybe have share/examples/oss and audio and midi under it? Anyway, when I started writing the examples I only wrote audio part, so "sound" was appropriate, which probably isn't the case any more.
Comment Actions
I would say putting everything under share/examples/sound is reasonable, since OSS is our built-in sound system, so IMHO it is implied that examples refer to OSS. MIDI examples can just live under the same directory, I don't see a problem here.
| share/examples/sound/oss/kqueue.c | ||
|---|---|---|
| 57 ↗ | (On Diff #165671) | |
| 59 ↗ | (On Diff #165671) | Alternatively, all the loop errors (and in the other examples) can be just a warn() followed by a break, so that we execute the code after the loop, otherwise it will be unreachable, considering we have an infinite loop. |
| share/examples/sound/oss/oss.h | ||
| 111 ↗ | (On Diff #165671) | We also have AFMT_U8. |
| 117 ↗ | (On Diff #165671) | AFMT_S24_* are missing. |
| 119 ↗ | (On Diff #165671) | I would say, for completeness, also add AFMT_F32_* since we support it now. |
| 127 ↗ | (On Diff #165671) | I suppose it might be helpful for someone, but I'm not sure setting the fragments for such a simple application is really necessary. |
| share/examples/sound/oss/simple.c | ||
| 41 ↗ | (On Diff #165671) | Doesn't the int32_t * presuppose that we use 32-bit samples? I think it would make more sense to pass uin8_t * and increment the pointer based on the sample size. I suspect this would break with a format smaller than 32-bits, no? |
Comment Actions
Some previous comments have not been addressed. It would help to be marking addressed comments as done since we have accumulated lots of comments.
| share/examples/sound/simple.c | ||
|---|---|---|
| 45 | Can't we just keep the loop as it was, but instead use only uint8_t * and advance the pointer like i += config->sample_size? | |
Comment Actions
I missed old comments as I moved files from oss directory. It took me a while to realize how to read them. Sorry for the noise.
Comment Actions
I hope I found all comments. It was not obvious I would have to click on a line number beside comment to get old comments.
Comment Actions
I just remembered one more comment I can't find. It is about setting fragments. I'd like to leave it there and in the comments mention that setting fragments is optional. I'd like to have a full examples so people interested in sound don't have to wonder how things are done. To be honest, official OSS documentation could use some improvement. At least I found it hard to understand what exactly I need to do.
Comment Actions
I think we are getting close to finishing this. Can you also make sure that the to_interleaved() and to_channels() functions are actually working properly?
| share/examples/sound/oss.h | ||
|---|---|---|
| 78 | I think we could improve the wording of this comment a bit. Some stuff seems to be repeating, and there are also a few typos here and there. | |
| 168–171 | You can update this to mention sndctl(8) instead of /dev/sndstat. You can view the software and hardware buffers for each channel by running sndctl swbuf hwbuf. | |
| share/examples/sound/simple.c | ||
| 40 | We should note that these two functions (to_interleaved() and to_channels()) are actually not actually necessary for the example here), and also explain why someone might want to use this functionality in their program. In what scenarios would we want to have the buffer laid out in channel format and in interleaved format? | |
| 44 | Why int8_t * and not uint8_t *? We are parsing the buffer as bytes here. | |
| 48 | This says sample but it doesn't actually hold a sample value, but the sample's index (?). I think this is a bit misleading. It would also be good, since this is an example, to explain what every line calculates exactly. | |
| 51–52 | ||
| 69–70 | ||
| share/examples/sound/simple.c | ||
|---|---|---|
| 51–52 | This can't be right. I want pointer to a location in a buffer, not it's value, which ((uint8_t *)config->buf)[i] will return. | |
Comment Actions
I tested all examples with real hardware. Well, all except kqueue, as the machine with the actual hardware is 14.3 based.
Comment Actions
I just realized I copy/pasted the same first license comment everywhere. Can you tell me what should I put in the "Copyright" lines, please?
Comment Actions
All of my inline comments are mostly grammar/typo fixes. If you tested the most recent code (especially simple.c) with the most recent changes, then we'll be good to go. I'll commit this once you address the last comments. Thanks a lot for this! :-)
They are fine. Only simple.c has an outdated year next to your name, but since you modified almost the entire file, you can update the year for your name.
| share/examples/sound/oss.h | ||
|---|---|---|
| 29 | ||
| 57 | ||
| 59 | ||
| 60 | ||
| 61 | ||
| 62 | ||
| 63 | ||
| 64 | ||
| 65 | ||
| 115 | ||
| 154 | ||
| 156 | ||
| 158–159 | ||
| 160 | ||
| 161 | ||
| 162 | ||
| 165 | ||
| 166 | ||
| 167 | ||
| share/examples/sound/simple.c | ||
| 36 | ||
| 38 | ||
| 66 | ||
| 76 | We should clarify whose "index" we are talking about. | |
| 85 | ||
| 119 | ||
| 120 | ||
| 121 | ||
Comment Actions
I can actually fix those last comments myself, no need to waste your time for this. Just let me know if tests worked fine on your machine. :-)
Comment Actions
I already fixed those as well as share/examples/Makefile, I just want to check if everything is working, and that has to wait for my day job to finish.
Comment Actions
I tested simple, select and poll on the real hardware. As that machine is not running CURRENT, I can't test kqueue. @christos, do you have any means on testing it on real hardware? I tried to test it in bhyve with -s 6,hda,play=/dev/dsp,rec=/dev/dsp but none of the examples work properly that way.
Comment Actions
All sound tests work fine! By the way, you can passthrough PCI slots to bhyve, this way you can, for example passthrough your USB soundcards etc. Look up the -p option of share/examples/bhyve/vmrun.sh, as well as the vmm.4 (man page).