[RFC PATCH] cplay: Always write frag * fragment_size

[RFC PATCH] cplay: Always write frag * fragment_size

[Daniel Baluta]

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);
 
-- 
2.7.4

Re: [RFC PATCH] cplay: Always write frag * fragment_size

[Daniel Baluta]

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);
>  
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH] cplay: Always write frag * fragment_size

[Charles Keepax]

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

Re: [RFC PATCH] cplay: Always write frag * fragment_size

[Vinod Koul]

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 :)

-- 
~Vinod

Re: [RFC PATCH] cplay: Always write frag * fragment_size

[Daniel Baluta]

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.