[FFmpeg-devel] DVCPRO HD: request for review
Michael Niedermayer
michaelni
Sat Aug 30 00:13:30 CEST 2008
On Fri, Aug 29, 2008 at 01:45:41PM -0700, Roman V. Shaposhnik wrote:
> 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?
sure
tough honestly i do not think practices have changed much ...
>
> > 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.
As you say you see the sense in it for a patch, why would that sense not
apply to svn?
Some people do try to follow svn log and do read the commits, its much
easier to read cleanly split commits than ones mixing functional changes
with reindentions.
Also when debuging some problem it is not rare to check svn and look up
some past commits or to do a binary search to find a offending commit.
All this is strongly affected by the readability of changes.
a odd bug caused by a 170k change is a lot harder to debug with a binary
search in the history than one caused by a 5k change, simply because there
is less code that can have caused the bug.
>
> > + .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?
maybe not but is that a reason not to vertically align it?
The code commited should be the best you can write not just
not bad enough to be "unanimously unacceptable" or some kind of
"whats the least cleanup i have to do to get it approved"
practice clearly is
"what is the best i can do, does anyone know anything i can improve?"
If you agree that vertically aligning this would be better, why dont
you do it?
If OTOH you think the code is clearer when its not vertically aligned
then fine, you are the maintainer, i do not really agree about the style
but so be it.
>
> > + /* 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?
yes
The code is IMO more readable and simpler and gcc cannot improve that
>
> > + 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.
yes, 1% overall speed absolutely is more important than 4 or 5 extra
lines of C code.
and code like
i++, block += 64, mb++, y_ptr += (1<<log2_blocksize)
does not qualify as maintainable or readable in my book
but as long as you are dv maintainer and prefer to put 4 statements
on a line iam fine with it.
[...]
>
> > 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?
no, any nitpick anyone can imagine is a dealbreaker, if diego finds
a missing ',' that has to be fixed before commit. This has been the
practice since as long as i can remember, any comments from anyone
have to be dealt with before commit.
> 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 would be very happy if you did review it and we could clean it up or
improve its speed as a result, even typo fixes surely are welcome.
a 1% speed improvment for mpeg1/2 would be completely fantastic!
[...]
>
> > > 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.
Iam fine with this, except point 2, that is i want ALL issues we agree
on fixed no matter how minor you think they are.
If you disagree about something that is different, but minorness is not
a excuse to not fix it.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080830/6c1acbf8/attachment.pgp>
More information about the ffmpeg-devel
mailing list