On Fri, Jun 30, 2017 at 5:20 PM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
