[alsa-devel] [RFC PATCH] cplay: Always write frag * fragment_size
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);
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#L...
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.
participants (4)
-
Charles Keepax
-
Daniel Baluta
-
Daniel Baluta
-
Vinod Koul