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

[Michel Bardiaux]

Michael Niedermayer wrote:
> Hi
> 
> 
> well, its a svn policy violation it mixes cosmetics with functional changes
> but even without that i see no sense in renaming libgsm_* to gsm_*
> and the rename makes no sense without duplicating the codec which ive
> rejected so the rename becomes sensless
> OTOH if the codec duplication where accpeted the rename would make sense
> (as a seperate patch) ...

So, the logic is that the issue of duplicating vs. parser has to be 
resolved first. Clear.

> 
> 
>>> 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.
> 
> i do blame ms, saving 4bit isnt worth breaking compatibility, also i doubt
> they did this to save 4bits ...

They did *not* break compatibility. The codec had been designed as a 
*bit*stream codec, not *byte*stream. Their solution is just as valid as 
the Berlin one. (Admittedly, this cant be said of MS most of the time...)

> 
> 
>>> ) 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.
> 
> every codec pads to bytes with very few exceptions, 

Which exceptions?

> also there are rules
> on what a AVPacket is, you cant just put 2 packets in it, thats violating
> the API, an AVPacket by definition is 1 packet
> 
> 
>>> 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.
> 
> a patch which violates several rules of our policy is rejected no matter
> what great things it implements
> 
> 
>> 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.
> 
> you dont have to, thats the beauty of free software, just fork

That's not realistic and you know it.

> 
> 
>> 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.
> 
> actually i dont have to, saying no is enough but ive no problem explaining
> my decission
> first 
>   it mixes cosmetics and functional changes (vioates policy, makes review
>   hard, look at the diff, try a search and replace to remove the rename and
>   then again look at the diff, you will see theres a big difference in
>   readablity)

I admit that *if* a rename is after all necessary, it might be better to 
do it later. Though personnally I hate committing to my repository 
sources containing misleading names.

> second 
>   it breaks the rule of 1 packet per AVPacket, all codecs be it mpeg1
>   mpeg2, mpeg4, mp3, h264, h263, ac3, ... get split to packets msgsm if it
>   is supported will be too (and yes there is some old code which doesnt
>   split things into packets but that is wrong and should be fixed)
> third
>   your implementation does not allow -acodec copy between msgsm<->gsm

That's circular reasoning: if the codecs are deemed different enough 
then its normal that copy does not work.

Now, I've completed my dev on basis of my initial patch, including the 
changes to the WAV header, producing a WAV file that works in WinAmp. 
Now that I have that as reference implementation, I'll have a go at the 
parser-based approach, but I'll need some help. ITIYM things like pnm_parse?

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/