[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