[Ffmpeg-devel] [PATCH] MS-GSM support: draft for review
Michael Niedermayer
michaelni
Wed Nov 8 18:20:00 CET 2006
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?
> +
> +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
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;
}
[...]
> @@ -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
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
[...]
> 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)
[...]
> @@ -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
> } 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list