[Ffmpeg-devel] [PATCH] MS-GSM support: draft for review

[Aurelien Jacobs]

On Thu, 9 Nov 2006 11:38:08 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> Hi
> 
> On Thu, Nov 09, 2006 at 10:56:45AM +0100, Michel Bardiaux wrote:
> > 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, 
> 
> every codec has its own
> --enable/disable automatically
> --enable-libgsm just enables/disabled if libgsm is linked
> 
> 
> > 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".
> 
> most people will either want both or none, and the way you want to
> implement it duplicates all the functions of the codec, and
> saving a few bytes in a very common case is better then saving a few
> in a rare case
> but you can add #ifdefs around the msgsm specific parts of the functons
> if you want which would combine the advantages of both, though it would
> make the code less readable

And if you want to keep all the advantages (no duplication, possibility
to disable codecs individualy, readability), you have the solution to use
the new ENABLE_* system (as soon as it's commited. Mans ? Had a look
at it ?).
This would look like this:

static int libgsm_init(AVCodecContext *avctx) {
    if (avctx->channels > 1 || avctx->sample_rate != 8000)
        return -1;

    avctx->priv_data = gsm_create();

    if (ENABLE_MSGSM_DECODER && 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 if (ENABLE_LIBGSM_DECODER) {
        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;
}

Aurel