[FFmpeg-devel] DVCPRO HD: request for review

[Roman V. Shaposhnik]

On Fri, 2008-08-29 at 19:50 +0200, Michael Niedermayer wrote:
> On Fri, Aug 29, 2008 at 09:54:25AM -0700, Roman V. Shaposhnik wrote:
> > On Fri, 2008-08-29 at 05:52 +0200, Michael Niedermayer wrote:
> > > > I'll commit everything today and this piece
> > > 
> > > Iam not sure if this and your commit of 170k of unapproved code is a joke.
> > > If it is, i do not like you humor.
> > 
> > No it isn't. It is an honest attempt to continue as a DV maintainer.
> > 
> > > Please revert it and commit the parts that have been approved cleanly
> > > in several seperate commits.
> > 
> > No. And if treating maintainers in this manner is your kind of humor
> > I don't think I appreciate it either. Moreover, this is not the first
> > time when you ridicule me after I honestly tried to work all the issues
> > with you and had an impression that they were all resolved.
> 
> They clearly where not resolved, 

Please point to a single issue (except the table) which wasn't resolved.

> > I do have
> > commitments as far as DVCPRO HD is concerned 
> 
> I could have guessed that much from the daily private mails from you about
> reviewing your patch soon and quickly.

Don't pretend you always know better. The commitments I have have more
to do with me feeling a responsibility as a DV maintainer, than with
anything you've imagined. If you think that NON having DVPRO HD in
FFmpeg is better for the project, or that you can implement it yourself
or whatever -- I'd be happy to stop being part of this project *right
now* since I'm quite fed up with you personally and with how you've
managed to ruin the social microcosm around here.

> > so I'm willing to tolerate
> > your behavior while I'm trying to integrate decoder and encoder. 
> > But once that happens, I believe I have no place in the project like
> > this.
> 
> That is sad, but i will not tolerate that people start commiting unclean
> code because their payment from some contact might depend on it.

I have 0 contract money at stake here. And trying to imply that I do
is just pathetic. I knew you were a pompous jerk, but I would never
imagined that you would go as low.

Besides, speaking of unlcean code -- lets have an experiment on the
mailing list. The code is now in SVN (not for long, though) so is
there a single developer on this list who has a major issue 
with what has been committed (again, with a sole exception of tables
which I WILL resolve given a chance)? You keep implying a peer review,
so lets have a peer review.

> Where exactly did you do something on roundup? I cannot find a single mail
> from you on the corresponding mailing list.

I started working on two issues if you must know. Yes I'm slow and 
stupid, but if you are so smart and fats why do we still have 200+
bugs logged there?

> Well, so the tables are in svn now, what holds you up with replacing them
> now as you promissed in the thread?

This thread. Michael, tell me, honestly -- is the tables bit the only
thing you are throwing this moody for? What else UGLY do you see there?

> > Now, as for committing individual changes. There's none to be committed
> > (except for isom.c) so that you have a *working* DV codec between the
> > commits. I have always though that people are using changesets for a
> 
> Some examples:
> 
> @@ -59,8 +64,8 @@ typedef struct DVVideoContext {
> 
>  /* MultiThreading - dv_anchor applies to entire DV codec, not just the avcontext */
>  /* one element is needed for each video segment in a DV frame */
> -/* at most there are 2 DIF channels * 12 DIF sequences * 27 video segments (PAL 50Mbps) */
> -#define DV_ANCHOR_SIZE (2*12*27)
> +/* at most there are 4 DIF channels * 12 DIF sequences * 27 video segments (1080i50) */
> +#define DV_ANCHOR_SIZE (4*12*27)

And what would be the point of a decoder allocing twice as much
memory as it actually needs?

>  static void* dv_anchor[DV_ANCHOR_SIZE];
> ------------------
> @@ -51,6 +55,7 @@ typedef struct DVVideoContext {
> 
>      uint8_t dv_zigzag[2][64];
>      uint32_t dv_idct_factor[2][2][22][64];
> +    uint32_t dv100_idct_factor[4][4][16][64];
> 
>      void (*get_pixels)(DCTELEM *block, const uint8_t *pixels, int line_size);
>      void (*fdct[2])(DCTELEM *block);
> @@ -102,6 +107,17 @@ static void dv_build_unquantize_tables(D
>              }
>          }
>      }
> +
> +    for(a = 0; a < 4; a++) {
> +        for(q = 0; q < 16; q++) {
> +            for(i = 1; i < 64; i++) {
> +                s->dv100_idct_factor[0][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_y[i];
> +                s->dv100_idct_factor[1][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_c[i];
> +                s->dv100_idct_factor[2][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_y[i];
> +                s->dv100_idct_factor[3][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_c[i];
> +            }
> +        }
> +    }
>  }

And what would be the point of eating up even more useless memory?
Honestly, what IS the point of having a decoder in a state where 
it does useless (I'll admit not harmful, but useless) things?

> > > Several people including me and vitor try hard to cleanup codecs that had
> > > been added without passing through proper review, we really do not need
> > > more such code.
> > 
> > What code?
> 
> the 170k you commited, i have no interrest in cleaning it up.

I will clean them up myself given a chance. I don't understand why you
don't give it to me and keep at it with your threats.

Thanks,
Roman.