Re: [FFmpeg-devel] [PATCH v2] avfilter/scale: refactor common code for scaling height/width expressions

2017-02-01 Thread Mark Thompson 
On 01/02/17 23:12, Aman Gupta wrote:
> From: Aman Gupta 
> 
> Implements support for height/width expressions in vf_scale_vaapi,
> by refactoring common code into a new libavfilter/scale.c

Some more minor points:

> ---
>  libavfilter/Makefile |   8 +--
>  libavfilter/scale.c  | 152 
> +++
>  libavfilter/scale.h  |  28 
>  libavfilter/vf_scale.c   | 109 +++
>  libavfilter/vf_scale_npp.c   |  94 +++---
>  libavfilter/vf_scale_vaapi.c |  19 --
>  6 files changed, 217 insertions(+), 193 deletions(-)
>  create mode 100644 libavfilter/scale.c
>  create mode 100644 libavfilter/scale.h
> 
> ...
> +int ff_scale_eval_dimensions(void *log_ctx,
> +const char *w_expr, const char *h_expr,
> +AVFilterLink *inlink, AVFilterLink *outlink,
> +int *ret_w, int *ret_h)
> +{
> +const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> +const AVPixFmtDescriptor *out_desc = 
> av_pix_fmt_desc_get(outlink->format);
> +const char *expr;
> +int w, h;
> +int factor_w, factor_h;
> +int eval_w, eval_h;
> +int ret;
> +double var_values[VARS_NB], res;
> +
> +var_values[VAR_PI]= M_PI;
> +var_values[VAR_PHI]   = M_PHI;
> +var_values[VAR_E] = M_E;
> +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;
> +var_values[VAR_A] = (double) inlink->w / inlink->h;
> +var_values[VAR_SAR]   = inlink->sample_aspect_ratio.num ?
> +(double) inlink->sample_aspect_ratio.num / 
> inlink->sample_aspect_ratio.den : 1;
> +var_values[VAR_DAR]   = var_values[VAR_A] * var_values[VAR_SAR];
> +var_values[VAR_HSUB]  = 1 << desc->log2_chroma_w;
> +var_values[VAR_VSUB]  = 1 << desc->log2_chroma_h;
> +var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
> +var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
> +
> +/* evaluate width and height */
> +av_expr_parse_and_eval(, (expr = w_expr),
> +   var_names, var_values,
> +   NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
> +eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res;
> +
> +if ((ret = av_expr_parse_and_eval(, (expr = h_expr),
> +  var_names, var_values,
> +  NULL, NULL, NULL, NULL, NULL, 0, 
> log_ctx)) < 0)
> +goto fail;
> +eval_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 = w_expr),
> +  var_names, var_values,
> +  NULL, NULL, NULL, NULL, NULL, 0, 
> log_ctx)) < 0)
> +goto fail;
> +eval_w = res;
> +
> +w = eval_w;
> +h = eval_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)
> +eval_w = eval_h = 0;
> +
> +if (!(w = eval_w))
> +w = inlink->w;
> +if (!(h = eval_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;
> +
> +*ret_w = w;
> +*ret_h = h;
> +
> +return 0;
> +
> +fail:
> +av_log(NULL, AV_LOG_ERROR,

Having passed the logging context, you should use it here.

> +   "Error when evaluating the expression '%s'.\n"
> +   "Maybe the expression for out_w:'%s' or for out_h:'%s' is 
> self-referencing.\n",
> +   expr, w_expr, h_expr);
> +return ret;
> +}
> ...
> @@ -359,64 +332,19 @@ static int nppscale_config_props(AVFilterLink *outlink)
>  {
>  AVFilterContext *ctx = outlink->src;
>  AVFilterLink *inlink = outlink->src->inputs[0];
> -NPPScaleContext  *s = ctx->priv;
> -int64_t w, h;
> -double var_values[VARS_NB], res;
> -char *expr;
> +NPPScaleContext *s = ctx->priv;
> +int w, h;
>  int ret;
>  
> -var_values[VAR_PI]= M_PI;
> -var_values[VAR_PHI]   = M_PHI;
> -var_values[VAR_E] = M_E;
> -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] =

[FFmpeg-devel] [PATCH v2] avfilter/scale: refactor common code for scaling height/width expressions

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

Implements support for height/width expressions in vf_scale_vaapi,
by refactoring common code into a new libavfilter/scale.c
---
 libavfilter/Makefile |   8 +--
 libavfilter/scale.c  | 152 +++
 libavfilter/scale.h  |  28 
 libavfilter/vf_scale.c   | 109 +++
 libavfilter/vf_scale_npp.c   |  94 +++---
 libavfilter/vf_scale_vaapi.c |  19 --
 6 files changed, 217 insertions(+), 193 deletions(-)
 create mode 100644 libavfilter/scale.c
 create mode 100644 libavfilter/scale.h

diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index 68a94be..3231f08 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -257,10 +257,10 @@ OBJS-$(CONFIG_REPEATFIELDS_FILTER)   += 
vf_repeatfields.o
 OBJS-$(CONFIG_REVERSE_FILTER)+= f_reverse.o
 OBJS-$(CONFIG_ROTATE_FILTER) += vf_rotate.o
 OBJS-$(CONFIG_SAB_FILTER)+= vf_sab.o
-OBJS-$(CONFIG_SCALE_FILTER)  += vf_scale.o
-OBJS-$(CONFIG_SCALE_NPP_FILTER)  += vf_scale_npp.o
-OBJS-$(CONFIG_SCALE_VAAPI_FILTER)+= vf_scale_vaapi.o
-OBJS-$(CONFIG_SCALE2REF_FILTER)  += vf_scale.o
+OBJS-$(CONFIG_SCALE_FILTER)  += vf_scale.o scale.o
+OBJS-$(CONFIG_SCALE_NPP_FILTER)  += vf_scale_npp.o scale.o
+OBJS-$(CONFIG_SCALE_VAAPI_FILTER)+= vf_scale_vaapi.o scale.o
+OBJS-$(CONFIG_SCALE2REF_FILTER)  += vf_scale.o scale.o
 OBJS-$(CONFIG_SELECT_FILTER) += f_select.o
 OBJS-$(CONFIG_SELECTIVECOLOR_FILTER) += vf_selectivecolor.o
 OBJS-$(CONFIG_SENDCMD_FILTER)+= f_sendcmd.o
diff --git a/libavfilter/scale.c b/libavfilter/scale.c
new file mode 100644
index 000..caa4680
--- /dev/null
+++ b/libavfilter/scale.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (c) 2007 Bobby Bingham
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include 
+#include "scale.h"
+#include "libavutil/eval.h"
+#include "libavutil/mathematics.h"
+#include "libavutil/pixdesc.h"
+
+static const char *const var_names[] = {
+"PI",
+"PHI",
+"E",
+"in_w",   "iw",
+"in_h",   "ih",
+"out_w",  "ow",
+"out_h",  "oh",
+"a",
+"sar",
+"dar",
+"hsub",
+"vsub",
+"ohsub",
+"ovsub",
+NULL
+};
+
+enum var_name {
+VAR_PI,
+VAR_PHI,
+VAR_E,
+VAR_IN_W,   VAR_IW,
+VAR_IN_H,   VAR_IH,
+VAR_OUT_W,  VAR_OW,
+VAR_OUT_H,  VAR_OH,
+VAR_A,
+VAR_SAR,
+VAR_DAR,
+VAR_HSUB,
+VAR_VSUB,
+VAR_OHSUB,
+VAR_OVSUB,
+VARS_NB
+};
+
+int ff_scale_eval_dimensions(void *log_ctx,
+const char *w_expr, const char *h_expr,
+AVFilterLink *inlink, AVFilterLink *outlink,
+int *ret_w, int *ret_h)
+{
+const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
+const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
+const char *expr;
+int w, h;
+int factor_w, factor_h;
+int eval_w, eval_h;
+int ret;
+double var_values[VARS_NB], res;
+
+var_values[VAR_PI]= M_PI;
+var_values[VAR_PHI]   = M_PHI;
+var_values[VAR_E] = M_E;
+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;
+var_values[VAR_A] = (double) inlink->w / inlink->h;
+var_values[VAR_SAR]   = inlink->sample_aspect_ratio.num ?
+(double) inlink->sample_aspect_ratio.num / 
inlink->sample_aspect_ratio.den : 1;
+var_values[VAR_DAR]   = var_values[VAR_A] * var_values[VAR_SAR];
+var_values[VAR_HSUB]  = 1 << desc->log2_chroma_w;
+var_values[VAR_VSUB]  = 1 << desc->log2_chroma_h;
+var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
+var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
+
+/* evaluate width and height */
+av_expr_parse_and_eval(, (expr = w_expr),
+   var_names, var_values,
+   NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
+eval_w = var_values[VAR_OUT_W] =