[FFmpeg-devel] [PATCH] mpeg12enc: Use all Closed Captions side data

[Reimar Döffinger]

On Mon, May 20, 2019 at 10:05:49AM +0200, Hendrik Leppkes wrote:
> On Mon, May 20, 2019 at 9:44 AM Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On 20.05.2019, at 09:23, Mathieu Duponchelle <mathieu at centricular.com> wrote:
> > > On 5/19/19 7:00 PM, Devin Heitmueller wrote:
> > >> Are you suggesting he should make a patch which results in the
> > >> indentation being wrong, and then submit a second patch which fixes
> > >> the incorrect indentation introduced by the first patch?
> >
> > I think it should probably be up to the maintainer, but possibly yes.
> > A lot of review still happens primarily by email, and if you re-indent code
> > it becomes impossible to see what, if anything, you changed in that block.
> > Enabling reviews of the patch via email means not re-indenting even if
> > it becomes "wrong".
> > Not everyone does reviews that way though, so some maintainers nowadays might prefer it differently?
>
> My main concern with such requests is that its added effort to even
> make these patches, since many developers will just automatically
> indent where appropriate (as it should be), or even have tooling to do
> it for them.
> Personally, I would have to use another editor to "un-indent" the
> code, since my main editor automatically corrects this stuff for me in
> newly written code. As such, I'll personally never comply with such
> requests, because I think its silly, and it would be extraordinary
> effort to even do it, first restoring original indent with a second
> editor.

There's many ways to do it, but the reviewability issue is already
solved by e.g. sending the patch as generated by diff -w (have not
tried, but git send-email seems to accept that option), so I
think you are overstating the effort a bit :)
There is also the point that this should not be an issue normally:
- It doesn't matter if it's just a small block of code.
- If it's a large block of code, many times it means the code
should have been refactored long ago and put into a separate function.
So there often is the option of "volunteering" for some code cleanup
first :)

> I don't think its that much to ask for a reviewer to also use a tool
> that can ignore whitespace. Maybe we can teach patchwork to offer a
> button to view diffs without whitespace, if people don't want to use a
> simple tool.

FFmpeg used to have far too few reviewers.
If that is still the case, I think it's reasonable to put reviewer
convenience over submitter convenience.
I don't see how patchwork is really useful: finding the patch
there is already a pain, and we don't use it for reviews AT ALL,
we use email.
Now there might be an argument if we should have alternative/additional
review tools to email, but it's a bit of a separate question.
Personally, my suggestion would be that if you re-indented a lot
and unless you know the most likely potential reviewers do not care,
send a patch without whitespace changes at least in addition.
I mean, I don't have a super-strong opinion, but if I have to
dig through whitespace changes I'll probably just not review a
patch, because far too often all I have at hand to do them is
an email client...

Best regards,
Reimar Döffinger