[FFmpeg-devel] avfilter/scale_vaapi: add support for basic height/width expressions

From: Aman Gupta <aman@tmm1.net>

Copied directly from vf_scale.c.

Implements the same expression logic, however not all the variables were copied over.
This patch was sufficient for my particular use-case `scale_vaapi=-2:min(ih\,720)`,
but perhaps it makes sense to add support for the remaining variables
and pull out shared code into a new vf_scale_common.c?
---
 libavfilter/vf_scale_vaapi.c | 98 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 3 deletions(-)

On 31/01/17 19:14, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> Copied directly from vf_scale.c.
> 
> Implements the same expression logic, however not all the variables were copied over.
> This patch was sufficient for my particular use-case `scale_vaapi=-2:min(ih\,720)`,
> but perhaps it makes sense to add support for the remaining variables
> and pull out shared code into a new vf_scale_common.c?

I would prefer the code fragment not to be copied again, yes.

(Implementing this and removing the duplication between scale, scale_npp, scale_qsv and scale_vaapi has been on my to-maybe-do-at-some-point list for quite a while, but I've never actually had a use-case for it to push me into actually doing it :)

If you can't be bothered, then the patch looks mostly sensible to me (some issues below, I think mainly coming from it being copied).

> ---
>  libavfilter/vf_scale_vaapi.c | 98 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
> index d1cb246..0d1e1b3 100644
> --- a/libavfilter/vf_scale_vaapi.c
> +++ b/libavfilter/vf_scale_vaapi.c
> @@ -22,6 +22,7 @@
>  #include <va/va_vpp.h>
>  
>  #include "libavutil/avassert.h"
> +#include "libavutil/eval.h"
>  #include "libavutil/hwcontext.h"
>  #include "libavutil/hwcontext_vaapi.h"
>  #include "libavutil/mem.h"
> @@ -32,6 +33,22 @@
>  #include "formats.h"
>  #include "internal.h"
>  
> +static const char *const var_names[] = {
> +    "in_w",   "iw",
> +    "in_h",   "ih",
> +    "out_w",  "ow",
> +    "out_h",  "oh",
> +    NULL
> +};
> +
> +enum var_name {
> +    VAR_IN_W,   VAR_IW,
> +    VAR_IN_H,   VAR_IH,
> +    VAR_OUT_W,  VAR_OW,
> +    VAR_OUT_H,  VAR_OH,
> +    VARS_NB
> +};
> +
>  typedef struct ScaleVAAPIContext {
>      const AVClass *class;
>  
> @@ -50,9 +67,21 @@ typedef struct ScaleVAAPIContext {
>  
>      char *output_format_string;
>      enum AVPixelFormat output_format;
> +
> +    /**
> +     * New dimensions. Special values are:
> +     *   0 = original width/height
> +     *  -1 = keep original aspect
> +     *  -N = try to keep aspect but make sure it is divisible by N
> +     */
> +    int w, h;

Why do these exist in addition to output_width and output_height?  They only seem to be used as temporaries duplicating w and h in scale_vaapi_config_output().

> +
> +    char *w_expr;               ///< width  expression string
> +    char *h_expr;               ///< height expression string
> +
> +    /* computed width/height */
>      int output_width;
>      int output_height;
> -
>  } ScaleVAAPIContext;
>  
>  
> @@ -118,6 +147,14 @@ static int scale_vaapi_config_output(AVFilterLink *outlink)
>      VAStatus vas;
>      int err, i;
>  
> +    AVFilterLink *inlink = outlink->src->inputs[0];
> +    ScaleVAAPIContext *scale = ctx;

Just use the ctx variable?

> +    int64_t w, h;
> +    double var_values[VARS_NB], res;
> +    char *expr;

This variable is write-only.

> +    int ret;

Just use err (because of the difference you aren't setting the correct error return if one of the evaluation operations fails).

> +    int factor_w, factor_h;
> +
>      scale_vaapi_pipeline_uninit(ctx);
>  
>      ctx->device_ref = av_buffer_ref(ctx->input_frames->device_ref);
> @@ -162,6 +199,61 @@ static int scale_vaapi_config_output(AVFilterLink *outlink)
>          }
>      }
>  
> +    var_values[VAR_IN_W]  = var_values[VAR_IW] = inlink->w;
> +    var_values[VAR_IN_H]  = var_values[VAR_IH] = inlink->h;
> +    var_values[VAR_OUT_W] = var_values[VAR_OW] = NAN;
> +    var_values[VAR_OUT_H] = var_values[VAR_OH] = NAN;
> +
> +    /* evaluate width and height */
> +    av_expr_parse_and_eval(&res, (expr = scale->w_expr),
> +                           var_names, var_values,
> +                           NULL, NULL, NULL, NULL, NULL, 0, ctx);
> +    scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
> +    if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr),
> +                                      var_names, var_values,
> +                                      NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
> +        goto fail;
> +    scale->h = var_values[VAR_OUT_H] = var_values[VAR_OH] = res;
> +    /* evaluate again the width, as it may depend on the output height */
> +    if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr),
> +                                      var_names, var_values,
> +                                      NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
> +        goto fail;
> +    scale->w = res;
> +
> +    w = scale->w;
> +    h = scale->h;
> +
> +    /* Check if it is requested that the result has to be divisible by a some
> +     * factor (w or h = -n with n being the factor). */
> +    factor_w = 1;
> +    factor_h = 1;
> +    if (w < -1) {
> +        factor_w = -w;
> +    }
> +    if (h < -1) {
> +        factor_h = -h;
> +    }
> +
> +    if (w < 0 && h < 0)
> +        scale->w = scale->h = 0;
> +
> +    if (!(w = scale->w))
> +        w = inlink->w;
> +    if (!(h = scale->h))
> +        h = inlink->h;
> +
> +    /* Make sure that the result is divisible by the factor we determined
> +     * earlier. If no factor was set, it is nothing will happen as the default
> +     * factor is 1 */
> +    if (w < 0)
> +        w = av_rescale(h, inlink->w, inlink->h * factor_w) * factor_w;
> +    if (h < 0)
> +        h = av_rescale(w, inlink->h, inlink->w * factor_h) * factor_h;
> +
> +    ctx->output_width = w;
> +    ctx->output_height = h;
> +
>      if (ctx->output_width  < constraints->min_width  ||
>          ctx->output_height < constraints->min_height ||
>          ctx->output_width  > constraints->max_width  ||
> @@ -414,9 +506,9 @@ static av_cold void scale_vaapi_uninit(AVFilterContext *avctx)
>  #define FLAGS (AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM)
>  static const AVOption scale_vaapi_options[] = {
>      { "w", "Output video width",
> -      OFFSET(output_width),  AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, .flags = FLAGS },
> +      OFFSET(w_expr), AV_OPT_TYPE_STRING, .flags = FLAGS },
>      { "h", "Output video height",
> -      OFFSET(output_height), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, .flags = FLAGS },
> +      OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = FLAGS },

