| Message ID | 1479296649-18278-1-git-send-email-rf@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > This patch adds a -I command line option to set the codec ID, > either from a defined set of string values or as a number. Can you explain why you want to add this? The utility cant really record a mp3 file! > > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> > --- > src/utils/crec.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 59 insertions(+), 7 deletions(-) > > diff --git a/src/utils/crec.c b/src/utils/crec.c > index 8d5b7b0..a586fc4 100644 > --- a/src/utils/crec.c > +++ b/src/utils/crec.c > @@ -83,6 +83,27 @@ static bool streamed; > static const unsigned int DEFAULT_CHANNELS = 1; > static const unsigned int DEFAULT_RATE = 44100; > static const unsigned int DEFAULT_FORMAT = SNDRV_PCM_FORMAT_S16_LE; > +static const unsigned int DEFAULT_CODEC_ID = SND_AUDIOCODEC_PCM; > + > +static const struct { > + const char *name; > + unsigned int id; > +} codec_ids[] = { > + { "PCM", SND_AUDIOCODEC_PCM }, > + { "MP3", SND_AUDIOCODEC_MP3 }, > + { "AMR", SND_AUDIOCODEC_AMR }, > + { "AMRWB", SND_AUDIOCODEC_AMRWB }, > + { "ARMWBPLUS", SND_AUDIOCODEC_AMRWBPLUS }, > + { "AAC", SND_AUDIOCODEC_AAC }, > + { "WMA", SND_AUDIOCODEC_WMA }, > + { "REAL", SND_AUDIOCODEC_REAL }, > + { "VORBIS", SND_AUDIOCODEC_VORBIS }, > + { "FLAC", SND_AUDIOCODEC_FLAC }, > + { "IEC61937", SND_AUDIOCODEC_IEC61937 }, > + { "G723_1", SND_AUDIOCODEC_G723_1 }, > + { "G729", SND_AUDIOCODEC_G729 }, > +}; > +#define CREC_NUM_CODEC_IDS (sizeof(codec_ids) / sizeof(codec_ids[0])) > > struct riff_chunk { > char desc[4]; > @@ -153,6 +174,8 @@ static void size_wave_header(struct wave_header *header, uint32_t size) > > static void usage(void) > { > + int i; > + > fprintf(stderr, "usage: crec [OPTIONS] [filename]\n" > "-c\tcard number\n" > "-d\tdevice node\n" > @@ -163,14 +186,22 @@ static void usage(void) > "-h\tPrints this help list\n\n" > "-C\tSpecify the number of channels (default %u)\n" > "-R\tSpecify the sample rate (default %u)\n" > - "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n\n" > + "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n" > + "-I\tSpecify codec ID (default PCM)\n\n" > "If filename is not given the output is\n" > "written to stdout\n\n" > "Example:\n" > "\tcrec -c 1 -d 2 test.wav\n" > - "\tcrec -f 5 test.wav\n", > + "\tcrec -f 5 test.wav\n\n" > + "Valid codec IDs:\n", > DEFAULT_CHANNELS, DEFAULT_RATE); > > + for (i = 0; i < CREC_NUM_CODEC_IDS; ++i) > + fprintf(stderr, "%s%c", codec_ids[i].name, > + (i % 8) ? ' ' : '\n'); > + > + fprintf(stderr, "\nor the value in decimal or hex\n"); > + > exit(EXIT_FAILURE); > } > > @@ -239,7 +270,8 @@ static int finish_record(void) > static void capture_samples(char *name, unsigned int card, unsigned int device, > unsigned long buffer_size, unsigned int frag, > unsigned int length, unsigned int rate, > - unsigned int channels, unsigned int format) > + unsigned int channels, unsigned int format, > + unsigned int codec_id) > { > struct compr_config config; > struct snd_codec codec; > @@ -288,7 +320,7 @@ static void capture_samples(char *name, unsigned int card, unsigned int device, > > memset(&codec, 0, sizeof(codec)); > memset(&config, 0, sizeof(config)); > - codec.id = SND_AUDIOCODEC_PCM; > + codec.id = codec_id; So we are going to dump raw encoded data. I am not sure thats a smart choice.. We should really support format headers and save proper files
On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > This patch adds a -I command line option to set the codec ID, > > either from a defined set of string values or as a number. > > Can you explain why you want to add this? The utility cant really record a > mp3 file! > > You need to be able to pass a codec ID that the driver supports, and to indicate which codec you're trying to use. It's not useful to only be able to open the "PCM" codec. It doesn't really matter whether crec understands the content of the data, we're just pulling raw data, most likely for test/debug. The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream ID so we need a way to pass that. And we'd also need it for any drivers that had streams using other codec IDs. > > > > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> > > --- > > src/utils/crec.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 59 insertions(+), 7 deletions(-) > > > > diff --git a/src/utils/crec.c b/src/utils/crec.c > > index 8d5b7b0..a586fc4 100644 > > --- a/src/utils/crec.c > > +++ b/src/utils/crec.c > > @@ -83,6 +83,27 @@ static bool streamed; > > static const unsigned int DEFAULT_CHANNELS = 1; > > static const unsigned int DEFAULT_RATE = 44100; > > static const unsigned int DEFAULT_FORMAT = SNDRV_PCM_FORMAT_S16_LE; > > +static const unsigned int DEFAULT_CODEC_ID = SND_AUDIOCODEC_PCM; > > + > > +static const struct { > > + const char *name; > > + unsigned int id; > > +} codec_ids[] = { > > + { "PCM", SND_AUDIOCODEC_PCM }, > > + { "MP3", SND_AUDIOCODEC_MP3 }, > > + { "AMR", SND_AUDIOCODEC_AMR }, > > + { "AMRWB", SND_AUDIOCODEC_AMRWB }, > > + { "ARMWBPLUS", SND_AUDIOCODEC_AMRWBPLUS }, > > + { "AAC", SND_AUDIOCODEC_AAC }, > > + { "WMA", SND_AUDIOCODEC_WMA }, > > + { "REAL", SND_AUDIOCODEC_REAL }, > > + { "VORBIS", SND_AUDIOCODEC_VORBIS }, > > + { "FLAC", SND_AUDIOCODEC_FLAC }, > > + { "IEC61937", SND_AUDIOCODEC_IEC61937 }, > > + { "G723_1", SND_AUDIOCODEC_G723_1 }, > > + { "G729", SND_AUDIOCODEC_G729 }, > > +}; > > +#define CREC_NUM_CODEC_IDS (sizeof(codec_ids) / sizeof(codec_ids[0])) > > > > struct riff_chunk { > > char desc[4]; > > @@ -153,6 +174,8 @@ static void size_wave_header(struct wave_header *header, uint32_t size) > > > > static void usage(void) > > { > > + int i; > > + > > fprintf(stderr, "usage: crec [OPTIONS] [filename]\n" > > "-c\tcard number\n" > > "-d\tdevice node\n" > > @@ -163,14 +186,22 @@ static void usage(void) > > "-h\tPrints this help list\n\n" > > "-C\tSpecify the number of channels (default %u)\n" > > "-R\tSpecify the sample rate (default %u)\n" > > - "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n\n" > > + "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n" > > + "-I\tSpecify codec ID (default PCM)\n\n" > > "If filename is not given the output is\n" > > "written to stdout\n\n" > > "Example:\n" > > "\tcrec -c 1 -d 2 test.wav\n" > > - "\tcrec -f 5 test.wav\n", > > + "\tcrec -f 5 test.wav\n\n" > > + "Valid codec IDs:\n", > > DEFAULT_CHANNELS, DEFAULT_RATE); > > > > + for (i = 0; i < CREC_NUM_CODEC_IDS; ++i) > > + fprintf(stderr, "%s%c", codec_ids[i].name, > > + (i % 8) ? ' ' : '\n'); > > + > > + fprintf(stderr, "\nor the value in decimal or hex\n"); > > + > > exit(EXIT_FAILURE); > > } > > > > @@ -239,7 +270,8 @@ static int finish_record(void) > > static void capture_samples(char *name, unsigned int card, unsigned int device, > > unsigned long buffer_size, unsigned int frag, > > unsigned int length, unsigned int rate, > > - unsigned int channels, unsigned int format) > > + unsigned int channels, unsigned int format, > > + unsigned int codec_id) > > { > > struct compr_config config; > > struct snd_codec codec; > > @@ -288,7 +320,7 @@ static void capture_samples(char *name, unsigned int card, unsigned int device, > > > > memset(&codec, 0, sizeof(codec)); > > memset(&config, 0, sizeof(config)); > > - codec.id = SND_AUDIOCODEC_PCM; > > + codec.id = codec_id; > > > So we are going to dump raw encoded data. I am not sure thats a smart > choice.. We should really support format headers and save proper files > >
On Wed, Nov 16, 2016 at 01:07:55PM +0000, Richard Fitzgerald wrote: > On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > > This patch adds a -I command line option to set the codec ID, > > > either from a defined set of string values or as a number. > > > > Can you explain why you want to add this? The utility cant really record a > > mp3 file! > > > > > > You need to be able to pass a codec ID that the driver supports, and to > indicate which codec you're trying to use. It's not useful to only be > able to open the "PCM" codec. It doesn't really matter whether crec > understands the content of the data, we're just pulling raw data, most > likely for test/debug. > > The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream > ID so we need a way to pass that. And we'd also need it for any drivers > that had streams using other codec IDs. > Is the objection here not that crec is wrapping the data with a WAV file? Should we perhaps just expand this so that if you request a different format it uses the raw data mode that it uses when you let the output go to stdout. Thanks, Charles
On Wed, 2016-11-16 at 13:48 +0000, Charles Keepax wrote: > On Wed, Nov 16, 2016 at 01:07:55PM +0000, Richard Fitzgerald wrote: > > On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > > > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > > > This patch adds a -I command line option to set the codec ID, > > > > either from a defined set of string values or as a number. > > > > > > Can you explain why you want to add this? The utility cant really record a > > > mp3 file! > > > > > > > > > > You need to be able to pass a codec ID that the driver supports, and to > > indicate which codec you're trying to use. It's not useful to only be > > able to open the "PCM" codec. It doesn't really matter whether crec > > understands the content of the data, we're just pulling raw data, most > > likely for test/debug. > > > > The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream > > ID so we need a way to pass that. And we'd also need it for any drivers > > that had streams using other codec IDs. > > > > Is the objection here not that crec is wrapping the data with > a WAV file? Should we perhaps just expand this so that if you > request a different format it uses the raw data mode that it uses > when you let the output go to stdout. > Funny I thought we'd already added a flag for saving to the file raw. It's raw when you pipe it to stdout. > Thanks, > Charles
On Wed, Nov 16, 2016 at 02:53:21PM +0000, Richard Fitzgerald wrote: > On Wed, 2016-11-16 at 13:48 +0000, Charles Keepax wrote: > > On Wed, Nov 16, 2016 at 01:07:55PM +0000, Richard Fitzgerald wrote: > > > On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > > > > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > > > > This patch adds a -I command line option to set the codec ID, > > > > > either from a defined set of string values or as a number. > > > > > > > > Can you explain why you want to add this? The utility cant really record a > > > > mp3 file! > > > > > > You need to be able to pass a codec ID that the driver supports, and to > > > indicate which codec you're trying to use. It's not useful to only be > > > able to open the "PCM" codec. It doesn't really matter whether crec > > > understands the content of the data, we're just pulling raw data, most > > > likely for test/debug. > > > > > > The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream > > > ID so we need a way to pass that. And we'd also need it for any drivers > > > that had streams using other codec IDs. Ah that was my guess too. I am okay to be able to add bespoke format and use that to dump raw data. But I am not okay to add codec formats which we dont support.. > > Is the objection here not that crec is wrapping the data with > > a WAV file? Should we perhaps just expand this so that if you > > request a different format it uses the raw data mode that it uses > > when you let the output go to stdout. > > > > Funny I thought we'd already added a flag for saving to the file raw. > It's raw when you pipe it to stdout.
On Fri, 2016-11-18 at 09:23 +0530, Vinod Koul wrote: > On Wed, Nov 16, 2016 at 02:53:21PM +0000, Richard Fitzgerald wrote: > > On Wed, 2016-11-16 at 13:48 +0000, Charles Keepax wrote: > > > On Wed, Nov 16, 2016 at 01:07:55PM +0000, Richard Fitzgerald wrote: > > > > On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > > > > > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > > > > > This patch adds a -I command line option to set the codec ID, > > > > > > either from a defined set of string values or as a number. > > > > > > > > > > Can you explain why you want to add this? The utility cant really record a > > > > > mp3 file! > > > > > > > > You need to be able to pass a codec ID that the driver supports, and to > > > > indicate which codec you're trying to use. It's not useful to only be > > > > able to open the "PCM" codec. It doesn't really matter whether crec > > > > understands the content of the data, we're just pulling raw data, most > > > > likely for test/debug. > > > > > > > > The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream > > > > ID so we need a way to pass that. And we'd also need it for any drivers > > > > that had streams using other codec IDs. > > Ah that was my guess too. > > I am okay to be able to add bespoke format and use that to dump raw data. > But I am not okay to add codec formats which we dont support.. > Well a raw dump would support MP3 because that is doesn't require any header and the framing is part of the data stream. So no special handling needed there. This may also apply to some of the other formats (I'm not an expert on them) so I'd rather have the option to be able to raw-dump any format and leave it to the user to decide whether that's sensible. If you're debugging you may not care about the file format, you're actually interested in the raw data you get from the codec so dumping the output to a raw file would be useful even if you can't load that file into a music player. > > > Is the objection here not that crec is wrapping the data with > > > a WAV file? Should we perhaps just expand this so that if you > > > request a different format it uses the raw data mode that it uses > > > when you let the output go to stdout. > > > > > > > Funny I thought we'd already added a flag for saving to the file raw. > > It's raw when you pipe it to stdout. >
On Fri, Nov 18, 2016 at 10:11:08AM +0000, Richard Fitzgerald wrote: > On Fri, 2016-11-18 at 09:23 +0530, Vinod Koul wrote: > > On Wed, Nov 16, 2016 at 02:53:21PM +0000, Richard Fitzgerald wrote: > > > On Wed, 2016-11-16 at 13:48 +0000, Charles Keepax wrote: > > > > On Wed, Nov 16, 2016 at 01:07:55PM +0000, Richard Fitzgerald wrote: > > > > > On Wed, 2016-11-16 at 18:35 +0530, Vinod Koul wrote: > > > > > > On Wed, Nov 16, 2016 at 11:44:09AM +0000, Richard Fitzgerald wrote: > > > > > > > This patch adds a -I command line option to set the codec ID, > > > > > > > either from a defined set of string values or as a number. > > > > > > > > > > > > Can you explain why you want to add this? The utility cant really record a > > > > > > mp3 file! > > > > > > > > > > You need to be able to pass a codec ID that the driver supports, and to > > > > > indicate which codec you're trying to use. It's not useful to only be > > > > > able to open the "PCM" codec. It doesn't really matter whether crec > > > > > understands the content of the data, we're just pulling raw data, most > > > > > likely for test/debug. > > > > > > > > > > The wm_adsp driver on Wolfson/Cirrus codecs uses the new BESPOKE stream > > > > > ID so we need a way to pass that. And we'd also need it for any drivers > > > > > that had streams using other codec IDs. > > > > Ah that was my guess too. > > > > I am okay to be able to add bespoke format and use that to dump raw data. > > But I am not okay to add codec formats which we dont support.. > > > > Well a raw dump would support MP3 because that is doesn't require any > header and the framing is part of the data stream. So no special > handling needed there. This may also apply to some of the other formats > (I'm not an expert on them) so I'd rather have the option to be able to > raw-dump any format and leave it to the user to decide whether that's > sensible. > > If you're debugging you may not care about the file format, you're > actually interested in the raw data you get from the codec so dumping > the output to a raw file would be useful even if you can't load that > file into a music player. While I agree with you on this, am worried that adding codecs may make people think that we can record mp3 file for exmaple, which is not the case. I think we can add pcm and bespoke as formats supported and allow any format to be dumped to stdio. That way it is pretty clear to people ... Thanks
>> If you're debugging you may not care about the file format, you're >> actually interested in the raw data you get from the codec so dumping >> the output to a raw file would be useful even if you can't load that >> file into a music player. > > While I agree with you on this, am worried that adding codecs may make > people think that we can record mp3 file for exmaple, which is not the case. > > I think we can add pcm and bespoke as formats supported and allow any format to > be dumped to stdio. That way it is pretty clear to people ... Crec as is it is pretty useless with PCM only...If we added the profile selection and things like bitrate information it'd be straightforward to support elementary bitstreams like MP3 or AAC ADTS, you just dump the data to a file. Things that require a header or MP4 integration would require additional libraries, this is no longer 'tiny'.
On Fri, Nov 18, 2016 at 08:39:38AM -0600, Pierre-Louis Bossart wrote: > > >>If you're debugging you may not care about the file format, you're > >>actually interested in the raw data you get from the codec so dumping > >>the output to a raw file would be useful even if you can't load that > >>file into a music player. > > > >While I agree with you on this, am worried that adding codecs may make > >people think that we can record mp3 file for exmaple, which is not the case. > > > >I think we can add pcm and bespoke as formats supported and allow any format to > >be dumped to stdio. That way it is pretty clear to people ... > > Crec as is it is pretty useless with PCM only...If we added the profile > selection and things like bitrate information it'd be straightforward to > support elementary bitstreams like MP3 or AAC ADTS, you just dump the data > to a file. Things that require a header or MP4 integration would require > additional libraries, this is no longer 'tiny'. What about this as a suggestion, if we remove the code that adds the WAV header. Then all the output from crecord is raw data and the addition of any headers or additional wrapping is up to the user. That keeps crecord 'tiny' and allows us to support all the formats in a consistent way so no one gets confused. We haven't actually used the WAV header stuff since the very early versions of our firmware that didn't use compression on the stream and actually did output PCM data and I don't think anyone else has ever used the compressed interface for PCM. Thanks, Charles
On Fri, Nov 18, 2016 at 04:17:37PM +0000, Charles Keepax wrote: > On Fri, Nov 18, 2016 at 08:39:38AM -0600, Pierre-Louis Bossart wrote: > > > > >>If you're debugging you may not care about the file format, you're > > >>actually interested in the raw data you get from the codec so dumping > > >>the output to a raw file would be useful even if you can't load that > > >>file into a music player. > > > > > >While I agree with you on this, am worried that adding codecs may make > > >people think that we can record mp3 file for exmaple, which is not the case. > > > > > >I think we can add pcm and bespoke as formats supported and allow any format to > > >be dumped to stdio. That way it is pretty clear to people ... > > > > Crec as is it is pretty useless with PCM only...If we added the profile > > selection and things like bitrate information it'd be straightforward to > > support elementary bitstreams like MP3 or AAC ADTS, you just dump the data > > to a file. Things that require a header or MP4 integration would require > > additional libraries, this is no longer 'tiny'. > > What about this as a suggestion, if we remove the code that adds > the WAV header. Then all the output from crecord is raw data and > the addition of any headers or additional wrapping is up to the > user. That keeps crecord 'tiny' and allows us to support all the > formats in a consistent way so no one gets confused. Naah, crecord is a utility. If you need to do above you have tinycompress APIs to get the data and pack it the way you like.. You don't and shouldn't use crecord for that. > We haven't actually used the WAV header stuff since the very > early versions of our firmware that didn't use compression on the > stream and actually did output PCM data and I don't think anyone > else has ever used the compressed interface for PCM. Thats fine by me. The only reason we have a WAV header is that we can pack PCM files, for rest of the formats it become tricky as Pierre mention so lets not go that road :)
On Wed, 2016-11-23 at 09:11 +0530, Vinod Koul wrote: > On Fri, Nov 18, 2016 at 04:17:37PM +0000, Charles Keepax wrote: > > On Fri, Nov 18, 2016 at 08:39:38AM -0600, Pierre-Louis Bossart wrote: > > > > > > >>If you're debugging you may not care about the file format, you're > > > >>actually interested in the raw data you get from the codec so dumping > > > >>the output to a raw file would be useful even if you can't load that > > > >>file into a music player. > > > > > > > >While I agree with you on this, am worried that adding codecs may make > > > >people think that we can record mp3 file for exmaple, which is not the case. > > > > > > > >I think we can add pcm and bespoke as formats supported and allow any format to > > > >be dumped to stdio. That way it is pretty clear to people ... > > > > > > Crec as is it is pretty useless with PCM only...If we added the profile > > > selection and things like bitrate information it'd be straightforward to > > > support elementary bitstreams like MP3 or AAC ADTS, you just dump the data > > > to a file. Things that require a header or MP4 integration would require > > > additional libraries, this is no longer 'tiny'. > > > > What about this as a suggestion, if we remove the code that adds > > the WAV header. Then all the output from crecord is raw data and > > the addition of any headers or additional wrapping is up to the > > user. That keeps crecord 'tiny' and allows us to support all the > > formats in a consistent way so no one gets confused. > > Naah, crecord is a utility. If you need to do above you have tinycompress > APIs to get the data and pack it the way you like.. You don't and shouldn't > use crecord for that. > But you can't call APIs from a shell command line, it needs a tool. For debugging and testing we need a utility that can extract the raw data from a codec. It's not the case that you _must_ have this data wrapped in a file format just because normally it would be - for debugging the raw dump may be sufficient, you aren't necessarily only interested in playing it in a music app, for debugging you are likely to be more interested in the actual bytes and in fact it could be preferable to have it raw and not modified into a container file. > > We haven't actually used the WAV header stuff since the very > > early versions of our firmware that didn't use compression on the > > stream and actually did output PCM data and I don't think anyone > > else has ever used the compressed interface for PCM. > > Thats fine by me. The only reason we have a WAV header is that we can pack > PCM files, for rest of the formats it become tricky as Pierre mention so lets > not go that road :) > I'm confused now about what you want. If we don't want to allow raw dumps in crecord we need another tool - say cdump - that can dump raw, but obviously it would be 99% identical to crecord except that it's blessed with the ability to dump raw, and I don't see the point of having two near-identical tools. The alternative is that Cirrus maintains its own branched version of tinycompress with useful tools but that doesn't seem like a sensible road to go down either.
On Wed, Nov 23, 2016 at 10:21:58AM +0000, Richard Fitzgerald wrote: > On Wed, 2016-11-23 at 09:11 +0530, Vinod Koul wrote: > > On Fri, Nov 18, 2016 at 04:17:37PM +0000, Charles Keepax wrote: > > > On Fri, Nov 18, 2016 at 08:39:38AM -0600, Pierre-Louis Bossart wrote: > > > > > > > > >>If you're debugging you may not care about the file format, you're > > > > >>actually interested in the raw data you get from the codec so dumping > > > > >>the output to a raw file would be useful even if you can't load that > > > > >>file into a music player. > > > > > > > > > >While I agree with you on this, am worried that adding codecs may make > > > > >people think that we can record mp3 file for exmaple, which is not the case. > > > > > > > > > >I think we can add pcm and bespoke as formats supported and allow any format to > > > > >be dumped to stdio. That way it is pretty clear to people ... > > > > > > > > Crec as is it is pretty useless with PCM only...If we added the profile > > > > selection and things like bitrate information it'd be straightforward to > > > > support elementary bitstreams like MP3 or AAC ADTS, you just dump the data > > > > to a file. Things that require a header or MP4 integration would require > > > > additional libraries, this is no longer 'tiny'. > > > > > > What about this as a suggestion, if we remove the code that adds > > > the WAV header. Then all the output from crecord is raw data and > > > the addition of any headers or additional wrapping is up to the > > > user. That keeps crecord 'tiny' and allows us to support all the > > > formats in a consistent way so no one gets confused. > > > > Naah, crecord is a utility. If you need to do above you have tinycompress > > APIs to get the data and pack it the way you like.. You don't and shouldn't > > use crecord for that. > > But you can't call APIs from a shell command line, it needs a tool. For > debugging and testing we need a utility that can extract the raw data > from a codec. It's not the case that you _must_ have this data wrapped > in a file format just because normally it would be - for debugging the > raw dump may be sufficient, you aren't necessarily only interested in > playing it in a music app, for debugging you are likely to be more > interested in the actual bytes and in fact it could be preferable to > have it raw and not modified into a container file. > > > > We haven't actually used the WAV header stuff since the very > > > early versions of our firmware that didn't use compression on the > > > stream and actually did output PCM data and I don't think anyone > > > else has ever used the compressed interface for PCM. > > > > Thats fine by me. The only reason we have a WAV header is that we can pack > > PCM files, for rest of the formats it become tricky as Pierre mention so lets > > not go that road :) > > > > I'm confused now about what you want. > If we don't want to allow raw dumps in crecord we need another tool - > say cdump - that can dump raw, but obviously it would be 99% identical > to crecord except that it's blessed with the ability to dump raw, and I > don't see the point of having two near-identical tools. Oops, sorry to confuse you, reading back I dont think I was clear enough.. I am not Ok to add support for any other format apart from bespoke (file dump) and PCM wav header and save to file. I am okay to add support to dump data to stdio for any format, so that we dont bother to support those file formats. Is this clear enough, does that work for you folks? > > The alternative is that Cirrus maintains its own branched version of > tinycompress with useful tools but that doesn't seem like a sensible > road to go down either. Agreed
> I am not Ok to add support for any other format apart from bespoke (file > dump) and PCM wav header and save to file. > > I am okay to add support to dump data to stdio for any format, so that we > dont bother to support those file formats. it's not clear to me why you would make an exception for bespoke, just dump everything to stdio and use a file indirection if you want the raw data in all cases? > > Is this clear enough, does that work for you folks? > >> >> The alternative is that Cirrus maintains its own branched version of >> tinycompress with useful tools but that doesn't seem like a sensible >> road to go down either. > > Agreed >
On Sun, 2016-11-27 at 11:52 -0600, Pierre-Louis Bossart wrote: > > I am not Ok to add support for any other format apart from bespoke (file > > dump) and PCM wav header and save to file. > > > > I am okay to add support to dump data to stdio for any format, so that we > > dont bother to support those file formats. > > it's not clear to me why you would make an exception for bespoke, just > dump everything to stdio and use a file indirection if you want the raw > data in all cases? > Yes, I think that's best. For one thing it's easier to explain in the -h help: PCM can go to wav file, everything can go to stdout, stdout is always raw. > > > > Is this clear enough, does that work for you folks? > > > >> > >> The alternative is that Cirrus maintains its own branched version of > >> tinycompress with useful tools but that doesn't seem like a sensible > >> road to go down either. > > > > Agreed > >
On Mon, Nov 28, 2016 at 09:47:19AM +0000, Richard Fitzgerald wrote: > On Sun, 2016-11-27 at 11:52 -0600, Pierre-Louis Bossart wrote: > > > I am not Ok to add support for any other format apart from bespoke (file > > > dump) and PCM wav header and save to file. > > > > > > I am okay to add support to dump data to stdio for any format, so that we > > > dont bother to support those file formats. > > > > it's not clear to me why you would make an exception for bespoke, just > > dump everything to stdio and use a file indirection if you want the raw > > data in all cases? > > > > Yes, I think that's best. For one thing it's easier to explain in the -h > help: PCM can go to wav file, everything can go to stdout, stdout is > always raw. Yeah lets do that :)
diff --git a/src/utils/crec.c b/src/utils/crec.c index 8d5b7b0..a586fc4 100644 --- a/src/utils/crec.c +++ b/src/utils/crec.c @@ -83,6 +83,27 @@ static bool streamed; static const unsigned int DEFAULT_CHANNELS = 1; static const unsigned int DEFAULT_RATE = 44100; static const unsigned int DEFAULT_FORMAT = SNDRV_PCM_FORMAT_S16_LE; +static const unsigned int DEFAULT_CODEC_ID = SND_AUDIOCODEC_PCM; + +static const struct { + const char *name; + unsigned int id; +} codec_ids[] = { + { "PCM", SND_AUDIOCODEC_PCM }, + { "MP3", SND_AUDIOCODEC_MP3 }, + { "AMR", SND_AUDIOCODEC_AMR }, + { "AMRWB", SND_AUDIOCODEC_AMRWB }, + { "ARMWBPLUS", SND_AUDIOCODEC_AMRWBPLUS }, + { "AAC", SND_AUDIOCODEC_AAC }, + { "WMA", SND_AUDIOCODEC_WMA }, + { "REAL", SND_AUDIOCODEC_REAL }, + { "VORBIS", SND_AUDIOCODEC_VORBIS }, + { "FLAC", SND_AUDIOCODEC_FLAC }, + { "IEC61937", SND_AUDIOCODEC_IEC61937 }, + { "G723_1", SND_AUDIOCODEC_G723_1 }, + { "G729", SND_AUDIOCODEC_G729 }, +}; +#define CREC_NUM_CODEC_IDS (sizeof(codec_ids) / sizeof(codec_ids[0])) struct riff_chunk { char desc[4]; @@ -153,6 +174,8 @@ static void size_wave_header(struct wave_header *header, uint32_t size) static void usage(void) { + int i; + fprintf(stderr, "usage: crec [OPTIONS] [filename]\n" "-c\tcard number\n" "-d\tdevice node\n" @@ -163,14 +186,22 @@ static void usage(void) "-h\tPrints this help list\n\n" "-C\tSpecify the number of channels (default %u)\n" "-R\tSpecify the sample rate (default %u)\n" - "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n\n" + "-F\tSpecify the format: S16_LE, S32_LE (default S16_LE)\n" + "-I\tSpecify codec ID (default PCM)\n\n" "If filename is not given the output is\n" "written to stdout\n\n" "Example:\n" "\tcrec -c 1 -d 2 test.wav\n" - "\tcrec -f 5 test.wav\n", + "\tcrec -f 5 test.wav\n\n" + "Valid codec IDs:\n", DEFAULT_CHANNELS, DEFAULT_RATE); + for (i = 0; i < CREC_NUM_CODEC_IDS; ++i) + fprintf(stderr, "%s%c", codec_ids[i].name, + (i % 8) ? ' ' : '\n'); + + fprintf(stderr, "\nor the value in decimal or hex\n"); + exit(EXIT_FAILURE); } @@ -239,7 +270,8 @@ static int finish_record(void) static void capture_samples(char *name, unsigned int card, unsigned int device, unsigned long buffer_size, unsigned int frag, unsigned int length, unsigned int rate, - unsigned int channels, unsigned int format) + unsigned int channels, unsigned int format, + unsigned int codec_id) { struct compr_config config; struct snd_codec codec; @@ -288,7 +320,7 @@ static void capture_samples(char *name, unsigned int card, unsigned int device, memset(&codec, 0, sizeof(codec)); memset(&config, 0, sizeof(config)); - codec.id = SND_AUDIOCODEC_PCM; + codec.id = codec_id; codec.ch_in = channels; codec.ch_out = channels; codec.sample_rate = rate; @@ -408,10 +440,11 @@ int main(int argc, char **argv) { char *file; unsigned long buffer_size = 0; - int c; + int c, i; unsigned int card = 0, device = 0, frag = 0, length = 0; unsigned int rate = DEFAULT_RATE, channels = DEFAULT_CHANNELS; unsigned int format = DEFAULT_FORMAT; + unsigned int codec_id = DEFAULT_CODEC_ID; if (signal(SIGINT, sig_handler) == SIG_ERR) { fprintf(stderr, "Error registering signal handler\n"); @@ -422,7 +455,7 @@ int main(int argc, char **argv) usage(); verbose = 0; - while ((c = getopt(argc, argv, "hvl:R:C:F:b:f:c:d:")) != -1) { + while ((c = getopt(argc, argv, "hvl:R:C:F:I:b:f:c:d:")) != -1) { switch (c) { case 'h': usage(); @@ -462,6 +495,25 @@ int main(int argc, char **argv) usage(); } break; + case 'I': + if (optarg[0] == '0') { + codec_id = strtol(optarg, NULL, 0); + } else { + for (i = 0; i < CREC_NUM_CODEC_IDS; ++i) { + if (strcmp(optarg, + codec_ids[i].name) == 0) { + codec_id = codec_ids[i].id; + break; + } + } + + if (i == CREC_NUM_CODEC_IDS) { + fprintf(stderr, "Unrecognised ID: %s\n", + optarg); + usage(); + } + } + break; default: exit(EXIT_FAILURE); } @@ -477,7 +529,7 @@ int main(int argc, char **argv) } capture_samples(file, card, device, buffer_size, frag, length, - rate, channels, format); + rate, channels, format, codec_id); fprintf(finfo, "Finish capturing... Close Normally\n");
This patch adds a -I command line option to set the codec ID, either from a defined set of string values or as a number. Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> --- src/utils/crec.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 7 deletions(-)