[PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option

[Grond <grond66@riseup.net>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6605 bytes --]



Hello.
(J. Random Hacker here)

While using aplay/arecord (version 1.0.28) to generate some noise, I ran
across a regression in the use of the '-d' option.
Specifically:
According to the man page:
       -d, --duration=#
              Interrupt after # seconds.  A value of zero means infinity.
              The default is zero, so if this option is  omitted  then  the
              arecord process will run until it is killed.
[snip]
       arecord -d 10 -f cd -t wav -D copy foobar.wav
              will record foobar.wav as a 10-second, CD-quality wave file,
              using the PCM "copy" (which might be defined  in  the  user's
              .asoundrc file as:
              pcm.copy {
                type plug
                slave {
                  pcm hw
                }
                route_policy copy
              }
[snip]

So the behavior I expect to see (and have experienced in the past) is:

$ arecord -d 3 -f cd dump.wav
Recording WAVE 'dump.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
$ ls -l
-rw------- 1 grond66 grond66 529244 Sep  5 00:34 dump.wav

What actually happens is:

$ arecord -d 3 -f cd dump.wav
Recording WAVE 'dump.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
[ program hangs here until SIGINT ]
^CAborted by signal Interrupt...
arecord: pcm_read:2031: read error: Interrupted system call
$ ls -l
-rw------- 1 grond66 grond66 526444 Sep  5 00:39 dump-01.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-02.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-03.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-04.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-05.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-06.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-07.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-08.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-09.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-100.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-101.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-102.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-103.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-104.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-105.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-106.wav
-rw------- 1 grond66 grond66     44 Sep  5 00:39 dump-107.wav
[ etcetera... ]
$ aplay dump-01.wav
[ three seconds of hiss, as expected ]
$ less dump-02.wav
RIFF$^@^@^@WAVEfmt ^P^@^@^@^A^@^B^@D<AC>^@^@^P<B1>^B^@^D^@^P^@data^@^@^@^@
$

So arecord is recording the requested audio and putting it into the first file,
then spamming WAV headers into all the subsequent files.
This is not terribly useful.

The problem appears to be six lines of code in aplay/aplay.c, in the functions
pcm_read() and pcm_readv():

   2006 static ssize_t pcm_read(u_char *data, size_t rcount)
   2007 {
   2008         ssize_t r;
   2009         size_t result = 0;
   2010         size_t count = rcount;
   2011 
)> 2012         if (count != chunk_size) {
)> 2013                 count = chunk_size;
)> 2014         }
   2015 
   2016         while (count > 0 && !in_aborting) {

   2045 static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount)
   2046 {
   2047         ssize_t r;
   2048         size_t result = 0;
   2049         size_t count = rcount;
   2050 
)> 2051         if (count != chunk_size) {
)> 2052                 count = chunk_size;
)> 2053         }
   2054 
   2055         while (count > 0 && !in_aborting) {

In this case, the 'rcount' arguments are the number of PCM
integers to be read from the capture device.
In theory. Because what lines 2012-2014 and 2051-2053 do is effectively:

count = chunk_size;

In other words, the argument is completely disregarded,
and 'chunk_size' is used no matter what the functions are passed.

But capture() and family use the following logic:
"If pcm_read{,v}() returns with a value other than the
number integers we told it to read off the wire,
assume something went wrong":

   2925	static void capture(char *orig_name)
   2926	{
   2927		int tostdout=0;		/* boolean which describes output stream */
   2928		int filecount=0;	/* number of files written */
   2929		char *name = orig_name;	/* current filename */
   2930		char namebuf[PATH_MAX+1];
   2931		off64_t count, rest;		/* number of bytes to capture */
[...]
   2964		do {
   2965			/* open a file to write */
   2966			if(!tostdout) {
   2967				/* upon the second file we start the numbering scheme */
   2968				if (filecount || use_strftime) {
   2969					filecount = new_capture_file(orig_name, namebuf,
   2970								     sizeof(namebuf),
   2971								     filecount);
   2972					name = namebuf;
   2973				}
   2974				
   2975				/* open a new file */
   2976				remove(name);
   2977				fd = safe_open(name);
   2978				f (fd < 0) {
   2979					perror(name);
   2980					prg_exit(EXIT_FAILURE);
   2981				}
   2982				filecount++;
   2983			}
   2984	
   2985			rest = count;
   2986			if (rest > fmt_rec_table[file_type].max_filesize)
   2987				rest = fmt_rec_table[file_type].max_filesize;
   2988			if (max_file_size && (rest > max_file_size)) 
   2989				rest = max_file_size;
   2990	
   2991			/* setup sample header */
   2992			if (fmt_rec_table[file_type].start)
   2993				fmt_rec_table[file_type].start(fd, rest);
   2994	
   2995			/* capture */
   2996			fdcount = 0;
   2997			while (rest > 0 && recycle_capture_file == 0 && !in_aborting) {
)> 2998				size_t c = (rest <= (off64_t)chunk_bytes) ?
)> 2999					(size_t)rest : chunk_bytes;
)> 3000				size_t f = c * 8 / bits_per_frame;
)> 3001				if (pcm_read(audiobuf, f) != f)
)> 3002					break;
   3003				if (write(fd, audiobuf, c) != c) {
   3004					perror(name);
   3005					prg_exit(EXIT_FAILURE);
   3006				}
   3007				count -= c;
   3008				rest -= c;
   3009				fdcount += c;
   3010			}
   3011	
[...]
   3027			/* repeat the loop when format is raw without timelimit or
   3028			 * requested counts of data are recorded
   3029			 */
   3030		} while ((file_type == FORMAT_RAW && !timelimit) || count > 0);
   3031	}

These two modes of operation are incompatible.
However, if we nix the strange requirement that all calls to pcm_read{,v}()
must be for exactly 'chunk_size' samples, the problem goes away.
This is the purpose of the patch I've included below.

[-- Attachment #1.1.2: 0001-aplay-Remove-weird-code-in-pcm_read-v.patch --]
[-- Type: text/x-diff, Size: 1116 bytes --]

From eaa3b4acc17bbd7180a46d5c9f3e63442b159cd7 Mon Sep 17 00:00:00 2001
From: Grond66 <grond66@riseup.net>
Date: Fri, 4 Sep 2015 20:05:19 -0700
Subject: [PATCH 1/1] aplay: Remove weird code in pcm_read{,v}

This fixes a regression in aplay/arecord which causes
arecord to loop creating numerous empty numbered output files
when the '-d' is used.

Signed-off-by: Grond66 <grond66@riseup.net>

diff --git a/aplay/aplay.c b/aplay/aplay.c
index 459f7dd..a26dbdb 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -2009,10 +2009,6 @@ static ssize_t pcm_read(u_char *data, size_t rcount)
 	size_t result = 0;
 	size_t count = rcount;
 
-	if (count != chunk_size) {
-		count = chunk_size;
-	}
-
 	while (count > 0 && !in_aborting) {
 		if (test_position)
 			do_test_position();
@@ -2048,10 +2044,6 @@ static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount)
 	size_t result = 0;
 	size_t count = rcount;
 
-	if (count != chunk_size) {
-		count = chunk_size;
-	}
-
 	while (count > 0 && !in_aborting) {
 		unsigned int channel;
 		void *bufs[channels];
-- 
2.1.4


[-- Attachment #1.1.3: PGP Key 0x55D89FD9. --]
[-- Type: application/pgp-keys, Size: 3888 bytes --]

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]