Should these have default values?   "iw", "ih", maybe.  (It currently segfaults in the evaluation if either of them are not set.)

>      { "format", "Output video format (software format of hardware frames)",
>        OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS },
>      { NULL },
>

On Tue, Jan 31, 2017 at 12:26 PM, Mark Thompson <sw@jkqxz.net> wrote:

> On 31/01/17 19:14, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > Copied directly from vf_scale.c.
> >
> > Implements the same expression logic, however not all the variables were
> copied over.
> > This patch was sufficient for my particular use-case
> `scale_vaapi=-2:min(ih\,720)`,
> > but perhaps it makes sense to add support for the remaining variables
> > and pull out shared code into a new vf_scale_common.c?
>
> I would prefer the code fragment not to be copied again, yes.
>
> (Implementing this and removing the duplication between scale, scale_npp,
> scale_qsv and scale_vaapi has been on my to-maybe-do-at-some-point list for
> quite a while, but I've never actually had a use-case for it to push me
> into actually doing it :)
>

Ok, I'll see if I can refactor things to avoid duplication.

You mentioned scale_qsv, and I see the git commit in the history that added
it. But vf_scale_qsv.c is no where to be found on master. Do you know what
happened to it? I'd like to implement the same logic there too if I'm
refactoring things..


>
> If you can't be bothered, then the patch looks mostly sensible to me (some
> issues below, I think mainly coming from it being copied).
>
> > ---
> >  libavfilter/vf_scale_vaapi.c | 98 ++++++++++++++++++++++++++++++
> ++++++++++++--
> >  1 file changed, 95 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
> > index d1cb246..0d1e1b3 100644
> > --- a/libavfilter/vf_scale_vaapi.c
> > +++ b/libavfilter/vf_scale_vaapi.c
> > @@ -22,6 +22,7 @@
> >  #include <va/va_vpp.h>
> >
> >  #include "libavutil/avassert.h"
> > +#include "libavutil/eval.h"
> >  #include "libavutil/hwcontext.h"
> >  #include "libavutil/hwcontext_vaapi.h"
> >  #include "libavutil/mem.h"
> > @@ -32,6 +33,22 @@
> >  #include "formats.h"
> >  #include "internal.h"
> >
> > +static const char *const var_names[] = {
> > +    "in_w",   "iw",
> > +    "in_h",   "ih",
> > +    "out_w",  "ow",
> > +    "out_h",  "oh",
> > +    NULL
> > +};
> > +
> > +enum var_name {
> > +    VAR_IN_W,   VAR_IW,
> > +    VAR_IN_H,   VAR_IH,
> > +    VAR_OUT_W,  VAR_OW,
> > +    VAR_OUT_H,  VAR_OH,
> > +    VARS_NB
> > +};
> > +
> >  typedef struct ScaleVAAPIContext {
> >      const AVClass *class;
> >
> > @@ -50,9 +67,21 @@ typedef struct ScaleVAAPIContext {
> >
> >      char *output_format_string;
> >      enum AVPixelFormat output_format;
> > +
> > +    /**
> > +     * New dimensions. Special values are:
> > +     *   0 = original width/height
> > +     *  -1 = keep original aspect
> > +     *  -N = try to keep aspect but make sure it is divisible by N
> > +     */
> > +    int w, h;
>
> Why do these exist in addition to output_width and output_height?  They
> only seem to be used as temporaries duplicating w and h in
> scale_vaapi_config_output().
>
> > +
> > +    char *w_expr;               ///< width  expression string
> > +    char *h_expr;               ///< height expression string
> > +
> > +    /* computed width/height */
> >      int output_width;
> >      int output_height;
> > -
> >  } ScaleVAAPIContext;
> >
> >
> > @@ -118,6 +147,14 @@ static int scale_vaapi_config_output(AVFilterLink
> *outlink)
> >      VAStatus vas;
> >      int err, i;
> >
> > +    AVFilterLink *inlink = outlink->src->inputs[0];
> > +    ScaleVAAPIContext *scale = ctx;
>
> Just use the ctx variable?
>
> > +    int64_t w, h;
> > +    double var_values[VARS_NB], res;
> > +    char *expr;
>
> This variable is write-only.
>
> > +    int ret;
>
> Just use err (because of the difference you aren't setting the correct
> error return if one of the evaluation operations fails).
>
> > +    int factor_w, factor_h;
> > +
> >      scale_vaapi_pipeline_uninit(ctx);
> >
> >      ctx->device_ref = av_buffer_ref(ctx->input_frames->device_ref);
> > @@ -162,6 +199,61 @@ static int scale_vaapi_config_output(AVFilterLink
> *outlink)
> >          }
> >      }
> >
> > +    var_values[VAR_IN_W]  = var_values[VAR_IW] = inlink->w;
> > +    var_values[VAR_IN_H]  = var_values[VAR_IH] = inlink->h;
> > +    var_values[VAR_OUT_W] = var_values[VAR_OW] = NAN;
> > +    var_values[VAR_OUT_H] = var_values[VAR_OH] = NAN;
> > +
> > +    /* evaluate width and height */
> > +    av_expr_parse_and_eval(&res, (expr = scale->w_expr),
> > +                           var_names, var_values,
> > +                           NULL, NULL, NULL, NULL, NULL, 0, ctx);
> > +    scale->w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
> > +    if ((ret = av_expr_parse_and_eval(&res, (expr = scale->h_expr),
> > +                                      var_names, var_values,
> > +                                      NULL, NULL, NULL, NULL, NULL, 0,
> ctx)) < 0)
> > +        goto fail;
> > +    scale->h = var_values[VAR_OUT_H] = var_values[VAR_OH] = res;
> > +    /* evaluate again the width, as it may depend on the output height
> */
> > +    if ((ret = av_expr_parse_and_eval(&res, (expr = scale->w_expr),
> > +                                      var_names, var_values,
> > +                                      NULL, NULL, NULL, NULL, NULL, 0,
> ctx)) < 0)
> > +        goto fail;
> > +    scale->w = res;
> > +
> > +    w = scale->w;
> > +    h = scale->h;
> > +
> > +    /* Check if it is requested that the result has to be divisible by
> a some
> > +     * factor (w or h = -n with n being the factor). */
> > +    factor_w = 1;
> > +    factor_h = 1;
> > +    if (w < -1) {
> > +        factor_w = -w;
> > +    }
> > +    if (h < -1) {
> > +        factor_h = -h;
> > +    }
> > +
> > +    if (w < 0 && h < 0)
> > +        scale->w = scale->h = 0;
> > +
> > +    if (!(w = scale->w))
> > +        w = inlink->w;
> > +    if (!(h = scale->h))
> > +        h = inlink->h;
> > +
> > +    /* Make sure that the result is divisible by the factor we
> determined
> > +     * earlier. If no factor was set, it is nothing will happen as the
> default
> > +     * factor is 1 */
> > +    if (w < 0)
> > +        w = av_rescale(h, inlink->w, inlink->h * factor_w) * factor_w;
> > +    if (h < 0)
> > +        h = av_rescale(w, inlink->h, inlink->w * factor_h) * factor_h;
> > +
> > +    ctx->output_width = w;
> > +    ctx->output_height = h;
> > +
> >      if (ctx->output_width  < constraints->min_width  ||
> >          ctx->output_height < constraints->min_height ||
> >          ctx->output_width  > constraints->max_width  ||
> > @@ -414,9 +506,9 @@ static av_cold void scale_vaapi_uninit(AVFilterContext
> *avctx)
> >  #define FLAGS (AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM)
> >  static const AVOption scale_vaapi_options[] = {
> >      { "w", "Output video width",
> > -      OFFSET(output_width),  AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX,
> .flags = FLAGS },
> > +      OFFSET(w_expr), AV_OPT_TYPE_STRING, .flags = FLAGS },
> >      { "h", "Output video height",
> > -      OFFSET(output_height), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX,
> .flags = FLAGS },
> > +      OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = FLAGS },
>
> Should these have default values?   "iw", "ih", maybe.  (It currently
> segfaults in the evaluation if either of them are not set.)
>
> >      { "format", "Output video format (software format of hardware
> frames)",
> >        OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS
> },
> >      { NULL },
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On 31/01/17 21:00, Aman Gupta wrote:
> On Tue, Jan 31, 2017 at 12:26 PM, Mark Thompson <sw@jkqxz.net> wrote:
> 
>> On 31/01/17 19:14, Aman Gupta wrote:
>>> From: Aman Gupta <aman@tmm1.net>
>>>
>>> Copied directly from vf_scale.c.
>>>
>>> Implements the same expression logic, however not all the variables were
>> copied over.
>>> This patch was sufficient for my particular use-case
>> `scale_vaapi=-2:min(ih\,720)`,
>>> but perhaps it makes sense to add support for the remaining variables
>>> and pull out shared code into a new vf_scale_common.c?
>>
>> I would prefer the code fragment not to be copied again, yes.
>>
>> (Implementing this and removing the duplication between scale, scale_npp,
>> scale_qsv and scale_vaapi has been on my to-maybe-do-at-some-point list for
>> quite a while, but I've never actually had a use-case for it to push me
>> into actually doing it :)
>>
> 
> Ok, I'll see if I can refactor things to avoid duplication.

Great, thank you :)

> You mentioned scale_qsv, and I see the git commit in the history that added
> it. But vf_scale_qsv.c is no where to be found on master. Do you know what
> happened to it? I'd like to implement the same logic there too if I'm
> refactoring things..

Right, yes, it's not merged yet - it's present in the libav tree and branch here but skipped from ffmpeg master.

Currently that merge is loosely blocked behind the ffmpeg.c filter setup being unsuitable for hardware transcode (the same issue that forces the 'format=vaapi|nv12,hwupload' hack for VAAPI transcode) - the patches were skipped at the time due to merging issues and it has not been revisited fully yet.  Now, the filter does not actually depend on that because it can work standalone in lavfi, so I would be happy to just merge it without it being usable in ffmpeg.c so that this could continue.  Alternatively, since all of the hardware filters were merged from libav, you could work there instead.

(Or feel free to ignore that and just deal with the code you care about directly.  If you only do some of them then hopefully I or someone else would be able to sort out the others at some point.)

- Mark