[FFmpeg-devel] control line spacing and support for text borders on drawtext filter (patch attached)

Nicolas George nicolas.george at normalesup.org
Tue Sep 11 21:04:46 CEST 2012


Le sextidi 26 fructidor, an CCXX, Leandro Santiago a écrit :
> Hello to all. I've made some modifications on drawtext filter which I
> currently use at work.
> 
> The modifications are the support of controlling the spacing between
> lines and a very primitive support for borders around text. It's
> primitive because it works only with one or 2px (I basically copied
> the shadow code to execute four times...). I know the correct is using
> the freetype support for text borders (which is very powerful), but
> for now this patch is working for me.
> 
> I'd enjoy if you revised it.

Thanks for the work. A few general remarks first:

- You should use git format-patch or git send-email, as the resulting patch
  is easier to apply, in particular it includes the author name, the date,
  the commit message. git format-patch outputs one file per commit that you
  can attach to your mail; git sent-email does that and sends it directly.

- (Minor) If you send your mail yourself and not with git send-email, please
  try to get the attachments to have content-type text/plain or
  text/something: it makes it slightly easier to read it directly in a MUA.

- Your patches implements two completely unrelated features; it is the
  policy in ffmpeg to require two separates commits for that. Fortunately,
  git helps a lot for that.

- You forgot to add the documentation to the options you added. It should go
  in doc/filters.texi, along with the rest of the drawtext documentation.

> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 729f152..e398732 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -133,6 +133,11 @@ typedef struct {
>      char *boxcolor_string;          ///< box color as string
>      char *shadowcolor_string;       ///< shadow color as string
>  
> +    int bordersize;                 ///< border weight
> +    char *bordercolor_string;       ///< border color as string
> +
> +    int line_spacing;               ///< lines spacing in pixels
> +
>      short int draw_box;             ///< draw box around text - true or false
>      int use_kerning;                ///< font kerning is used - true/false
>      int tabsize;                    ///< tab size
> @@ -142,6 +147,7 @@ typedef struct {
>      FFDrawColor fontcolor;          ///< foreground color
>      FFDrawColor shadowcolor;        ///< shadow color
>      FFDrawColor boxcolor;           ///< background color
> +    FFDrawColor bordercolor;        ///< border color
>  
>      FT_Library library;             ///< freetype font library handle
>      FT_Face face;                   ///< freetype font face handle
> @@ -188,6 +194,11 @@ static const AVOption drawtext_options[]= {
>  {"rate",     "set rate (timecode only)", OFFSET(tc_rate),        AV_OPT_TYPE_RATIONAL, {.dbl=0},          0,  INT_MAX, FLAGS},
>  {"fix_bounds", "if true, check and fix text coords to avoid clipping", OFFSET(fix_bounds), AV_OPT_TYPE_INT, {.i64=1}, 0, 1, FLAGS},
>  

> +{"bordercolor", "set border color",  OFFSET(bordercolor_string), AV_OPT_TYPE_STRING, {.str="yellow"}, CHAR_MIN, CHAR_MAX,FLAGS },
> +{"bordersize",  "set border weight", OFFSET(bordersize),   AV_OPT_TYPE_INT,    {.dbl=0},     INT_MIN,  INT_MAX,FLAGS  },

weight -> width?

> +

> +{"line_spacing",  "set line spacing in pixels", OFFSET(line_spacing),   AV_OPT_TYPE_INT,    {.dbl=0},     INT_MIN,  INT_MAX,FLAGS  },

It would be nicer if it was an expression that could use the variables, like
x and y. I do not consider that kind of critics blocking, though, especially
in that case since the font size should be an expression too and is not.

> +
>  /* FT_LOAD_* flags */
>  {"ft_load_flags", "set font loading flags for libfreetype",   OFFSET(ft_load_flags),  AV_OPT_TYPE_FLAGS,  {.i64=FT_LOAD_DEFAULT|FT_LOAD_RENDER}, 0, INT_MAX, FLAGS, "ft_load_flags"},
>  {"default",                     "set default",                     0, AV_OPT_TYPE_CONST, {.i64=FT_LOAD_DEFAULT},                     INT_MIN, INT_MAX, FLAGS, "ft_load_flags"},
> @@ -455,6 +466,13 @@ static av_cold int init(AVFilterContext *ctx, const char *args)
>          return err;
>      }
>  
> +    if ((err = av_parse_color(dtext->bordercolor.rgba, dtext->bordercolor_string, -1, ctx))) {
> +        av_log(ctx, AV_LOG_ERROR,
> +               "Invalid border color '%s'\n", dtext->shadowcolor_string);
> +        return err;
> +    }
> +
> +

Stray extra new line.

>      if ((err = FT_Init_FreeType(&(dtext->library)))) {
>          av_log(ctx, AV_LOG_ERROR,
>                 "Could not load FreeType: %s\n", FT_ERRMSG(err));
> @@ -538,6 +556,7 @@ static int config_input(AVFilterLink *inlink)
>      ff_draw_color(&dtext->dc, &dtext->fontcolor,   dtext->fontcolor.rgba);
>      ff_draw_color(&dtext->dc, &dtext->shadowcolor, dtext->shadowcolor.rgba);
>      ff_draw_color(&dtext->dc, &dtext->boxcolor,    dtext->boxcolor.rgba);
> +    ff_draw_color(&dtext->dc, &dtext->bordercolor, dtext->bordercolor.rgba);
>  
>      dtext->var_values[VAR_w]     = dtext->var_values[VAR_W]     = dtext->var_values[VAR_MAIN_W] = inlink->w;
>      dtext->var_values[VAR_h]     = dtext->var_values[VAR_H]     = dtext->var_values[VAR_MAIN_H] = inlink->h;
> @@ -713,7 +732,7 @@ static int draw_text(AVFilterContext *ctx, AVFilterBufferRef *picref,
>          prev_code = code;
>          if (is_newline(code)) {
>              max_text_line_w = FFMAX(max_text_line_w, x);

> -            y += dtext->max_glyph_h;
> +            y += dtext->max_glyph_h + dtext->line_spacing;

If I read this correctly, line_spacing is "extra" line spacing according to
a few layout engines who call "line spacing" the distance between the base
line of successive lines. This is not a problem, but please make sure this
is clear in the docs.

>              x = 0;
>              continue;
>          }
> @@ -772,6 +791,27 @@ static int draw_text(AVFilterContext *ctx, AVFilterBufferRef *picref,
>              return ret;
>      }
>  
> +    // draw text border.
> +    if (dtext->bordersize) {
> +      int offset = dtext->bordersize;
> +      int pos[8][2] = {
> +        {0,-offset},
> +        {-offset,-offset},
> +        {offset,0},
> +        {0,offset},
> +
> +        {-offset,offset},
> +        {offset,offset},
> +        {offset,-offset},
> +        {-offset,-offset}
> +      };

Please align the table to make it more readable.

> +
> +      int i;
> +      for (i = 0; i < 4; i++) // using 8 also draws on "diagonals"

The unused part of the table should be commented out too, I think.

> +        if ((ret = draw_glyphs(dtext, picref, width, height, dtext->bordercolor.rgba,&dtext->bordercolor,pos[i][0],pos[i][1])) < 0)
> +          return ret;
> +    }
> +

I had not realized what you meant by "border" until I read that part of the
code. I believe "outline" is usually used for that.

Also, note that the algorithm you are implementing gives ugly results when
the offset is larger than 1. I just looked at the implementation in libass
(stroke_outline in ass_render.c): it seems FreeType has builtin functions to
do that correctly.

>      if ((ret = draw_glyphs(dtext, picref, width, height, dtext->fontcolor.rgba,
>                             &dtext->fontcolor, 0, 0)) < 0)
>          return ret;

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120911/f027920c/attachment.asc>


More information about the ffmpeg-devel mailing list