This example combines mmap and kqueue, but for some reason sound is bad sometimes, which suggests race condition. If kqueue+mmap combination has some fundamental problem, we should at least document why this approach is a bad idea.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
This is an initial quick review, as I haven't had a chance to properly test it yet.
Both configs in main() open /dev/dsp, so there is no need two have two configs and duplicate the logic twice. You can have one config but with mode = O_RDWR | O_EXCL | O_NONBLOCK, and call mmap() with PROT_READ | PROT_WRITE on the same FD. Or am I missing something?
| share/examples/sound/mmap.c | ||
|---|---|---|
| 40 ↗ | (On Diff #166400) | |
| 42 ↗ | (On Diff #166400) | |
| 47–49 ↗ | (On Diff #166400) | |
| 53 ↗ | (On Diff #166400) | |
| 54–56 ↗ | (On Diff #166400) | |
Comment Actions
The Makefile needs to be updated to install mmap.c.
I also did some tests and there is a lot of distortion.
| share/examples/sound/mmap.c | ||
|---|---|---|
| 4–8 ↗ | (On Diff #166400) | |
Comment Actions
That's the strange thing. If I run it 20-30 times, I would get roughly half of the times distorted sound, half of the times normal sound. Also, I tried having one config and two events for read and write, but I couldn't produce any sound like that. Anyway, let me fix this example to conform to the comments and lets go from there.
Comment Actions
I think this is probably a timing issue. I haven't looked into it yet but my guess it that we are not reading/writing exactly when we should.
| share/examples/sound/mmap.c | ||
|---|---|---|
| 70 ↗ | (On Diff #166783) | Since we have a mmap field in struct config, wouldn't it make sense to do that in oss_init()? |
Comment Actions
Let me try to compensate the drift by reading SNDCTL_DSP_GETIPTR and SNDCTL_DSP_GETOPTR and experiment with that for a bit.
Comment Actions
Map or allocate buffer in oss_init. For some reason with this patch probability of producing proper sound is way higher. I still have to add GETIPTR/GETOPTR checks and usleep based on the position.
| share/examples/sound/mmap.c | ||
|---|---|---|
| 30 ↗ | (On Diff #166857) | This could go in oss.h, and we could remove it from other examples files as well. Again, best in a separate patch. |
| share/examples/sound/oss.h | ||
| 230 ↗ | (On Diff #166857) | We don't check for NULL here. You can do that in a separate patch though, it's not related to mmap. |
Comment Actions
I am looking into the code in general and I want to figure out how to display information so it makes more sense, not just dump printf.
Comment Actions
I think I'll try my luck with http://manuals.opensound.com/developer/mmap_test.c.html first and fit it into our example and once the usleep version works, see what are the values of ptr compared to the ones in kqueue scenario. My thinking is "if I can't fix it, move to simpler example", which I'm hoping the official docs are. Anyway, I didn't give up on this code, I just need to approach it from the different angle, for now.
Comment Actions
The usleep based mmap example based on official OSS example. There are three big things about it right now that make it not-so-perfect:
- It works for cases where in and out config have matching numbers for buffer (channels, fragments, ...)
- Calculating duration for usleep would be more optimal. Asking the kernel to fetch playback pointer every 1ms is a big overhead.
- It still has glitches sometimes, but the probability of that happening is way lower than previous examples.
I added -v which will print ci.ptr so it is a bit easier to see where the playback pointer is at. Once the example works, I'll remove this option.
Comment Actions
Lowering usleep time and checking if ci.ptr is zero made all the glitches in the sound go away. I do need to run it for quite a few times to validate that, but in the worst case I significantly lowered the probability of gibberish on the output. Next, I need to calculate the usleep time based on ci.ptr.