Here are the changes requested On Thu, Sep 8, 2016 at 8:50 AM, Michael Niedermayer <mich...@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
> <mich...@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 <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: 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.harri...@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
>
>
0001-added-expr-evaluation-to-drawtext-fontsize.patch
Description: Binary data
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
