[FFmpeg-devel] [Ffmpeg-devel] [PATCH] DV timecode

[Michael Niedermayer]

On Thu, Oct 09, 2008 at 03:20:25PM -0700, Baptiste Coudurier wrote:
> Hi guys,
> 
> I'd like to resurrect this thread, I hope this won't cause flames this
> time, and I'll address all comments.
> 
> Baptiste Coudurier wrote:
> > Hi
> > 
> > 3 patches:
> > 
> > - add timecode.h to avoid code duplication for drop frame timecode
> > adjustment
> 
> I moved code to utils.c in ff_timecode_drop_frame_adjust function, added
> doxygen.

> Index: libavcodec/mpeg12enc.c
> ===================================================================
> --- libavcodec/mpeg12enc.c	(revision 15589)
> +++ libavcodec/mpeg12enc.c	(working copy)
> @@ -32,8 +32,8 @@
>  #include "mpeg12.h"
>  #include "mpeg12data.h"
>  #include "bytestream.h"
> +#include "timecode.h"

>  
> -
>  static const uint8_t inv_non_linear_qscale[13] = {

cosmetic

>      0, 2, 4, 6, 8,
>      9,10,11,12,13,14,15,16,
[...]
> Index: libavcodec/utils.c
> ===================================================================
> --- libavcodec/utils.c	(revision 15589)
> +++ libavcodec/utils.c	(working copy)
> @@ -1518,6 +1518,20 @@
>          return 0;
>  }
>  
> +/*
> + * Adjust frame number for NTSC drop frame time code
> + * @param frame_num Actual frame number to adjust
> + * @return Adjusted frame number
> + */

doxy is missing a '/**'


> +int ff_timecode_drop_frame_adjust(int frame_num)

i think the name "ff_timecode_drop_frame_adjust" will be confusing when
a function doing the inverse is added, that is one that converts the stored
timecode into the frame number.
Maybe a name like ff_framenum_to_drop_timecode() would be better.


[...]
> Index: libavcodec/timecode.h
> ===================================================================
> --- libavcodec/timecode.h	(revision 0)
> +++ libavcodec/timecode.h	(revision 0)
[...]
> +#ifndef AVCODEC_TIMECODE_H
> +#define AVCODEC_TIMECODE_H
> +
> +int ff_timecode_drop_frame_adjust(int frame_num);

i think the doxy should be in the header

except these iam ok with this patch

[...]


> 
> > - use new lavc timecode api to set timecode in DV. Regression tests
> > change because now default behaviour is non drop frame timecode, you can
> > still activate it by using -flags2 +drop_frame_timecode
> 
> This still apply, however I notice that I can move the code without
> using the new adjust function, and will commit that first.

i leave the dv stuff to roman to decide/review

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081012/6b136bb0/attachment.pgp>