[SoX-devel] [PATCH] use non-blocking stdin for interactive mode

[Eric Wong]

I'm now thinking that this is actually a bug in the latest Ubuntu kernel
3.13.0-65.105-generic where select's FD_ISSET returns true even when
there is no data to read!

This is a kernel bug from a performance standpoint, but the select
manpage explicitly documents spurious wakeups as a possibility which
userspace must be prepared for.

The following patch should fix the problem:

------------------8<------------------
Subject: [PATCH] use non-blocking stdin for interactive mode

When accepting keyboard input, it is possible for select() to
return a false-positive with spurious wakeups from inside the
update_status callback.

Using the FIONREAD ioctl in place of select is also a possibility,
but may be less portable.
---
Downloadable patch:
http://80x24.org/spew/1443909787-27821-1-git-send-email-e%4080x24.org/raw

In case upstream prefers a pull request:

The following changes since commit 7e74b254b2a7c963be0bfce751fc5911fe681c12:

Remove hepler script. It's mostly unmaintained, I don't know if anyone but me ever used it. In any case, those who want a custom Debian package should be capable of updating the debian/changelog entry on their own. (2015-02-26 22:48:40 -0500)

are available in the git repository at:

git://bogomips.org/sox nb-stdin

for you to fetch changes up to 62a370a2a01cf1dcc0d5f9b9f49d1515706346cd:

use non-blocking stdin for interactive mode (2015-10-03 22:04:50 +0000)

----------------------------------------------------------------
Eric Wong (1):
use non-blocking stdin for interactive mode

src/sox.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/src/sox.c b/src/sox.c
index bab0f45..fdb7616 100644
--- a/src/sox.c
+++ b/src/sox.c
@@ -1789,6 +1789,18 @@ static int process(void)
tcsetattr(fileno(stdin), TCSANOW, &modified_termios);
}
#endif
+#if defined(F_GETFL) && defined(F_SETFL) && defined(O_NONBLOCK)
+ if (interactive) {
+ int fd = fileno(stdin);
+ int flags = fcntl(fd, F_GETFL);
+ if (flags == -1) {
+ lsx_warn("error getting flags on stdin descriptor: %s", strerror(errno));
+ } else if (!(flags & O_NONBLOCK)) {
+ if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1)
+ lsx_warn("error setting non-blocking on stdin: %s", strerror(errno));
+ }
+ }
+#endif

signal(SIGTERM, sigint); /* Stop gracefully, as soon as we possibly can. */
signal(SIGINT , sigint); /* Either skip current input or behave as SIGTERM. */

[Tarim]

Thanks Eric, that makes things a lot cleaner, more resilient and, at a
guess, more portable (surely any OS with select will have O_NONBLOCK?)

I agree it's more a performance bug but, I think, if select returns a
spurious wakeup then the read should set EAGAIN (EWOULDBLOCK) - which
the Linux 3.13.0-65.105-generic kernel doesn't. (Not that this would
have helped the unpatched sox).

Cheers,
Tarim

Post by Eric Wong

I'm now thinking that this is actually a bug in the latest Ubuntu kernel
3.13.0-65.105-generic where select's FD_ISSET returns true even when
there is no data to read!

This is a kernel bug from a performance standpoint, but the select
manpage explicitly documents spurious wakeups as a possibility which
userspace must be prepared for.
------------------8<------------------
Subject: [PATCH] use non-blocking stdin for interactive mode
When accepting keyboard input, it is possible for select() to
return a false-positive with spurious wakeups from inside the
update_status callback.
Using the FIONREAD ioctl in place of select is also a possibility,
but may be less portable.
---
http://80x24.org/spew/1443909787-27821-1-git-send-email-e%4080x24.org/raw
The following changes since commit
Remove hepler script. It's mostly unmaintained, I don't know if anyone
but me ever used it. In any case, those who want a custom Debian
package should be capable of updating the debian/changelog entry on
their own. (2015-02-26 22:48:40 -0500)
git://bogomips.org/sox nb-stdin
use non-blocking stdin for interactive mode (2015-10-03 22:04:50 +0000)
----------------------------------------------------------------
use non-blocking stdin for interactive mode
src/sox.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/sox.c b/src/sox.c
index bab0f45..fdb7616 100644
--- a/src/sox.c
+++ b/src/sox.c
@@ -1789,6 +1789,18 @@ static int process(void)
tcsetattr(fileno(stdin), TCSANOW, &modified_termios);
}
#endif
+#if defined(F_GETFL) && defined(F_SETFL) && defined(O_NONBLOCK)
+ if (interactive) {
+ int fd = fileno(stdin);
+ int flags = fcntl(fd, F_GETFL);
+ if (flags == -1) {
+ lsx_warn("error getting flags on stdin descriptor: %s",
strerror(errno));
+ } else if (!(flags & O_NONBLOCK)) {
+ if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1)
+ lsx_warn("error setting non-blocking on stdin: %s",
strerror(errno));
+ }
+ }
+#endif
signal(SIGTERM, sigint); /* Stop gracefully, as soon as we possibly can. */
signal(SIGINT , sigint); /* Either skip current input or behave as SIGTERM. */
--
EW

------------------------------------------------------------------------------

[Eric Wong]

Post by Tarim
Thanks Eric, that makes things a lot cleaner, more resilient and, at a
guess, more portable (surely any OS with select will have O_NONBLOCK?)

O_NONBLOCK is common, yes, but I have zero knowledge or interest in
proprietary OSes. For systems with O_NONBLOCK, the select calls in
kbhit are not even necessary anymore. I didn't change kbhit to minimize
the patch size.

Did you get a chance to test and confirm my patch fixes things for you?

Post by Tarim
I agree it's more a performance bug but, I think, if select returns a
spurious wakeup then the read should set EAGAIN (EWOULDBLOCK) - which
the Linux 3.13.0-65.105-generic kernel doesn't. (Not that this would
have helped the unpatched sox).

read may only return EAGAIN when O_NONBLOCK is set; otherwise existing
applications which depend on blocking read will break.

Fwiw, blocking read can be a performance win in some cases: it can avoid
extra syscalls, avoid thundering herd, and improve fairness when
multiple threads/processes are reading a off the same pipe.

------------------------------------------------------------------------------

[Eric Wong]

Please don't drop Cc: to public lists, others can help while I'm away.
I welcome direct emails, just keep others in the loop :)

Having problems building sox from your git repository at the moment.
./configure: line 14592: syntax error near unexpected token `OPUS,'
./configure: line 14592: ` PKG_CHECK_MODULES(OPUS, opusfile, ,
using_opus=no)'
Try ./configure --with-opus=no - but that doesn't help.
Comment out PKG_CHECK_MODULES line (but still need --with-opus-no to get
it to compile).
Now I have a sox but it has no default sound device, and only seems to
AUDIODRIVER=oss LD_LIBRARY_PATH=/usr/local/lib:/lib:/usr/lib play xx.wav
but this also seems to need an AUDIODEV and I can't find anything in
/dev/snd which satisfies it.
Think I'm probably overlooking something obvious but I've been going
round in circles with this for a while now.

You're probably missing dependencies and sox is falling back
to non-ideal audio playback (OSS, instead of ALSA or PulseAudio).

Try to install the same build-dependencies for sox used by your system.
On Debian/Ubuntu systems, this can be done easily using:

apt-get install build-essential
apt-get build-dep sox

(Be sure you have the "deb-src" lines in your /etc/apt/sources.list
file uncommented).