Created attachment 69774 [details] [review]
patch fixing open to be non-blocking

Created attachment 70617 [details] [review]
patch

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
     busy:

        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.


Also

  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
     behavior.

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

{
   do {
      ret = read (sunaudiosrc->fd, data, length);
   while (ret == EINTR || ret == EAGAIN);

   return ret
}

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]
possible alternative

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>
        Fixes: #349015