[PATCH] aplay: Fix header for raw format

[PATCH] aplay: Fix header for raw format

[Ion-Horia Petrisor]

Raw format has no header, so use 0 when calling playback_go.

Signed-off-by: Ion-Horia Petrisor <ion-horia.petrisor@nxp.com>
---
 aplay/aplay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/aplay/aplay.c b/aplay/aplay.c
index 00af662..fe274ed 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -2831,7 +2831,7 @@ static void playback(char *name)
 		/* should be raw data */
 		init_raw_data();
 		pbrec_count = calc_count();
-		playback_go(fd, dta, pbrec_count, FORMAT_RAW, name);
+		playback_go(fd, 0, pbrec_count, FORMAT_RAW, name);
 	}
       __end:
 	if (fd != 0)
-- 
2.7.4

Re: [PATCH] aplay: Fix header for raw format

[Takashi Iwai]

On Thu, 29 Jun 2017 13:17:16 +0200,
Ion-Horia Petrisor wrote:
> 
> Raw format has no header, so use 0 when calling playback_go.

It's the value passed for the length that has been already loaded.
The program has read dta bytes for parsing the header, and it's raw
data without header.  Thus you need to pass dta there.


thanks,

Takashi

> 
> Signed-off-by: Ion-Horia Petrisor <ion-horia.petrisor@nxp.com>
> ---
>  aplay/aplay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index 00af662..fe274ed 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -2831,7 +2831,7 @@ static void playback(char *name)
>  		/* should be raw data */
>  		init_raw_data();
>  		pbrec_count = calc_count();
> -		playback_go(fd, dta, pbrec_count, FORMAT_RAW, name);
> +		playback_go(fd, 0, pbrec_count, FORMAT_RAW, name);
>  	}
>        __end:
>  	if (fd != 0)
> -- 
> 2.7.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] aplay: Fix header for raw format

[Daniel Baluta]

Hi Takashi,

Thanks a lot for your feedback.

On Fri, Jun 30, 2017 at 9:54 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 29 Jun 2017 13:17:16 +0200,
> Ion-Horia Petrisor wrote:
>>
>> Raw format has no header, so use 0 when calling playback_go.
>
> It's the value passed for the length that has been already loaded.
> The program has read dta bytes for parsing the header, and it's raw
> data without header.  Thus you need to pass dta there.
>
With the current code we assume that the raw files have at least 26 bytes.
(sizeof (VocHeader).

I think it doesn't matter here that dta bytes has already been loaded. These
bytes useful to figure out the file header type.

To keep things simple we can assume that in the case of raw files there is no
header, so second parameter of playback_go (named loaded) can be set to 0.

The worst things that can happen is that we re-read the first 26 bytes
(previously
read as VoC header).

We are trying to introduce a new parameter --samples which tells aplay to  only
playback/record s number of samples.

We modified calc_count() accordingly so that when receives the
--samples parameter
computes the correct number of bytes to be read.

Anyhow, it doesn't work as expected because aplay expects files to have at least
26 bytes.

The correct solution would be that in the case of raw files not assume
any header
and read samples * sample_size bytes from the beginning of the file.

>
> thanks,
>
> Takashi
>
>>
>> Signed-off-by: Ion-Horia Petrisor <ion-horia.petrisor@nxp.com>
>> ---
>>  aplay/aplay.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/aplay/aplay.c b/aplay/aplay.c
>> index 00af662..fe274ed 100644
>> --- a/aplay/aplay.c
>> +++ b/aplay/aplay.c
>> @@ -2831,7 +2831,7 @@ static void playback(char *name)
>>               /* should be raw data */
>>               init_raw_data();
>>               pbrec_count = calc_count();
>> -             playback_go(fd, dta, pbrec_count, FORMAT_RAW, name);
>> +             playback_go(fd, 0, pbrec_count, FORMAT_RAW, name);
>>       }
>>        __end:
>>       if (fd != 0)
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] aplay: Fix header for raw format

[Takashi Iwai]

