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 <ffm...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
