GNOME Bugzilla – Bug 349015
[sunaudio] open source with O_NONBLOCK
Last modified: 2006-12-08 14:33:15 UTC
Attaching patch that simply modifies the open() call to add O_NONBLOCK so
the device is opened in non-blocking mode. Otherwise programs like gnome-sound-recorder hang if you try to record more than one file at the same time.
Created attachment 69774 [details] [review]
patch fixing open to be non-blocking
Created attachment 70617 [details] [review]
please use this patch,
under non-blocking mode, read may lost some data, according to audio man page on solaris.
so please use this patch instead, thanks.
This patch doesn't look right - it takes away the NONBLOCK parameter to open, which means open will block and the ioctl won't get to run until later, no?
From the audio man page on Solaris 9:
When a process cannot open /dev/audio because the device is
o if either the O_NDELAY or O_NONBLOCK flags are set in
the open() oflag argument, then -1 is immediately
returned, with errno set to EBUSY.
o if neither the O_NDELAY nor the O_NONBLOCK flag are
set, then open() hangs until the device is available
or a signal is delivered to the process, in which case
a -1 is returned with errno set to EINTR. This allows
a process to block in the open call while waiting for
the audio device to become available.
As Jan already said, the purpose of the NONBLOCK parameter was to avoid hanging in open if opening the device is not possible right away.
Recording Audio Data
The read() system call copies data from the system's buffers
to the application. Ordinarily, read() blocks until the user
buffer is filled. The I_NREAD ioctl (see streamio(7I)) may
be used to determine the amount of data that may be read
without blocking. The device may alternatively be set to a
non-blocking mode, in which case read() completes immedi-
ately, but may return fewer bytes than requested. Refer to
the read(2) manual page for a complete description of this
only says that in non-blocking mode read() may return fewer bytes than requested, which doesn't mean data is lost, does it? (this is standard behaviour for read() in non-blocking mode for all devices as far as I know).
But maybe we should just set the fd back to non-blocking again with fcntl() after opening it successfully?
The main reason that we want to remove NONBLOCK is because when we use this flag we notice that the recorded input "stutters" so there are segments of silence in the recorded data, like it is only recording sometimes.
I think we need to research this a bit more and make sure that we use the correct options here.
Note that we might not be managing the return code from read() properly. When a device is non-blocking, it will reutrn empty stuff from read with EINTR, indicating that we should retry the read() or it will return partial reads. This may be the cause of the "stuttering" effect we notice when we turn on NONBLOCK mode. Can we check this?
Also, according to Jerry, multiple processes can open the audio device even if it open in NONBLOCK mode. Using NONBLOCK only allows the same program to open the device more than once. Can we verify that this is all that NONBLOCK provides?
The device may alternatively be set to a
non-blocking mode, in which case read() completes immedi-
ately, but may return fewer bytes than requested.
according to my testcase, it really lost data.
so I make the patch to remove it.
And for the hang issue,
I have verified that two process open /dev/audio device wont hang,
only when a process open it twice, it will hang.
to avoid it, I set its properties as AUDIO_MIXER_MULTIPLE_OPEN
Jerry, it doesn't seem like you have done any of the research that was pointed out by the engineers who have reviewed your patch.
Some people have suggested that NONBLOCK may be better, if we can get it working. Just because we notice lost data when we have NONBLOCK does not mean that the NONBLOCK flag is the problem. As I mentioned in my previous comment, we need to test to see if NONBLOCK works if we manage EINTR/EAGAIN making the read
function look something like this
ret = read (sunaudiosrc->fd, data, length);
while (ret == EINTR || ret == EAGAIN);
Does NONBLOCK work if you try something like this?
In other words, the patch you provided may be a good workaround to make things
work for now, but might not really be the best solution.
Also, I'm not sure that the MIXER ioctl belongs in the source. Note that the source plugin calls gst_sunaudiomixer_ctrl_new, which calls gst_sunaudiomixer_ctrl_open. Might be better to put this ioctl in the mixer code.
After further review and testing with Jerry we have determined that the MIXER ioctl does belong in the source plugin since the ioctl must be called on /dev/audio and not /dev/audioctrl. So I think that part of the patch is ready to go in. After Jerry tests to see if handling EINTR and EAGAIN makes the plugin work with NONBLOCK turned on, we can decide whether to go with this patch or a modified patch.
Note that we here at Sun are doing a lot of work trying to figure out the best way to call the SunAudio interfaces to support the various GStreamer-based applications. I suspect we will be suggesting various tweaks to this code over the next few weeks as we figure things out.
I do recommend that we get something like the proposed patch into the codebase soon, though, because the source plugin doesn't work if NONBLOCK is there, and gnome-sound-recorder hangs without the ioctl call. So without fixing these issues a lot of functionality is unuable. Even if this isn't the final patch, this at least gets things so they mostly work.
Any update on this one? Is there a modified patch you would like committed in the meantime?
Please accept Jerry's patch. I think this is the best solution for now. Without this patch gnome-sound-recorder simply does not work on Solaris.
We are seeing some performance issues on Solaris, so we may further rework the way we manage the audio device. But let's handle that in a separate bug if we discover ways to improve it.
Created attachment 72771 [details] [review]
What about something like this? Does this make things work as well?
Basically, IMHO at least, open() should never ever block under any circumstances, not if done from within the same process or done from multiple processes, not if done once or twice or fifteen times concurrently. It doesn't look like your patch achieves this to me.
Tim's patch doesnot work, I have verified on my sparc.
open with NON_BLOCK option lost some data.
for example, when recording, I count 9, it only record 3 or less.
for sunaudio's API, I found that open device twice will be blocked for one process,
but it works in two process.
one engineer tell me that "There is a multiple open ioctl that needs to be called first before a process can do multiple opens. It's documented in mixer (7i) man page."
so, I suggest to use my patch, since it is simple and works.
I think we should accept this patch for now. Recording doesn't work at all with solaris without this fix. This fix seems to work okay, and unless you have other tests for us to try out, let's go with this workaround for now. Okay? We can leave the bug open if we want, but the patch should go in.
* sys/sunaudio/gstsunaudiosrc.c: (gst_sunaudiosrc_open):
Apply patch to open the mixer control and set the MULTIPLE_OPEN
ioctl. On solaris, the mixer device doesn't need opening non-blocking
- it can be opened by multiple processes by default, but needs the ioctl
for multiple opens within 1 process.
Patch by: Jerry Tan <jerry.tan at sun dot com>