On Fri, 30 Jun 2017 15:11:38 +0200,
Daniel Baluta wrote:
> 
> Hi Takashi,
> 
> Thanks a lot for your feedback.
> 
> On Fri, Jun 30, 2017 at 9:54 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Thu, 29 Jun 2017 13:17:16 +0200,
> > Ion-Horia Petrisor wrote:
> >>
> >> Raw format has no header, so use 0 when calling playback_go.
> >
> > It's the value passed for the length that has been already loaded.
> > The program has read dta bytes for parsing the header, and it's raw
> > data without header.  Thus you need to pass dta there.
> >
> With the current code we assume that the raw files have at least 26 bytes.
> (sizeof (VocHeader).
> 
> I think it doesn't matter here that dta bytes has already been loaded. These
> bytes useful to figure out the file header type.
> 
> To keep things simple we can assume that in the case of raw files there is no
> header, so second parameter of playback_go (named loaded) can be set to 0.

The loaded parameter of playback_go() doesn't mean that.  It's the
size that has been already read onto the audio buffer.  If you pass 0
there, it means that you discard the first 26 bytes samples you have
already read without playing.

> The worst things that can happen is that we re-read the first 26 bytes
> (previously
> read as VoC header).

Where is re-read...?

> We are trying to introduce a new parameter --samples which tells aplay to  only
> playback/record s number of samples.
> 
> We modified calc_count() accordingly so that when receives the
> --samples parameter
> computes the correct number of bytes to be read.
> 
> Anyhow, it doesn't work as expected because aplay expects files to have at least
> 26 bytes.
> 
> The correct solution would be that in the case of raw files not assume
> any header
> and read samples * sample_size bytes from the beginning of the file.

I see now what you want, but the patch is incorrect.
I guess a patch something like below is what you want.


thanks,

Takashi

---
diff --git a/aplay/aplay.c b/aplay/aplay.c
index 00af66246cad..a902be5cf9cb 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -2782,12 +2782,65 @@ static void playback_go(int fd, size_t loaded, off64_t count, int rtype, char *n
  *  let's play or capture it (capture_type says VOC/WAVE/raw)
  */
 
-static void playback(char *name)
+static void read_header(int *loaded, int header_size)
+{
+	if (*loaded < header_size) {
+		header_size -= *loaded;
+		if (safe_read(fd, audiobuf, header_size) != header_size) {
+			error(_("read error"));
+			prg_exit(EXIT_FAILURE);
+		}
+		*loaded += header_size;
+	}
+}
+
+static int playback_au(char *name, int *loaded)
+{
+	read_header(loaded, sizeof(AuHeader));
+	if (test_au(fd, audiobuf) < 0)
+		return -1;
+	rhwparams.format = hwparams.format;
+	pbrec_count = calc_count();
+	playback_go(fd, *loaded - sizeof(AuHeader), pbrec_count, FORMAT_AU, name);
+	return 0;
+}
+
+static int playback_voc(char *name, int *loaded)
 {
 	int ofs;
-	size_t dta;
+
+	read_header(loaded, sizeof(VocHeader));
+	if ((ofs = test_vocfile(audiobuf)) < 0)
+		return -1;
+	pbrec_count = calc_count();
+	voc_play(fd, ofs, name);
+	return 0;
+}
+
+static int playback_wave(char *name, int *loaded)
+{
 	ssize_t dtawave;
 
+	read_header(loaded, sizeof(WaveHeader));
+	if ((dtawave = test_wavefile(fd, audiobuf, *loaded)) < 0)
+		return -1;
+	pbrec_count = calc_count();
+	playback_go(fd, dtawave, pbrec_count, FORMAT_WAVE, name);
+	return 0;
+}
+
+static int playback_raw(char *name, int *loaded)
+{
+	init_raw_data();
+	pbrec_count = calc_count();
+	playback_go(fd, *loaded, pbrec_count, FORMAT_RAW, name);
+	return 0;
+}
+
+static void playback(char *name)
+{
+	int loaded = 0;
+
 	pbrec_count = LLONG_MAX;
 	fdcount = 0;
 	if (!name || !strcmp(name, "-")) {
@@ -2800,40 +2853,29 @@ static void playback(char *name)
 			prg_exit(EXIT_FAILURE);
 		}
 	}
-	/* read the file header */
-	dta = sizeof(AuHeader);
-	if ((size_t)safe_read(fd, audiobuf, dta) != dta) {
-		error(_("read error"));
-		prg_exit(EXIT_FAILURE);
-	}
-	if (test_au(fd, audiobuf) >= 0) {
-		rhwparams.format = hwparams.format;
-		pbrec_count = calc_count();
-		playback_go(fd, 0, pbrec_count, FORMAT_AU, name);
-		goto __end;
-	}
-	dta = sizeof(VocHeader);
-	if ((size_t)safe_read(fd, audiobuf + sizeof(AuHeader),
-		 dta - sizeof(AuHeader)) != dta - sizeof(AuHeader)) {
-		error(_("read error"));
-		prg_exit(EXIT_FAILURE);;
-	}
-	if ((ofs = test_vocfile(audiobuf)) >= 0) {
-		pbrec_count = calc_count();
-		voc_play(fd, ofs, name);
-		goto __end;
-	}
-	/* read bytes for WAVE-header */
-	if ((dtawave = test_wavefile(fd, audiobuf, dta)) >= 0) {
-		pbrec_count = calc_count();
-		playback_go(fd, dtawave, pbrec_count, FORMAT_WAVE, name);
-	} else {
-		/* should be raw data */
-		init_raw_data();
-		pbrec_count = calc_count();
-		playback_go(fd, dta, pbrec_count, FORMAT_RAW, name);
+
+	switch (file_type) {
+	case FORMAT_AU:
+		playback_au(name, &loaded);
+		break;
+	case FORMAT_VOC:
+		playback_voc(name, &loaded);
+		break;
+	case FORMAT_WAVE:
+		playback_wave(name, &loaded);
+		break;
+	case FORMAT_RAW:
+		playback_raw(name, &loaded);
+		break;
+	default:
+		/* parse the file header */
+		if (playback_au(name, &loaded) < 0 &&
+		    playback_voc(name, &loaded) < 0 &&
+		    playback_wave(name, &loaded) < 0)
+			playback_raw(name, &loaded); /* should be raw data */
+		break;
 	}
-      __end:
+
 	if (fd != 0)
 		close(fd);
 }

Re: [PATCH] aplay: Fix header for raw format

[Daniel Baluta]

On Fri, Jun 30, 2017 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 30 Jun 2017 15:11:38 +0200,
> Daniel Baluta wrote:
>>
>> Hi Takashi,
>>
>> Thanks a lot for your feedback.
>>
>> On Fri, Jun 30, 2017 at 9:54 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Thu, 29 Jun 2017 13:17:16 +0200,
>> > Ion-Horia Petrisor wrote:
>> >>
>> >> Raw format has no header, so use 0 when calling playback_go.
>> >
>> > It's the value passed for the length that has been already loaded.
>> > The program has read dta bytes for parsing the header, and it's raw
>> > data without header.  Thus you need to pass dta there.
>> >
>> With the current code we assume that the raw files have at least 26 bytes.
>> (sizeof (VocHeader).
>>
>> I think it doesn't matter here that dta bytes has already been loaded. These
>> bytes useful to figure out the file header type.
>>
>> To keep things simple we can assume that in the case of raw files there is no
>> header, so second parameter of playback_go (named loaded) can be set to 0.
>
> The loaded parameter of playback_go() doesn't mean that.  It's the
> size that has been already read onto the audio buffer.  If you pass 0
> there, it means that you discard the first 26 bytes samples you have
> already read without playing.

Agree. Passing 0 discards the first 26 bytes that have already been read
but these bytes will be re-read inside playback_go so they will be played.

playback_go( .. loaded = 0)
..

r = safe_read(fd, audio_buf + loaded, c). [aplay.c: 2757]

>
>> The worst things that can happen is that we re-read the first 26 bytes
>> (previously
>> read as VoC header).
>
> Where is re-read...?

As explained above, we first read the header [be it .au, .voc or .wav] in
playback() function and then because we pass [with our proposed patch
loaded = 0]
we re-read those bytes in playback_go function.

Of course, is not optimal. Let me look at your patch.
>
>> We are trying to introduce a new parameter --samples which tells aplay to  only
>> playback/record s number of samples.
>>
>> We modified calc_count() accordingly so that when receives the
>> --samples parameter
>> computes the correct number of bytes to be read.
>>
>> Anyhow, it doesn't work as expected because aplay expects files to have at least
>> 26 bytes.
>>
>> The correct solution would be that in the case of raw files not assume
>> any header
>> and read samples * sample_size bytes from the beginning of the file.
>
> I see now what you want, but the patch is incorrect.
> I guess a patch something like below is what you want.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index 00af66246cad..a902be5cf9cb 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -2782,12 +2782,65 @@ static void playback_go(int fd, size_t loaded, off64_t count, int rtype, char *n
>   *  let's play or capture it (capture_type says VOC/WAVE/raw)
>   */
>
> -static void playback(char *name)
> +static void read_header(int *loaded, int header_size)
> +{
> +       if (*loaded < header_size) {
> +               header_size -= *loaded;
> +               if (safe_read(fd, audiobuf, header_size) != header_size) {

audio_buf + *loaded here.

> +                       error(_("read error"));
> +                       prg_exit(EXIT_FAILURE);
> +               }
> +               *loaded += header_size;
> +       }
> +}
> +
> +static int playback_au(char *name, int *loaded)
> +{
> +       read_header(loaded, sizeof(AuHeader));
> +       if (test_au(fd, audiobuf) < 0)
> +               return -1;
> +       rhwparams.format = hwparams.format;
> +       pbrec_count = calc_count();
> +       playback_go(fd, *loaded - sizeof(AuHeader), pbrec_count, FORMAT_AU, name);
> +       return 0;
> +}
> +
> +static int playback_voc(char *name, int *loaded)
>  {
>         int ofs;
> -       size_t dta;
> +
> +       read_header(loaded, sizeof(VocHeader));
> +       if ((ofs = test_vocfile(audiobuf)) < 0)
> +               return -1;
> +       pbrec_count = calc_count();
> +       voc_play(fd, ofs, name);
> +       return 0;
> +}
> +
> +static int playback_wave(char *name, int *loaded)
> +{
>         ssize_t dtawave;
>
> +       read_header(loaded, sizeof(WaveHeader));
> +       if ((dtawave = test_wavefile(fd, audiobuf, *loaded)) < 0)
> +               return -1;
> +       pbrec_count = calc_count();
> +       playback_go(fd, dtawave, pbrec_count, FORMAT_WAVE, name);
> +       return 0;
> +}
> +
> +static int playback_raw(char *name, int *loaded)
> +{
> +       init_raw_data();
> +       pbrec_count = calc_count();
> +       playback_go(fd, *loaded, pbrec_count, FORMAT_RAW, name);
> +       return 0;
> +}
> +
> +static void playback(char *name)
> +{
> +       int loaded = 0;
> +
>         pbrec_count = LLONG_MAX;
>         fdcount = 0;
>         if (!name || !strcmp(name, "-")) {
> @@ -2800,40 +2853,29 @@ static void playback(char *name)
>                         prg_exit(EXIT_FAILURE);
>                 }
>         }
> -       /* read the file header */
> -       dta = sizeof(AuHeader);
> -       if ((size_t)safe_read(fd, audiobuf, dta) != dta) {
> -               error(_("read error"));
> -               prg_exit(EXIT_FAILURE);
> -       }
> -       if (test_au(fd, audiobuf) >= 0) {
> -               rhwparams.format = hwparams.format;
> -               pbrec_count = calc_count();
> -               playback_go(fd, 0, pbrec_count, FORMAT_AU, name);
> -               goto __end;
> -       }
> -       dta = sizeof(VocHeader);
> -       if ((size_t)safe_read(fd, audiobuf + sizeof(AuHeader),
> -                dta - sizeof(AuHeader)) != dta - sizeof(AuHeader)) {
> -               error(_("read error"));
> -               prg_exit(EXIT_FAILURE);;
> -       }
> -       if ((ofs = test_vocfile(audiobuf)) >= 0) {
> -               pbrec_count = calc_count();
> -               voc_play(fd, ofs, name);
> -               goto __end;
> -       }
> -       /* read bytes for WAVE-header */
> -       if ((dtawave = test_wavefile(fd, audiobuf, dta)) >= 0) {
> -               pbrec_count = calc_count();
> -               playback_go(fd, dtawave, pbrec_count, FORMAT_WAVE, name);
> -       } else {
> -               /* should be raw data */
> -               init_raw_data();
> -               pbrec_count = calc_count();
> -               playback_go(fd, dta, pbrec_count, FORMAT_RAW, name);
> +
> +       switch (file_type) {
> +       case FORMAT_AU:
> +               playback_au(name, &loaded);
> +               break;
> +       case FORMAT_VOC:
> +               playback_voc(name, &loaded);
> +               break;
> +       case FORMAT_WAVE:
> +               playback_wave(name, &loaded);
> +               break;
> +       case FORMAT_RAW:
> +               playback_raw(name, &loaded);
> +               break;
> +       default:
> +               /* parse the file header */
> +               if (playback_au(name, &loaded) < 0 &&
> +                   playback_voc(name, &loaded) < 0 &&
> +                   playback_wave(name, &loaded) < 0)
> +                       playback_raw(name, &loaded); /* should be raw data */
> +               break;
>         }
> -      __end:
> +
>         if (fd != 0)
>                 close(fd);
>  }

This is more elegant and also fixes our problem. Care to send a formal patch?

thanks,
daniel.

Re: [PATCH] aplay: Fix header for raw format

[Takashi Iwai]

On Mon, 03 Jul 2017 11:41:44 +0200,
Daniel Baluta wrote:
> 
> On Fri, Jun 30, 2017 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 30 Jun 2017 15:11:38 +0200,
> > Daniel Baluta wrote:
> >>
> >> Hi Takashi,
> >>
> >> Thanks a lot for your feedback.
> >>
> >> On Fri, Jun 30, 2017 at 9:54 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > On Thu, 29 Jun 2017 13:17:16 +0200,
> >> > Ion-Horia Petrisor wrote:
> >> >>
> >> >> Raw format has no header, so use 0 when calling playback_go.
> >> >
> >> > It's the value passed for the length that has been already loaded.
> >> > The program has read dta bytes for parsing the header, and it's raw
> >> > data without header.  Thus you need to pass dta there.
> >> >
> >> With the current code we assume that the raw files have at least 26 bytes.
> >> (sizeof (VocHeader).
> >>
> >> I think it doesn't matter here that dta bytes has already been loaded. These
> >> bytes useful to figure out the file header type.
> >>
> >> To keep things simple we can assume that in the case of raw files there is no
> >> header, so second parameter of playback_go (named loaded) can be set to 0.
> >
> > The loaded parameter of playback_go() doesn't mean that.  It's the
> > size that has been already read onto the audio buffer.  If you pass 0
> > there, it means that you discard the first 26 bytes samples you have
> > already read without playing.
> 
> Agree. Passing 0 discards the first 26 bytes that have already been read
> but these bytes will be re-read inside playback_go so they will be played.
> 
> playback_go( .. loaded = 0)
> ..
> 
> r = safe_read(fd, audio_buf + loaded, c). [aplay.c: 2757]

No no, safe_read won't *re*-read.  It just reads the data after the
point you've already read without seeking back.  So the first 26 bytes
will be lost by your patch.


thanks,

Takashi

Re: [PATCH] aplay: Fix header for raw format

[Daniel Baluta]

On Mon, Jul 3, 2017 at 1:14 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 03 Jul 2017 11:41:44 +0200,
> Daniel Baluta wrote:
>>
>> On Fri, Jun 30, 2017 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Fri, 30 Jun 2017 15:11:38 +0200,
>> > Daniel Baluta wrote:
>> >>
>> >> Hi Takashi,
>> >>
>> >> Thanks a lot for your feedback.
>> >>
>> >> On Fri, Jun 30, 2017 at 9:54 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> > On Thu, 29 Jun 2017 13:17:16 +0200,
>> >> > Ion-Horia Petrisor wrote:
>> >> >>
>> >> >> Raw format has no header, so use 0 when calling playback_go.
>> >> >
>> >> > It's the value passed for the length that has been already loaded.
>> >> > The program has read dta bytes for parsing the header, and it's raw
>> >> > data without header.  Thus you need to pass dta there.
>> >> >
>> >> With the current code we assume that the raw files have at least 26 bytes.
>> >> (sizeof (VocHeader).
>> >>
>> >> I think it doesn't matter here that dta bytes has already been loaded. These
>> >> bytes useful to figure out the file header type.
>> >>
>> >> To keep things simple we can assume that in the case of raw files there is no
>> >> header, so second parameter of playback_go (named loaded) can be set to 0.
>> >
>> > The loaded parameter of playback_go() doesn't mean that.  It's the
>> > size that has been already read onto the audio buffer.  If you pass 0
>> > there, it means that you discard the first 26 bytes samples you have
>> > already read without playing.
>>
>> Agree. Passing 0 discards the first 26 bytes that have already been read
>> but these bytes will be re-read inside playback_go so they will be played.
>>
>> playback_go( .. loaded = 0)
>> ..
>>
>> r = safe_read(fd, audio_buf + loaded, c). [aplay.c: 2757]
>
> No no, safe_read won't *re*-read.  It just reads the data after the
> point you've already read without seeking back.  So the first 26 bytes
> will be lost by your patch.

Oh, got it! Thanks for your patience.