[FFmpeg-devel] [PATCH] avfilter/scale: allow dynamic output via expressions

Michael Niedermayer michael at niedermayer.cc
Sat Nov 16 14:59:23 EET 2019


On Fri, Nov 15, 2019 at 11:10:26AM +0530, Gyan wrote:
> 
> 
> On 15-11-2019 04:01 am, Michael Niedermayer wrote:
> >On Thu, Nov 14, 2019 at 11:12:23PM +0530, Gyan wrote:
> >>
> >>On 14-11-2019 01:12 am, Michael Niedermayer wrote:
> >>>Moving and changing code at the same time makes it hard to see th difference.
> >>>Idealy all code moves should be seperate from changes to the code.
> >>>
> >>>also more generally, spliting this patch up would simpify review
> >>Split into two. First patch mostly moves code and keeps existing
> >>functionality. 2nd patch introduces new features and requires the new eval
> >>function.
> >>
> >>Thanks,
> >>Gyan
> >>  Makefile   |    4 -
> >>  scale.c    |   72 +---------------------
> >>  vf_scale.c |  192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  3 files changed, 196 insertions(+), 72 deletions(-)
> >>77579fdbd7add3be08bada5ee401df41f60ea236  v2-0001-avfilter-scale-shift-ff_scale_eval_dimensions-inl.patch
> >> From 359f538703865c8ebeda16b5d1846d2cf1cf9c4d Mon Sep 17 00:00:00 2001
> >>From: Gyan Doshi <ffmpeg at gyani.pro>
> >>Date: Thu, 14 Nov 2019 21:08:32 +0530
> >>Subject: [PATCH v2 1/2] avfilter/scale: shift ff_scale_eval_dimensions inline
> >>
> >>This is a perfunctory change in preparation of adding
> >>direct animation support to scale and scale2ref filters
> >>---
> >>  libavfilter/Makefile   |   4 +-
> >>  libavfilter/scale.c    |  72 +---------------
> >>  libavfilter/vf_scale.c | 192 ++++++++++++++++++++++++++++++++++++++++-
> >>  3 files changed, 196 insertions(+), 72 deletions(-)
> >>
> >>diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> >>index fce930360d..f1f6994574 100644
> >>--- a/libavfilter/Makefile
> >>+++ b/libavfilter/Makefile
> >>@@ -358,12 +358,12 @@ OBJS-$(CONFIG_ROBERTS_OPENCL_FILTER)         += vf_convolution_opencl.o opencl.o
> >>                                                  opencl/convolution.o
> >>  OBJS-$(CONFIG_ROTATE_FILTER)                 += vf_rotate.o
> >>  OBJS-$(CONFIG_SAB_FILTER)                    += vf_sab.o
> >>-OBJS-$(CONFIG_SCALE_FILTER)                  += vf_scale.o scale.o
> >>+OBJS-$(CONFIG_SCALE_FILTER)                  += vf_scale.o
> >>  OBJS-$(CONFIG_SCALE_CUDA_FILTER)             += vf_scale_cuda.o vf_scale_cuda.ptx.o
> >>  OBJS-$(CONFIG_SCALE_NPP_FILTER)              += vf_scale_npp.o scale.o
> >>  OBJS-$(CONFIG_SCALE_QSV_FILTER)              += vf_scale_qsv.o
> >>  OBJS-$(CONFIG_SCALE_VAAPI_FILTER)            += vf_scale_vaapi.o scale.o vaapi_vpp.o
> >>-OBJS-$(CONFIG_SCALE2REF_FILTER)              += vf_scale.o scale.o
> >>+OBJS-$(CONFIG_SCALE2REF_FILTER)              += vf_scale.o
> >>  OBJS-$(CONFIG_SCROLL_FILTER)                 += vf_scroll.o
> >>  OBJS-$(CONFIG_SELECT_FILTER)                 += f_select.o
> >>  OBJS-$(CONFIG_SELECTIVECOLOR_FILTER)         += vf_selectivecolor.o
> >>diff --git a/libavfilter/scale.c b/libavfilter/scale.c
> >>index eaee95fac6..668aa04622 100644
> >>--- a/libavfilter/scale.c
> >>+++ b/libavfilter/scale.c
> >>@@ -60,49 +60,6 @@ enum var_name {
> >>      VARS_NB
> >>  };
> >>-/**
> >>- * This must be kept in sync with var_names so that it is always a
> >>- * complete list of var_names with the scale2ref specific names
> >>- * appended. scale2ref values must appear in the order they appear
> >>- * in the var_name_scale2ref enum but also be below all of the
> >>- * non-scale2ref specific values.
> >>- */
> >>-static const char *const var_names_scale2ref[] = {
> >>-    "PI",
> >>-    "PHI",
> >>-    "E",
> >>-    "in_w",   "iw",
> >>-    "in_h",   "ih",
> >>-    "out_w",  "ow",
> >>-    "out_h",  "oh",
> >>-    "a",
> >>-    "sar",
> >>-    "dar",
> >>-    "hsub",
> >>-    "vsub",
> >>-    "ohsub",
> >>-    "ovsub",
> >>-    "main_w",
> >>-    "main_h",
> >>-    "main_a",
> >>-    "main_sar",
> >>-    "main_dar", "mdar",
> >>-    "main_hsub",
> >>-    "main_vsub",
> >>-    NULL
> >>-};
> >>-
> >>-enum var_name_scale2ref {
> >>-    VAR_S2R_MAIN_W,
> >>-    VAR_S2R_MAIN_H,
> >>-    VAR_S2R_MAIN_A,
> >>-    VAR_S2R_MAIN_SAR,
> >>-    VAR_S2R_MAIN_DAR, VAR_S2R_MDAR,
> >>-    VAR_S2R_MAIN_HSUB,
> >>-    VAR_S2R_MAIN_VSUB,
> >>-    VARS_S2R_NB
> >>-};
> >>-
> >>  int ff_scale_eval_dimensions(void *log_ctx,
> >>      const char *w_expr, const char *h_expr,
> >>      AVFilterLink *inlink, AVFilterLink *outlink,
> >>@@ -115,16 +72,7 @@ int ff_scale_eval_dimensions(void *log_ctx,
> >>      int factor_w, factor_h;
> >>      int eval_w, eval_h;
> >>      int ret;
> >>-    const char scale2ref = outlink->src->nb_inputs == 2 && outlink->src->inputs[1] == inlink;
> >>-    double var_values[VARS_NB + VARS_S2R_NB], res;
> >>-    const AVPixFmtDescriptor *main_desc;
> >>-    const AVFilterLink *main_link;
> >>-    const char *const *names = scale2ref ? var_names_scale2ref : var_names;
> >>-
> >>-    if (scale2ref) {
> >>-        main_link = outlink->src->inputs[0];
> >>-        main_desc = av_pix_fmt_desc_get(main_link->format);
> >>-    }
> >>+    double var_values[VARS_NB], res;
> >>      var_values[VAR_PI]    = M_PI;
> >>      var_values[VAR_PHI]   = M_PHI;
> >>@@ -142,32 +90,20 @@ int ff_scale_eval_dimensions(void *log_ctx,
> >>      var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w;
> >>      var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h;
> >>-    if (scale2ref) {
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_W] = main_link->w;
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_H] = main_link->h;
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
> >>-            (double) main_link->sample_aspect_ratio.num / main_link->sample_aspect_ratio.den : 1;
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_DAR] = var_values[VARS_NB + VAR_S2R_MDAR] =
> >>-            var_values[VARS_NB + VAR_S2R_MAIN_A] * var_values[VARS_NB + VAR_S2R_MAIN_SAR];
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
> >>-        var_values[VARS_NB + VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
> >>-    }
> >>-
> >>      /* evaluate width and height */
> >>      av_expr_parse_and_eval(&res, (expr = w_expr),
> >>-                           names, var_values,
> >>+                           var_names, var_values,
> >>                             NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
> >>      eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
> >>      if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr),
> >>-                                      names, var_values,
> >>+                                      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] = (int) res == 0 ? inlink->h : (int) res;
> >>      /* evaluate again the width, as it may depend on the output height */
> >>      if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr),
> >>-                                      names, var_values,
> >>+                                      var_names, var_values,
> >>                                        NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
> >>          goto fail;
> >>      eval_w = (int) res == 0 ? inlink->w : (int) res;
> >>diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> >>index 41ddec7661..e7d52faccc 100644
> >>--- a/libavfilter/vf_scale.c
> >>+++ b/libavfilter/vf_scale.c
> >>@@ -29,9 +29,9 @@
> >>  #include "avfilter.h"
> >>  #include "formats.h"
> >>  #include "internal.h"
> >>-#include "scale.h"
> >>  #include "video.h"
> >>  #include "libavutil/avstring.h"
> >>+#include "libavutil/eval.h"
> >>  #include "libavutil/internal.h"
> >>  #include "libavutil/mathematics.h"
> >>  #include "libavutil/opt.h"
> >>@@ -41,6 +41,85 @@
> >>  #include "libavutil/avassert.h"
> >>  #include "libswscale/swscale.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
> >
> >
> >[...]
> >
> >>+static int 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;
> >>+    const char scale2ref = outlink->src->nb_inputs == 2 && outlink->src->inputs[1] == inlink;
> >>+    double var_values[VARS_NB + VARS_S2R_NB], res;
> >>+    const AVPixFmtDescriptor *main_desc;
> >>+    const AVFilterLink *main_link;
> >>+    const char *const *names = scale2ref ? var_names_scale2ref : var_names;
> >>+
> >>+    if (scale2ref) {
> >>+        main_link = outlink->src->inputs[0];
> >>+        main_desc = av_pix_fmt_desc_get(main_link->format);
> >>+    }
> >>+
> >>+    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;
> >>+
> >>+    if (scale2ref) {
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_W] = main_link->w;
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_H] = main_link->h;
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_A] = (double) main_link->w / main_link->h;
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_SAR] = main_link->sample_aspect_ratio.num ?
> >>+            (double) main_link->sample_aspect_ratio.num / main_link->sample_aspect_ratio.den : 1;
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_DAR] = var_values[VARS_NB + VAR_S2R_MDAR] =
> >>+            var_values[VARS_NB + VAR_S2R_MAIN_A] * var_values[VARS_NB + VAR_S2R_MAIN_SAR];
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_HSUB] = 1 << main_desc->log2_chroma_w;
> >>+        var_values[VARS_NB + VAR_S2R_MAIN_VSUB] = 1 << main_desc->log2_chroma_h;
> >>+    }
> >>+
> >>+    /* evaluate width and height */
> >>+    av_expr_parse_and_eval(&res, (expr = w_expr),
> >>+                           names, var_values,
> >>+                           NULL, NULL, NULL, NULL, NULL, 0, log_ctx);
> >>+    eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res;
> >>+
> >>+    if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr),
> >>+                                      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] = (int) res == 0 ? inlink->h : (int) res;
> >>+    /* evaluate again the width, as it may depend on the output height */
> >>+    if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr),
> >>+                                      names, var_values,
> >>+                                      NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0)
> >>+        goto fail;
> >>+    eval_w = (int) res == 0 ? inlink->w : (int) 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) {
> >>+        w = inlink->w;
> >>+        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(log_ctx, AV_LOG_ERROR,
> >>+           "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;
> >>+}
> >duplicated code
> >it would be more ideal if no code duplication is required
> 
> This function is modified in next patch. This patch is only for convenient
> review, as you requested.
> 
> The existing scale.c function is still used by three hw filters., but it
> can't be used for the animation feature in the next patch since scale.c uses
> av_expr_parse_and_eval.
> For animation, I need to retain the parsed expression, which means storing
> it the filter's private context..etc.

I still think this can be done with less duplication

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20191116/191d43bd/attachment.sig>


More information about the ffmpeg-devel mailing list