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 <geo...@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.harri...@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.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
