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

[Michael Niedermayer]

Hi

On Mon, Nov 06, 2006 at 06:13:37PM +0100, Michel Bardiaux wrote:
> 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.

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) ...


> 
> >
> >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 ...


> 
> >) 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, 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


> 
> 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)
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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is