[alsa-devel] alsa-lib changes in pcm file plugin
Hi,
This short patch series contains three independent changes in pcm file plugin. The first two of them are trivial fixes and should not cause any behavior changes and regressions. The third one contains changes to make pcm_file.c access input/output files only when needed and thus to make its behavior more intuitive (from my point of view) (i.e., write to output file only on playback and read from input file only on capture).
The patches should apply cleanly against current HEAD (ae035b7fe5620fcaf4f5ea33ecabcf93b8e056cd).
Commit 5c5f1358123af69155267463a0b6254ad9cbecc4 requires both file and infile options to be missing to report a failure. In fact, only file option is mandatory and should be checked there. Otherwise, NULL file triggers segfault in snd_pcm_file_replace_fname() called from snd_pcm_file_open_output_file(). infile option is optional, so don't report fatal error if it's missing.
Signed-off-by: Andrey Mazo mazo@telum.ru --- src/pcm/pcm_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 5950401..3d14090 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -948,7 +948,7 @@ int _snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, err = snd_pcm_slave_conf(root, slave, &sconf, 0); if (err < 0) return err; - if ((!fname || strlen(fname) == 0) && fd < 0 && !ifname) { + if ((!fname || strlen(fname) == 0) && fd < 0) { snd_config_delete(sconf); SNDERR("file is not defined"); return -EINVAL;
Valgrind report for this leak was:
Command: aplay -Dfile:'/tmp/qqq',raw qqq.wav
14 bytes in 1 blocks are definitely lost in loss record 1 of 2 at 0x402BF5C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) by 0x40D7557: snd_pcm_file_hw_params (in /usr/lib/libasound.so.2.0.0) by 0x40BA093: _snd_pcm_hw_params_internal (in /usr/lib/libasound.so.2.0.0) by 0x40AB831: snd_pcm_hw_params (in /usr/lib/libasound.so.2.0.0) by 0x804C523: ??? (in /usr/bin/aplay) by 0x804E5B7: ??? (in /usr/bin/aplay) by 0x804FC8C: ??? (in /usr/bin/aplay) by 0x80520FB: ??? (in /usr/bin/aplay) by 0x4184942: (below main) (libc-start.c:226)
Signed-off-by: Andrey Mazo mazo@telum.ru --- src/pcm/pcm_file.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 3d14090..c3e67b2 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -592,8 +592,10 @@ static int snd_pcm_file_hw_free(snd_pcm_t *pcm) snd_pcm_file_t *file = pcm->private_data; free(file->wbuf); free(file->wbuf_areas); + free(file->final_fname); file->wbuf = NULL; file->wbuf_areas = NULL; + file->final_fname = NULL; return snd_pcm_hw_free(file->gen.slave); }
Commit 1d80c5b901baf7e1b7998dfa518532fbd64e4283 message describes behaviour in case of specified infile option as 'No file writes will take place in this case'. But this is clearly not the case as output file gets truncated while running `arecord -Dtestin >/dev/null`, where "testin" is defined as pcm.testin { type file slave.pcm null file "/tmp/qqqq.out" infile "/tmp/qqqq.in" format "raw" }
Besides that, the existing behaviour is rather counterintuitive, requiring both output and input files to exist and making access to them regardless of playback or capture intention. Also, it's very confusing to get output file truncated while trying to just capture from the device.
Current changeset introduces the following behaviour: - output file ("file" option) is only (p)open()'ed for writing only on playback to the device - any data is written to the output file descriptor (provided with "file" option) only on playback to the device - input file ("infile" option) is only open()'ed for reading only on capture from the device - any data is read from the input file descriptor (provided with the "infile" option) only on capture from the device
Signed-off-by: Andrey Mazo mazo@telum.ru --- src/pcm/pcm_file.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index c3e67b2..25055d0 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -406,7 +406,9 @@ static int snd_pcm_file_close(snd_pcm_t *pcm) if (file->wav_header.fmt) fixup_wav_header(pcm); free((void *)file->fname); - close(file->fd); + if (file->fd >= 0) { + close(file->fd); + } } if (file->ifname) { free((void *)file->ifname); @@ -533,7 +535,6 @@ static snd_pcm_sframes_t snd_pcm_file_writen(snd_pcm_t *pcm, void **bufs, snd_pc static snd_pcm_sframes_t snd_pcm_file_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size) { snd_pcm_file_t *file = pcm->private_data; - snd_pcm_channel_area_t areas[pcm->channels]; snd_pcm_sframes_t n;
n = snd_pcm_readi(file->gen.slave, buffer, size); @@ -545,15 +546,12 @@ static snd_pcm_sframes_t snd_pcm_file_readi(snd_pcm_t *pcm, void *buffer, snd_pc return n; return n * 8 / pcm->frame_bits; } - snd_pcm_areas_from_buf(pcm, areas, buffer); - snd_pcm_file_add_frames(pcm, areas, 0, n); return n; }
static snd_pcm_sframes_t snd_pcm_file_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size) { snd_pcm_file_t *file = pcm->private_data; - snd_pcm_channel_area_t areas[pcm->channels]; snd_pcm_sframes_t n;
if (file->ifd >= 0) { @@ -562,10 +560,6 @@ static snd_pcm_sframes_t snd_pcm_file_readn(snd_pcm_t *pcm, void **bufs, snd_pcm }
n = snd_pcm_readn(file->gen.slave, bufs, size); - if (n > 0) { - snd_pcm_areas_from_bufs(pcm, areas, bufs); - snd_pcm_file_add_frames(pcm, areas, 0, n); - } return n; }
@@ -629,7 +623,7 @@ static int snd_pcm_file_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params) a->first = slave->sample_bits * channel; a->step = slave->frame_bits; } - if (file->fd < 0) { + if ((file->fd < 0) && (pcm->stream == SND_PCM_STREAM_PLAYBACK)) { err = snd_pcm_file_open_output_file(file); if (err < 0) { SYSERR("failed opening output file %s", file->fname); @@ -728,7 +722,8 @@ static const snd_pcm_fast_ops_t snd_pcm_file_fast_ops = { int snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, const char *fname, int fd, const char *ifname, int ifd, int trunc, - const char *fmt, int perm, snd_pcm_t *slave, int close_slave) + const char *fmt, int perm, snd_pcm_t *slave, int close_slave, + snd_pcm_stream_t stream) { snd_pcm_t *pcm; snd_pcm_file_t *file; @@ -758,7 +753,7 @@ int snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, file->trunc = trunc; file->perm = perm;
- if (ifname) { + if (ifname && (stream == SND_PCM_STREAM_CAPTURE)) { ifd = open(ifname, O_RDONLY); /* TODO: mind blocking mode */ if (ifd < 0) { SYSERR("open %s for reading failed", ifname); @@ -790,6 +785,7 @@ int snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, #else pcm->monotonic = 0; #endif + pcm->stream = stream; snd_pcm_link_hw_ptr(pcm, slave); snd_pcm_link_appl_ptr(pcm, slave); *pcmp = pcm; @@ -960,7 +956,7 @@ int _snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, if (err < 0) return err; err = snd_pcm_file_open(pcmp, name, fname, fd, ifname, ifd, - trunc, format, perm, spcm, 1); + trunc, format, perm, spcm, 1, stream); if (err < 0) snd_pcm_close(spcm); return err;
At Sun, 17 Nov 2013 01:11:53 +0400, Andrey Mazo wrote:
Hi,
This short patch series contains three independent changes in pcm file plugin. The first two of them are trivial fixes and should not cause any behavior changes and regressions. The third one contains changes to make pcm_file.c access input/output files only when needed and thus to make its behavior more intuitive (from my point of view) (i.e., write to output file only on playback and read from input file only on capture).
The patches should apply cleanly against current HEAD (ae035b7fe5620fcaf4f5ea33ecabcf93b8e056cd).
Thanks, applied all three patches.
Takashi
Thank you very much for replying so quickly!
On Sun, 17 Nov 2013 13:16:28 +0400, Takashi Iwai tiwai@suse.de wrote:
At Sun, 17 Nov 2013 01:11:53 +0400, Andrey Mazo wrote:
Hi,
This short patch series contains three independent changes in pcm file plugin. The first two of them are trivial fixes and should not cause any behavior changes and regressions. The third one contains changes to make pcm_file.c access input/output files only when needed and thus to make its behavior more intuitive (from my point of view) (i.e., write to output file only on playback and read from input file only on capture).
The patches should apply cleanly against current HEAD (ae035b7fe5620fcaf4f5ea33ecabcf93b8e056cd).
Thanks, applied all three patches.
Takashi
Document function argument, added in commit 4081be0b87ab9fa53a8906e66bc240f18a7a9a54.
Signed-off-by: Andrey Mazo mazo@telum.ru --- src/pcm/pcm_file.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 25055d0..7123025 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -714,6 +714,7 @@ static const snd_pcm_fast_ops_t snd_pcm_file_fast_ops = { * \param perm File permission * \param slave Slave PCM handle * \param close_slave When set, the slave PCM handle is closed with copy PCM + * \param stream the direction of PCM stream * \retval zero on success otherwise a negative error code * \warning Using of this function might be dangerous in the sense * of compatibility reasons. The prototype might be freely
At Sun, 17 Nov 2013 19:45:19 +0400, Andrey Mazo wrote:
Document function argument, added in commit 4081be0b87ab9fa53a8906e66bc240f18a7a9a54.
Signed-off-by: Andrey Mazo mazo@telum.ru
Thanks, applied.
Takashi
src/pcm/pcm_file.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 25055d0..7123025 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -714,6 +714,7 @@ static const snd_pcm_fast_ops_t snd_pcm_file_fast_ops = {
- \param perm File permission
- \param slave Slave PCM handle
- \param close_slave When set, the slave PCM handle is closed with copy PCM
- \param stream the direction of PCM stream
- \retval zero on success otherwise a negative error code
- \warning Using of this function might be dangerous in the sense
of compatibility reasons. The prototype might be freely
-- 1.8.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thank you very much! Sorry, I've missed that in the first place.
On Mon, 18 Nov 2013 12:19:34 +0400, Takashi Iwai tiwai@suse.de wrote:
At Sun, 17 Nov 2013 19:45:19 +0400, Andrey Mazo wrote:
Document function argument, added in commit 4081be0b87ab9fa53a8906e66bc240f18a7a9a54.
Signed-off-by: Andrey Mazo mazo@telum.ru
Thanks, applied.
Takashi
src/pcm/pcm_file.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 25055d0..7123025 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -714,6 +714,7 @@ static const snd_pcm_fast_ops_t snd_pcm_file_fast_ops = {
- \param perm File permission
- \param slave Slave PCM handle
- \param close_slave When set, the slave PCM handle is closed with copy PCM
- \param stream the direction of PCM stream
- \retval zero on success otherwise a negative error code
- \warning Using of this function might be dangerous in the sense
of compatibility reasons. The prototype might be freely
-- 1.8.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (2)
-
Andrey Mazo
-
Takashi Iwai