[FFmpeg-devel] Commit messages (was: [PATCH]Allow easy png streamcopying)

[Nicolas George]

Le quartidi 4 floréal, an CCXXIII, Carl Eugen Hoyos a écrit :
> I believe it is much more important to explain why.

Sorry to insist on that, but that is something that is bothering me and this
is as good an occasion as any.

I find that our side of the fork in general (I do not want to point any
name specifically) does not pay as much attention to the commit messages as
we should.

OTOH, I realize that mail has grown much too long. I post it anyway because
I believe it has some interesting points.

To build on your message: "more important to explain why": I believe there
are roughly four sides to a commit message:

What: what part of the code was changed and what the change does.

Why: why is the new version better than the original.

How: how did we achieve the intended result.

When (for lack of a better adverb): at what occasion did you notice that the
change was needed.

In this particular instance, that would be:

What: the APNG muxer to make it accept plain PNG.

Why: because it works and is convenient. Actually, in this case, the why is
not convincing, because, as it happens, dropping the extension is a better
idea. But in other case, "Why" would involve explaining why a pointer can be
NULL. And sometimes, it is obvious, if what is "fix unchecked malloc" for
example.

How: by making a special case for AV_CODEC_ID_PNG. Once again, it is
frequently obvious by reading the code.

When: when trying to stream-copy PNG, because ffmpeg.c automatically chooses
the APNG muxer rather than the PNG muxer. In other cases, it may be a trac
ticket number.

Note that in my formulation, you did not explain Why, you explained When.

Now, there are two places these information must be stated: in the Git
commit message and in the preamble of the mail to the mailing-list. With git
send-email, these are more or less the same; you can add some more info to
the mail.

Note: since the commit message is important, it should be reviewed as part
of the patch. Manual replacements for git send-email that separate the
commit metadata from the code diff should be avoided.

Personally, I consider that the parts should be, roughly, in the order I
have given, with not much preference between How and When.

When reading the mail: What should appear in the mail Subject. Compelling
reason: a maintainer who has ten minutes and fifty unread mails needs to
spot the one that needs reading immediately. Why is a direct follow-up on
What.

In the commit message: we read the commit messages to look for a commit that
introduced a bug, a change of behaviour, etc. We need to know What.

Let us take this particular patch as an example and examine what is most
convenient in a few situations.

A is maintainer of ffmpeg.c, B is maintainer of lavf/apngenc.c. They both
read the index of their inbox and see "Allow easy png streamcopying". A says
"'streamcopying' is a task done by ffmpeg.c" and reads the mail immediately;
B says the same, but adds "PNG is something I know a bit about, I'll read
that later". Both were wrong. With "apng" in the subject, B would have read
the mail immediately. Without "streamcopying", A would not have wasted
immediate time.

C is looking for a regression in AAC stream copy, sees "streamcopying" in
the commit message, looks at the patch. Wasted time.

Here are a few commits that I believe have good commit messages, taken at
random:

commit e4fe535d12f4f30df2dd672e30304af112a5a827
Author: Vittorio Giovara <vittorio.giovara at gmail.com>
Date:   2015-03-17 17:38:48 +0000

[What] mov: Write the display matrix in order
    
[Why] This will allow to copy the matrix as is and it is just cleaner to keep
    the matrix in the same order specified by the mov standard (which is
    also explicitly described in the documentation).
    
[How] In order to preserve compatibility, flip the angle sign in the display API
    av_display_rotation_set() and av_display_rotation_get(), and improve the
    documentation mentioning the rotation direction.

commit e8446a685077b071361cc997116c315c68ef8da3
Author: Michael Niedermayer <michaelni at gmx.at>
Date:   2015-03-16 15:31:57 +0100

[What] configure: Silence warnings about constant unsigned overflows in MSVC
    
[Why] unsigned overflows are well defined in C and used for example in crypto
    and various other places.
    None of the affected warnings currently shown points to an actual defect
    
    untested

commit 7b05b5093ea67a3397b0c37cf398bab471e1ce2b
Author: Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>
Date:   2015-03-13 22:28:42 +0100

[What] ac3dec_fixed: always use the USE_FIXED=1 variant of the AC3DecodeContext
    
[Why] The AC3DecodeContext has a float (USE_FIXED=0) and an integer
    (USE_FIXED=1) variant, both of which can be present in the same binary.
    This is not only very confusing, but it also breaks horribly, when one
    variant is used by code expecting the other.
    
    This currently happens, because eac3dec.c is only compiled for the float
    variant, but also used from ac3dec_fixed.c, which uses the integer
    variant.
    
    The result is memory corruption, leading to crashes.
    
[How] So compile eac3dec.c once for each variant and adapt it, so that it
    works with the integer variant.
    
    A loss of precission and scaling bug has been fixed by the committer
    Signed-off-by: Michael Niedermayer <michaelni at gmx.at>

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150423/a815ec6b/attachment.asc>