[Ffmpeg-devel] [PATCH] DV timecode

[Michael Niedermayer]

Hi

On Wed, Apr 11, 2007 at 02:29:35PM +0200, Baptiste Coudurier wrote:
> Hi
> 
> 3 patches:
> 
> - add timecode.h to avoid code duplication for drop frame timecode
> adjustment
> - 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
> - timecode helper function for ffmpeg to set timecode.
> 
> Maybe moving brktimegm to lavu and using it is better ?

could you explain what good all the timecode stuff does? that is what
didnt work before and works after the patch?


[...]
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 8712)
> +++ ffmpeg.c	(working copy)
> @@ -2499,6 +2499,46 @@
>      input_ts_offset = parse_date(arg, 1);
>  }
>  
> +static void opt_timecode(const char *arg)
> +{
> +    int hours, minutes, seconds, frames, time_code, fps, drop = 0;
> +    const AVOption *o;
> +    const char *p = arg;
> +
> +    hours   = strtol(p,     (char **)&p, 10);
> +    if (*p != ':')
> +        goto syntax_error;
> +    minutes = strtol(p + 1, (char **)&p, 10);
> +    if (*p != ':')
> +        goto syntax_error;
> +    seconds = strtol(p + 1, (char **)&p, 10);
> +    if (*p == ';')
> +        drop = 1;
> +    else if (*p != ':')
> +        goto syntax_error;
> +    frames  = strtol(p + 1, (char **)&p, 10);
> +
> +    fps = (frame_rate + frame_rate_base / 2) / frame_rate_base;
> +    time_code = (hours * 3600 + minutes * 60 + seconds) * fps + frames;
> +    if (drop) { /* adjust */
> +        int tminutes = 60 * hours + minutes;
> +        if (fps != 30) {
> +            fprintf(stderr, "Drop frame timecode only supported with 29.97 fps\n");
> +            exit(1);
> +        }
> +        time_code -= 2 * (tminutes - tminutes / 10);
> +        opt_default("flags2", "+drop_frame_timecode");
> +    }
> +
> +    o = av_set_int(avctx_opts[CODEC_TYPE_VIDEO], "timecode_frame_start", time_code);
> +    opt_names= av_realloc(opt_names, sizeof(void*)*(opt_name_count+1));
> +    opt_names[opt_name_count++]= o->name;
> +    return;
> + syntax_error:
> +    fprintf(stderr, "Syntax error, usage: hours:minutes:seconds[:;]frames\n");
> +    exit(1);
> +}

what a mess
see opt_rec_timestamp()


> +
>  static void opt_input_file(const char *filename)
>  {
>      AVFormatContext *ic;
> @@ -3617,6 +3657,7 @@
>      { "vtag", HAS_ARG | OPT_EXPERT | OPT_VIDEO, {(void*)opt_video_tag}, "force video tag/fourcc", "fourcc/tag" },
>      { "newvideo", OPT_VIDEO, {(void*)opt_new_video_stream}, "add a new video stream to the current output stream" },
>      { "qphist", OPT_BOOL | OPT_EXPERT | OPT_VIDEO, { (void *)&qp_hist }, "show QP histogram" },
> +    { "timecode", HAS_ARG | OPT_EXPERT | OPT_VIDEO, {(void*)opt_timecode}, "set timecode value", "timecode" },

please provide a proper description of what timecode is


>  
>      /* audio options */
>      { "aframes", OPT_INT | HAS_ARG | OPT_AUDIO, {(void*)&max_frames[CODEC_TYPE_AUDIO]}, "set the number of audio frames to record", "number" },

> Index: libavcodec/timecode.h
> ===================================================================
> --- libavcodec/timecode.h	(revision 0)
> +++ libavcodec/timecode.h	(revision 0)
[...]
> +#ifndef FFMPEG_TIMECODE_H
> +#define FFMPEG_TIMECODE_H
> +
> +static av_always_inline int ff_drop_frame_timecode_adjust(int time_code)

no, a function executed once every 10-300 frames is not going to
be av_always_inline it also doesnt belong into a header


[...]
> Index: libavcodec/mpeg12.c
> ===================================================================
> --- libavcodec/mpeg12.c	(revision 8712)
> +++ libavcodec/mpeg12.c	(working copy)
> @@ -32,6 +32,7 @@
>  
>  #include "mpeg12data.h"
>  #include "bytestream.h"
> +#include "timecode.h"
>  
>  //#undef NDEBUG
>  //#include <assert.h>
> @@ -378,13 +379,8 @@
>              time_code = s->current_picture_ptr->coded_picture_number + s->avctx->timecode_frame_start;
>  
>              s->gop_picture_number = s->current_picture_ptr->coded_picture_number;
> -            if (s->avctx->flags2 & CODEC_FLAG2_DROP_FRAME_TIMECODE) {
> -                /* only works for NTSC 29.97 */
> -                int d = time_code / 17982;
> -                int m = time_code % 17982;
> -                //if (m < 2) m += 2; /* not needed since -2,-1 / 1798 in C returns 0 */
> -                time_code += 18 * d + 2 * ((m - 2) / 1798);
> -            }
> +            if (s->avctx->flags2 & CODEC_FLAG2_DROP_FRAME_TIMECODE)
> +                time_code = ff_drop_frame_timecode_adjust(time_code);
>              put_bits(&s->pb, 5, (uint32_t)((time_code / (fps * 3600)) % 24));
>              put_bits(&s->pb, 6, (uint32_t)((time_code / (fps * 60)) % 60));
>              put_bits(&s->pb, 1, 1);

while it has been added by you in the past and not now the use of
coded_picture_number is completely wrong, you MUST use the proper
timestamp, even with mpeg1/2 timestamps differ from frame numbers
due to frame / field repetion amongth others

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070415/acc13570/attachment.pgp>