sox-devel@lists.sourceforge.net unofficial mirror
 help / color / mirror / code / Atom feed
* [PATCH] use non-blocking stdin for interactive mode
       [not found] ` <d06a5a840dc9844a8ac389dcba91c7bd.squirrel@mail.mediaplaygrounds.co.uk>
@ 2015-10-03 22:13   ` Eric Wong
  2015-10-04  9:10     ` Tarim
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2015-10-03 22:13 UTC (permalink / raw)
  To: Tarim, sox-users; +Cc: sox-devel

Tarim <sox@mediaplaygrounds.co.uk> wrote:
>   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. */
-- 
EW

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] use non-blocking stdin for interactive mode
  2015-10-03 22:13   ` [PATCH] use non-blocking stdin for interactive mode Eric Wong
@ 2015-10-04  9:10     ` Tarim
  2015-10-04 11:22       ` Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Tarim @ 2015-10-04  9:10 UTC (permalink / raw)
  To: Eric Wong; +Cc: sox-users, sox-devel

  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


Eric Wong wrote:
> Tarim <sox@mediaplaygrounds.co.uk> wrote:
>>   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. */
> --
> EW
>


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] use non-blocking stdin for interactive mode
  2015-10-04  9:10     ` Tarim
@ 2015-10-04 11:22       ` Eric Wong
       [not found]         ` <80d9684bb05ff4318a7a28ccddbcf860.squirrel@mail.mediaplaygrounds.co.uk>
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2015-10-04 11:22 UTC (permalink / raw)
  To: Tarim; +Cc: sox-users, sox-devel

Tarim <sox@mediaplaygrounds.co.uk> wrote:
>   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?

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] use non-blocking stdin for interactive mode
       [not found]         ` <80d9684bb05ff4318a7a28ccddbcf860.squirrel@mail.mediaplaygrounds.co.uk>
@ 2015-10-07 10:23           ` Eric Wong
  0 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2015-10-07 10:23 UTC (permalink / raw)
  To: Tarim; +Cc: sox-users, sox-devel

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

Tarim <tarim@mediaplaygrounds.co.uk> wrote:
>   Having problems building sox from your git repository at the moment.
> 
> I get:
> ./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
> know about oss or ossdsp.  Try:
> 
> 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).

------------------------------------------------------------------------------
Full-scale, agent-less Infrastructure Monitoring from a single dashboard
Integrate with 40+ ManageEngine ITSM Solutions for complete visibility
Physical-Virtual-Cloud Infrastructure monitoring from one console
Real user monitoring with APM Insights and performance trend reports 
Learn More http://pubads.g.doubleclick.net/gampad/clk?id=247754911&iu=/4140

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-10-07 10:24 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <f5d0acadb8966e2192aa5d7c1ca6f6e2.squirrel@mail.mediaplaygrounds.co.uk>
     [not found] ` <d06a5a840dc9844a8ac389dcba91c7bd.squirrel@mail.mediaplaygrounds.co.uk>
2015-10-03 22:13   ` [PATCH] use non-blocking stdin for interactive mode Eric Wong
2015-10-04  9:10     ` Tarim
2015-10-04 11:22       ` Eric Wong
     [not found]         ` <80d9684bb05ff4318a7a28ccddbcf860.squirrel@mail.mediaplaygrounds.co.uk>
2015-10-07 10:23           ` Eric Wong

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/sox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).