Re: [TINYCOMPRESS PATCH 1/2] crec: Initial version of a compressed capture utility

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

On Wed, Nov 27, 2013 at 01:38:24PM +0530, Vinod Koul wrote:
> On Tue, Nov 19, 2013 at 04:46:18PM +0000, Charles Keepax wrote:
> > This version only supports capture of PCM streams over a compressed
> > device and saves these as WAV files.
> Thanks for posting this :) I was about to ping you ad get these done if you were
> busy...

Sorry about the delay on looking at this again, I keep getting
pulled off in other directions :-)

> > +static void usage(void)
> > +{
> > +	fprintf(stderr, "usage: crec [OPTIONS] filename\n"
> > +		"-c\tcard number\n"
> > +		"-d\tdevice node\n"
> > +		"-b\tbuffer size\n"
> > +		"-f\tfragments\n"
> > +		"-v\tverbose mode\n"
> > +		"-l\tlength of record in seconds\n"
> perhaps it would be more intutive for time. We cant use d here :(

Yeah can't use 'd', I guess I could use 't' if preferred?


> 
> > +		"-h\tPrints this help list\n\n"
> > +		"-C\tSpecify the number of channels (default %u)\n"
> > +		"-R\tSpecify the sample rate (default %u)\n"
> can these be lower case?

Unfortunately, 'c', card and 'f', fragments are both used so I
opted to use capitals for the stream parameters (channels, rate,
format). I am open to changing these to something else but you
end up using weird letters for things and caps felt most clear.

> 
> > +		"-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n\n"
> > +		"Example:\n"
> > +		"\tcrec -c 1 -d 2 test.wav\n"
> > +		"\tcrec -f 5 test.wav\n",
> > +		DEFAULT_CHANNELS, DEFAULT_RATE);
> > +
> > +	exit(EXIT_FAILURE);
> > +}

<snip>

> > +	if (verbose)
> > +		printf("%s: entry, reading %u bytes\n", __func__, length);
> > +
> > +	file = fopen(name, "w+b");
> why w+?

Oops, yeah that should be changed.

> > +	if (!file) {
> > +		fprintf(stderr, "Unable to open file '%s'\n", name);
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	/* Write a header, will update with size once record is complete */
> > +	init_wave_header(&header, channels, rate, samplebits);
> > +	written = fwrite(&header, sizeof(header), 1, file);
> > +	if (written != 1) {
> > +		fprintf(stderr, "Error writing output file header: %d\n",
> > +			ferror(file));
> > +		goto file_exit;
> > +	}
> > +
> > +	codec.id = SND_AUDIOCODEC_PCM;
> > +	codec.ch_in = channels;
> > +	codec.ch_out = channels;
> > +	codec.sample_rate = compress_get_alsa_rate(rate);
> > +	if (!codec.sample_rate) {
> > +		fprintf(stderr, "invalid sample rate %d\n", rate);
> > +		goto file_exit;
> > +	}
> > +	codec.bit_rate = 0;
> > +	codec.rate_control = 0;
> > +	codec.profile = 0;
> > +	codec.level = 0;
> > +	codec.ch_mode = 0;
> > +	codec.format = format;
> why not do memset of the codec and configure only what we need...

Yeah, that is a good point I will update this as well.

Thanks,
Charles