[FFmpeg-devel] [RFC] Reviewing merges

[wm4]

On Mon, 24 Apr 2017 16:19:00 -0300
James Almer <jamrial at gmail.com> wrote:

> On 4/24/2017 4:08 PM, wm4 wrote:
> > On Mon, 24 Apr 2017 20:27:45 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> >> On Mon, Apr 24, 2017 at 11:23:16AM -0300, James Almer wrote:  
> >>> On 4/23/2017 11:07 PM, Michael Niedermayer wrote:    
> >>>> Hi all
> >>>>
> >>>> Should changes ported from libav (what we call merges) be reviewed
> >>>> before being pushed?    
> >>>
> >>> The lot of merges are simple things like "Fix this bug that was already
> >>> fixed in ffmpeg months ago", "K&R", etc. Lately we are even no-oping a
> >>> good amount of them as they don't even apply.    
> >>  
> >>> Only a small set of merges are big API changes, and those are always
> >>> handled by more than one developer, either by reviewing the changes,
> >>> testing them or by helping getting the thing working. And of course, any
> >>> bug that was not caught before pushing is fixed afterwards.
> >>> In fact, it should be noted that if they are initially skipped for
> >>> requiring more thought and for blocking unrelated merges, when we get
> >>> them working at a latter point we always send them to the ML instead of
> >>> simply pushing them.    
> >>
> >> yes
> >>
> >> could you send more changes to the ML instead of just ones
> >> initially skiped ? (and of course i dont mean trivial / clearly correct
> >> stuff)
> >>
> >> More eyes may catch more issues also i belive its important that we
> >> all see the changes being done, we all have to maintain the code
> >> which interacts with these changes.  
> > 
> > Sorry, from you I've mostly seen weird command lines failing (and the
> > devs had to find out themselves whether they were legitimate failures).
> > From "eyes catching issues" I'd expect something more concrete, like
> > pointing out errors in the code or fixing them.  
> 
> I agree that pointing out where's the error would be helpful, especially
> if doing so can confirm if it's a legitimate failure or not, but
> detailing a way to reproduce a bug is valid report, especially now that
> he's linking to the failing samples after you complained about it.

A bug is a bug, and bugs should be fixed. So far we agree.

But I think anyone finding a bug and being a developer should at least
do some basic analysis. Apparently that's a weird expectation from me,
not shared by others.

Yes, I appreciate that he posts links to the samples now.

> >   
> >>
> >> [...]
> >>  
> >>> We have recently been able to go through six hundred or so commits in a
> >>> month or two this way after being stuck for the longest time by a few of
> >>> those big API changes. If we start requiring every commit to go through
> >>> a review process on the ML then we will never catch up with the backlog.
> >>> In short, things as they are right now are smooth. Changing it will only
> >>> make this slower.    
> >>
> >> Maybe, but is merging more faster also better for FFmpeg ?  
> > 
> > Oh come on, the merge backlog we had almost meant that we were giving
> > up on merging, causing huge problems to various people (including
> > myself).
> >   
> >> I did not analyze the bugs on our bug tracker but subjectivly the
> >> number of regressions seems much larger than a year or 2 ago.  
> > 
> > Because you're not doing the merges yourself anymore, which means that
> > things you test might result in a regression? When you were doing the
> > merges, you probably did this before you merged, meaning you'd of
> > course not find as many regressions after the merge.
> > 
> > Now that's what I call bias.  
> 
> No, regressions Michael finds are rarely if ever reported to trac. He
> instead pokes the merger on IRC or sends an email to the ml.
> Trac gets mostly regression reports from library or CLI users.
> 
> Don't go accusing so lightly of bias...

I think you misunderstood what I was trying to say.

Michael does his own tests, that tend to find issues others don't
find. (Some have joking called this Michael-fate, because he finds
the weirdest issues as if he had a private FATE variant somewhere.)

When Michael merged, he did his tests before he pushed the merge
commit, and of course he fixed them all before pushing. And of course
he didn't find many new issues after the merge, because he was out of
things to test.

Now other peoples merge, and Michael randomly tests FFmpeg, whether
it's on a temporary merge-tree, or after the merge commit was pushed,
so of course he finds a bunch of issues. And of course he's finding
unfixed issues this way, _especially_ if the merge commit has already
been pushed to git.

So he _might_ have some systematic bias in his perception here. It's a
bias because other mergers could be seeing the opposite. They're likely
to test cases they care about before they push their merges, so they
won't see many regressions for their own use cases, while Michael's
merges might have had more regressions from their perspective because
Michael didn't test the cases they care about.

That doesn't have to be true, but it sounds like it could make sense to
me.

> >   
> >> and i just yesterday found 2 issues in a merge (which you fixed)  
> > 
> > So where's the problem?
> >   
> >>  
> >>>
> >>> That aside, may i ask what prompted this question? Did someone complain
> >>> to you privately? No merge recently seems to have broken anything that
> >>> hasn't been already fixed, beyond one threading bug that's being
> >>> investigated right now, so i wonder why ask this now.    
> >>
> >> It was on my mind since a long time. I guess the recent surge of merges
> >> and the uneasiness of doing the 3.3 release in the middle kind of was
> >> bringing it up into relevance again.  
> > 
> > Well, you insisted on a release at this time.
> >   
> >> Before that with the merges falling behind, multiple libav developers
> >> did sent patches to ffmpeg-devel which passed through review on our
> >> codebase before application. The merges restarting stoped this it
> >> seems.  
> > 
> > Uh, what.  
> 
> I think he's talking about Martin Storsjö sending patchsets like VP9
> arm/aarch64 asm.
> 
> >   
> >> I strongly belive that libav developers sending patches to ffmpeg
> >> and thus the author of a change testing it on top of ffmpeg results
> >> in better code than if its merged by a 3rd party.  
> > 
> > You know what would be even better? If there were only a single project
> > on which Libav and FFmpeg developer worked.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >   
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel