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

[Michel Bardiaux]

Michael Niedermayer wrote:
> Hi
> 
> On Mon, Nov 06, 2006 at 05:10:22PM +0100, Michel Bardiaux wrote:
>> Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Mon, Nov 06, 2006 at 04:32:53PM +0100, Michel Bardiaux wrote:
>>>> There are some pending issues:
>>>>
>>>> 1. Still no configure control except --xable-libgsm. IMHO this is a 
>>>> separate issue and can be done in a later patch.
>>>>
>>>> 2. Whether the rename gsm to toastgsm is a good idea is very debatable.
>>> iam against the rename
>>>
>>> and the rename makes reviewing the changes below hard (ill wait for a
>>> patch without that before ill review it)
>>>
>>> [...]
>> Ecco.
> 
> [...]
>>  #define GSM_BLOCK_SIZE 33
>> +#define MSGSM_BLOCK_SIZE 65
>>  #define GSM_FRAME_SIZE 160
>>  
>> -static int libgsm_init(AVCodecContext *avctx) {
>> +static int gsm_init(AVCodecContext *avctx) {
> 
> sensless rename, and the whole patch is full of these

Its not senseless. libgsm is now in configure the cover for *both* 
codecs. The one with the name string "gsm" should use methods named 
gsm_something. Its the previous naming scheme that is senseless.

> 
> also upon closer ispection the new "codec" seems to be the same as the old
> except that 2 260bit (note not a multiple of 8

Dont blame MS, blame the designers of the codec.

> ) are packed together into one
> 65 byte packet instead of each padded to a multiple of 8 (=33 byte)
> this is nasty, MS programmers should be shot for this

The alternative, 4 bits of padding per frame, isnt clean either.

> the solution is IMHO a AVParser which splits these packet pairs before the
> gsm decoder

This is not the way I see collaboration to an open-source project. When 
someone has something that works (in a domain where things didnt work or 
were not implemented before), you may reject code that is *grossly* 
wrong or bloated or slow; not because you find the style not one-liney 
enough, or complicated enough, or obfuscated enough.

Then *you* are free to submit something better.

With 30 years of experience under my belt, I would not tolerate that 
attitude from my head of department, and I dont think I have to accept 
it from you.

IOW you have to *convince* me that my solution is *SO* bad it should 
never be committed, or that yours is *SO* much better and easier that I 
should use it.

> 
> for encoding a AVBitstreamFilter could be written which merges 2 packets
> together if encoding is needed
> 
> [...]

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/