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

[Baptiste Coudurier]

Michel Bardiaux wrote:
> Baptiste Coudurier wrote:
>> Michel Bardiaux wrote:
>>> Baptiste Coudurier wrote:
>>>> Hi,
>>>>
>>>> Michel Bardiaux wrote:
>>>>> Baptiste Coudurier wrote:
>>>>>> Michael Niedermayer wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> On Mon, Feb 19, 2007 at 11:59:14AM +0100, Michel Bardiaux wrote:
>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> On Sat, Feb 17, 2007 at 03:42:52PM +0100, Michel Bardiaux wrote:
>>>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>>>> Hi
>>>>>>>> [snip]
>>>>>>>>>>> is 13000 exactly correct isnt it 13200 for CODEC_ID_GSM?
>>>>>>>>>> Yes and no. The last 4 bits of each frame are not actually used, they
>>>>>>>>>> come on stage only because we (both libgsm and lavc) use a
>>>>>>>>>> byte-oriented
>>>>>>>>>> API, and files made of bytes. They should be considered as container
>>>>>>>>>> overhead in TOAST files, hence not part of the codec bitrate.
>>>>>>>>> but with this argumentation we would have to subtract the padding
>>>>>>>>> bits from
>>>>>>>>> mpeg from the bitrate too and thats something we dont 
>>>>>>>> I think you're talking codec-level padding here, but I wrote of
>>>>>>>> container overhead.
>>>>>>>>
>>>>>>>>> also it would cause
>>>>>>>>> problems for containers which expect the bitrate well to be the
>>>>>>>>> bitrate of
>>>>>>>>> what they get, not to be slightly less due to some padding bits
>>>>>>>>> they dont
>>>>>>>>> know about ...
>>>>>>>> AFAIK currently only some MS containers (AVI, WAV) accept GSM, and
>>>>>>>> then only MS-GSM, which does not have the problem. The only
>>>>>>>> container for non-MS-GSM is TOAST (currently not implemented) and
>>>>>>>> that one knows about the 4 extra bytes.
>>>>>>> well a grep for gsm shows a hit in aiff.c so it seems it is supported
>>>>>>> in a
>>>>>>> non toast format currently and as there is no gsm specific code in it
>>>>>>> i would
>>>>>>> guess it will end with 13200 as bitrate
>>>>>>>
>>>>>>> anyway i wont fight about this appl the patch, i will remove the
>>>>>>> 13000 check
>>>>>>> when it breaks something
>>>>>>>
>>>>>> I was fixing agsm in mov, and this bitrate check cause init to fail for
>>>>>> decoding (encoding shares the function), IMHO we don't care about
>>>>>> bitrate when decoding.
>>>>>>
>>>>>> Can I remove it ?
>>>>>>
>>>>> Oops, forgot I am maintainer for libgsm!
>>>>>
>>>>> Since bitrate is also irrelevant for encoding, what about this?
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>> Index: libavcodec/libgsm.c
>>>>> ===================================================================
>>>>> --- libavcodec/libgsm.c	(revision 12579)
>>>>> +++ libavcodec/libgsm.c	(working copy)
>>>>> @@ -36,8 +36,10 @@
>>>>>  #define GSM_FRAME_SIZE 160
>>>>>  
>>>>>  static av_cold int libgsm_init(AVCodecContext *avctx) {
>>>>> -    if (avctx->channels > 1 || avctx->sample_rate != 8000 || avctx->bit_rate != 13000)
>>>>> +    if (avctx->channels > 1 || avctx->sample_rate != 8000)
>>>>>          return -1;
>>>>> +    if (avctx->bit_rate != 13000)
>>>>> +        av_log(avctx, AV_LOG_WARNING, "Bit rate %d instead of expected 13000 - ignored\n");
>>>>>  
>>>> Well, Im not really ok to annoy user with a false message, when decoding
>>>> this is irrelevant.
>>> It is not false. IMO a file with any bitrate other than 13000 is 
>>> corrupted - well, at least it is not strictly according to specs. Yes, 
>>> this is debatable, but to debate it we need facts, that is a collection 
>>> of files with that problem. Printing the message will inform us about that.
>> In wav/avi maybe, but this is not the case of aiff/mov, which does not
>> supply bitrates, so this will annoy them everytime.
>>
>> Now IMHO, strictly speaking, a wav header with bit_rate field set to 0
>> does not necessarly mean that gsm bitstream is corrupted, so again I
>> think this is irrelevant.
>>
>> You could maybe check that bit_rate is actually != 0 && != 13000
>>
>>>> What is this check good for, since bitrate is irrelevant ?
>>>>
>>> Remove the message on encoding, and very soon we will have someone 
>>> complaining that he used -acodec libgsm -ab 26000 but the output file is 
>>> too small and WMP says it is 13kbits...
>>>
>> Well, Im not against removing it for encoding, I'd like it removed for
>> decoding :>
>>
> Michael has reported 13200 in aiff, do you also see 13200 for mov? Then 
> accepting (OK, silently) 0, 13000 and 13200 when decoding solves your 
> problem while leaving some safety.

Yes.

[...]
-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG SAS                                     http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312