[alsa-devel] [PATCH] [alsa-utils.git] Fix a strange regression in aplay's '-d' option
Grond
grond66 at riseup.net
Wed Sep 9 08:11:36 CEST 2015
I did a little bit more digging; the regression was introduced in:
commit 8aa13eec80eac312e4b99423909387660fb99b8f
Author: Olivier Langlois <olivier at 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 at trillion01.com>
Signed-off-by: Takashi Iwai <tiwai at 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 at 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 at 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 at 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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-keys
Size: 3888 bytes
Desc: PGP Key 0x55D89FD9.
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150908/e6567d7d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150908/e6567d7d/attachment.sig>
More information about the Alsa-devel
mailing list