@@ -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).
@@ -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;
}
/*
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> 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