[FFmpeg-devel] [PATCH] avfilter/scale: allow dynamic output via expressions
Gyan
ffmpeg at gyani.pro
Mon Nov 18 07:03:38 EET 2019
On 16-11-2019 06:29 pm, Michael Niedermayer wrote:
> 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
The scale.c function uses av_expr_parse_and_eval, which means I can't
parse expressions in vf_scale.c. I can't check expressions at all. I'll
have to create new constant and value arrays for sw scale. To
accommodate the different filters using this code, I'll need to add many
conditional codepaths. It'll be harder and uglier to maintain.
What do you suggest?
Gyan
More information about the ffmpeg-devel
mailing list