Discussion:
[FFmpeg-devel] [PATCH V1 1/2] lavfi/vf_scale_vaapi: add scaling mode setting support.
Jun Zhao
2018-12-06 10:39:57 UTC
Permalink
before this change, scale_vaapi hard coding the scaling mode, add a
new option "mode" to setting the scaling mode, it can be use to change
scaling algorithm for performance/quality trade off.

Signed-off-by: Jun Zhao <***@gmail.com>
---
libavfilter/vf_scale_vaapi.c | 33 ++++++++++++++++++++++++++++++---
1 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
index d6529d5..ad222e6 100644
--- a/libavfilter/vf_scale_vaapi.c
+++ b/libavfilter/vf_scale_vaapi.c
@@ -35,10 +35,26 @@ typedef struct ScaleVAAPIContext {

char *output_format_string;

+ int mode;
+
char *w_expr; // width expression string
char *h_expr; // height expression string
} ScaleVAAPIContext;

+static const char *scale_vaapi_mode_name(int mode)
+{
+ switch (mode) {
+#define D(name) case VA_FILTER_SCALING_ ## name: return #name
+ D(DEFAULT);
+ D(FAST);
+ D(HQ);
+#undef D
+ default:
+ return "Invalid";
+ }
+}
+
+
static int scale_vaapi_config_output(AVFilterLink *outlink)
{
AVFilterLink *inlink = outlink->src->inputs[0];
@@ -70,6 +86,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
AVFilterContext *avctx = inlink->dst;
AVFilterLink *outlink = avctx->outputs[0];
VAAPIVPPContext *vpp_ctx = avctx->priv;
+ ScaleVAAPIContext *ctx = avctx->priv;
AVFrame *output_frame = NULL;
VASurfaceID input_surface, output_surface;
VAProcPipelineParameterBuffer params;
@@ -119,7 +136,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
params.output_color_standard = params.surface_color_standard;

params.pipeline_flags = 0;
- params.filter_flags = VA_FILTER_SCALING_HQ;
+ params.filter_flags |= ctx->mode;

err = ff_vaapi_vpp_render_picture(avctx, &params, output_surface);
if (err < 0)
@@ -131,9 +148,10 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)

av_frame_free(&input_frame);

- av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64").\n",
+ av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64"), mode: %s.\n",
av_get_pix_fmt_name(output_frame->format),
- output_frame->width, output_frame->height, output_frame->pts);
+ output_frame->width, output_frame->height, output_frame->pts,
+ scale_vaapi_mode_name(ctx->mode));

return ff_filter_frame(outlink, output_frame);

@@ -174,6 +192,15 @@ static const AVOption scale_vaapi_options[] = {
OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, .flags = FLAGS },
{ "format", "Output video format (software format of hardware frames)",
OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS },
+ { "mode", "Scaling mode",
+ OFFSET(mode), AV_OPT_TYPE_INT, { .i64 = VA_FILTER_SCALING_HQ },
+ 0, VA_FILTER_SCALING_NL_ANAMORPHIC, FLAGS, "mode" },
+ { "default", "Use the default (depend on the driver) scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_DEFAULT }, 0, 0, FLAGS, "mode" },
+ { "fast", "Use fast scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_FAST }, 0, 0, FLAGS, "mode" },
+ { "hq", "Use high quality scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_HQ }, 0, 0, FLAGS, "mode" },
{ NULL },
};
--
1.7.1
Jun Zhao
2018-12-06 10:39:58 UTC
Permalink
Fix slice number check logic. Only when the user setting slices
number more than the driver constraints dump the rounded up warning
message.

Signed-off-by: Jun Zhao <***@gmail.com>
---
libavcodec/vaapi_encode.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index eda8a36..3c8a33d 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1572,7 +1572,7 @@ static av_cold int vaapi_encode_init_slice_structure(AVCodecContext *avctx)
return AVERROR(EINVAL);
}

- if (ctx->nb_slices > avctx->slices) {
+ if (ctx->nb_slices < avctx->slices) {
av_log(avctx, AV_LOG_WARNING, "Slice count rounded up to "
"%d (from %d) due to driver constraints on slice "
"structure.\n", ctx->nb_slices, avctx->slices);
--
1.7.1
m***@gmail.com
2018-12-10 01:56:41 UTC
Permalink
Post by Jun Zhao
Fix slice number check logic. Only when the user setting slices
number more than the driver constraints dump the rounded up warning
message.
---
libavcodec/vaapi_encode.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index eda8a36..3c8a33d 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1572,7 +1572,7 @@ static av_cold int vaapi_encode_init_slice_structure(AVCodecContext *avctx)
, tnereturn AVERROR(EINVAL);
}
- if (ctx->nb_slices > avctx->slices) {
+ if (ctx->nb_slices < avctx->slices) {
av_log(avctx, AV_LOG_WARNING, "Slice count rounded up to "
"%d (from %d) due to driver constraints on slice "
"structure.\n", ctx->nb_slices, avctx->slices);
No? The existing check looks right to me - we warn if the number got increased over what the user specified due to driver constraints. (We don't allow it to decrease unless the supplied number is larger than the number of rows, which gets warned about above there.)
- Mark
I think avctx->slices is user setting value and ctx->nb_slices is the
driver constraints, so I think we need to check avctx->slices >
ctx->nb_slices, then give a rounded up warning. :)
Mark Thompson
2018-12-10 22:59:28 UTC
Permalink
Post by m***@gmail.com
Post by Jun Zhao
Fix slice number check logic. Only when the user setting slices
number more than the driver constraints dump the rounded up warning
message.
---
libavcodec/vaapi_encode.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index eda8a36..3c8a33d 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1572,7 +1572,7 @@ static av_cold int vaapi_encode_init_slice_structure(AVCodecContext *avctx)
, tnereturn AVERROR(EINVAL);
}
- if (ctx->nb_slices > avctx->slices) {
+ if (ctx->nb_slices < avctx->slices) {
av_log(avctx, AV_LOG_WARNING, "Slice count rounded up to "
"%d (from %d) due to driver constraints on slice "
"structure.\n", ctx->nb_slices, avctx->slices);
No? The existing check looks right to me - we warn if the number got increased over what the user specified due to driver constraints. (We don't allow it to decrease unless the supplied number is larger than the number of rows, which gets warned about above there.)
I think avctx->slices is user setting value and ctx->nb_slices is the
driver constraints,
This is correct.
Post by m***@gmail.com
so I think we need to check avctx->slices >
ctx->nb_slices, then give a rounded up warning. :)
avctx->slice > ctx->nb_slices would mean that the driver constraint number is /less/ than number the user set; that would be rounding down. This case isn't allowed by the code above except for the degenerate example where the user setting is greater than the number of rows, which is already warned about.

What we are actually checking for here is the driver constraint number being /greater/ than the number the user set (that is, that ctx->nb_slices > avctx->slices), and then warning the user that their supplied number has been rounded up to the next available possibility and that the output stream will therefore contain more slices than expected.

- Mark

Mark Thompson
2018-12-09 18:25:45 UTC
Permalink
Post by Jun Zhao
before this change, scale_vaapi hard coding the scaling mode, add a
new option "mode" to setting the scaling mode, it can be use to change
scaling algorithm for performance/quality trade off.
---
libavfilter/vf_scale_vaapi.c | 33 ++++++++++++++++++++++++++++++---
1 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
index d6529d5..ad222e6 100644
--- a/libavfilter/vf_scale_vaapi.c
+++ b/libavfilter/vf_scale_vaapi.c
@@ -35,10 +35,26 @@ typedef struct ScaleVAAPIContext {
char *output_format_string;
+ int mode;
+
char *w_expr; // width expression string
char *h_expr; // height expression string
} ScaleVAAPIContext;
+static const char *scale_vaapi_mode_name(int mode)
+{
+ switch (mode) {
+#define D(name) case VA_FILTER_SCALING_ ## name: return #name
+ D(DEFAULT);
+ D(FAST);
+ D(HQ);
+#undef D
+ return "Invalid";
+ }
+}
+
+
static int scale_vaapi_config_output(AVFilterLink *outlink)
{
AVFilterLink *inlink = outlink->src->inputs[0];
@@ -70,6 +86,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
AVFilterContext *avctx = inlink->dst;
AVFilterLink *outlink = avctx->outputs[0];
VAAPIVPPContext *vpp_ctx = avctx->priv;
+ ScaleVAAPIContext *ctx = avctx->priv;
AVFrame *output_frame = NULL;
VASurfaceID input_surface, output_surface;
VAProcPipelineParameterBuffer params;
@@ -119,7 +136,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
params.output_color_standard = params.surface_color_standard;
params.pipeline_flags = 0;
- params.filter_flags = VA_FILTER_SCALING_HQ;
+ params.filter_flags |= ctx->mode;
"=" would feel safer - "|=" implies something else might have been setting it as well.
Post by Jun Zhao
err = ff_vaapi_vpp_render_picture(avctx, &params, output_surface);
if (err < 0)
@@ -131,9 +148,10 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
av_frame_free(&input_frame);
- av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64").\n",
+ av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64"), mode: %s.\n",
av_get_pix_fmt_name(output_frame->format),
- output_frame->width, output_frame->height, output_frame->pts);
+ output_frame->width, output_frame->height, output_frame->pts,
+ scale_vaapi_mode_name(ctx->mode));
return ff_filter_frame(outlink, output_frame);
@@ -174,6 +192,15 @@ static const AVOption scale_vaapi_options[] = {
OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, .flags = FLAGS },
{ "format", "Output video format (software format of hardware frames)",
OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS },
+ { "mode", "Scaling mode",
+ OFFSET(mode), AV_OPT_TYPE_INT, { .i64 = VA_FILTER_SCALING_HQ },
+ 0, VA_FILTER_SCALING_NL_ANAMORPHIC, FLAGS, "mode" },
NL_ANAMORPHIC mentioned in this limit but not offered as an option?
Post by Jun Zhao
+ { "default", "Use the default (depend on the driver) scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_DEFAULT }, 0, 0, FLAGS, "mode" },
+ { "fast", "Use fast scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_FAST }, 0, 0, FLAGS, "mode" },
+ { "hq", "Use high quality scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_HQ }, 0, 0, FLAGS, "mode" },
{ NULL },
};
LGTM in any case.

(Ack on keeping the HQ default - IIRC the reason for choosing HQ as the mode when this was written was that the default/fast mode was not faster and had much worse quality on some of the older Intel platforms (I would guess Bay Trail based on what I probably had available at the time). Might be worth investigating further if you have such machines available to test.)

Thanks,

- Mark
m***@gmail.com
2018-12-10 06:52:27 UTC
Permalink
now,
Post by Mark Thompson
Post by Jun Zhao
before this change, scale_vaapi hard coding the scaling mode, add a
new option "mode" to setting the scaling mode, it can be use to change
scaling algorithm for performance/quality trade off.
---
libavfilter/vf_scale_vaapi.c | 33 ++++++++++++++++++++++++++++++---
1 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
index d6529d5..ad222e6 100644
--- a/libavfilter/vf_scale_vaapi.c
+++ b/libavfilter/vf_scale_vaapi.c
@@ -35,10 +35,26 @@ typedef struct ScaleVAAPIContext {
char *output_format_string;
+ int mode;
+
char *w_expr; // width expression string
char *h_expr; // height expression string
} ScaleVAAPIContext;
+static const char *scale_vaapi_mode_name(int mode)
+{
+ switch (mode) {
+#define D(name) case VA_FILTER_SCALING_ ## name: return #name
+ D(DEFAULT);
+ D(FAST);
+ D(HQ);
+#undef D
+ return "Invalid";
+ }
+}
+
+
static int scale_vaapi_config_output(AVFilterLink *outlink)
{
AVFilterLink *inlink = outlink->src->inputs[0];
@@ -70,6 +86,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
AVFilterContext *avctx = inlink->dst;
AVFilterLink *outlink = avctx->outputs[0];
VAAPIVPPContext *vpp_ctx = avctx->priv;
+ ScaleVAAPIContext *ctx = avctx->priv;
AVFrame *output_frame = NULL;
VASurfaceID input_surface, output_surface;
VAProcPipelineParameterBuffer params;
@@ -119,7 +136,7 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
params.output_color_standard = params. surface_color_standard;
params.pipeline_flags = 0;
- params.filter_flags = VA_FILTER_SCALING_HQ;
+ params.filter_flags |= ctx->mode;
"=" would feel safer - "|=" implies something else might have been setting it as well.
Will keep the old way
Post by Mark Thompson
Post by Jun Zhao
err = ff_vaapi_vpp_render_picture(avctx, &params, output_surface);
if (err < 0)
@@ -131,9 +148,10 @@ static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame *input_frame)
av_frame_free(&input_frame);
- av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64").\n",
+ av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64"), mode: %s.\n",
av_get_pix_fmt_name(output_frame->format),
- output_frame->width, output_frame->height, output_frame->pts);
+ output_frame->width, output_frame->height, output_frame->pts,
+ scale_vaapi_mode_name(ctx->mode));
return ff_filter_frame(outlink, output_frame);
@@ -174,6 +192,15 @@ static const AVOption scale_vaapi_options[] = {
OFFSET(h_expr), AV_OPT_TYPE_STRING, {.str = "ih"}, .flags = FLAGS },
{ "format", "Output video format (software format of hardware frames)",
OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS },
+ { "mode", "Scaling mode",
+ OFFSET(mode), AV_OPT_TYPE_INT, { .i64 = VA_FILTER_SCALING_HQ },
+ 0, VA_FILTER_SCALING_NL_ANAMORPHIC, FLAGS, "mode" },
NL_ANAMORPHIC mentioned in this limit but not offered as an option?
NL_ANAMORPHIC never be support in the driver (both i965 and iHD), and
will be obsolete from libva, so I will change this part and remove
NL_ANAMORPHIC
Post by Mark Thompson
Post by Jun Zhao
+ { "default", "Use the default (depend on the driver) scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_DEFAULT }, 0, 0, FLAGS, "mode" },
+ { "fast", "Use fast scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_FAST }, 0, 0, FLAGS, "mode" },
+ { "hq", "Use high quality scaling algorithm",
+ 0, AV_OPT_TYPE_CONST, { .i64 = VA_FILTER_SCALING_HQ }, 0, 0, FLAGS, "mode" },
{ NULL },
};
LGTM in any case.
(Ack on keeping the HQ default - IIRC the reason for choosing HQ as the mode when this was written was that the default/fast mode was not faster and had much worse quality on some of the older Intel platforms (I would guess Bay Trail based on what I probably had available at the time). Might be worth investigating further if you have such machines available to test.)
I don't have Bay Trail (just Skylark and Kabylake), will investigate
this issue if I found this model. Tks.
Post by Mark Thompson
Thanks,
- Mark
Loading...