[MPlayer-dev-eng] [PATCH]VO_VDPAU, round 5

[Diego Biurrun]

On Thu, Jan 22, 2009 at 09:00:10AM +0000, Carl Eugen Hoyos wrote:
> compn <tempn <at> twmi.rr.com> writes:
> 
> > [15:35] <Compn> carl got those patches accepted to ffmpeg quickly :)
> > [15:36] <Compn> hes waiting for a review of vo_vdpau too 
> > [15:36] <Compn> a fourth review that is
> > [16:04] <uau> IMO he could have done a better job with that
> > [16:04] <uau> what he posted had some rather obvious problems
> > [16:05] <uau> feels somewhat like posting dirty patches and expecting
> > others to do the actual work to make them usable... (not necessarily
> > wrong but i think it could at least be expressed more openly rather
> > than asking "for review")
> 
> I thought I did that, but I agree after re-reading the mails that "more openly"
> would have been possible.

I agree.

> > [16:23] <uau> there were several things that should not have needed
> > pointing out
> > [16:33] <DonDiego> i also pointed out some issues with carl's patches
> > every time he posted them
> > [16:33] <DonDiego> and he never made changes..
> 
> I found one occurrence of a whitespace ending between three and four and one
> additional yesterday: I didn't know how to find them with grep, I tried
> grep ' \n'.

I assumed this was trivial to find.  If it is not, ask, you will receive
answers right away.

> > did you miss some comments that diego made ? or was anything unclear
> > or unsaid?
> 
> The file has several hundred lines, I ran through it to find those issues
> between every iteration.
> Since I except the file to need many non-cosmetic fixes until improvement, I
> definitely did not spend maximum time on it (but I did spend time on those
> cosmetic issues).

Then just say so.

> > do some rules need to be put in patches.txt (e.g. k&r rules
> > or run patch thru ispell) ?
> 
> It seems I missed the ispell part.
> Apart from that, if you find those issues, I still find it makes much more sense
> to point to them (exactly): They are fixed MUCH faster like that (and I do mean
> accumulated time), imo.

IMO some effort should be spent on eliminating those issues in the first
place.  This effort is best spent by the patch submitter.  Carefully
reading the patch/file top to bottom once is not too much to ask.  It
saves reviewers from repeating the same exercise multiple times.  This
saves accumulated time.

I was under the impression that you were not investing this effort and
this got annoying.

> Other than that, to point out indentation problems in an mplayer patch is
> extremely near to pick on somebody giving the current indentation in many files.

I disagree.  New files should sport sane indentation.  And this does not
cause much extra effort.

> Except for one very helpful review, I had the general feeling that the patch is
> simply unwelcome.

I appreciate your efforts around picking up patches and getting them
ready for inclusion.  However, the process you use can be improved in
multiple ways:

- I think I prefer keeping pathces in one thread unless the thread
  becomes very long.  New threads per round are not optimal.
- It is not clear what sorts of issues a patch still has when you submit
  it.
- It is not clear what sorts of changes you make between rounds.
- If you have trouble fixing some issue that gets pointed out, ask.
- Make sure you address as many cosmetic issues as you can before
  sending a patch.  They only distract from more substantive reviews.

Diego