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

Devin Heitmueller dheitmueller at kernellabs.com
Sun May 19 20:00:39 EEST 2019


Hello Paul,

On Fri, May 17, 2019 at 4:44 AM Paul B Mahol <onemda at gmail.com> wrote:
>
> On 5/17/19, Mathieu Duponchelle <mathieu at centricular.com> wrote:
> > There isn't one, as I said the added indentation is because of the new loop!
>
> To get this committed to tree you need to comply to review requests.

I think Mathieu's point is that the code indentation change was not
cosmetic - it's because the code in question is now inside a for loop,
and thus it needed to be indented another level.

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'm confident that Mathieu is trying to comply with review requests so
he can get his code merged.  In this particular case, Carl raised his
concern about the indentation, Mathieu responded by suggesting that
given there was a functional change the re-indent was correct, and
then there was silence (i.e. neither agreement nor disagreement).

I'm also asking because I have work which I'm hoping to get upstreamed
as well at some point, and I'm sure I've got similar things in my
patches.  And having spent 20+ years reviewing patches on lots of
other open source projects, I wouldn't have thought twice about
accepting the patch as-is (given the change in indentation is a result
of a functional change and is not cosmetic).  In my experience, this
particular patch isn't an example of what maintainers mean when they
say "don't mix cosmetic whitespace changes with functional changes".

Regards,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com


More information about the ffmpeg-devel mailing list