[FFmpeg-devel] [PATCH] Add support for drawing PTS in HHMMSSms format to vf_drawtext

[Nicolas George]

Le septidi 7 germinal, an CCXXII, hjiodjf 97xgw46 a écrit :
> There is support in vf_drawtext to draw the video time as SMPTE
> timestamp. However, no corresponding support exists for wall time.
> This patch adds to vf_drawtext a "timecodems" argument with similar
> syntax to the "timecode" argument.

Thanks for the patch. Below are some technical comments.

> From 9e5dce8438b772acd8d57722af0f18217c43bcaa Mon Sep 17 00:00:00 2001

> From: tue46wsdgxfjrt <jfbvxt at gmail.com>

Is it not possible to get a correct identity? For authorship and copyright
tracking, this is much more useful.

> Date: Wed, 19 Mar 2014 13:26:11 -0700
> Subject: [PATCH 2/2] Add support for drawing wall clock timestamps to
>  vf_drawtext.
> 
> ---

>  libavfilter/vf_drawtext.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)

You forgot to update the documentation.

> 
> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 1690b38..f50848d 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -57,6 +57,8 @@
>  #include <fontconfig/fontconfig.h>
>  #endif
>  

> + #define HUNDRED_NS ((int64_t)10000000)

Can you explain why you work with that strange unit instead of just plain
milliseconds?

> +
>  static const char *const var_names[] = {
>      "dar",
>      "hsub", "vsub",
> @@ -166,6 +168,8 @@ typedef struct {
>      int draw;                       ///< set to zero to prevent drawing
>  #endif
>      AVLFG  prng;                    ///< random
> +    char       *ts_opt_string;      ///< specified timestamp option string
> +    int64_t     ts_offset;          ///< parsed initial timestamp (100 ns timescale)
>      char       *tc_opt_string;      ///< specified timecode option string
>      AVRational  tc_rate;            ///< frame rate for timecode
>      AVTimecode  tc;                 ///< timecode context
> @@ -204,6 +208,7 @@ static const AVOption drawtext_options[]= {
>          {"normal",   "set normal expansion",                OFFSET(exp_mode), AV_OPT_TYPE_CONST, {.i64=EXP_NORMAL},   0, 0, FLAGS, "expansion"},
>          {"strftime", "set strftime expansion (deprecated)", OFFSET(exp_mode), AV_OPT_TYPE_CONST, {.i64=EXP_STRFTIME}, 0, 0, FLAGS, "expansion"},
>  
> +    {"timestamp",       "set initial timestamp",            OFFSET(ts_opt_string), AV_OPT_TYPE_STRING,   {.str=NULL}, CHAR_MIN, CHAR_MAX, FLAGS},
>      {"timecode",        "set initial timecode",             OFFSET(tc_opt_string), AV_OPT_TYPE_STRING,   {.str=NULL}, CHAR_MIN, CHAR_MAX, FLAGS},
>      {"tc24hmax",        "set 24 hours max (timecode only)", OFFSET(tc24hmax),      AV_OPT_TYPE_INT,      {.i64=0},           0,        1, FLAGS},
>      {"timecode_rate",   "set rate (timecode only)",         OFFSET(tc_rate),       AV_OPT_TYPE_RATIONAL, {.dbl=0},           0,  INT_MAX, FLAGS},
> @@ -475,7 +480,18 @@ static av_cold int init(AVFilterContext *ctx)
>      if (s->reload && !s->textfile)
>          av_log(ctx, AV_LOG_WARNING, "No file to reload\n");
>  
> -    if (s->tc_opt_string) {

> +    if (s->ts_opt_string) {
> +    	int hh, mm, ss, ms;
> +
> +    	if ((sscanf(s->ts_opt_string, "%d:%d:%d.%d", &hh, &mm, &ss, &ms)) < 4) {
> +    		av_log(ctx, AV_LOG_ERROR, "Error parsing timestamp string: %s\n", s->ts_opt_string);
> +    		return AVERROR_UNKNOWN;
> +    	}
> +    	s->ts_offset = hh * 3600 * HUNDRED_NS + mm * 60 * HUNDRED_NS + ss * HUNDRED_NS + ms * (HUNDRED_NS / 1000);
> +
> +    	if (!s->text)
> +    		s->text = av_strdup("");
> +    } else if (s->tc_opt_string) {
>          int ret = av_timecode_init_from_string(&s->tc, s->tc_rate,
>                                                 s->tc_opt_string, ctx);
>          if (ret < 0)
> @@ -926,7 +942,25 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame,
>          break;
>      }
>  
> -    if (s->tc_opt_string) {
> +    if (s->ts_opt_string) {
> +    	char tsbuf[32];
> +    	int hh, mm, ss, ms;
> +
> +    	int64_t ts = s->ts_offset + av_rescale_q(frame->pts, inlink->time_base, (AVRational){1, HUNDRED_NS});
> +
> +    	hh = ts / (3600 * HUNDRED_NS);
> +    	ts = ts % (3600 * HUNDRED_NS);
> +    	mm = ts / (60 * HUNDRED_NS);
> +    	ts = ts % (60 * HUNDRED_NS);
> +    	ss = ts / HUNDRED_NS;
> +    	ts = ts % HUNDRED_NS;
> +    	ms = ts / (HUNDRED_NS / 1000);
> +
> +    	snprintf(tsbuf, sizeof(tsbuf), "%02d:%02d:%02d.%03d", hh, mm, ss, ms);
> +
> +    	av_bprint_clear(bp);
> +    	av_bprintf(bp, "%s%s", s->text, tsbuf);
> +    } else if (s->tc_opt_string) {

The place where you added the expansion, the place that handles the SMPTE
formatting, is an old ad-hoc implementation. A more powerful expansion
scheme was implemented later: see the expand_text() function and the
drawtext_function structure for dispatch. I believe new features would be
better in that framework than imitating the old SMPTE code.

(In fact, I believe someone should probably update the SMPTE to fit the new
expansion mechanism. But I may be missing something.)

>          char tcbuf[AV_TIMECODE_STR_SIZE];
>          av_timecode_make_string(&s->tc, tcbuf, inlink->frame_count);
>          av_bprint_clear(bp);

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: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140327/4ac63c14/attachment.asc>