[FFmpeg-devel] [PATCH 02/19] mmvideo: count preamble size in return value.

Alexander Strasser eclipse7 at gmx.net
Tue Jul 31 00:07:19 CEST 2012


Reimar Döffinger wrote:
> On Sun, Jul 29, 2012 at 03:57:20PM +0200, Nicolas George wrote:
> > 
> > Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> > ---
> >  libavcodec/mmvideo.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > 
> > MM_PREAMBLE_SIZE is substracted to buf_size almost immediately.
> 
> I suspect you meant "subtracted from"? Only one 's' in subtract.

  Why is that interesting information not in the body of the
commit message?

  To make it clear this is no critique of your patch but rather
a general concern. I know it is a (big) burden for the programmer
to write good commit messages, but the return is hopefully larger
than the hassle. As always it is yet another trade-off to be made...

  IMHO in a commit message only 2 things are really required:
  1) What you change
  2) Why you change it

  Point 1 should be answered directly in the first line (though it
may be repeated and extended in the commit message body).

  Point 2 is most of the times in the body as you usually cannot
meaningfully encode the answer in one line with about 50 characters
while also obeying English grammar.

  Following those guidelines is helpful to both the committer as
well as the commit log readers.

  To come back to this commit specifically and assuming I understood
your patch correctly I would have written something like this:

  mmvideo: decode: Include preamble size in returned count

  MM_PREAMBLE_SIZE is subtracted from buf_size almost immediately. Save
  a copy of the original size and return it later.


  BTW while writing down that commit message a thought came to my mind.
Wouldn't it be better to:

  1) initialize the bytestream right after the (buf_size < MM_PREAMBLE_SIZE) check
  2) use the bytestream to peek at the type
  3) use the bytestream to skip the preamble

  Peter, any comments from you?

  Alexander


More information about the ffmpeg-devel mailing list