Any feedback on the most recent patch?
On Fri, Sep 9, 2016 at 5:26 PM, Brett Harrison <brett.harri...@zyamusic.com>
wrote:
> 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
>>
>>
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
