| Message ID | 1541427790-27876-1-git-send-email-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] cplay: Always write frag * fragment_size | expand |
Hi Vinod, Just noticed that this patch is similar with this one for crecord http://git.alsa-project.org/?p=tinycompress.git;a=commit;h=e8e36567438c 16a5121943205a0cd8c63924d0d8 So, I think we can remove the RFC tag :). thanks, Daniel. On Lu, 2018-11-05 at 14:23 +0000, Daniel Baluta wrote: > cplay first writes frag * fragment_size and then > it only writes one fragment at a time. > > This means for example than if the user supplied a buffer_size > it will only be used for the first write. > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > I noticed this while investigating why cplay prints buffer_size as > 0 when not specified as command line argument to cplay. > > I also noticed that cred always reads frag * frament_size, so I think > this patch should be OK, but marking it as RFC to get your thoughts. > > src/utils/cplay.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/utils/cplay.c b/src/utils/cplay.c > index 2a52b52..b016f52 100644 > --- a/src/utils/cplay.c > +++ b/src/utils/cplay.c > @@ -435,15 +435,15 @@ void play_samples(char *name, unsigned int > card, unsigned int device, > }; > if (verbose) > printf("%s: Opened compress device\n", __func__); > - size = config.fragment_size; > - buffer = malloc(size * config.fragments); > + size = config.fragments * config.fragment_size; > + buffer = malloc(size); > if (!buffer) { > fprintf(stderr, "Unable to allocate %d bytes\n", > size); > goto COMP_EXIT; > } > > /* we will write frag fragment_size and then start */ > - num_read = fread(buffer, 1, size * config.fragments, file); > + num_read = fread(buffer, 1, size, file); > if (num_read > 0) { > if (verbose) > printf("%s: Doing first buffer write of > %d\n", __func__, num_read); > @@ -459,7 +459,7 @@ void play_samples(char *name, unsigned int card, > unsigned int device, > } > } > printf("Playing file %s On Card %u device %u, with buffer of > %lu bytes\n", > - name, card, device, buffer_size); > + name, card, device, size); > printf("Format %u Channels %u, %u Hz, Bit Rate %d\n", > codec.id, codec.ch_in, codec.sample_rate, > codec.bit_rate); >
On Thu, Nov 08, 2018 at 02:37:43PM +0000, Daniel Baluta wrote: > Hi Vinod, > > Just noticed that this patch is similar with this one for crecord > > http://git.alsa-project.org/?p=tinycompress.git;a=commit;h=e8e36567438c > 16a5121943205a0cd8c63924d0d8 > > So, I think we can remove the RFC tag :). > > thanks, > Daniel. > On Lu, 2018-11-05 at 14:23 +0000, Daniel Baluta wrote: > > cplay first writes frag * fragment_size and then > > it only writes one fragment at a time. > > > > This means for example than if the user supplied a buffer_size > > it will only be used for the first write. > > > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > > --- > > I noticed this while investigating why cplay prints buffer_size as > > 0 when not specified as command line argument to cplay. > > > > I also noticed that cred always reads frag * frament_size, so I think > > this patch should be OK, but marking it as RFC to get your thoughts. > > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> Yeah looks good to me. Thanks, Charles
On 05-11-18, 14:23, Daniel Baluta wrote: > cplay first writes frag * fragment_size and then > it only writes one fragment at a time. > > This means for example than if the user supplied a buffer_size > it will only be used for the first write. > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > I noticed this while investigating why cplay prints buffer_size as > 0 when not specified as command line argument to cplay. > > I also noticed that cred always reads frag * frament_size, so I think > this patch should be OK, but marking it as RFC to get your thoughts. > > src/utils/cplay.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/utils/cplay.c b/src/utils/cplay.c > index 2a52b52..b016f52 100644 > --- a/src/utils/cplay.c > +++ b/src/utils/cplay.c > @@ -435,15 +435,15 @@ void play_samples(char *name, unsigned int card, unsigned int device, > }; > if (verbose) > printf("%s: Opened compress device\n", __func__); > - size = config.fragment_size; > - buffer = malloc(size * config.fragments); > + size = config.fragments * config.fragment_size; > + buffer = malloc(size); net result of this change is no change. Leaving alone the variable names from this discussion, we allocate memory for size of config.fragments * config.fragment_size > if (!buffer) { > fprintf(stderr, "Unable to allocate %d bytes\n", size); > goto COMP_EXIT; > } > > /* we will write frag fragment_size and then start */ > - num_read = fread(buffer, 1, size * config.fragments, file); > + num_read = fread(buffer, 1, size, file); > if (num_read > 0) { > if (verbose) > printf("%s: Doing first buffer write of %d\n", __func__, num_read); > @@ -459,7 +459,7 @@ void play_samples(char *name, unsigned int card, unsigned int device, > } > } > printf("Playing file %s On Card %u device %u, with buffer of %lu bytes\n", > - name, card, device, buffer_size); > + name, card, device, size); I would prefer to fix the buffer_size when you dont specify in cmdline and we use driver defaults, in that case we calculate this, something like.. diff --git a/src/utils/cplay.c b/src/utils/cplay.c index 87863a307b47..a77e417ddf85 100644 --- a/src/utils/cplay.c +++ b/src/utils/cplay.c @@ -347,6 +347,11 @@ void play_samples(char *name, unsigned int card, unsigned int device, }; if (verbose) printf("%s: Opened compress device\n", __func__); + + /* check if buffer_size is used (driver defaults being used */ + if (!buffer_size) + buffer_size = config.fragment_size * config.fragments; + size = config.fragment_size; buffer = malloc(size * config.fragments); if (!buffer) { And of course, we need to optimize the size variable usage! This does not seem optimal :)
Hi Vinod, Thanks a lot for having a look at this. Please check my comments inline: <snip> > > - size = config.fragment_size; > > - buffer = malloc(size * config.fragments); > > + size = config.fragments * config.fragment_size; > > + buffer = malloc(size); > > net result of this change is no change. Leaving alone the variable names > from this discussion, we allocate memory for size of config.fragments * config.fragment_size Yes, you are right on this. There is no change here. But notice that we change the value of size to config.fragments * config.fragment_size; Now, please take a look at the inner loop at: https://github.com/alsa-project/tinycompress/blob/master/src/utils/cplay.c#L382 num_read = fread(buffer, 1, size, file); So, now instead of reading config.fragment_size we read config.fragments * config.fragment_size thus we fully utilize the buffer. I'm pretty sure this is the correct fix. See also crecord fix from Charles: http://git.alsa-project.org/?p=tinycompress.git;a=commit;h=e8e36567438c 16a5121943205a0cd8c63924d0d8 thanks, daniel.
diff --git a/src/utils/cplay.c b/src/utils/cplay.c index 2a52b52..b016f52 100644 --- a/src/utils/cplay.c +++ b/src/utils/cplay.c @@ -435,15 +435,15 @@ void play_samples(char *name, unsigned int card, unsigned int device, }; if (verbose) printf("%s: Opened compress device\n", __func__); - size = config.fragment_size; - buffer = malloc(size * config.fragments); + size = config.fragments * config.fragment_size; + buffer = malloc(size); if (!buffer) { fprintf(stderr, "Unable to allocate %d bytes\n", size); goto COMP_EXIT; } /* we will write frag fragment_size and then start */ - num_read = fread(buffer, 1, size * config.fragments, file); + num_read = fread(buffer, 1, size, file); if (num_read > 0) { if (verbose) printf("%s: Doing first buffer write of %d\n", __func__, num_read); @@ -459,7 +459,7 @@ void play_samples(char *name, unsigned int card, unsigned int device, } } printf("Playing file %s On Card %u device %u, with buffer of %lu bytes\n", - name, card, device, buffer_size); + name, card, device, size); printf("Format %u Channels %u, %u Hz, Bit Rate %d\n", codec.id, codec.ch_in, codec.sample_rate, codec.bit_rate);
cplay first writes frag * fragment_size and then it only writes one fragment at a time. This means for example than if the user supplied a buffer_size it will only be used for the first write. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> --- I noticed this while investigating why cplay prints buffer_size as 0 when not specified as command line argument to cplay. I also noticed that cred always reads frag * frament_size, so I think this patch should be OK, but marking it as RFC to get your thoughts. src/utils/cplay.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)