[FFmpeg-devel] [PATCH] GSM-MS decoder and encoder

[Michel Bardiaux]

Michael Niedermayer wrote:
[snip]
> 
> Until we find a file (and not a intentionally created one for this) where
> the demuxer says X but the samplerate is not X but 8000 i think the
> assumtation that it is always X is pretty good.

As long as you are aware that it is an *assumption* based on absence of 
evidence...

> 
> 
>>>> And 2 sample rate fields will just force the user app to duplicate 
>>>> the logic, the situation would be different if we didnt knew what
>>>> was correct but heres its clear, if the demuser says X its X if the
>>>> demuxer doesnt say anything its 8000.
>>>>
>>>> Most sane would be if(!avctx->sample_rate) avctx->sample_rate=
>>>> 8000;
>>>>
>>>> IMO
>>> Heres a proper patch for that:
>>>
>> Rejected. It would accept, without any warning, completely invalid
>> values for encoding.
>>
>> Unless, of course, if it's an emergency, in that case do zap all the guards.
>>
>> But what we need is an agreed-upon policy. You have stated rather 
>> clearly that ffmpeg/ffplay has to be as permissive as possible on 
>> decoding; I accept that, but IMNSHO loud warnings are needed.
>>
> 
>> On encoding, the most sane is to reject non-conforming parameters. If 
> 
> Yes, but its only the samplerate the bitstream is completely conforming

I would not say so...

> and such bending of the spec is clearly wanted by some people otherwise
> such files would not exist.

...I rather think they are due to cluelessness...

> GSM could easily be higher quality per
> bitrate than several other codecs.

... but it is indeed worth trying (as long as the warnings are 
vociferous enough). How does one measure the quality?

> Also we do have AVCodecContext.strict_std_compliance for the encoder
> side.

I had the same idea.

> 
> 
>> codec and muxers disagree, as with MOV wanting to write 13200 in the 
>> header, it will be the muxer's job to do the change. Tit for tat.
> 
> The bitrate for GSM at 8000hz is 13200 other bitrates are incorrect.
> 33byte/block / 160samples/block * 8000samples/sec * 8bit/byte = 13200 bit/sec

This is what started the whole thread, and I have to admit that 
re-reading the source (of libgsm) shows that the 4-bits padding is 
prepended by the codec itself, so 13200 it is.

What confused me is that the padding is mentioned in the man page for 
toast (the CLI for libgsm), not the man page for libgsm itself.

> 
> The bitrate for MS-GSM at 8000hz is 13000 other bitrates are incorrect.
> 65byte/block / 320samples/block * 8000samples/sec * 8bit/byte = 13000 bit/sec
> 
> New patch (which i will apply in 24h) below
> 
> Index: libavcodec/libgsm.c
> ===================================================================
> --- libavcodec/libgsm.c	(revision 13005)
> +++ libavcodec/libgsm.c	(working copy)
> @@ -41,9 +41,18 @@
>                 avctx->channels);
>          return -1;
>      }
> +
> +    if(avctx->codec->decode){
> +        if(!avctx->channels)
> +            avctx->channels= 1;
> +
> +        if(!avctx->sample_rate)
> +            avctx->sample_rate= 8000;
> +    }else{
>      if (avctx->sample_rate != 8000) {
>          av_log(avctx, AV_LOG_ERROR, "Sample rate 8000Hz required for GSM, got %dHz\n",
>                 avctx->sample_rate);
> +        if(avctx->strict_std_compliance > FF_COMPLIANCE_INOFFICIAL)
>          return -1;
>      }
>      if (avctx->bit_rate != 13000 /* Official */ &&
> @@ -51,8 +60,10 @@
>          avctx->bit_rate != 0 /* Unknown; a.o. mov does not set bitrate when decoding */ ) {
>          av_log(avctx, AV_LOG_ERROR, "Bitrate 13000bps required for GSM, got %dbps\n",
>                 avctx->bit_rate);
> +        if(avctx->strict_std_compliance > FF_COMPLIANCE_INOFFICIAL)
>          return -1;
>      }
> +    }
>  
>      avctx->priv_data = gsm_create();
> 

Maybe switch on codecID: 0 and 13200 for GSM, 0 and 13000 for GSM MS? 
But I can do that later. So, OK for me. assuming a patch that results in 
incorrect identation is OK, I suppose you will follow with a correction?)

Greetings,
-- 
Michel Bardiaux
http://www.mediaxim.com/