On Tue, Jan 31, 2017 at 12:26 PM, Mark Thompson <s...@jkqxz.net> wrote:
> On 31/01/17 19:14, Aman Gupta wrote:
> > From: Aman Gupta <a...@tmm1.net>
> >
> > 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 <va/va_vpp.h>
> >
> > #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(&res, (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(&res, (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(&res, (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 },
> > { "h", "Output video height",
> > - OFFSET(output_height), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX,
> .flags = FLAGS },
> > + OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = FLAGS },
>
> Should these have default values? "iw", "ih", maybe. (It currently
> segfaults in the evaluation if either of them are not set.)
>
> > { "format", "Output video format (software format of hardware
> frames)",
> > OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS
> },
> > { NULL },
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
