[FFmpeg-devel] DVCPRO HD: request for review

[Roman V. Shaposhnik]

On Fri, 2008-08-29 at 21:37 +0200, Michael Niedermayer wrote:
> > > Please point to a single issue (except the table) which wasn't resolved.
> 
> if you seriously want to know, there are plenty, some examples (from
> what you commited)

Yes I do seriously want to know. And I clearly don't want to contribute
a single email to the "michael is evil" thread. As you might have
noticed I'm returning to FFmpeg after some time of absence, and I would
like to understand the NEW established practices around here. So that 
I don't step on the toes of the others. But also so that they don't step
on mine's. In every case where we might disagree (not over the code, but
over the practices) I would also very much like to see the feedback of
other developers so that I can have a sense of whether its just your
personal opinion or an established practice shared by others. Do
these request seem fair to you?

> it seems you have missed that this contains a cosmetic change (reindent) that
> should be seperate from functional changes

And I missed it deliberately. I can see quite clearly why submitting
patches generated with the moral equivalent of "diff -w" is a very
desirable thing to do. However, as a matter of established *commit*
practice I don't see any value in doing a commit that butchers 
established indentation rules be followed by the commit that restores
them. Please explain.
   
> +      .audio_stride = 90,
> +      .audio_min_samples = { 1580, 1452, 1053 }, /* for 48, 44.1 and 32Khz */
> +      .audio_samples_dist = { 1600, 1602, 1602, 1602, 1602 }, /* per SMPTE-314M */
> +      .audio_shuffle = dv_audio_shuffle525,
> 
> that surely could have been vertically aligned

I guess so, but it doesn't really jump at me as ugly either. It is
also NOT something I would consider to be a deal breaker for 
the SVN commit. Am I the only one thinking that way?

> +        /* We work with 720p frames split in half. The odd half-frame (chan==2,3) is displaced :-( */
> +        if (s->sys->height == 720 && ((s->buf[1]>>2)&0x3) == 0) {
> +               mb_y -= (mb_y>17)?18:-72; /* shifting the Y coordinate down by 72/2 macro blocks */
> +        }
> 
> ((s->buf[1]>>2)&0x3) == 0
> can be simplified to
> (s->buf[1]&0xC) == 0

Yes it can. But you do believe me that even gcc generates an equivalent
code for both cases? Once again, this is not something I would consider
to be a deal breaker for the SVN inclusion. Do you consider it to be
a deal breaker?

> +        for(j = 0; j < 2; j++, y_ptr += y_stride) {
> +            for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))
> +                 if (s->sys->pix_fmt == PIX_FMT_YUV422P && s->sys->width == 720 && i)
> +                     y_ptr -= (1<<log2_blocksize);
> +                 else
> +                     mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);
> +        }
> 
> this can be optimized to
> 
> if (s->sys->pix_fmt == PIX_FMT_YUV422P && s->sys->width == 720)
>     for(j = 0; j < 2; j++, y_ptr += y_stride) {
>         mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);
>         block += 128;
>         mb+=2;
>         y_ptr += (1<<log2_blocksize));
>     }
> else
>     for(j = 0; j < 2; j++, y_ptr += y_stride)
>         for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))
>             mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);

In book this change hurts readability. And here we touch upon an
interesting issue, that is very important to me: if FFmpeg is 
the kind of project that *always* puts even 1% of performance
over readability and maintainability -- please tell me now. It'll
save a lot of aggravation later on.

> besides
> i++, block += 64, mb++, y_ptr += (1<<log2_blocksize)
> is really
>     i++;
>     block += 64;
>     mb++;
>     y_ptr += (1<<log2_blocksize);
> and that is even when its all on one line still rather slow.

How faster you expect your version to be? (and yes, I've just 
benchmarked and I do know the number but I'm curious to see
what's your take).

> But note this is not a complete review, iam sure there is more ...

So, are you really insisting that *every* single nitpick that you
can possibly imagine should be a dealbreaker for the SVN inclusion?
None of the issues you've mentioned above even comes close to
something like your comment on dv100_dct_mode[] vs. mb->dct_mode
which was a very reasonable and helpful technical suggestion which
I took care of without making a single ounce of noise.

Do you really think that if I take libavcodec/mpeg.c and look at
it long enough I won't be able to come up with the same level
of nitpicks you've just had?

> i agree, but there are many bugs that are non trivial to fix, and especially
> if taken litterally i do not think i could fix all bugs in code i maintain.
> Some of these (like 4xm) are reverse engeneered formats where fixing a not
> correctly decoded file can be very hard.

My point was simply that taking even that level of puny help that I can
provide is still better than not taking it. I believe I have reasonably
demonstrated my willingness to tackle all of the reasonable comments 
that you make without any kind of fuzz at all. I also believe that the
price you pay for accepting the help of others is a tolerance for *some*
level of technical nitpicks that can be made otherwise. There's a
difference between spending 1 minute to quietly commit (like you did for
g726.c):

- ((s->buf[1]>>2)&0x3) == 0
+ (s->buf[1]&0xC) == 0

and spending dozens of minutes writing emails trying to prove that
something like that should be a dealbreaker for SVN inclusion. Now,
don't get me wrong may be in your world that IS a dealbreaker. My
only point then, would be that it is very unfortunate choice for
the project. It'll certainly prevent me from contributing, and
having exchange some personal emails with the former members of
the FFmpeg community I know that I'm not alone.

> > Now I honestly really like you both, and I hope we all can settle these
> > problems for the sake of FFmpeg project.
> 
> /me invites roman to a virtual glass of vodka to discuss what we shall
> do now with the 170k in svn.

Accepted. Here's what I can propose:
  1. You enumerate all the issues you want me to deal with.
  2. The issues have to be reasonable and not the kind of nitpicks
     that anybody can find in any code (even yours). For example
     the ((s->buf[1]>>2)&0x3) vs. (s->buf[1]&0xC) is a silly 
     nitpick. dv100_dct_mode vs. mb->dct_mode was a very legitimate
     issue. for(j = 0; j < 2; j++, y_ptr += y_stride) is a tougher
     case but I'm willing to compromise for your benefit here.
  3. If the number of issues identified (and better yet agreed upon
     by at least one extra developer except you e.g. Baptiste) is
     small enough (say 1-5) we leave the code be and I quickly
     deal with them.
  4. If the number is 10+ the code clearly has to be reverted.
  5. If the number is 6-9 -- I don't know what to do.

Thanks,
Roman.