[Ffmpeg-devel] [PATCH] MS-GSM support: draft for review
Michel Bardiaux
mbardiaux
Thu Nov 9 10:56:45 CET 2006
Michael Niedermayer wrote:
> Hi
>
> On Wed, Nov 08, 2006 at 11:58:55AM +0100, Michel Bardiaux wrote:
>> Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Tue, Nov 07, 2006 at 04:47:52PM +0100, Michel Bardiaux wrote:
>>> [...]
>> [snip]
>>> codec_tag could be used ... but this is getting more messy then i hoped,
>>> i thought we just have to split packets up / merge them :(((
>>>
>>> so after reconsidering all that mess, lets go back to your patch
>>> IMHO the formats are sufficiently different to have individual codec_ids
>>> bitstream filters to convert still would be welcome of course
>>>
>>> could you resubmit your patch without ANY renamings and cosmetics?
>>>
>> I think I've removed all the cosmetics and renamings, and even reordered
>> the code to make the patch more readable (adds rather than changes).
>>
>> I've limited my change to the displaying of the bitrate to the minimum
>> so that ffmpeg produces correct messages for both gsm flavors.
>
> [...]
>
>> Index: libavcodec/libgsm.c
>> ===================================================================
>> --- libavcodec/libgsm.c (revision 6940)
>> +++ libavcodec/libgsm.c (working copy)
>> @@ -29,6 +29,7 @@
>>
>> // gsm.h miss some essential constants
>> #define GSM_BLOCK_SIZE 33
>> +#define MSGSM_BLOCK_SIZE 65
>> #define GSM_FRAME_SIZE 160
>>
>> static int libgsm_init(AVCodecContext *avctx) {
>> @@ -73,6 +74,46 @@
>> libgsm_close,
>> };
>>
>> +static int msgsm_init(AVCodecContext *avctx) {
>> + int one = 1;
>> +
>> + if (avctx->channels > 1 || avctx->sample_rate != 8000)
>> + return -1;
>> +
>> + avctx->frame_size = 2*GSM_FRAME_SIZE;
>> + avctx->block_align = MSGSM_BLOCK_SIZE;
>> +
>> + avctx->priv_data = gsm_create();
>> +
>> + gsm_option(avctx->priv_data, GSM_OPT_WAV49, &one);
>> +
>> + avctx->coded_frame= avcodec_alloc_frame();
>> + avctx->coded_frame->key_frame= 1;
>> +
>> + return 0;
>> +}
>
> why duplicate libgsm_init and not merge it like:
>
> static int libgsm_init(AVCodecContext *avctx) {
> if (avctx->channels > 1 || avctx->sample_rate != 8000)
> return -1;
>
> avctx->priv_data = gsm_create();
>
> if(avctx->codec_id == CODEC_ID_MSGSM){
> int one = 1;
> avctx->frame_size = 2*GSM_FRAME_SIZE;
> avctx->block_align = MSGSM_BLOCK_SIZE;
> gsm_option(avctx->priv_data, GSM_OPT_WAV49, &one);
> }else{
> avctx->frame_size = GSM_FRAME_SIZE;
> avctx->block_align = GSM_BLOCK_SIZE;
> }
>
> avctx->coded_frame= avcodec_alloc_frame();
> avctx->coded_frame->key_frame= 1;
>
> return 0;
> }
>
> its not hackish or anything or?
No, but it would mean having the MS and toast variants under the same
configure option --xable-libgsm. I was planning to make them separate
configure options in a subsequent patch, and maybe rename libgsm (which
unfortunately uses the codec-string "gsm"...) to toastgsm in yet another
patch.
Admittedly, the amount of code space saved by having one and not the
other would not be big, but you seem to work by the principle "There
aint no little profit".
>
>
>> +
>> +static int msgsm_encode_frame(AVCodecContext *avctx,
>> + unsigned char *frame, int buf_size, void *data) {
>> + // we need a full block
>> + if(buf_size < MSGSM_BLOCK_SIZE) return 0;
>> +
>> + gsm_encode(avctx->priv_data,data,frame);
>> + gsm_encode(avctx->priv_data,((short*)data)+GSM_FRAME_SIZE,frame+32);
>> +
>> + return MSGSM_BLOCK_SIZE;
>> +}
>
> same here
Same.
>
> static int libgsm_encode_frame(AVCodecContext *avctx,
> unsigned char *frame, int buf_size, void *data) {
> assert(avctx->block_align == MSGSM_BLOCK_SIZE || avctx->block_align == GSM_BLOCK_SIZE);
> // we need a full block
> if(buf_size < avctx->block_align) return 0;
>
> gsm_encode(avctx->priv_data,data,frame);
> if(avctx->codec_id == CODEC_ID_MSGSM)
> gsm_encode(avctx->priv_data,((short*)data)+GSM_FRAME_SIZE,frame+32);
>
> return avctx->block_align;
> }
>
>
>
>
> [...]
Same
>> @@ -95,3 +136,26 @@
>> libgsm_close,
>> libgsm_decode_frame,
>> };
>> +
>> +static int msgsm_decode_frame(AVCodecContext *avctx,
>> + void *data, int *data_size,
>> + uint8_t *buf, int buf_size) {
>> + if(buf_size < MSGSM_BLOCK_SIZE) return 0;
>> +
>> + if(gsm_decode(avctx->priv_data,buf,data)) return -1;
>> + if(gsm_decode(avctx->priv_data,buf+33,((short*)data)+160)) return -1;
>> +
>> + *data_size = GSM_FRAME_SIZE*4;
>> + return MSGSM_BLOCK_SIZE;
>> +}
>
> and here
Same
>
> static int libgsm_decode_frame(AVCodecContext *avctx,
> void *data, int *data_size,
> uint8_t *buf, int buf_size) {
> assert(avctx->block_align == MSGSM_BLOCK_SIZE || avctx->block_align == GSM_BLOCK_SIZE);
> assert(avctx->frame_size == 2*GSM_FRAME_SIZE || avctx->frame_size == GSM_FRAME_SIZE);
> if(buf_size < avctx->block_align) return 0;
>
> if(gsm_decode(avctx->priv_data,buf,data)) return -1;
> if(avctx->codec_id == CODEC_ID_MSGSM)
> if(gsm_decode(avctx->priv_data,buf+33,((short*)data)+160)) return -1;
>
> *data_size = avctx->frame_size*2;
> return avctx->block_align;
> }
>
>
>
> [...]
>> Index: libavcodec/avcodec.h
>> ===================================================================
>> --- libavcodec/avcodec.h (revision 6940)
>> +++ libavcodec/avcodec.h (working copy)
>> @@ -241,6 +241,8 @@
>>
>> CODEC_ID_MPEG2TS= 0x20000, /* _FAKE_ codec to indicate a raw MPEG2 transport
>> stream (only used by libavformat) */
>> +
>> + CODEC_ID_MSGSM, /* As found in WAV */
>
> as ive alraedy said this belongs to the audio codecs, the spot under
> CODEC_ID_IMC seems the correct one, please put it rather there
I had, and Benjamin Larsson recommended (on -user) to put it at the end
of the enum to preserve binary compatibility.
>
>
> [...]
>> Index: libavformat/riff.c
>> ===================================================================
>> --- libavformat/riff.c (revision 6940)
>> +++ libavformat/riff.c (working copy)
>> @@ -195,6 +195,7 @@
> [...]
>> @@ -305,6 +306,8 @@
>> bps = 24;
>> } else if (enc->codec_id == CODEC_ID_PCM_S32LE) {
>> bps = 32;
>> + } else if (enc->codec_id == CODEC_ID_MSGSM) {
>> + bps = 0; /* That is what dumps show in wav */
>
> ok but could you add this as a || enc->codec_id == CODEC_ID_MSGSM to the
> bps=0 case (= similar to how all other cases are handled)
OK. But to me it was the MPEGAUDIO case, not the bps=0 case.
>
>
> [...]
>> @@ -323,6 +326,8 @@
>> enc->codec_id == CODEC_ID_PCM_S32LE ||
>> enc->codec_id == CODEC_ID_PCM_S16LE) {
>> bytespersec = enc->sample_rate * blkalign;
>> + } else if(enc->codec_id == CODEC_ID_MSGSM) {
>> + bytespersec = 13000 / 8;
>
> didnt you rather want to fix enc->bit_rate? if no then i would rather
> leave this out until someone does
Yes, I would, but neither you nor I have yet come with an architecture
to do that cleanly (thread "Audio bitrate"). That value above for
bytespersec is *mandated* for msgsm in wav. Any other value, neither WMP
nor winamp will play the wav!
>
>
>> } else {
>> bytespersec = enc->bit_rate / 8;
>> }
> [...]
>> Index: libavformat/wav.c
>> ===================================================================
>> --- libavformat/wav.c (revision 6940)
>> +++ libavformat/wav.c (working copy)
> [...]
>> @@ -46,6 +46,12 @@
>> }
>> end_tag(pb, fmt);
>>
>> + if(s->streams[0]->codec->codec_id == CODEC_ID_MSGSM) {
>> + fact = start_tag(pb, "fact");
>> + put_le32(pb, 0);
>> + end_tag(pb, fact);
>> + }
>
> interresting, accoridng to microsofts excelent and unambigous documentation
> (not kidding ...)
> Fact Chunk
> This chunk is required for all WAVE formats other than WAVE_FORMAT_PCM. It stores file
> dependent information about the contents of the WAVE data. It currently specifies the time length of the
> data in samples.
>
> so this must not be under CODEC_ID_MSGSM, also it must be a seperate patch
> as its not CODEC_ID_MSGSM specific
I didnt because I dont have any other reference wav except PCM or MSGSM,
and I dont like to introduce things that I cant test. And the fact chunk
is certainly required for msgsm, so it has to be a *preliminary* patch,
otherwise there will be an svn with an incorrect msgsm.
Other patch, other thread, coming up.
Greetings,
--
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be
Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/
More information about the ffmpeg-devel
mailing list