* [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option
@ 2015-09-05 9:39 Grond
2015-09-07 15:27 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Grond @ 2015-09-05 9:39 UTC (permalink / raw)
To: patch; +Cc: alsa-devel
[-- 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 --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option
2015-09-05 9:39 [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option Grond
@ 2015-09-07 15:27 ` Takashi Iwai
2015-09-09 6:11 ` Grond
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2015-09-07 15:27 UTC (permalink / raw)
To: Grond; +Cc: alsa-devel
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.
> 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?
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)>]
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option
2015-09-07 15:27 ` Takashi Iwai
@ 2015-09-09 6:11 ` Grond
0 siblings, 0 replies; 3+ messages in thread
From: Grond @ 2015-09-09 6:11 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 14302 bytes --]
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
--
Attached is my PGP public key.
Primary key fingerprint: B7C7 AD66 D9AF 4348 0238 168E 2C53 D8FA 55D8 9FD9
If you have a PGP key (and a minute to spare)
please send it in reply to this email.
If you have no idea what PGP is, feel free
to ignore all this gobbledegook.
[-- Attachment #1.1.2: 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 --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-09 6:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-05 9:39 [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option Grond
2015-09-07 15:27 ` Takashi Iwai
2015-09-09 6:11 ` Grond
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 NNTP newsgroup(s).