I did a little bit more digging; the regression was introduced in:
commit 8aa13eec80eac312e4b99423909387660fb99b8f Author: Olivier Langlois olivier@trillion01.com Date: Tue Jan 7 23:18:17 2014 -0500
aplay: fix pcm_read() return value
Because of the way the pcm_read() functions are currently used, returning rcount or result is equivalent but I feel it is more accurate to return 'result'.
Signed-off-by: Olivier Langlois olivier@trillion01.com Signed-off-by: Takashi Iwai tiwai@suse.de
diff --git a/aplay/aplay.c b/aplay/aplay.c index e0631c4..69e8bda 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -2039,7 +2039,7 @@ static ssize_t pcm_read(u_char *data, size_t rcount) data += r * bits_per_frame / 8; } } - return rcount; + return result; }
static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount) @@ -2084,7 +2084,7 @@ static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount) count -= r; } } - return rcount; + return result; }
/*
Which was made between versions 1.0.27 and 1.0.28. The assertion made in the commit message is wrong though; 'rcount' and 'result' are not nessisarily equal after the pcm_read{,v} functions have run. Specifically they differ if 'rcount' is not equal 'chunk_size'.
This got fixed in:
commit 8f361d83cfcb39887f5fc591633e68d9448e3425 Author: Jaroslav Kysela perex@perex.cz Date: Wed Oct 1 15:43:57 2014 +0200
Revert "aplay: fix pcm_read() return value"
This reverts commit 8aa13eec80eac312e4b99423909387660fb99b8f.
The semantics for pcm_read() and pcm_readv() was changed, but the callers expect the exact frame count as requested. It's possible to fix callers, but the fix is more complicated than to revert the change. Note that '-d' processing was broken in some cases.
Note: The reverted commit allows that the return value might be greater than requested (see the first condition in read routines).
diff --git a/aplay/aplay.c b/aplay/aplay.c index 30d3f31..e58e1bc 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -2039,7 +2039,7 @@ static ssize_t pcm_read(u_char *data, size_t rcount) data += r * bits_per_frame / 8; } } - return result; + return rcount; }
static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount) @@ -2084,7 +2084,7 @@ static ssize_t pcm_readv(u_char **data, unsigned int channels, size_t rcount) count -= r; } } - return result; + return rcount; }
/*
Which got applied after version 1.0.28. So all-in-all, the problem appears to be more or less fixed on your end. Though I have to say that from an objective standpoint, the processing that happens between capture{,v} and pcm_read{,v}, makes very little sense, and really ought to be fixed more completely.
Sorry for all the noise.
On Mon, Sep 07, 2015 at 05:27:04PM +0200, Takashi Iwai wrote:
On Sat, 05 Sep 2015 11:39:10 +0200, Grond wrote:
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.
Do you mean a regression from any previous versions? I couldn't find any relevant change in the code you cited. It's been so from the very original version, as far as I look at git commits.
Oops. Forgot to give you that. The working version is 1.0.25. Although I must disagree with you about the aforesaid lines never changing:
$ git blame -L 2012,2014 aplay/aplay.c 4cb74aed (Takashi Iwai 2008-01-08 18:38:32 +0100 2012) if (count != chunk_size) { 744c8798 (Abramo Bagnara 2001-01-15 11:06:55 +0000 2013) count = chunk_size; c88189e4 (Abramo Bagnara 2000-07-06 17:20:49 +0000 2014) } $ git blame -L 2051,2053 aplay/aplay.c 4cb74aed (Takashi Iwai 2008-01-08 18:38:32 +0100 2051) if (count != chunk_size) { 744c8798 (Abramo Bagnara 2001-01-15 11:06:55 +0000 2052) count = chunk_size; c88189e4 (Abramo Bagnara 2000-07-06 17:20:49 +0000 2053) }
A little more digging shows that these lines were assembled seemingly by accident in three seperate commits spread out over 8 years.
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.
Hmm, this doesn't happen on my machine. Which system and which alsa-lib / alsa-utils versions are you using?
I'm using Debian Jessie. (And before you ask, they don't patch aplay.c) Also: after doing more testing, it seems that the bug may or may not trigger, depending on what hardware you're using, and the sample rate/bit depth you request. This is because the total number of samples to be recorded (which is calculated from the record time specified with '-d' and the number of channels/bit depth/sample rate) may happen to be a multiple of chunk_size. So if you're having problems reproducing the bug in v1.0.28, try changing PCM format. ('-f cd' reliably triggers it on all three of the boxes I've tested.)
Takashi
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. 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
[1.3 PGP Key 0x55D89FD9. <application/pgp-keys (7bit)>]
[2 Digital signature <application/pgp-signature (7bit)>]
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel