[FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements

[FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements

[Michael Niedermayer]

This set contains 2 additions to the patch review and commit policy/guidlines


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Michael Niedermayer]

Suggested text is from Anton

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 doc/developer.texi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/developer.texi b/doc/developer.texi
index 0c2f2cd7d1..383120daaa 100644
--- a/doc/developer.texi
+++ b/doc/developer.texi
@@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are waiting for your patch
 to be reviewed, please consider helping to review other patches, that is a great
 way to get everyone's patches reviewed sooner.
 
+Reviews must be constructive and when rejecting a patch the reviewer must explain
+their reasons and ideally suggest an alternative approach.
+
 @anchor{Regression tests}
 @chapter Regression tests
 
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Michael Niedermayer]

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 doc/developer.texi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/doc/developer.texi b/doc/developer.texi
index 383120daaa..1c0091fc74 100644
--- a/doc/developer.texi
+++ b/doc/developer.texi
@@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
 Reviews must be constructive and when rejecting a patch the reviewer must explain
 their reasons and ideally suggest an alternative approach.
 
+If a change is pushed without being sent to ffmpeg-devel, the developer
+pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly.
+@example
+forgot a semicolon in this patch, pushed a seperate fix
+pushed my new autograd engine and stable diffusion filter. Didnt want to
+go through the bikeshed if that belongs in FFmpeg, go to the GA if you want
+it removed. Otherwise Just tell me what i should improve and ill look into it.
+@end example
+
 @anchor{Regression tests}
 @chapter Regression tests
 
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Andreas Rheinhardt]

Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  doc/developer.texi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 383120daaa..1c0091fc74 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
>  Reviews must be constructive and when rejecting a patch the reviewer must explain
>  their reasons and ideally suggest an alternative approach.
>  
> +If a change is pushed without being sent to ffmpeg-devel, the developer
> +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly.
> +@example
> +forgot a semicolon in this patch, pushed a seperate fix
> +pushed my new autograd engine and stable diffusion filter. Didnt want to
> +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want
> +it removed. Otherwise Just tell me what i should improve and ill look into it.
> +@end example

This encourages pushing patches (even completely new filters) without
sending them to the ML.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[James Almer]

On 8/24/2023 4:56 PM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   doc/developer.texi | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 383120daaa..1c0091fc74 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
>   Reviews must be constructive and when rejecting a patch the reviewer must explain
>   their reasons and ideally suggest an alternative approach.
>   
> +If a change is pushed without being sent to ffmpeg-devel, the developer
> +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly.

immediately.

This chunk is ok, but...

> +@example
> +forgot a semicolon in this patch, pushed a seperate fix
> +pushed my new autograd engine and stable diffusion filter. Didnt want to
> +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want
> +it removed. Otherwise Just tell me what i should improve and ill look into it.
> +@end example

...this one isn't. It's very passive aggressive and not informative at all.
This instead should state that patches pushed without passing through 
the ML have to either be trivial, to push fixes to mistakes in recently 
pushed changes that did go through the ML, or for code you maintain.

> +
>   @anchor{Regression tests}
>   @chapter Regression tests
>   
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Andreas Rheinhardt]

James Almer:
> On 8/24/2023 4:56 PM, Michael Niedermayer wrote:
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>   doc/developer.texi | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/doc/developer.texi b/doc/developer.texi
>> index 383120daaa..1c0091fc74 100644
>> --- a/doc/developer.texi
>> +++ b/doc/developer.texi
>> @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
>>   Reviews must be constructive and when rejecting a patch the reviewer
>> must explain
>>   their reasons and ideally suggest an alternative approach.
>>   +If a change is pushed without being sent to ffmpeg-devel, the
>> developer
>> +pushing it must annouce doing so on the ffmpeg-devel mailing list
>> immedeatly.
> 
> immediately.
> 
> This chunk is ok, but...
> 
>> +@example
>> +forgot a semicolon in this patch, pushed a seperate fix
>> +pushed my new autograd engine and stable diffusion filter. Didnt want to
>> +go through the bikeshed if that belongs in FFmpeg, go to the GA if
>> you want
>> +it removed. Otherwise Just tell me what i should improve and ill look
>> into it.
>> +@end example
> 
> ...this one isn't. It's very passive aggressive and not informative at all.
> This instead should state that patches pushed without passing through
> the ML have to either be trivial, to push fixes to mistakes in recently
> pushed changes that did go through the ML, or for code you maintain.
> 

The maintainer exception has actually been removed in
6a3e174ad1921ba6aec473a2224c71610de3329b.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Vittorio Giovara]

On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Suggested text is from Anton
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  doc/developer.texi | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 0c2f2cd7d1..383120daaa 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are
> waiting for your patch
>  to be reviewed, please consider helping to review other patches, that is
> a great
>  way to get everyone's patches reviewed sooner.
>
> +Reviews must be constructive and when rejecting a patch the reviewer must
> explain
> +their reasons and ideally suggest an alternative approach.
>

NAK
we shouldn't put extra burden on reviewers, nor guilt trap them into
suggesting an alternative approach
offlist and irc discussion is of course recommended, but writing this rules
in stone will only deter good reviews, in my opinion
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Nicolas George]

Vittorio Giovara (12023-08-25):
> NAK
> we shouldn't put extra burden on reviewers, nor guilt trap them into
> suggesting an alternative approach

It is hilarious, in a very sad way, that you prefer put extra burden on
people who do things than on people who block them.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Paul B Mahol]

On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote:

> Vittorio Giovara (12023-08-25):
> > NAK
> > we shouldn't put extra burden on reviewers, nor guilt trap them into
> > suggesting an alternative approach
>
> It is hilarious, in a very sad way, that you prefer put extra burden on
> people who do things than on people who block them.
>

It is easier to post dubious solutions instead of doing proper research
or/and review.


>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Anton Khirnov]

Quoting Vittorio Giovara (2023-08-25 03:56:44)
> On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Suggested text is from Anton
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  doc/developer.texi | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index 0c2f2cd7d1..383120daaa 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are
> > waiting for your patch
> >  to be reviewed, please consider helping to review other patches, that is
> > a great
> >  way to get everyone's patches reviewed sooner.
> >
> > +Reviews must be constructive and when rejecting a patch the reviewer must
> > explain
> > +their reasons and ideally suggest an alternative approach.
> >
> 
> NAK
> we shouldn't put extra burden on reviewers, nor guilt trap them into
> suggesting an alternative approach

I don't understand this argument at all.

First, "ideally suggest an alternative approach" is an aspiration, not a
hard requirement.

Second, I don't think reviewers should be able to reject patches with no
explanation. The author/submitter spent time and effort on writing
and submitting the patch - it is only fair that if it's to be rejected,
it should be done for a clear reason.

> offlist and irc discussion is of course recommended,

I absolutely do not recommend offlist discussion, as it is not visible
to other developers or preserved in the archives.

> but writing this rules in stone will only deter good reviews, in my
> opinion

Non-constructive reviews without an explanation are never good reviews.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Rémi Denis-Courmont]

Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit :
> Suggested text is from Anton
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  doc/developer.texi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 0c2f2cd7d1..383120daaa 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are
> waiting for your patch to be reviewed, please consider helping to review
> other patches, that is a great way to get everyone's patches reviewed
> sooner.
> 
> +Reviews must be constructive

Well, frankly, no. You're really confusing code reviews with teaching here. A 
code review is first and foremost meant to find problems and avoid adding bugs 
or bad designs into the code base. It is not meant to solve problems. Of 
course, it is preferable for a review to be constructive, but it is not always 
possible or reasonable.

Sometimes code just does not belong in.

Sometimes the reviewer can prove or make strong arguments that a patch is 
broken or bad, without having constructive feedback to give. This is just like 
mathematical proofs. Some are constructive, some aren't.

And then sometimes an argument has been argued to death previously and there 
is really no point to rehash it again and again. If people cannot agree, they 
should refer to the TC, not brute force the review through overwhelming 
insistance.

Also what Vittorio already pointed out. Ultimately, this is also a question of 
what to optimise for. And in my 20+ years experience with software development 
and open-source, I think it's abundantly clear that available skilled 
reviewers are an ever scarcer resource than skilled available developers, so 
you should not optimise for the later.

So -1 as far as I am concerned.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Anton Khirnov]

Quoting Rémi Denis-Courmont (2023-08-25 16:22:45)
> Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit :
> > Suggested text is from Anton
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  doc/developer.texi | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index 0c2f2cd7d1..383120daaa 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are
> > waiting for your patch to be reviewed, please consider helping to review
> > other patches, that is a great way to get everyone's patches reviewed
> > sooner.
> > 
> > +Reviews must be constructive
> 
> Well, frankly, no. You're really confusing code reviews with teaching here. A 
> code review is first and foremost meant to find problems and avoid adding bugs 
> or bad designs into the code base. It is not meant to solve problems. Of 
> course, it is preferable for a review to be constructive, but it is not always 
> possible or reasonable.
> 
> Sometimes code just does not belong in.
> 
> Sometimes the reviewer can prove or make strong arguments that a patch is 
> broken or bad, without having constructive feedback to give. This is just like 
> mathematical proofs. Some are constructive, some aren't.
> 
> And then sometimes an argument has been argued to death previously and there 
> is really no point to rehash it again and again. If people cannot agree, they 
> should refer to the TC, not brute force the review through overwhelming 
> insistance.

I think we just have different interpretations of the word
'constructive' here. I certainly agree that some patches are just not
acceptable - I certainly did not mean to imply that there must be a way
forward for all patches. The intent is rather that every review (other
than OK) should be based on technical arguments and not be a semantic
equivalent of 'no'. In case you did not notice, we have a persistent
problem with some people who are sending "reviews" of exactly this type.
I don't think that should be acceptable.

Would you be happier with some reformulation of the text that makes this
more clear?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Rémi Denis-Courmont]

Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
> > And then sometimes an argument has been argued to death previously and
> > there is really no point to rehash it again and again. If people cannot
> > agree, they should refer to the TC, not brute force the review through
> > overwhelming insistance.
> 
> I think we just have different interpretations of the word
> 'constructive' here.
> I certainly agree that some patches are just not acceptable - I certainly
> did not mean to imply that there must be a way forward for all patches.

I think that you do not agree with the generally accepted meaning of 
"constructive" in this context. By definition a review cannot be constructive, 
as in helpful or conducive of a way forward, if it argues that there are no 
ways forward.

Maybe you meant "supported" or "corroborated".

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Anton Khirnov]

Quoting Rémi Denis-Courmont (2023-08-25 17:09:55)
> Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
> > > And then sometimes an argument has been argued to death previously and
> > > there is really no point to rehash it again and again. If people cannot
> > > agree, they should refer to the TC, not brute force the review through
> > > overwhelming insistance.
> > 
> > I think we just have different interpretations of the word
> > 'constructive' here.
> > I certainly agree that some patches are just not acceptable - I certainly
> > did not mean to imply that there must be a way forward for all patches.
> 
> I think that you do not agree with the generally accepted meaning of 
> "constructive" in this context. By definition a review cannot be constructive, 
> as in helpful or conducive of a way forward, if it argues that there are no 
> ways forward.

Explaining why a patch is not acceptable is helpful IMO.
Saying 'no', on the other hand, is not.

> Maybe you meant "supported" or "corroborated".

Might as well describe it in more than one word, since apparently it's
so unclear. Would you be in favor of something along the lines of

  Nontrivial (i.e. other than cosmetics or accepting the patch) reviews
  must be based on technical arguments. If the reviewer fails to provide
  arguments for rejecting the patch or requesting changes, then the
  review may be disregarded.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1789 bytes --]

On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  doc/developer.texi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index 383120daaa..1c0091fc74 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
> >  Reviews must be constructive and when rejecting a patch the reviewer must explain
> >  their reasons and ideally suggest an alternative approach.
> >  
> > +If a change is pushed without being sent to ffmpeg-devel, the developer
> > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly.
> > +@example
> > +forgot a semicolon in this patch, pushed a seperate fix
> > +pushed my new autograd engine and stable diffusion filter. Didnt want to
> > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want
> > +it removed. Otherwise Just tell me what i should improve and ill look into it.
> > +@end example
> 
> This encourages pushing patches (even completely new filters) without
> sending them to the ML.

That was not the intend but if you look at "cvslog" and ffmpeg-devel, you will
notice that there are things being pushed that have not been seen on the
ffmpeg-devel mailing list.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Jean-Baptiste Kempf]

On Fri, 25 Aug 2023, at 17:34, Michael Niedermayer wrote:
> On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  doc/developer.texi | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> > 
>> > diff --git a/doc/developer.texi b/doc/developer.texi
>> > index 383120daaa..1c0091fc74 100644
>> > --- a/doc/developer.texi
>> > +++ b/doc/developer.texi
>> > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
>> >  Reviews must be constructive and when rejecting a patch the reviewer must explain
>> >  their reasons and ideally suggest an alternative approach.
>> >  
>> > +If a change is pushed without being sent to ffmpeg-devel, the developer
>> > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly.
>> > +@example
>> > +forgot a semicolon in this patch, pushed a seperate fix
>> > +pushed my new autograd engine and stable diffusion filter. Didnt want to
>> > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want
>> > +it removed. Otherwise Just tell me what i should improve and ill look into it.
>> > +@end example
>> 
>> This encourages pushing patches (even completely new filters) without
>> sending them to the ML.
>
> That was not the intend but if you look at "cvslog" and ffmpeg-devel, you will
> notice that there are things being pushed that have not been seen on the
> ffmpeg-devel mailing list.

So that means mandatory sending to the mailing list :)

-- 
Jean-Baptiste Kempf -  President
+33 672 704 734
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Paul B Mahol]

On Fri, Aug 25, 2023 at 5:36 PM Jean-Baptiste Kempf <jb@videolan.org> wrote:

>
>
> On Fri, 25 Aug 2023, at 17:34, Michael Niedermayer wrote:
> > On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  doc/developer.texi | 9 +++++++++
> >> >  1 file changed, 9 insertions(+)
> >> >
> >> > diff --git a/doc/developer.texi b/doc/developer.texi
> >> > index 383120daaa..1c0091fc74 100644
> >> > --- a/doc/developer.texi
> >> > +++ b/doc/developer.texi
> >> > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner.
> >> >  Reviews must be constructive and when rejecting a patch the reviewer
> must explain
> >> >  their reasons and ideally suggest an alternative approach.
> >> >
> >> > +If a change is pushed without being sent to ffmpeg-devel, the
> developer
> >> > +pushing it must annouce doing so on the ffmpeg-devel mailing list
> immedeatly.
> >> > +@example
> >> > +forgot a semicolon in this patch, pushed a seperate fix
> >> > +pushed my new autograd engine and stable diffusion filter. Didnt
> want to
> >> > +go through the bikeshed if that belongs in FFmpeg, go to the GA if
> you want
> >> > +it removed. Otherwise Just tell me what i should improve and ill
> look into it.
> >> > +@end example
> >>
> >> This encourages pushing patches (even completely new filters) without
> >> sending them to the ML.
> >
> > That was not the intend but if you look at "cvslog" and ffmpeg-devel,
> you will
> > notice that there are things being pushed that have not been seen on the
> > ffmpeg-devel mailing list.
>
> So that means mandatory sending to the mailing list :)
>
>
So that means I will fork librempeg and stop sending patches/commits here.


> --
> Jean-Baptiste Kempf -  President
> +33 672 704 734
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 152 bytes --]

Jean-Baptiste Kempf (12023-08-25):
> So that means mandatory sending to the mailing list :)

And change the name to libav?

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Jean-Baptiste Kempf]

On Fri, 25 Aug 2023, at 18:27, Nicolas George wrote:
> Jean-Baptiste Kempf (12023-08-25):
>> So that means mandatory sending to the mailing list :)
>
> And change the name to libav?

Trolling is not going to get you anywhere.

-- 
Jean-Baptiste Kempf -  President
+33 672 704 734
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 308 bytes --]

Jean-Baptiste Kempf (12023-08-25):
> >> So that means mandatory sending to the mailing list :)

> Trolling is not going to get you anywhere.

What trolling? You mean the suggestion to enforce a rule that is known
to have caused a fork to wither and die? That was not from me.

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Vittorio Giovara]

On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote:

> Vittorio Giovara (12023-08-25):
> > NAK
> > we shouldn't put extra burden on reviewers, nor guilt trap them into
> > suggesting an alternative approach
>
> It is hilarious, in a very sad way, that you prefer put extra burden on
> people who do things than on people who block them.
>

I am confused, your email is the epitome of not suggesting any alternative
approach, so you're agreeing with it..?
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[James Almer]

On 8/25/2023 2:16 PM, Nicolas George wrote:
> Jean-Baptiste Kempf (12023-08-25):
>>>> So that means mandatory sending to the mailing list :)
> 
>> Trolling is not going to get you anywhere.
> 
> What trolling? You mean the suggestion to enforce a rule that is known
> to have caused a fork to wither and die? That was not from me.

As i said on IRC, there's a difference between "Unless justified, please 
send things to the ML before pushing" and "Everything must have at least 
two reviews from long standing developers".

This is not requiring everything to have been reviewed before being 
pushed, it's asking for non trivial things to hit the ML so people have 
at least a chance at finding problems.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Vittorio Giovara]

On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Rémi Denis-Courmont (2023-08-25 17:09:55)
> > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
> > > > And then sometimes an argument has been argued to death previously
> and
> > > > there is really no point to rehash it again and again. If people
> cannot
> > > > agree, they should refer to the TC, not brute force the review
> through
> > > > overwhelming insistance.
> > >
> > > I think we just have different interpretations of the word
> > > 'constructive' here.
> > > I certainly agree that some patches are just not acceptable - I
> certainly
> > > did not mean to imply that there must be a way forward for all patches.
> >
> > I think that you do not agree with the generally accepted meaning of
> > "constructive" in this context. By definition a review cannot be
> constructive,
> > as in helpful or conducive of a way forward, if it argues that there are
> no
> > ways forward.
>
> Explaining why a patch is not acceptable is helpful IMO.
> Saying 'no', on the other hand, is not.
>

that is true, but saying "no" and preventing some bad code from making it
in the codebase is better than not saying anything


> Maybe you meant "supported" or "corroborated".
>
> Might as well describe it in more than one word, since apparently it's
> so unclear. Would you be in favor of something along the lines of
>
>   Nontrivial (i.e. other than cosmetics or accepting the patch) reviews
>   must be based on technical arguments. If the reviewer fails to provide
>   arguments for rejecting the patch or requesting changes, then the
>   review may be disregarded.
>

I agree with the text suggested, but I don't understand why it needs to be
set in stone in the first place...
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Leo Izen]

On 8/25/23 11:09, Rémi Denis-Courmont wrote:
> Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
>>> And then sometimes an argument has been argued to death previously and
>>> there is really no point to rehash it again and again. If people cannot
>>> agree, they should refer to the TC, not brute force the review through
>>> overwhelming insistance.
>>
>> I think we just have different interpretations of the word
>> 'constructive' here.
>> I certainly agree that some patches are just not acceptable - I certainly
>> did not mean to imply that there must be a way forward for all patches.
> 
> I think that you do not agree with the generally accepted meaning of
> "constructive" in this context. By definition a review cannot be constructive,
> as in helpful or conducive of a way forward, if it argues that there are no
> ways forward.
> 
> Maybe you meant "supported" or "corroborated".
> 

FWIW I read it the same way Anton did but if it's unclear then perhaps 
it could be modified. Essentially, I think what's going on is we don't 
want "NAK" without a reason. If you want to say a patch shouldn't make 
it in, there should at least be a reason. Even if the reason is "this 
API/module has no place in FFmpeg."

- Leo Izen (Traneptora)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Anton Khirnov]

Quoting Vittorio Giovara (2023-08-25 19:26:21)
> On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Rémi Denis-Courmont (2023-08-25 17:09:55)
> > > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
> > > > > And then sometimes an argument has been argued to death previously
> > and
> > > > > there is really no point to rehash it again and again. If people
> > cannot
> > > > > agree, they should refer to the TC, not brute force the review
> > through
> > > > > overwhelming insistance.
> > > >
> > > > I think we just have different interpretations of the word
> > > > 'constructive' here.
> > > > I certainly agree that some patches are just not acceptable - I
> > certainly
> > > > did not mean to imply that there must be a way forward for all patches.
> > >
> > > I think that you do not agree with the generally accepted meaning of
> > > "constructive" in this context. By definition a review cannot be
> > constructive,
> > > as in helpful or conducive of a way forward, if it argues that there are
> > no
> > > ways forward.
> >
> > Explaining why a patch is not acceptable is helpful IMO.
> > Saying 'no', on the other hand, is not.
> >
> 
> that is true, but saying "no" and preventing some bad code from making it
> in the codebase is better than not saying anything

If the code is so bad that it should not go in then surely someone can
find it in themselves to write two sentences about the reason why it is
so bad. Nobody is saying you have to produce a 10-page manifesto.

> > Maybe you meant "supported" or "corroborated".
> >
> > Might as well describe it in more than one word, since apparently it's
> > so unclear. Would you be in favor of something along the lines of
> >
> >   Nontrivial (i.e. other than cosmetics or accepting the patch) reviews
> >   must be based on technical arguments. If the reviewer fails to provide
> >   arguments for rejecting the patch or requesting changes, then the
> >   review may be disregarded.
> >
> 
> I agree with the text suggested, but I don't understand why it needs to be
> set in stone in the first place...

There is a persistent problem with certain people rejecting patches for
no clear reason, and then refusing to elaborate.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 819 bytes --]

Leo Izen (12023-08-25):
> FWIW I read it the same way Anton did but if it's unclear then perhaps it
> could be modified. Essentially, I think what's going on is we don't want
> "NAK" without a reason. If you want to say a patch shouldn't make it in,
> there should at least be a reason.

I agree on this too.

> Even if the reason is "this API/module has no place in FFmpeg."

But not on this example: what has place in FFmpeg or not is anybody's
arbitrary opinion, saying “no place in FFmpeg” alone is just a fancy way
of saying “NAK” with no reason. It must be substantiated too, for
example “the same feature is already possible [like that]”.

And if the same feature is *not* already possible, then it surely means
the code *does* belong in FFmpeg.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 722 bytes --]

James Almer (12023-08-25):
> As i said on IRC,

A medium that can only be accessed by a small fraction of the
contributors due to timeliness constraints.

>		    there's a difference between "Unless justified, please
> send things to the ML before pushing" and "Everything must have at least two
> reviews from long standing developers".
> 
> This is not requiring everything to have been reviewed before being pushed,
> it's asking for non trivial things to hit the ML so people have at least a
> chance at finding problems.

Faire enough. Just let us make it very clear it is the former and not
the latter. The I-don't-know-the-code-“LGTM” mandatory review were a
disgrace.

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML

[Ronald S. Bultje]

Hi,

On Fri, Aug 25, 2023 at 1:42 PM Nicolas George <george@nsup.org> wrote:

> James Almer (12023-08-25):
> > As i said on IRC,
>
> A medium that can only be accessed by a small fraction of the
> contributors due to timeliness constraints.
>

I suspect you misunderstand IRC. With matrix, a bouncer or a 24/7 client,
most of us are online with 24/7 history. Chat logs even provide google
searchability.

I prefer IRC over mailinglist for most technical discussions.

Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".