[PATCH] added expr evaluation to drawtext - fontsize
Allows expr evaluation in the fontsize parameter for drawtext. examples (fontsize 1/3 of video height): ffmpeg -i http://i.giphy.com/kwAi4WrChkSfm.gif -vf "drawtext=fontfile=/Library/Fonts/Verdana Bold.ttf:text='HI':fontcolor=yellow:x=(w-tw)/2:y=(h-th)/1.3:fontsize=h/3" out.gif ffmpeg -i http://i.giphy.com/3o6ozzX4mAcwkkgzG8.gif -vf "drawtext=fontfile=/Library/Fonts/Verdana Bold.ttf:text='HI':fontcolor=yellow:x=(w-tw)/2:y=(h-th)/1.3:fontsize=h/3" out.gif --- libavfilter/vf_drawtext.c | 89 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 7 deletions(-) diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..1b9d2ca 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,6 +156,8 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use short int draw_box; ///< draw box around text - true or false @@ -209,7 +211,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str="16"}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +282,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +295,14 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) { + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + } + else { + int64_t diff = (int64_t)a->fontsize - (int64_t)bb->fontsize; + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + } } /** @@ -316,6 +326,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize; if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -591,12 +602,62 @@ out: } #endif +static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Font not open\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = 16; + + double size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); + + if (isnan(size)) + fontsize = 16; + else if (round(size) <= 0) + fontsize = 1; + else + fontsize = round(size); + + // no change + if (fontsize == s->fontsize) { + return 0; + } + + s->fontsize = fontsize; + + return set_fontsize(ctx); +} + static av_cold int init(AVFilterContext *ctx) { int err; DrawTextContext *s = ctx->priv; Glyph *glyph; + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx) < 0) + return AVERROR(EINVAL); + if (!s->fontfile && !CONFIG_LIBFONTCONFIG) { av_log(ctx, AV_LOG_ERROR, "No font filename provided\n"); return AVERROR(EINVAL); @@ -647,11 +708,8 @@ static av_cold int init(AVFilterContext *ctx) err = load_font(ctx); if (err) return err; - if (!s->fontsize) - s->fontsize = 16; - if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { - av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", - s->fontsize, FT_ERRMSG(err)); + + if ((err = update_fontsize(ctx))) { return AVERROR(EINVAL); } @@ -708,6 +766,10 @@ static av_cold void uninit(AVFilterContext *ctx) av_expr_free(s->x_pexpr); av_expr_free(s->y_pexpr); s->x_pexpr = s->y_pexpr = NULL; + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + av_freep(&s->positions); s->nb_positions = 0; @@ -748,6 +810,9 @@ static int config_input(AVFilterLink *inlink) av_lfg_init(&s->prng, av_get_random_seed()); + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + av_expr_free(s->x_pexpr); av_expr_free(s->y_pexpr); s->x_pexpr = s->y_pexpr = NULL; @@ -757,6 +822,8 @@ static int config_input(AVFilterLink *inlink) (ret = av_expr_parse(&s->y_pexpr, s->y_expr, var_names, NULL, NULL, fun2_names, fun2, 0, ctx)) < 0 || (ret = av_expr_parse(&s->a_pexpr, s->a_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0 || + (ret = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) return AVERROR(EINVAL); @@ -1084,6 +1151,7 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame, for (i = 0, p = text; *p; i++) { FT_Bitmap bitmap; Glyph dummy = { 0 }; + GET_UTF8(code, *p++, continue;); /* skip new line chars, just go to new line */ @@ -1091,6 +1159,7 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame, continue; dummy.code = code; + dummy.fontsize = s->fontsize; glyph = av_tree_find(s->glyphs, &dummy, glyph_cmp, NULL); bitmap = borderw ? glyph->border_bitmap : glyph->bitmap; @@ -1222,6 +1291,7 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, /* get glyph */ dummy.code = code; + dummy.fontsize = s->fontsize; glyph = av_tree_find(s->glyphs, &dummy, glyph_cmp, NULL); if (!glyph) { ret = load_glyph(ctx, &glyph, code); @@ -1258,6 +1328,7 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, /* get glyph */ prev_glyph = glyph; dummy.code = code; + dummy.fontsize = s->fontsize; glyph = av_tree_find(s->glyphs, &dummy, glyph_cmp, NULL); /* kerning */ @@ -1345,6 +1416,10 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) #endif } + if ((ret = update_fontsize(ctx))) { + return AVERROR(EINVAL); + } + s->var_values[VAR_N] = inlink->frame_count+s->start_number; s->var_values[VAR_T] = frame->pts == AV_NOPTS_VALUE ? NAN : frame->pts * av_q2d(inlink->time_base); -- 2.1.3
On Fri, Aug 26, 2016 at 02:37:42PM -0700, Brett Harrison wrote:
Allows expr evaluation in the fontsize parameter for drawtext.
examples (fontsize 1/3 of video height):
ffmpeg -i http://i.giphy.com/kwAi4WrChkSfm.gif -vf "drawtext=fontfile=/Library/Fonts/Verdana Bold.ttf:text='HI':fontcolor=yellow:x=(w-tw)/2:y=(h-th)/1.3:fontsize=h/3" out.gif
ffmpeg -i http://i.giphy.com/3o6ozzX4mAcwkkgzG8.gif -vf "drawtext=fontfile=/Library/Fonts/Verdana Bold.ttf:text='HI':fontcolor=yellow:x=(w-tw)/2:y=(h-th)/1.3:fontsize=h/3" out.gif
--- libavfilter/vf_drawtext.c | 89 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 7 deletions(-)
The patch is corrupted by newlines Applying: Allows expr evaluation in the fontsize parameter for drawtext. fatal: patch fragment without header at line 20: @@ -209,7 +211,7 @@ static const AVOption drawtext_options[]= { error: could not build fake ancestor Patch failed at 0001 Allows expr evaluation in the fontsize parameter for drawtext. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Fri, Aug 26, 2016 at 14:37:42 -0700, Brett Harrison wrote:
+ if (diff != 0) { + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
If diff != 0, it can only be >0 or <0, nothing else: if (diff != 0) return diff > 0 ? 1 : -1; (And you can drop the curly brackets.)
+ else { + int64_t diff = (int64_t)a->fontsize - (int64_t)bb->fontsize; + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
There's a macro for this: else return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); Moritz
Fixed patch based on comments. This time attaching patch file. On Sat, Aug 27, 2016 at 6:21 AM, Moritz Barsnick <barsnick@gmx.net> wrote:
On Fri, Aug 26, 2016 at 14:37:42 -0700, Brett Harrison wrote:
+ if (diff != 0) { + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
If diff != 0, it can only be >0 or <0, nothing else: if (diff != 0) return diff > 0 ? 1 : -1; (And you can drop the curly brackets.)
+ else { + int64_t diff = (int64_t)a->fontsize - (int64_t)bb->fontsize; + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
There's a macro for this: else return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize);
Moritz _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Sat, Aug 27, 2016 at 04:30:05PM -0700, Brett Harrison wrote:
Fixed patch based on comments.
This time attaching patch file.
On Sat, Aug 27, 2016 at 6:21 AM, Moritz Barsnick <barsnick@gmx.net> wrote:
On Fri, Aug 26, 2016 at 14:37:42 -0700, Brett Harrison wrote:
+ if (diff != 0) { + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
If diff != 0, it can only be >0 or <0, nothing else: if (diff != 0) return diff > 0 ? 1 : -1; (And you can drop the curly brackets.)
+ else { + int64_t diff = (int64_t)a->fontsize - (int64_t)bb->fontsize; + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
There's a macro for this: else return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize);
Moritz _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 7 deletions(-) dd219d9b4d4f02ca4299a0bfd6ea3d5c15545f2b 0001-added-expr-evaluation-to-drawtext-fontsize.patch From 5c9d7d18a5d2f15f48605021d7f5a7890a318cc4 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
this changes the output and fontsize when fontsize is not explicitly specified for example: -vf drawtext=text=abc:fontcolor=white
--- libavfilter/vf_drawtext.c | 86 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 7 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..5311e29 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,6 +156,8 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use
short int draw_box; ///< draw box around text - true or false @@ -209,7 +211,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str="16"}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +282,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +295,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +323,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -591,12 +599,62 @@ out: } #endif
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Font not open\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = 16; + + double size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); + + if (isnan(size)) + fontsize = 16;
+ else if (round(size) <= 0) + fontsize = 1; + else + fontsize = round(size);
you should check fontsize not round(size) and then re-evaluate round(size), C doesnt gurantee that to be teh same
+ + // no change + if (fontsize == s->fontsize) { + return 0; + }
inconsistet indention and supeflous {} [...]
@@ -1084,6 +1148,7 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame, for (i = 0, p = text; *p; i++) { FT_Bitmap bitmap; Glyph dummy = { 0 }; + GET_UTF8(code, *p++, continue;);
/* skip new line chars, just go to new line */
stray change [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
Before I fix the patch, can you clarify the intended functionality? The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with: -vf drawtext=text=abc:fontcolor=white on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12. However if ffmpeg is called with: -vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fonts/TTF/Vera.ttf This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs. The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5; I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is? On Sun, Aug 28, 2016 at 6:09 PM, Michael Niedermayer <michael@niedermayer.cc
wrote:
On Sat, Aug 27, 2016 at 04:30:05PM -0700, Brett Harrison wrote:
Fixed patch based on comments.
This time attaching patch file.
On Sat, Aug 27, 2016 at 6:21 AM, Moritz Barsnick <barsnick@gmx.net> wrote:
On Fri, Aug 26, 2016 at 14:37:42 -0700, Brett Harrison wrote:
+ if (diff != 0) { + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
If diff != 0, it can only be >0 or <0, nothing else: if (diff != 0) return diff > 0 ? 1 : -1; (And you can drop the curly brackets.)
+ else { + int64_t diff = (int64_t)a->fontsize - (int64_t)bb->fontsize; + return diff > 0 ? 1 : diff < 0 ? -1 : 0; + }
There's a macro for this: else return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize);
Moritz _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 86 ++++++++++++++++++++++++++++++ +++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 7 deletions(-) dd219d9b4d4f02ca4299a0bfd6ea3d5c15545f2b 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 5c9d7d18a5d2f15f48605021d7f5a7890a318cc4 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
this changes the output and fontsize when fontsize is not explicitly specified
for example: -vf drawtext=text=abc:fontcolor=white
--- libavfilter/vf_drawtext.c | 86 ++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+), 7 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..5311e29 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,6 +156,8 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use
short int draw_box; ///< draw box around text - true or false @@ -209,7 +211,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str="16"}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +282,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +295,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +323,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -591,12 +599,62 @@ out: } #endif
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Font not open\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d
+++++++++++++---- pixels: %s\n",
+ s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = 16; + + double size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); + + if (isnan(size)) + fontsize = 16;
+ else if (round(size) <= 0) + fontsize = 1; + else + fontsize = round(size);
you should check fontsize not round(size) and then re-evaluate round(size), C doesnt gurantee that to be teh same
+ + // no change + if (fontsize == s->fontsize) { + return 0; + }
inconsistet indention and supeflous {}
[...]
@@ -1084,6 +1148,7 @@ static int draw_glyphs(DrawTextContext *s, AVFrame *frame, for (i = 0, p = text; *p; i++) { FT_Bitmap bitmap; Glyph dummy = { 0 }; + GET_UTF8(code, *p++, continue;);
/* skip new line chars, just go to new line */
stray change
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Aug 29, 2016 at 02:23:44PM -0700, Brett Harrison wrote:
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fonts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think no can you send a seperate patch that changes this ? (it should really not change as a sideeffect of this patch) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
Le tridi 13 fructidor, an CCXXIV, Brett Harrison a écrit :
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fonts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think the current behaviour is correct. I start with the following principle: when users want something precise about aesthetic or other arbitrary settings, they have to say it. Default values are for the lazy or the careless ones: quick profiling and testing when the exact result does not matter much. But as soon as the result matters, explicit values must be given. Do not take me wrong, the default values should be, as much as possible, sensible. But for a font size, 12 is as sensible as 16. Most importantly, backward compatibility should not be an hindrance to choosing better default values. We should not change them lightly, but not feel forbidden to do so either. Fontconfig is not just a path search library for finding font files. It is a complete mechanism for choosing the right font according to several conditions set by the user. Including the font size. There are no less than four levels for choosing the font size: (1) lavfi's default, 16; (2) fontconfig's default; (3) fontconfig's explicit value, as in "Times-12:bold"; (4) lavfi's explicit value, as in "fontsize=16". I think the order of precedence should be just that, for the following reasons: - First, of course, (3), (4) > (1), (2), because explicit values are always more important. - Second, conflicting explicit values are the users' problem. We can produce warnings to help diagnose, but in the end it is their choice. (4) > (3) is slightly easier to implement (distinguishing (3) from (2) requires a bit of work), and (4) is more supple, especially when your patch gets applied. - Last, (2) > (1), because fontconfig is more cross-applications than lavfi, and also because it includes a mechanism for explicit configuration. Still, the documentation should be clarified, possibly something like that: "The default value of fontsize is provided by fontconfig if in use or 16 when using a font file directly." Regards, -- Nicolas George
Since there are differing opinions on how the default fontsize should be established this patch adds my changes while preserving the current behavior when fontsize is not specified. On Tue, Aug 30, 2016 at 2:43 AM, Nicolas George <george@nsup.org> wrote:
Le tridi 13 fructidor, an CCXXIV, Brett Harrison a écrit :
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/ fonts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think the current behaviour is correct.
I start with the following principle: when users want something precise about aesthetic or other arbitrary settings, they have to say it.
Default values are for the lazy or the careless ones: quick profiling and testing when the exact result does not matter much. But as soon as the result matters, explicit values must be given.
Do not take me wrong, the default values should be, as much as possible, sensible. But for a font size, 12 is as sensible as 16.
Most importantly, backward compatibility should not be an hindrance to choosing better default values. We should not change them lightly, but not feel forbidden to do so either.
Fontconfig is not just a path search library for finding font files. It is a complete mechanism for choosing the right font according to several conditions set by the user. Including the font size.
There are no less than four levels for choosing the font size:
(1) lavfi's default, 16;
(2) fontconfig's default;
(3) fontconfig's explicit value, as in "Times-12:bold";
(4) lavfi's explicit value, as in "fontsize=16".
I think the order of precedence should be just that, for the following reasons:
- First, of course, (3), (4) > (1), (2), because explicit values are always more important.
- Second, conflicting explicit values are the users' problem. We can produce warnings to help diagnose, but in the end it is their choice. (4) > (3) is slightly easier to implement (distinguishing (3) from (2) requires a bit of work), and (4) is more supple, especially when your patch gets applied.
- Last, (2) > (1), because fontconfig is more cross-applications than lavfi, and also because it includes a mechanism for explicit configuration.
Still, the documentation should be clarified, possibly something like that: "The default value of fontsize is provided by fontconfig if in use or 16 when using a font file directly."
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Any feedback on this newest patch? On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison <brett.harrison@zyamusic.com
wrote:
Since there are differing opinions on how the default fontsize should be established this patch adds my changes while preserving the current behavior when fontsize is not specified.
On Tue, Aug 30, 2016 at 2:43 AM, Nicolas George <george@nsup.org> wrote:
Le tridi 13 fructidor, an CCXXIV, Brett Harrison a écrit :
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fo nts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think the current behaviour is correct.
I start with the following principle: when users want something precise about aesthetic or other arbitrary settings, they have to say it.
Default values are for the lazy or the careless ones: quick profiling and testing when the exact result does not matter much. But as soon as the result matters, explicit values must be given.
Do not take me wrong, the default values should be, as much as possible, sensible. But for a font size, 12 is as sensible as 16.
Most importantly, backward compatibility should not be an hindrance to choosing better default values. We should not change them lightly, but not feel forbidden to do so either.
Fontconfig is not just a path search library for finding font files. It is a complete mechanism for choosing the right font according to several conditions set by the user. Including the font size.
There are no less than four levels for choosing the font size:
(1) lavfi's default, 16;
(2) fontconfig's default;
(3) fontconfig's explicit value, as in "Times-12:bold";
(4) lavfi's explicit value, as in "fontsize=16".
I think the order of precedence should be just that, for the following reasons:
- First, of course, (3), (4) > (1), (2), because explicit values are always more important.
- Second, conflicting explicit values are the users' problem. We can produce warnings to help diagnose, but in the end it is their choice. (4) > (3) is slightly easier to implement (distinguishing (3) from (2) requires a bit of work), and (4) is more supple, especially when your patch gets applied.
- Last, (2) > (1), because fontconfig is more cross-applications than lavfi, and also because it includes a mechanism for explicit configuration.
Still, the documentation should be clarified, possibly something like that: "The default value of fontsize is provided by fontconfig if in use or 16 when using a font file directly."
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Most recent patch. I was evaluating fontsize too early before when using the 'n' variable in equations. On Thu, Sep 1, 2016 at 3:44 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Any feedback on this newest patch?
On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison < brett.harrison@zyamusic.com> wrote:
Since there are differing opinions on how the default fontsize should be established this patch adds my changes while preserving the current behavior when fontsize is not specified.
On Tue, Aug 30, 2016 at 2:43 AM, Nicolas George <george@nsup.org> wrote:
Le tridi 13 fructidor, an CCXXIV, Brett Harrison a écrit :
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fo nts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think the current behaviour is correct.
I start with the following principle: when users want something precise about aesthetic or other arbitrary settings, they have to say it.
Default values are for the lazy or the careless ones: quick profiling and testing when the exact result does not matter much. But as soon as the result matters, explicit values must be given.
Do not take me wrong, the default values should be, as much as possible, sensible. But for a font size, 12 is as sensible as 16.
Most importantly, backward compatibility should not be an hindrance to choosing better default values. We should not change them lightly, but not feel forbidden to do so either.
Fontconfig is not just a path search library for finding font files. It is a complete mechanism for choosing the right font according to several conditions set by the user. Including the font size.
There are no less than four levels for choosing the font size:
(1) lavfi's default, 16;
(2) fontconfig's default;
(3) fontconfig's explicit value, as in "Times-12:bold";
(4) lavfi's explicit value, as in "fontsize=16".
I think the order of precedence should be just that, for the following reasons:
- First, of course, (3), (4) > (1), (2), because explicit values are always more important.
- Second, conflicting explicit values are the users' problem. We can produce warnings to help diagnose, but in the end it is their choice. (4) > (3) is slightly easier to implement (distinguishing (3) from (2) requires a bit of work), and (4) is more supple, especially when your patch gets applied.
- Last, (2) > (1), because fontconfig is more cross-applications than lavfi, and also because it includes a mechanism for explicit configuration.
Still, the documentation should be clarified, possibly something like that: "The default value of fontsize is provided by fontconfig if in use or 16 when using a font file directly."
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Sep 01, 2016 at 06:17:11PM -0700, Brett Harrison wrote:
Most recent patch. I was evaluating fontsize too early before when using the 'n' variable in equations.
On Thu, Sep 1, 2016 at 3:44 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Any feedback on this newest patch?
On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison < brett.harrison@zyamusic.com> wrote:
Since there are differing opinions on how the default fontsize should be established this patch adds my changes while preserving the current behavior when fontsize is not specified.
On Tue, Aug 30, 2016 at 2:43 AM, Nicolas George <george@nsup.org> wrote:
Le tridi 13 fructidor, an CCXXIV, Brett Harrison a écrit :
Before I fix the patch, can you clarify the intended functionality?
The docs say that 16 is the default fontsize, however if CONFIG_LIBFONTCONFIG is configured and ffmpeg if called with:
-vf drawtext=text=abc:fontcolor=white
on my system the font used will be /opt/X11/share/fonts/TTF/Vera.ttf (the default chosen by libfontconfig) and the fontsize will be set to 12.
However if ffmpeg is called with:
-vf drawtext=text=abc:fontcolor=white:fontfile=/opt/X11/share/fo nts/TTF/Vera.ttf
This is the same font that libfontconfig used, however this time fontsize 16 is used as stated in the docs.
The difference is this line of code in load_font_fontconfig if (!s->fontsize) s->fontsize = size + 0.5;
I didn't set the fontsize in either command, but the output was different. Do we want to keep this as is?
I think the current behaviour is correct.
I start with the following principle: when users want something precise about aesthetic or other arbitrary settings, they have to say it.
Default values are for the lazy or the careless ones: quick profiling and testing when the exact result does not matter much. But as soon as the result matters, explicit values must be given.
Do not take me wrong, the default values should be, as much as possible, sensible. But for a font size, 12 is as sensible as 16.
Most importantly, backward compatibility should not be an hindrance to choosing better default values. We should not change them lightly, but not feel forbidden to do so either.
Fontconfig is not just a path search library for finding font files. It is a complete mechanism for choosing the right font according to several conditions set by the user. Including the font size.
There are no less than four levels for choosing the font size:
(1) lavfi's default, 16;
(2) fontconfig's default;
(3) fontconfig's explicit value, as in "Times-12:bold";
(4) lavfi's explicit value, as in "fontsize=16".
I think the order of precedence should be just that, for the following reasons:
- First, of course, (3), (4) > (1), (2), because explicit values are always more important.
- Second, conflicting explicit values are the users' problem. We can produce warnings to help diagnose, but in the end it is their choice. (4) > (3) is slightly easier to implement (distinguishing (3) from (2) requires a bit of work), and (4) is more supple, especially when your patch gets applied.
- Last, (2) > (1), because fontconfig is more cross-applications than lavfi, and also because it includes a mechanism for explicit configuration.
Still, the documentation should be clarified, possibly something like that: "The default value of fontsize is provided by fontconfig if in use or 16 when using a font file directly."
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 109 insertions(+), 12 deletions(-) e87e1b5f3fa57f9c0dee2d30e9889ba0503c0a0c 0001-added-expr-evaluation-to-drawtext-fontsize.patch From 1913b998490df8ae2bca4a5b3034b19f5e44928f Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] also patch breaks: ./ffmpeg -i m.mpg -vf drawtext=fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a -f null - [AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args 'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a' [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides
On Thu, Sep 01, 2016 at 18:17:11 -0700, Brett Harrison wrote:
Most recent patch. I was evaluating fontsize too early before when using the 'n' variable in equations.
+ av_log(ctx, AV_LOG_ERROR, "Font not open\n");
I was wondering: Was does this message tell the user?
+ if ((ret = update_fontsize(ctx))) { You were meaning to write if (ret = update_fontsize(ctx)) { or if ((ret = update_fontsize(ctx)) < 0) { ?? (Too many brackets the way you did it.)
Moritz
On Fri, Sep 02, 2016 at 14:10:41 +0200, Moritz Barsnick wrote:
+ if ((ret = update_fontsize(ctx))) { You were meaning to write if (ret = update_fontsize(ctx)) { or if ((ret = update_fontsize(ctx)) < 0) { ?? (Too many brackets the way you did it.)
Sorry, probably makes sense, equal to if ((ret = update_fontsize(ctx)) != 0) { I'm still not *really* sure whether the second set of brackets is required to have "if" check the assigned ret (I thought not). I'll let someone else judge. Moritz
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
Sorry, probably makes sense, equal to if ((ret = update_fontsize(ctx)) != 0) {
I'm still not *really* sure whether the second set of brackets is required to have "if" check the assigned ret (I thought not). I'll let someone else judge.
You mean the parentheses? Well, if you are not sure, then the parentheses are necessary. We are not playing golf, we want code that is readable and robust. For reference, comparisons have precedence over assignments. That means "a = b < c" is equivalent to "a = (b < c)", not "(a = b) < c". Therefore, the parentheses are necessary even for golf. Regards, -- Nicolas George
On Fri, Sep 02, 2016 at 14:20:37 +0200, Nicolas George wrote:
You mean the parentheses? Well, if you are not sure, then the parentheses are necessary. We are not playing golf, we want code that is readable and robust.
For reference, comparisons have precedence over assignments. That means "a = b < c" is equivalent to "a = (b < c)", not "(a = b) < c". Therefore, the parentheses are necessary even for golf.
No, I meant Brett's: if ((ret = update_fontsize(ctx))) { *Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) { is it sufficient to say: if (ret = update_fontsize(ctx)) { or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used, or even if ((ret = update_fontsize(ctx)) != 0) { ? I'm very well aware that brackets are safer over guessing what operator precedence is, even though ffmpeg code often prefers to leave away unneeded brackets. In this particular case, Brett used no second operator. Moritz
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in this particular case, there is a good reason: <stdin>:4:1: warning: suggest parentheses around assignment used as truth value [-Wparentheses] Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional. Regards, -- Nicolas George
Addressed the following concerns. ===
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [->Wdeclaration-after-statement]
Fixed this.
also patch breaks: ./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a'
* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
This is fixed. === *
I was wondering: Was does this message tell the user?
I changed this error to read "Unable to initialize font". This error should only be seen if set_fontsize() is called before the font is loaded. I don't think a user would be able to cause this because if the font fails to load ffmpeg would error out before this. It is a sanity check. === For the concerns about multiple to many brackets on "if ((ret = update_fontsize(ctx)))", I removed the "ret =" part as I wasn't using the value anyway. On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org> wrote:
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in this particular case, there is a good reason:
<stdin>:4:1: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional.
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, Sep 02, 2016 at 03:31:21PM -0700, Brett Harrison wrote:
Addressed the following concerns.
===
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [->Wdeclaration-after-statement]
Fixed this.
also patch breaks: ./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a'
This is fixed.
===
I was wondering: Was does this message tell the user?
* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
I changed this error to read "Unable to initialize font". This error should only be seen if set_fontsize() is called before the font is loaded. I don't think a user would be able to cause this because if the font fails to load ffmpeg would error out before this. It is a sanity check.
===
For the concerns about multiple to many brackets on "if ((ret = update_fontsize(ctx)))",
I removed the "ret =" part as I wasn't using the value anyway.
On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org> wrote:
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in this particular case, there is a good reason:
<stdin>:4:1: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional.
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 112 insertions(+), 13 deletions(-) 311d60f04d959ddfd47ed8ea66d0f015725dd511 0001-added-expr-evaluation-to-drawtext-fontsize.patch From 665b3f1c458222d64a9ba4f1c71d343766ee9e6b Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 112 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..a483679 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text - true or false int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,72 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; +
+ if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx) < 0) + return AVERROR(EINVAL);
the error code should probably be forwarded
+ + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) {
+ if ((err = parse_fontsize(ctx) < 0)) + return err;
This looks wrong, the () is placed incorrectly [...]
@@ -1352,7 +1450,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height);
+ if ((ret = draw_text(ctx, frame, frame->width, frame->height) < 0)) + return ret;
same also error codes in ffmpeg are negative, this if its intended to be as is woul introduce a positive error code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
This patch addresses your concerns. On Fri, Sep 2, 2016 at 5:05 PM, Michael Niedermayer <michael@niedermayer.cc> wrote:
Addressed the following concerns.
===
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [->Wdeclaration-after-statement]
Fixed this.
also patch breaks: ./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/ fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a'
This is fixed.
===
I was wondering: Was does this message tell the user?
* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
I changed this error to read "Unable to initialize font". This error should only be seen if set_fontsize() is called before the font is loaded. I don't think a user would be able to cause this because if the font fails to load ffmpeg would error out before this. It is a sanity check.
===
For the concerns about multiple to many brackets on "if ((ret = update_fontsize(ctx)))",
I removed the "ret =" part as I wasn't using the value anyway.
On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org> wrote:
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in
particular case, there is a good reason:
<stdin>:4:1: warning: suggest parentheses around assignment used as
On Fri, Sep 02, 2016 at 03:31:21PM -0700, Brett Harrison wrote: this truth
value [-Wparentheses]
Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional.
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++++++++++++------- 1 file changed, 112 insertions(+), 13 deletions(-) 311d60f04d959ddfd47ed8ea66d0f015725dd511 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 665b3f1c458222d64a9ba4f1c71d343766ee9e6b Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++----- 1 file changed, 112 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..a483679 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text - true or false int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,72 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; +
+ if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx) < 0) + return AVERROR(EINVAL);
the error code should probably be forwarded
+ + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) {
+ if ((err = parse_fontsize(ctx) < 0)) + return err;
This looks wrong, the () is placed incorrectly
[...]
@@ -1352,7 +1450,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height);
+ if ((ret = draw_text(ctx, frame, frame->width, frame->height) < 0)) + return ret;
same
also error codes in ffmpeg are negative, this if its intended to be as is woul introduce a positive error code
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Tue, Sep 06, 2016 at 10:27:24AM -0700, Brett Harrison wrote:
This patch addresses your concerns.
On Fri, Sep 2, 2016 at 5:05 PM, Michael Niedermayer <michael@niedermayer.cc> wrote:
Addressed the following concerns.
===
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [->Wdeclaration-after-statement]
Fixed this.
also patch breaks: ./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/ fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf:text=a'
This is fixed.
===
I was wondering: Was does this message tell the user?
* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
I changed this error to read "Unable to initialize font". This error should only be seen if set_fontsize() is called before the font is loaded. I don't think a user would be able to cause this because if the font fails to load ffmpeg would error out before this. It is a sanity check.
===
For the concerns about multiple to many brackets on "if ((ret = update_fontsize(ctx)))",
I removed the "ret =" part as I wasn't using the value anyway.
On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org> wrote:
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in
particular case, there is a good reason:
<stdin>:4:1: warning: suggest parentheses around assignment used as
On Fri, Sep 02, 2016 at 03:31:21PM -0700, Brett Harrison wrote: this truth
value [-Wparentheses]
Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional.
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++++++++++++------- 1 file changed, 112 insertions(+), 13 deletions(-) 311d60f04d959ddfd47ed8ea66d0f015725dd511 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 665b3f1c458222d64a9ba4f1c71d343766ee9e6b Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++----- 1 file changed, 112 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..a483679 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text - true or false int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,72 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; +
+ if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx) < 0) + return AVERROR(EINVAL);
the error code should probably be forwarded
+ + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) {
+ if ((err = parse_fontsize(ctx) < 0)) + return err;
This looks wrong, the () is placed incorrectly
[...]
@@ -1352,7 +1450,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height);
+ if ((ret = draw_text(ctx, frame, frame->width, frame->height) < 0)) + return ret;
same
also error codes in ffmpeg are negative, this if its intended to be as is woul introduce a positive error code
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 13 deletions(-) d9624210e5ec9554f037959b3ca6e240b76c27db 0001-added-expr-evaluation-to-drawtext-fontsize.patch From 42687746fac27416c6b1d3de29142b2448bdd66e Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 126 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 113 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..b345cfc 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text - true or false int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,73 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; +
+ if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL);
How can this be NULL ? [...]
@@ -1216,12 +1310,16 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, x = 0; y = 0;
+ if (update_fontsize(ctx) != 0) + return AVERROR(EINVAL);
error checks should be < 0 and the error code should be forwarded [...]
@@ -1352,7 +1451,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height); + if ((ret = draw_text(ctx, frame, frame->width, frame->height)) < 0) + return ret;
this change looks unrelated and should be in a seperate patch. Also i think this leaks frame if you return here [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch
Here are the changes requested On Thu, Sep 8, 2016 at 8:50 AM, Michael Niedermayer <michael@niedermayer.cc> wrote:
This patch addresses your concerns.
On Fri, Sep 2, 2016 at 5:05 PM, Michael Niedermayer <michael@niedermayer.cc> wrote:
Addressed the following concerns.
===
libavfilter/vf_drawtext.c: In function ‘update_fontsize’: libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed declarations and code [->Wdeclaration-after-statement]
Fixed this.
also patch breaks: ./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/ fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext' with args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial. ttf:text=a'
This is fixed.
===
I was wondering: Was does this message tell the user?
* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
I changed this error to read "Unable to initialize font". This error should only be seen if set_fontsize() is called before the font is loaded. I don't think a user would be able to cause this because if the font fails to load ffmpeg would error out before this. It is a sanity check.
===
For the concerns about multiple to many brackets on "if ((ret = update_fontsize(ctx)))",
I removed the "ret =" part as I wasn't using the value anyway.
On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org> wrote:
Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
*Assuming* he means "assign update_fontsize()'s return value to ret, and check whether ret is != 0", which would correspond to the more verbose if ((ret = update_fontsize(ctx)) != 0) {
is it sufficient to say: if (ret = update_fontsize(ctx)) {
or is it required, or is it more readable or even desired by "style guide" to say: if ((ret = update_fontsize(ctx))) { (to clarify it's a check of an assignment) - this is what Brett used,
Ah. Parentheses over the whole expression are always optional, but in
particular case, there is a good reason:
<stdin>:4:1: warning: suggest parentheses around assignment used as
On Fri, Sep 02, 2016 at 03:31:21PM -0700, Brett Harrison wrote: this truth
value [-Wparentheses]
Forgetting to double the equal in a comparison is a common mistake, compilers detect it. But then you need a way of stating when it is intentional.
Regards,
-- Nicolas George
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++++++++++++------- 1 file changed, 112 insertions(+), 13 deletions(-) 311d60f04d959ddfd47ed8ea66d0f015725dd511 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 665b3f1c458222d64a9ba4f1c71d343766ee9e6b Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++++----- 1 file changed, 112 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..a483679 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text -
On Tue, Sep 06, 2016 at 10:27:24AM -0700, Brett Harrison wrote: true or
false
int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color",
OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
{"box", "set box", OFFSET(draw_box),
AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS},
{"boxborderw", "set box border width", OFFSET(boxborderw),
- {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,72 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL); + } + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d
AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, pixels: %s\n",
+ s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; +
+ if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx) < 0) + return AVERROR(EINVAL);
the error code should probably be forwarded
+ + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) {
+ if ((err = parse_fontsize(ctx) < 0)) + return err;
This looks wrong, the () is placed incorrectly
[...]
@@ -1352,7 +1450,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height);
+ if ((ret = draw_text(ctx, frame, frame->width, frame->height) < 0)) + return ret;
same
also error codes in ffmpeg are negative, this if its intended to be as is woul introduce a positive error code
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 87040B0FAB
Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
vf_drawtext.c | 126 ++++++++++++++++++++++++++++++ ++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 13 deletions(-) d9624210e5ec9554f037959b3ca6e240b76c27db 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 42687746fac27416c6b1d3de29142b2448bdd66e Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Fri, 26 Aug 2016 14:29:34 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 126 ++++++++++++++++++++++++++++++ +++++++++++----- 1 file changed, 113 insertions(+), 13 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..b345cfc 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
short int draw_box; ///< draw box around text - true or false int boxborderw; ///< box border width @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= { {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -280,6 +283,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -365,6 +374,73 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; +
+ if (s->face == NULL) { + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n"); + return AVERROR(EINVAL);
How can this be NULL ?
[...]
@@ -1216,12 +1310,16 @@ static int draw_text(AVFilterContext *ctx, AVFrame *frame, x = 0; y = 0;
+ if (update_fontsize(ctx) != 0) + return AVERROR(EINVAL);
error checks should be < 0 and the error code should be forwarded
[...]
@@ -1352,7 +1451,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame) s->var_values[VAR_PICT_TYPE] = frame->pict_type; s->metadata = av_frame_get_metadata(frame);
- draw_text(ctx, frame, frame->width, frame->height); + if ((ret = draw_text(ctx, frame, frame->width, frame->height)) < 0) + return ret;
this change looks unrelated and should be in a seperate patch. Also i think this leaks frame if you return here
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Any feedback on the most recent patch?
On Fri, Sep 9, 2016 at 5:26 PM, Brett Harrison <brett.harrison@zyamusic.com>
wrote:
> Here are the changes requested
>
> On Thu, Sep 8, 2016 at 8:50 AM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
>
>> On Tue, Sep 06, 2016 at 10:27:24AM -0700, Brett Harrison wrote:
>> > This patch addresses your concerns.
>> >
>> > On Fri, Sep 2, 2016 at 5:05 PM, Michael Niedermayer
>> <michael@niedermayer.cc>
>> > wrote:
>> >
>> > > On Fri, Sep 02, 2016 at 03:31:21PM -0700, Brett Harrison wrote:
>> > > > Addressed the following concerns.
>> > > >
>> > > > ===
>> > > >
>> > > > >libavfilter/vf_drawtext.c: In function ‘update_fontsize’:
>> > > > >libavfilter/vf_drawtext.c:422:5: warning: ISO C90 forbids mixed
>> > > declarations and code [->Wdeclaration-after-statement]
>> > > >
>> > > > Fixed this.
>> > > >
>> > > > >also patch breaks:
>> > > > >./ffmpeg -i m.mpg -vf >drawtext=fontfile=/usr/share/
>> > > fonts/truetype/msttcorefonts/arial.ttf:text=a -f null -
>> > > >
>> > > > >[AVFilterGraph @ 0x37a6960] Error initializing filter 'drawtext'
>> with
>> > > args >'fontfile=/usr/share/fonts/truetype/msttcorefonts/arial.ttf
>> :text=a'
>> > > >
>> > > > This is fixed.
>> > > >
>> > > > ===
>> > > >
>> > > > >>* + av_log(ctx, AV_LOG_ERROR, "Font not open\n");
>> > > > *
>> > > > >I was wondering: Was does this message tell the user?
>> > > >
>> > > > I changed this error to read "Unable to initialize font". This
>> error
>> > > > should only be seen if set_fontsize() is called before the font is
>> > > > loaded. I don't think a user would be able to cause this because if
>> > > > the font fails to load ffmpeg would error out before this. It is a
>> > > > sanity check.
>> > > >
>> > > > ===
>> > > >
>> > > > For the concerns about multiple to many brackets on "if ((ret =
>> > > > update_fontsize(ctx)))",
>> > > >
>> > > > I removed the "ret =" part as I wasn't using the value anyway.
>> > > >
>> > > >
>> > > > On Fri, Sep 2, 2016 at 6:13 AM, Nicolas George <george@nsup.org>
>> wrote:
>> > > >
>> > > > > Le septidi 17 fructidor, an CCXXIV, Moritz Barsnick a écrit :
>> > > > > > *Assuming* he means "assign update_fontsize()'s return value to
>> ret,
>> > > > > > and check whether ret is != 0", which would correspond to the
>> more
>> > > > > > verbose
>> > > > > > if ((ret = update_fontsize(ctx)) != 0) {
>> > > > > >
>> > > > > > is it sufficient to say:
>> > > > > > if (ret = update_fontsize(ctx)) {
>> > > > > >
>> > > > > > or is it required, or is it more readable or even desired by
>> "style
>> > > > > > guide" to say:
>> > > > > > if ((ret = update_fontsize(ctx))) {
>> > > > > > (to clarify it's a check of an assignment) - this is what Brett
>> used,
>> > > > >
>> > > > > Ah. Parentheses over the whole expression are always optional,
>> but in
>> > > this
>> > > > > particular case, there is a good reason:
>> > > > >
>> > > > > <stdin>:4:1: warning: suggest parentheses around assignment used
>> as
>> > > truth
>> > > > > value [-Wparentheses]
>> > > > >
>> > > > > Forgetting to double the equal in a comparison is a common
>> mistake,
>> > > > > compilers detect it. But then you need a way of stating when it is
>> > > > > intentional.
>> > > > >
>> > > > > Regards,
>> > > > >
>> > > > > --
>> > > > > Nicolas George
>> > > > >
>> > > > > _______________________________________________
>> > > > > ffmpeg-devel mailing list
>> > > > > ffmpeg-devel@ffmpeg.org
>> > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > > > >
>> > > > >
>> > >
>> > > > vf_drawtext.c | 125 ++++++++++++++++++++++++++++++
>> > > +++++++++++++++++++++-------
>> > > > 1 file changed, 112 insertions(+), 13 deletions(-)
>> > > > 311d60f04d959ddfd47ed8ea66d0f015725dd511
>> 0001-added-expr-evaluation-to-
>> > > drawtext-fontsize.patch
>> > > > From 665b3f1c458222d64a9ba4f1c71d343766ee9e6b Mon Sep 17 00:00:00
>> 2001
>> > > > From: Brett Harrison <brett.harrison@musicmastermind.com>
>> > > > Date: Fri, 26 Aug 2016 14:29:34 -0700
>> > > > Subject: [PATCH] added expr evaluation to drawtext - fontsize
>> > > >
>> > > > ---
>> > > > libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++
>> > > +++++++++++-----
>> > > > 1 file changed, 112 insertions(+), 13 deletions(-)
>> > > >
>> > > > diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
>> > > > index 214aef0..a483679 100644
>> > > > --- a/libavfilter/vf_drawtext.c
>> > > > +++ b/libavfilter/vf_drawtext.c
>> > > > @@ -156,7 +156,10 @@ typedef struct DrawTextContext {
>> > > > int max_glyph_h; ///< max glyph height
>> > > > int shadowx, shadowy;
>> > > > int borderw; ///< border width
>> > > > + char *fontsize_expr; ///< expression for fontsize
>> > > > + AVExpr *fontsize_pexpr; ///< parsed expressions for
>> fontsize
>> > > > unsigned int fontsize; ///< font size to use
>> > > > + unsigned int default_fontsize; ///< default font size to use
>> > > >
>> > > > short int draw_box; ///< draw box around text -
>> true or
>> > > false
>> > > > int boxborderw; ///< box border width
>> > > > @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= {
>> > > > {"shadowcolor", "set shadow color",
>> OFFSET(shadowcolor.rgba),
>> > > AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > > > {"box", "set box", OFFSET(draw_box),
>> > > AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS},
>> > > > {"boxborderw", "set box border width", OFFSET(boxborderw),
>> > > AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS},
>> > > > - {"fontsize", "set font size", OFFSET(fontsize),
>> > > AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS},
>> > > > + {"fontsize", "set font size", OFFSET(fontsize_expr),
>> > > AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS},
>> > > > {"x", "set x expression", OFFSET(x_expr),
>> > > AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > > > {"y", "set y expression", OFFSET(y_expr),
>> > > AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > > > {"shadowx", "set shadow x offset", OFFSET(shadowx),
>> > > AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS},
>> > > > @@ -280,6 +283,7 @@ typedef struct Glyph {
>> > > > FT_Glyph glyph;
>> > > > FT_Glyph border_glyph;
>> > > > uint32_t code;
>> > > > + unsigned int fontsize;
>> > > > FT_Bitmap bitmap; ///< array holding bitmaps of font
>> > > > FT_Bitmap border_bitmap; ///< array holding bitmaps of font
>> border
>> > > > FT_BBox bbox;
>> > > > @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const
>> void *b)
>> > > > {
>> > > > const Glyph *a = key, *bb = b;
>> > > > int64_t diff = (int64_t)a->code - (int64_t)bb->code;
>> > > > - return diff > 0 ? 1 : diff < 0 ? -1 : 0;
>> > > > +
>> > > > + if (diff != 0)
>> > > > + return diff > 0 ? 1 : -1;
>> > > > + else
>> > > > + return FFDIFFSIGN((int64_t)a->fontsize,
>> > > (int64_t)bb->fontsize);
>> > > > }
>> > > >
>> > > > /**
>> > > > @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx,
>> Glyph
>> > > **glyph_ptr, uint32_t code)
>> > > > goto error;
>> > > > }
>> > > > glyph->code = code;
>> > > > + glyph->fontsize = s->fontsize;
>> > > >
>> > > > if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) {
>> > > > ret = AVERROR(EINVAL);
>> > > > @@ -365,6 +374,72 @@ error:
>> > > > return ret;
>> > > > }
>> > > >
>> > > > +static av_cold int set_fontsize(AVFilterContext *ctx)
>> > > > +{
>> > > > + int err;
>> > > > + DrawTextContext *s = ctx->priv;
>> > > > +
>> > > > + if (s->face == NULL) {
>> > > > + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n");
>> > > > + return AVERROR(EINVAL);
>> > > > + }
>> > > > +
>> > > > + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) {
>> > > > + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d
>> > > pixels: %s\n",
>> > > > + s->fontsize, FT_ERRMSG(err));
>> > > > + return AVERROR(EINVAL);
>> > > > + }
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static av_cold int parse_fontsize(AVFilterContext *ctx)
>> > > > +{
>> > > > + DrawTextContext *s = ctx->priv;
>> > > > +
>> > > > + if (s->fontsize_expr == NULL)
>> > > > + return AVERROR(EINVAL);
>> > > > +
>> > > > + av_expr_free(s->fontsize_pexpr);
>> > > > + s->fontsize_pexpr = NULL;
>> > > > +
>> > >
>> > > > + if (av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr,
>> var_names,
>> > > > + NULL, NULL, fun2_names, fun2, 0, ctx)
>> < 0)
>> > > > + return AVERROR(EINVAL);
>> > >
>> > > the error code should probably be forwarded
>> > >
>> > >
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +static av_cold int update_fontsize(AVFilterContext *ctx)
>> > > > +{
>> > > > + DrawTextContext *s = ctx->priv;
>> > > > + unsigned int fontsize = s->default_fontsize;
>> > > > + int err;
>> > > > + double size;
>> > > > +
>> > > > + // if no fontsize specified use the default
>> > > > + if (s->fontsize_expr != NULL) {
>> > >
>> > > > + if ((err = parse_fontsize(ctx) < 0))
>> > > > + return err;
>> > >
>> > > This looks wrong, the () is placed incorrectly
>> > >
>> > >
>> > > [...]
>> > > > @@ -1352,7 +1450,8 @@ static int filter_frame(AVFilterLink *inlink,
>> > > AVFrame *frame)
>> > > > s->var_values[VAR_PICT_TYPE] = frame->pict_type;
>> > > > s->metadata = av_frame_get_metadata(frame);
>> > > >
>> > > > - draw_text(ctx, frame, frame->width, frame->height);
>> > >
>> > > > + if ((ret = draw_text(ctx, frame, frame->width, frame->height)
>> < 0))
>> > > > + return ret;
>> > >
>> > > same
>> > >
>> > > also error codes in ffmpeg are negative, this if its intended to be
>> > > as is woul introduce a positive error code
>> > >
>> > > [...]
>> > >
>> > > --
>> > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
>> 87040B0FAB
>> > >
>> > > Old school: Use the lowest level language in which you can solve the
>> > > problem
>> > > conveniently.
>> > > New school: Use the highest level language in which the latest
>> > > supercomputer
>> > > can solve the problem without the user falling asleep
>> waiting.
>> > >
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel@ffmpeg.org
>> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >
>> > >
>>
>> > vf_drawtext.c | 126 ++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++------
>> > 1 file changed, 113 insertions(+), 13 deletions(-)
>> > d9624210e5ec9554f037959b3ca6e240b76c27db
>> 0001-added-expr-evaluation-to-drawtext-fontsize.patch
>> > From 42687746fac27416c6b1d3de29142b2448bdd66e Mon Sep 17 00:00:00 2001
>> > From: Brett Harrison <brett.harrison@musicmastermind.com>
>> > Date: Fri, 26 Aug 2016 14:29:34 -0700
>> > Subject: [PATCH] added expr evaluation to drawtext - fontsize
>> >
>> > ---
>> > libavfilter/vf_drawtext.c | 126 ++++++++++++++++++++++++++++++
>> +++++++++++-----
>> > 1 file changed, 113 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
>> > index 214aef0..b345cfc 100644
>> > --- a/libavfilter/vf_drawtext.c
>> > +++ b/libavfilter/vf_drawtext.c
>> > @@ -156,7 +156,10 @@ typedef struct DrawTextContext {
>> > int max_glyph_h; ///< max glyph height
>> > int shadowx, shadowy;
>> > int borderw; ///< border width
>> > + char *fontsize_expr; ///< expression for fontsize
>> > + AVExpr *fontsize_pexpr; ///< parsed expressions for
>> fontsize
>> > unsigned int fontsize; ///< font size to use
>> > + unsigned int default_fontsize; ///< default font size to use
>> >
>> > short int draw_box; ///< draw box around text - true
>> or false
>> > int boxborderw; ///< box border width
>> > @@ -209,7 +212,7 @@ static const AVOption drawtext_options[]= {
>> > {"shadowcolor", "set shadow color", OFFSET(shadowcolor.rgba),
>> AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > {"box", "set box", OFFSET(draw_box),
>> AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS},
>> > {"boxborderw", "set box border width", OFFSET(boxborderw),
>> AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS},
>> > - {"fontsize", "set font size", OFFSET(fontsize),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS},
>> > + {"fontsize", "set font size", OFFSET(fontsize_expr),
>> AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS},
>> > {"x", "set x expression", OFFSET(x_expr),
>> AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > {"y", "set y expression", OFFSET(y_expr),
>> AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS},
>> > {"shadowx", "set shadow x offset", OFFSET(shadowx),
>> AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS},
>> > @@ -280,6 +283,7 @@ typedef struct Glyph {
>> > FT_Glyph glyph;
>> > FT_Glyph border_glyph;
>> > uint32_t code;
>> > + unsigned int fontsize;
>> > FT_Bitmap bitmap; ///< array holding bitmaps of font
>> > FT_Bitmap border_bitmap; ///< array holding bitmaps of font border
>> > FT_BBox bbox;
>> > @@ -292,7 +296,11 @@ static int glyph_cmp(const void *key, const void
>> *b)
>> > {
>> > const Glyph *a = key, *bb = b;
>> > int64_t diff = (int64_t)a->code - (int64_t)bb->code;
>> > - return diff > 0 ? 1 : diff < 0 ? -1 : 0;
>> > +
>> > + if (diff != 0)
>> > + return diff > 0 ? 1 : -1;
>> > + else
>> > + return FFDIFFSIGN((int64_t)a->fontsize,
>> (int64_t)bb->fontsize);
>> > }
>> >
>> > /**
>> > @@ -316,6 +324,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph
>> **glyph_ptr, uint32_t code)
>> > goto error;
>> > }
>> > glyph->code = code;
>> > + glyph->fontsize = s->fontsize;
>> >
>> > if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) {
>> > ret = AVERROR(EINVAL);
>> > @@ -365,6 +374,73 @@ error:
>> > return ret;
>> > }
>> >
>> > +static av_cold int set_fontsize(AVFilterContext *ctx)
>> > +{
>> > + int err;
>> > + DrawTextContext *s = ctx->priv;
>> > +
>>
>> > + if (s->face == NULL) {
>> > + av_log(ctx, AV_LOG_ERROR, "Unable to initialize font\n");
>> > + return AVERROR(EINVAL);
>>
>> How can this be NULL ?
>>
>>
>> [...]
>>
>> > @@ -1216,12 +1310,16 @@ static int draw_text(AVFilterContext *ctx,
>> AVFrame *frame,
>> > x = 0;
>> > y = 0;
>> >
>> > + if (update_fontsize(ctx) != 0)
>> > + return AVERROR(EINVAL);
>>
>> error checks should be < 0 and the error code should be forwarded
>>
>>
>> [...]
>>
>> > @@ -1352,7 +1451,8 @@ static int filter_frame(AVFilterLink *inlink,
>> AVFrame *frame)
>> > s->var_values[VAR_PICT_TYPE] = frame->pict_type;
>> > s->metadata = av_frame_get_metadata(frame);
>> >
>> > - draw_text(ctx, frame, frame->width, frame->height);
>> > + if ((ret = draw_text(ctx, frame, frame->width, frame->height)) < 0)
>> > + return ret;
>>
>> this change looks unrelated and should be in a seperate patch.
>> Also i think this leaks frame if you return here
>>
>>
>> [...]
>>
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Let us carefully observe those good qualities wherein our enemies excel us
>> and endeavor to excel them, by avoiding what is faulty, and imitating what
>> is excellent in them. -- Plutarch
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) + return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
Resurrecting this patch. On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer <michael@niedermayer.cc
wrote:
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) + return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
Pinging for comments / review... On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Resurrecting this patch.
On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) <
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote: 0)
+ return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
Any comments on this patch? ---------- Forwarded message ---------- From: Brett Harrison <brett.harrison@zyamusic.com> Date: Tue, Apr 11, 2017 at 1:37 PM Subject: Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Pinging for comments / review... On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Resurrecting this patch.
On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) <
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote: 0)
+ return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
On Sun, Apr 16, 2017 at 10:01:01PM -0700, Brett Harrison wrote:
Any comments on this patch?
---------- Forwarded message ---------- From: Brett Harrison <brett.harrison@zyamusic.com> Date: Tue, Apr 11, 2017 at 1:37 PM Subject: Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Pinging for comments / review...
On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Resurrecting this patch.
On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) <
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote: 0)
+ return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 17 deletions(-) 085506596906b7f89f46edf6d21d34374e92d994 0001-added-expr-evaluation-to-drawtext-fontsize.patch From 8647e01f8ac2cd622e0ff5c1257773cfffa01ed9 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Tue, 4 Apr 2017 15:39:06 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 108 insertions(+), 17 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 8b24f50..77209f3 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
int line_spacing; ///< lines spacing in pixels short int draw_box; ///< draw box around text - true or false @@ -211,7 +214,7 @@ static const AVOption drawtext_options[]= { {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, {"line_spacing", "set line spacing in pixels", OFFSET(line_spacing), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX,FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -281,6 +284,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -293,7 +297,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -317,6 +325,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -366,6 +375,68 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_pexpr) + return 0;
indention is inconsistent
+ + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) + return err; + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) { + if ((err = parse_fontsize(ctx)) < 0) + return err; + + size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); +
+ if (!isnan(size)) + fontsize = round(size);
round() returns a double fontsize is unsigend int. the cast can overflow
+ } + + if (fontsize == 0) + fontsize = 1;
+ + // no change + if (fontsize == s->fontsize) + return 0; + + s->fontsize = fontsize; + + return set_fontsize(ctx);
set_fontsize(ctx, fontsize); seems cleaner to me [...] thx -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On Mon, Apr 17, 2017 at 5:48 PM, Michael Niedermayer <michael@niedermayer.cc
wrote:
On Sun, Apr 16, 2017 at 10:01:01PM -0700, Brett Harrison wrote:
Any comments on this patch?
---------- Forwarded message ---------- From: Brett Harrison <brett.harrison@zyamusic.com> Date: Tue, Apr 11, 2017 at 1:37 PM Subject: Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Pinging for comments / review...
On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison < brett.harrison@zyamusic.com> wrote:
Resurrecting this patch.
On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) <
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote: 0)
+ return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ ++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 17 deletions(-) 085506596906b7f89f46edf6d21d34374e92d994 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 8647e01f8ac2cd622e0ff5c1257773cfffa01ed9 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Tue, 4 Apr 2017 15:39:06 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++------- 1 file changed, 108 insertions(+), 17 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 8b24f50..77209f3 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
int line_spacing; ///< lines spacing in pixels short int draw_box; ///< draw box around text - true or false @@ -211,7 +214,7 @@ static const AVOption drawtext_options[]= { {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, {"line_spacing", "set line spacing in pixels", OFFSET(line_spacing), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX,FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -281,6 +284,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -293,7 +297,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -317,6 +325,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -366,6 +375,68 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_pexpr) + return 0;
indention is inconsistent
+ + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) + return err; + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) { + if ((err = parse_fontsize(ctx)) < 0) + return err; + + size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); +
+ if (!isnan(size)) + fontsize = round(size);
round() returns a double fontsize is unsigend int. the cast can overflow
+ } + + if (fontsize == 0) + fontsize = 1;
+ + // no change + if (fontsize == s->fontsize) + return 0; + + s->fontsize = fontsize; + + return set_fontsize(ctx);
set_fontsize(ctx, fontsize); seems cleaner to me
[...]
thx
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Everything should be made as simple as possible, but not simpler. -- Albert Einstein
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
+ if (s->fontsize_pexpr) + return 0;
indention is inconsistent
fixed
+ if (!isnan(size)) + fontsize = round(size);
round() returns a double fontsize is unsigend int. the cast can overflow
added check for overflow and returned an error
+ return set_fontsize(ctx);
set_fontsize(ctx, fontsize); seems cleaner to me
added second parameter and set s->fontsize in the set_fontsize call
On Tue, Apr 18, 2017 at 06:20:51PM -0700, Brett Harrison wrote:
On Mon, Apr 17, 2017 at 5:48 PM, Michael Niedermayer <michael@niedermayer.cc
wrote:
On Sun, Apr 16, 2017 at 10:01:01PM -0700, Brett Harrison wrote:
Any comments on this patch?
---------- Forwarded message ---------- From: Brett Harrison <brett.harrison@zyamusic.com> Date: Tue, Apr 11, 2017 at 1:37 PM Subject: Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Pinging for comments / review...
On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison < brett.harrison@zyamusic.com> wrote:
Resurrecting this patch.
On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer < michael@niedermayer.cc> wrote:
Here are the changes requested [...] +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + av_expr_free(s->fontsize_pexpr); + s->fontsize_pexpr = NULL; + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) <
On Fri, Sep 09, 2016 at 05:26:03PM -0700, Brett Harrison wrote: 0)
+ return err; + + return 0; +}
why is av_expr_parse() not executed where the other av_expr_parse() are ?
I needed to perform av_expr_parse() during init() to resolve the default fontsize. init() is called before config_input() where the other av_expr_parse() calls are.
vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ ++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 17 deletions(-) 085506596906b7f89f46edf6d21d34374e92d994 0001-added-expr-evaluation-to- drawtext-fontsize.patch From 8647e01f8ac2cd622e0ff5c1257773cfffa01ed9 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Tue, 4 Apr 2017 15:39:06 -0700 Subject: [PATCH] added expr evaluation to drawtext - fontsize
--- libavfilter/vf_drawtext.c | 125 ++++++++++++++++++++++++++++++ +++++++++------- 1 file changed, 108 insertions(+), 17 deletions(-)
diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 8b24f50..77209f3 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -156,7 +156,10 @@ typedef struct DrawTextContext { int max_glyph_h; ///< max glyph height int shadowx, shadowy; int borderw; ///< border width + char *fontsize_expr; ///< expression for fontsize + AVExpr *fontsize_pexpr; ///< parsed expressions for fontsize unsigned int fontsize; ///< font size to use + unsigned int default_fontsize; ///< default font size to use
int line_spacing; ///< lines spacing in pixels short int draw_box; ///< draw box around text - true or false @@ -211,7 +214,7 @@ static const AVOption drawtext_options[]= { {"box", "set box", OFFSET(draw_box), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1 , FLAGS}, {"boxborderw", "set box border width", OFFSET(boxborderw), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, {"line_spacing", "set line spacing in pixels", OFFSET(line_spacing), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX,FLAGS}, - {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX , FLAGS}, + {"fontsize", "set font size", OFFSET(fontsize_expr), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX , FLAGS}, {"x", "set x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"y", "set y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str="0"}, CHAR_MIN, CHAR_MAX, FLAGS}, {"shadowx", "set shadow x offset", OFFSET(shadowx), AV_OPT_TYPE_INT, {.i64=0}, INT_MIN, INT_MAX , FLAGS}, @@ -281,6 +284,7 @@ typedef struct Glyph { FT_Glyph glyph; FT_Glyph border_glyph; uint32_t code; + unsigned int fontsize; FT_Bitmap bitmap; ///< array holding bitmaps of font FT_Bitmap border_bitmap; ///< array holding bitmaps of font border FT_BBox bbox; @@ -293,7 +297,11 @@ static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; - return diff > 0 ? 1 : diff < 0 ? -1 : 0; + + if (diff != 0) + return diff > 0 ? 1 : -1; + else + return FFDIFFSIGN((int64_t)a->fontsize, (int64_t)bb->fontsize); }
/** @@ -317,6 +325,7 @@ static int load_glyph(AVFilterContext *ctx, Glyph **glyph_ptr, uint32_t code) goto error; } glyph->code = code; + glyph->fontsize = s->fontsize;
if (FT_Get_Glyph(s->face->glyph, &glyph->glyph)) { ret = AVERROR(EINVAL); @@ -366,6 +375,68 @@ error: return ret; }
+static av_cold int set_fontsize(AVFilterContext *ctx) +{ + int err; + DrawTextContext *s = ctx->priv; + + if ((err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize))) { + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %d pixels: %s\n", + s->fontsize, FT_ERRMSG(err)); + return AVERROR(EINVAL); + } + + return 0; +} + +static av_cold int parse_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + int err; + + if (s->fontsize_pexpr) + return 0;
indention is inconsistent
+ + if (s->fontsize_expr == NULL) + return AVERROR(EINVAL); + + if ((err = av_expr_parse(&s->fontsize_pexpr, s->fontsize_expr, var_names, + NULL, NULL, fun2_names, fun2, 0, ctx)) < 0) + return err; + + return 0; +} + +static av_cold int update_fontsize(AVFilterContext *ctx) +{ + DrawTextContext *s = ctx->priv; + unsigned int fontsize = s->default_fontsize; + int err; + double size; + + // if no fontsize specified use the default + if (s->fontsize_expr != NULL) { + if ((err = parse_fontsize(ctx)) < 0) + return err; + + size = av_expr_eval(s->fontsize_pexpr, s->var_values, &s->prng); +
+ if (!isnan(size)) + fontsize = round(size);
round() returns a double fontsize is unsigend int. the cast can overflow
+ } + + if (fontsize == 0) + fontsize = 1;
+ + // no change + if (fontsize == s->fontsize) + return 0; + + s->fontsize = fontsize; + + return set_fontsize(ctx);
set_fontsize(ctx, fontsize); seems cleaner to me
[...]
thx
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Everything should be made as simple as possible, but not simpler. -- Albert Einstein
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
+ if (s->fontsize_pexpr) + return 0;
indention is inconsistent
fixed
+ if (!isnan(size)) + fontsize = round(size);
round() returns a double fontsize is unsigend int. the cast can overflow
added check for overflow and returned an error
+ return set_fontsize(ctx);
set_fontsize(ctx, fontsize); seems cleaner to me
added second parameter and set s->fontsize in the set_fontsize call
vf_drawtext.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 116 insertions(+), 17 deletions(-) 3adf54dd36434e02e3b70e63ebd77147febf6133 0001-added-expr-evaluation-to-drawtext-fontsize.patch From c43567075c55defd30cf1717589a7c715a27de31 Mon Sep 17 00:00:00 2001 From: Brett Harrison <brett.harrison@musicmastermind.com> Date: Tue, 18 Apr 2017 18:15:39 -0700 Subject: [PATCH] added expr evaluation to drawtext fontsize
applied can you add a fate test for this feature ? (is probably not completely easy due to non bitexactness) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
Hi, On Fri, Aug 26, 2016 at 10:37 PM, Brett Harrison <brett.harrison@zyamusic.com> wrote:
Allows expr evaluation in the fontsize parameter for drawtext.
Thanks for making this. I regularly use drawtext to create watermarked/timecoded access copies in a moving image archive and I have to use workarounds in scripts in order to make relative font sizes. I'm not sure if this kind of 'thanks' email is against ffmpeg-devel rules as I didn't see it listed here: https://ffmpeg.org/contact.html#MailingLists -Kieran.
participants (5)
-
Brett Harrison -
Kieran O Leary -
Michael Niedermayer -
Moritz Barsnick -
Nicolas George