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

2017-01-31 Thread Mark Thompson 
On 31/01/17 21:00, Aman Gupta wrote:
> On Tue, Jan 31, 2017 at 12:26 PM, Mark Thompson  wrote:
> 
>> On 31/01/17 19:14, Aman Gupta wrote:
>>> From: Aman Gupta 
>>>
>>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-31 Thread Aman Gupta 
On Tue, Jan 31, 2017 at 12:26 PM, Mark Thompson  wrote:

> On 31/01/17 19:14, Aman Gupta wrote:
> > From: Aman Gupta 
> >
> > 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 
> >
> >  #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(, (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(, (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(,

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

2017-01-31 Thread Mark Thompson 
On 31/01/17 19:14, Aman Gupta wrote:
> From: Aman Gupta 
> 
> 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 
>  
>  #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(, (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(, (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(, (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)
> +

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

2017-01-31 Thread Aman Gupta 
From: Aman Gupta 

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

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 
 
 #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;
+
+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;
+int64_t w, h;
+double var_values[VARS_NB], res;
+char *expr;
+int ret;
+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(, (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(, (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(, (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 },
 {