[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