Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Pinging for comments / review...

On Tue, Apr 4, 2017 at 3:45 PM, Brett Harrison 
wrote:

> Resurrecting this patch.
>
> On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer <
> mich...@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(>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.
>
>


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Resurrecting this patch.

On Thu, Sep 15, 2016 at 3:20 AM, Michael Niedermayer  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(>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.


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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(>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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Any feedback on the most recent patch?

On Fri, Sep 9, 2016 at 5:26 PM, Brett Harrison 
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
>> 
>> > 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 
>> 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:
>> > > > >
>> > > > > :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 
>> > > > 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 

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Here are the changes requested

On Thu, Sep 8, 2016 at 8:50 AM, Michael Niedermayer 
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
> 
> > 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 
> 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:
> > > > >
> > > > > :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 
> > > > 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[]= {
> > > > 

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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 
> 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  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:
> > > >
> > > > :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 
> > > 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},  

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

This patch addresses your concerns.

On Fri, Sep 2, 2016 at 5:05 PM, Michael Niedermayer 
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  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:
> > >
> > > :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 
> > 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},
> >  

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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  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:
> >
> > :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 
> 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},
>  

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

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  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:
>
> :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
>
>


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Nicolas George]

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:

: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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Moritz Barsnick]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Nicolas George]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Moritz Barsnick]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Moritz Barsnick]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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 
> wrote:
> 
> > Any feedback on this newest patch?
> >
> > On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison <
> > brett.harri...@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  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 
> 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 

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

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 
wrote:

> Any feedback on this newest patch?
>
> On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison <
> brett.harri...@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  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
>>>
>>>
>>
>


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Any feedback on this newest patch?

On Tue, Aug 30, 2016 at 2:17 PM, Brett Harrison  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  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
>>
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Kieran O Leary]

Hi,

On Fri, Aug 26, 2016 at 10:37 PM, Brett Harrison
 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

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  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
>
>


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Nicolas George]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

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  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 
> 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 
> > 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 

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Michael Niedermayer]

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  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 
> 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)) {
>  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))) {
> +  

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

Fixed patch based on comments.

This time attaching patch file.

On Sat, Aug 27, 2016 at 6:21 AM, Moritz Barsnick  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
>


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

Re: [FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Moritz Barsnick]

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

[FFmpeg-devel] [PATCH] added expr evaluation to drawtext - fontsize

[Brett Harrison]

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)) {
 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, >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(>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