[Ffmpeg-devel] Re: [Ffmpeg-cvslog] r8465 - trunk/libavformat/utils.c

Uoti Urpala uoti.urpala
Wed Mar 21 16:46:35 CET 2007


On Wed, 2007-03-21 at 14:12 +0100, Baptiste Coudurier wrote:
> Reimar D?ffinger wrote:
> > So in short: there are cases where adding such a thread reference
> > makes sense, but in most cases IMO it is not necessary (no real
> > discussion about the patch) or misplaced (documentation should be in the
> > code, not in an email thread) or the essence of the discussion can be
> > captured in a two-line log message.
> > 
> 
> Well, two lines is often longer than thread title (not mentioning email
> and commit log which are in common).

Longer maybe but a LOT more useful if it contains most of the same
information. Going through mail threads which almost invariably contain
a lot of irrelevant things and using a separate interface for reading
the mails wastes a lot of time compared to having the information in the
log message. I also think length isn't that important even if it takes
more than two lines.

> Many patches are being discussed longer than one reply, thread is very
> useful when you need to understand/remember why it was done that way.
> 
> By experience, I thought many times "thanks Guillaume for mentioning
> thread reference".

But maybe in some of those cases a better log message would have given
most of the information too without needing to go through mail threads?

I think the average commit messages in both FFmpeg and MPlayer aren't
very good and could be improved significantly. Some examples what I
think could be done better from the most recent FFmpeg commits:

8465:
=====
av_estimate_timings_from_pts() flushes the packet queue but doesn't
reset the streams' cur_dts values.  This can lead to a fatal "error,
non monotone timestamps ..." message later, because the out-of-date
cur_dts values are used to compute some packet's dts.

Fix this by calling av_read_frame_flush() and eliminate code
duplication in the process.

The additional hunk gives more detailed error messages.

patch by Wolfram Gloger, wmglo dent.med.uni-muenchen de
====

This seems like a reasonable description of what the commit does, but
there could be a short summary first. Now it's hard to say whether this
might make changes relevant to given other code without reading the
whole text.


8461:
=====
treat frame_size > 1 as compressed audio
=====

This requires at least checking which file(s) the commit changes to make
much sense, and still doesn't tell what kind of practical effect this
change might have. I think it would be better to mention "mov/mp4 muxer"
in the commit message itself and possibly mention which practical case
this improves/fixes.


8452:
=====
i think this is more correct
=====

This basically requires reading the diff and is useless when skimming
through a list of commit messages. At least mention "packet timestamps"
to give some idea what the change does without reading the diff.


Some commit message guidelines which I think make sense:
- Try to describe the change, what practical effects it has, and
possible nontrivial justification for doing things this way (within sane
limits, it's not worth using a lot of extra time to do every commit in a
"perfect" manner).
- For multiline messages include a summary on the first line which
contains enough information to allow people looking for other things to
tell in most cases that this commit does NOT contain anything relevant.
- Don't write the message only to explain your changes to someone who is
already looking at the diff. The message should make sense without
context other than being a log message of this project. People should be
able to check the message and _then_ decide whether they want to see the
diff. Preferably the message should make sense without looking at the
list of files the commit changes either.





More information about the ffmpeg-devel mailing list