[FFmpeg-devel] [PATCH V6] libavfilter: add transpose_vaapi filter
Zhou, Zachary
zachary.zhou at intel.com
Mon Dec 24 09:02:58 EET 2018
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Friday, December 21, 2018 5:46 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH V6] libavfilter: add transpose_vaapi filter
>
> On 18/12/2018 09:54, Zachary Zhou wrote:
> > Swap width and height when do clock/cclock rotation Add
> > reveral/reveral_flip options
> >
> > ffmpeg -hwaccel vaapi -vaapi_device /dev/dri/renderD128
> > -hwaccel_output_format vaapi -i input.264 -vf "transpose_vaapi=clock_flip"
> > -c:v h264_vaapi output.h264
> >
> > Signed-off-by: Zachary Zhou <zachary.zhou at intel.com>
> > ---
> > configure | 2 +
> > libavfilter/Makefile | 1 +
> > libavfilter/allfilters.c | 1 +
> > libavfilter/vf_transpose_vaapi.c | 360
> > +++++++++++++++++++++++++++++++
> > 4 files changed, 364 insertions(+)
> > create mode 100644 libavfilter/vf_transpose_vaapi.c
> >
> > ...
> > diff --git a/libavfilter/vf_transpose_vaapi.c
> > b/libavfilter/vf_transpose_vaapi.c
> > new file mode 100644
> > index 0000000000..a2bf245da9
> > --- /dev/null
> > +++ b/libavfilter/vf_transpose_vaapi.c
> > @@ -0,0 +1,360 @@
> > +/*
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > +02110-1301 USA */ #include <string.h>
> > +
> > +#include "libavutil/avassert.h"
> > +#include "libavutil/mem.h"
> > +#include "libavutil/opt.h"
> > +#include "libavutil/pixdesc.h"
> > +
> > +#include "avfilter.h"
> > +#include "formats.h"
> > +#include "internal.h"
> > +#include "vaapi_vpp.h"
> > +
> > +typedef enum {
> > + TRANSPOSE_PT_TYPE_NONE,
> > + TRANSPOSE_PT_TYPE_LANDSCAPE,
> > + TRANSPOSE_PT_TYPE_PORTRAIT,
> > +} PassthroughType;
> > +
> > +enum TransposeDir {
> > + TRANSPOSE_CCLOCK_FLIP,
> > + TRANSPOSE_CLOCK,
> > + TRANSPOSE_CCLOCK,
> > + TRANSPOSE_CLOCK_FLIP,
> > + TRANSPOSE_REVERAL,
> > + TRANSPOSE_REVERAL_FLIP,
>
> "REVERAL"? I guess that's meant to be "REVERSAL". I'm not convinced it's
> quite the right word, though. REVERSAL_FLIP is standalone as hflip - if you
> included vflip as well (the missing case from the eight possible transforms,
> excluding identity) then that name might be better?
Yes it should be "REVERSAL".
I added 180 degrees, just want to say VAAPI can easy do 180 agree in one time, will better than twice call in software solution.
if remove "REVERSAL" options will make transpose vaapi has exactly same options with other transpose filters, if add another two options (rotation, mirror) may best use VAAPI functions (covering all rotations and mirrors). which one you prefer ?
From my view, just remove "REVERSAL" options may make it simple.
>
> Also, please use the enums in transpose.h directly rather than duplicating
> them.
Got it will reuse those enums.
>
> > +};
> > +
> > +typedef struct TransposeVAAPIContext {
> > + VAAPIVPPContext vpp_ctx; // must be the first field
> > + int passthrough; // PassthroughType, landscape passthrough mode
> enabled
> > + int dir; // TransposeDir
> > +
> > + int rotation_state;
> > + int mirror_state;
> > +} TransposeVAAPIContext;
> > +
> > +static int transpose_vaapi_build_filter_params(AVFilterContext
> > +*avctx) {
> > + VAAPIVPPContext *vpp_ctx = avctx->priv;
> > + TransposeVAAPIContext *ctx = avctx->priv;
> > + VAStatus vas;
> > + int support_flag;
> > + VAProcPipelineCaps pipeline_caps;
> > +
> > + memset(&pipeline_caps, 0, sizeof(pipeline_caps));
> > + vas = vaQueryVideoProcPipelineCaps(vpp_ctx->hwctx->display,
> > + vpp_ctx->va_context,
> > + NULL, 0,
> > + &pipeline_caps);
> > + if (vas != VA_STATUS_SUCCESS) {
> > + av_log(avctx, AV_LOG_ERROR, "Failed to query pipeline "
> > + "caps: %d (%s).\n", vas, vaErrorStr(vas));
> > + return AVERROR(EIO);
> > + }
> > +
> > + if (!pipeline_caps.rotation_flags) {
>
> This condition doesn't guarantee that all possibly rotations are supported -
> <https://github.com/intel/libva/blob/master/va/va_vpp.h#L611-L612>.
I think I did such check in end of this function.
>
> The mirror flags also need the same treatment.
Yes, mirror flags should be checked. I filed a bug to iHD driver.
https://github.com/intel/media-driver/issues/382
would you like me to resend the patch after the bug fixed, or I send a separate patch for the patch later ?
>
> > + av_log(avctx, AV_LOG_ERROR, "VAAPI driver doesn't support
> transpose\n");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + switch (ctx->dir) {
> > + case TRANSPOSE_CCLOCK_FLIP:
> > + ctx->rotation_state = VA_ROTATION_270;
> > + ctx->mirror_state = VA_MIRROR_VERTICAL;
> > + break;
> > + case TRANSPOSE_CLOCK:
> > + ctx->rotation_state = VA_ROTATION_90;
> > + ctx->mirror_state = VA_MIRROR_NONE;
> > + break;
> > + case TRANSPOSE_CCLOCK:
> > + ctx->rotation_state = VA_ROTATION_270;
> > + ctx->mirror_state = VA_MIRROR_NONE;
> > + break;
> > + case TRANSPOSE_CLOCK_FLIP:
> > + ctx->rotation_state = VA_ROTATION_90;
> > + ctx->mirror_state = VA_MIRROR_VERTICAL;
> > + break;
> > + case TRANSPOSE_REVERAL:
> > + ctx->rotation_state = VA_ROTATION_180;
> > + ctx->mirror_state = VA_MIRROR_NONE;
> > + break;
> > + case TRANSPOSE_REVERAL_FLIP:
> > + ctx->rotation_state = VA_ROTATION_180;
> > + ctx->mirror_state = VA_MIRROR_VERTICAL;
> > + break;
> > + default:
> > + av_log(avctx, AV_LOG_ERROR, "Failed to set direction to %d\n", ctx-
> >dir);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + support_flag = pipeline_caps.rotation_flags & (1 << ctx->rotation_state);
> > + if (!support_flag) {
> > + av_log(avctx, AV_LOG_ERROR, "VAAPI driver doesn't support %d\n",
> > + ctx->rotation_state);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int transpose_vaapi_filter_frame(AVFilterLink *inlink, AVFrame
> > +*input_frame) {
> > + AVFilterContext *avctx = inlink->dst;
> > + AVFilterLink *outlink = avctx->outputs[0];
> > + VAAPIVPPContext *vpp_ctx = avctx->priv;
> > + TransposeVAAPIContext *ctx = avctx->priv;
> > + AVFrame *output_frame = NULL;
> > + VASurfaceID input_surface, output_surface;
> > + VARectangle input_region, output_region;
> > +
> > + VAProcPipelineParameterBuffer params;
> > + int err;
> > +
> > + if (ctx->passthrough)
> > + return ff_filter_frame(outlink, input_frame);
> > +
> > + av_log(avctx, AV_LOG_DEBUG, "Filter input: %s, %ux%u (%"PRId64").\n",
> > + av_get_pix_fmt_name(input_frame->format),
> > + input_frame->width, input_frame->height,
> > + input_frame->pts);
> > +
> > + if (vpp_ctx->va_context == VA_INVALID_ID)
> > + return AVERROR(EINVAL);
> > +
> > + input_surface = (VASurfaceID)(uintptr_t)input_frame->data[3];
> > + av_log(avctx, AV_LOG_DEBUG, "Using surface %#x for transpose vpp
> input.\n",
> > + input_surface);
> > +
> > + output_frame = ff_get_video_buffer(outlink, vpp_ctx->output_width,
> > + vpp_ctx->output_height);
> > + if (!output_frame) {
> > + err = AVERROR(ENOMEM);
> > + goto fail;
> > + }
> > +
> > + output_surface = (VASurfaceID)(uintptr_t)output_frame->data[3];
> > + av_log(avctx, AV_LOG_DEBUG, "Using surface %#x for transpose vpp
> output.\n",
> > + output_surface);
> > + memset(¶ms, 0, sizeof(params));
> > + input_region = (VARectangle) {
> > + .x = 0,
> > + .y = 0,
> > + .width = input_frame->width,
> > + .height = input_frame->height,
> > + };
> > +
> > + output_region = (VARectangle) {
> > + .x = 0,
> > + .y = 0,
> > + .width = output_frame->width,
> > + .height = output_frame->height,
> > + };
> > +
> > + switch (ctx->rotation_state) {
> > + case VA_ROTATION_NONE:
> > + case VA_ROTATION_90:
> > + case VA_ROTATION_180:
> > + case VA_ROTATION_270:
> > + params.rotation_state = ctx->rotation_state;
> > + break;
> > + default:
> > + av_log(avctx, AV_LOG_ERROR, "VAAPI doesn't support rotation %d\n",
> > + ctx->rotation_state);
>
> This case...
>
> > + err = AVERROR(EINVAL);
> > + goto fail;
> > + }
> > +
> > + switch (ctx->mirror_state) {
> > + case VA_MIRROR_HORIZONTAL:
> > + case VA_MIRROR_VERTICAL:
> > + case VA_MIRROR_NONE:
> > + params.mirror_state = ctx->mirror_state;
> > + break;
> > + default:
> > + av_log(avctx, AV_LOG_ERROR, "VAAPI doesn't support
> mirroring %d\n",
> > + ctx->mirror_state);
>
> ... and this case can't ever be hit, can they? (They are only ever set to these
> values in build_filter_params().) I wouldn't bother checking, but if you do
> then an assert would be best - EINVAL indicates that the user passed some
> invalid option, which isn't the case here.
Yes, you are right, they can't be hit.
I am changing it to add av_assert0(0);
>
> > + err = AVERROR(EINVAL);
> > + goto fail;
> > + }
> > +
> > + if (vpp_ctx->nb_filter_buffers) {
> > + params.filters = &vpp_ctx->filter_buffers[0];
> > + params.num_filters = vpp_ctx->nb_filter_buffers;
> > + }
> > +
> > + params.surface = input_surface;
> > + params.surface_region = &input_region;
> > + params.surface_color_standard =
> > + ff_vaapi_vpp_colour_standard(input_frame->colorspace);
> > +
> > + params.output_region = &output_region; #define BG_BLACK
> > +0xff000000
> > + params.output_background_color = BG_BLACK; #undef BG_BLACK
>
> I'm not sure the inline constant adds anything. All the other VAAPI filters have
> it too, so maybe a separate patch to add the constant to vaapi_vpp.h
> (VAAPI_VPP_BACKGROUND_BLACK?) and use it everywhere would make
> sense?
I am changing it back, and try to add a comments in the end
params.output_background_color = 0xff000000; //Black in ARGB format
And I think it is good to add the constant to vaapi_vpp.h(VAAPI_VPP_BACKGROUND_BLACK),
because I think people usually don't change this value, and it may more understandable.
I am happy to do the change if you think we need do it.
>
> > + params.output_color_standard = params.surface_color_standard;
> > +
> > + params.pipeline_flags = 0;
> > + params.filter_flags = VA_FRAME_PICTURE;
> > +
> > + err = ff_vaapi_vpp_render_picture(avctx, ¶ms, output_surface);
> > + if (err < 0)
> > + goto fail;
> > +
> > + err = av_frame_copy_props(output_frame, input_frame);
> > + if (err < 0)
> > + goto fail;
> > + av_frame_free(&input_frame);
> > +
> > + av_log(avctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u
> (%"PRId64").\n",
> > + av_get_pix_fmt_name(output_frame->format),
> > + output_frame->width, output_frame->height,
> > + output_frame->pts);
> > +
> > + return ff_filter_frame(outlink, output_frame);
> > +
> > +fail:
> > + av_frame_free(&input_frame);
> > + av_frame_free(&output_frame);
> > + return err;
> > +}
> > +
> > +static av_cold int transpose_vaapi_init(AVFilterContext *avctx) {
> > + VAAPIVPPContext *vpp_ctx = avctx->priv;
> > +
> > + ff_vaapi_vpp_ctx_init(avctx);
> > + vpp_ctx->pipeline_uninit = ff_vaapi_vpp_pipeline_uninit;
> > + vpp_ctx->build_filter_params = transpose_vaapi_build_filter_params;
> > + vpp_ctx->output_format = AV_PIX_FMT_NONE;
> > +
> > + return 0;
> > +}
> > +
> > +static void transpose_vaapi_vpp_ctx_uninit(AVFilterContext *avctx) {
> > + return ff_vaapi_vpp_ctx_uninit(avctx); }
> > +
> > +static int transpose_vaapi_vpp_query_formats(AVFilterContext *avctx)
> > +{
> > + return ff_vaapi_vpp_query_formats(avctx);
> > +}
> > +
> > +static int transpose_vaapi_vpp_config_input(AVFilterLink *inlink) {
> > + return ff_vaapi_vpp_config_input(inlink);
> > +}
> > +
> > +static int transpose_vaapi_vpp_config_output(AVFilterLink *outlink) {
> > + AVFilterContext *avctx = outlink->src;
> > + VAAPIVPPContext *vpp_ctx = avctx->priv;
> > + TransposeVAAPIContext *ctx = avctx->priv;
> > + AVFilterLink *inlink = avctx->inputs[0];
> > +
> > + if ((inlink->w >= inlink->h && ctx->passthrough ==
> TRANSPOSE_PT_TYPE_LANDSCAPE) ||
> > + (inlink->w <= inlink->h && ctx->passthrough ==
> TRANSPOSE_PT_TYPE_PORTRAIT)) {
> > + av_log(avctx, AV_LOG_VERBOSE,
> > + "w:%d h:%d -> w:%d h:%d (passthrough mode)\n",
> > + inlink->w, inlink->h, inlink->w, inlink->h);
> > + return ff_vaapi_vpp_config_output(outlink);
> > + }
> > +
> > + ctx->passthrough = TRANSPOSE_PT_TYPE_NONE;
> > +
> > + switch (ctx->dir) {
> > + case TRANSPOSE_CCLOCK_FLIP:
> > + case TRANSPOSE_CCLOCK:
> > + case TRANSPOSE_CLOCK:
> > + case TRANSPOSE_CLOCK_FLIP:
> > + vpp_ctx->output_width = avctx->inputs[0]->h;
> > + vpp_ctx->output_height = avctx->inputs[0]->w;
> > + av_log(avctx, AV_LOG_DEBUG, "swap width and height for clock/cclock
> rotation\n");
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return ff_vaapi_vpp_config_output(outlink);
> > +}
> > +
> > +static AVFrame *get_video_buffer(AVFilterLink *inlink, int w, int h)
> > +{
> > + TransposeVAAPIContext *ctx = inlink->dst->priv;
> > +
> > + return ctx->passthrough ?
> > + ff_null_get_video_buffer(inlink, w, h) :
> > + ff_default_get_video_buffer(inlink, w, h); }
> > +
> > +#define OFFSET(x) offsetof(TransposeVAAPIContext, x) #define FLAGS
> > +(AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_FILTERING_PARAM) static
> const
> > +AVOption transpose_vaapi_options[] = {
> > + { "dir", "set transpose direction", OFFSET(dir), AV_OPT_TYPE_INT, { .i64 =
> TRANSPOSE_CCLOCK_FLIP }, 0, 5, FLAGS, "dir" },
> > + { "cclock_flip", "rotate counter-clockwise with vertical flip", 0,
> AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CCLOCK_FLIP }, .flags=FLAGS, .unit
> = "dir" },
> > + { "clock", "rotate clockwise", 0,
> AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CLOCK }, .flags=FLAGS, .unit =
> "dir" },
> > + { "cclock", "rotate counter-clockwise", 0,
> AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CCLOCK }, .flags=FLAGS, .unit =
> "dir" },
> > + { "clock_flip", "rotate clockwise with vertical flip", 0,
> AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_CLOCK_FLIP }, .flags=FLAGS, .unit
> = "dir" },
> > + { "reveral", "rotate clockwise 180 degrees", 0,
> AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_REVERAL }, .flags=FLAGS, .unit =
> "dir" },
> > + { "reveral_flip", "rotate clockwise 180 degrees with vertical
> > +flip", 0, AV_OPT_TYPE_CONST, { .i64 = TRANSPOSE_REVERAL_FLIP },
> > +.flags=FLAGS, .unit = "dir" },
>
> "rotate clockwise 180 degrees with vertical flip" is I think better known as "flip
> horizontally".
I will change it.
>
> > +
> > + { "passthrough", "do not apply transposition if the input matches the
> specified geometry",
> > + OFFSET(passthrough), AV_OPT_TYPE_INT,
> {.i64=TRANSPOSE_PT_TYPE_NONE}, 0, INT_MAX, FLAGS, "passthrough" },
> > + { "none", "always apply transposition", 0, AV_OPT_TYPE_CONST,
> {.i64=TRANSPOSE_PT_TYPE_NONE}, INT_MIN, INT_MAX, FLAGS,
> "passthrough" },
> > + { "portrait", "preserve portrait geometry", 0, AV_OPT_TYPE_CONST,
> {.i64=TRANSPOSE_PT_TYPE_PORTRAIT}, INT_MIN, INT_MAX, FLAGS,
> "passthrough" },
> > + { "landscape", "preserve landscape geometry", 0,
> > + AV_OPT_TYPE_CONST, {.i64=TRANSPOSE_PT_TYPE_LANDSCAPE}, INT_MIN,
> > + INT_MAX, FLAGS, "passthrough" },
> > +
> > + { NULL }
> > +};
> > +
> > +
> > +AVFILTER_DEFINE_CLASS(transpose_vaapi);
> > +
> > +static const AVFilterPad transpose_vaapi_inputs[] = {
> > + {
> > + .name = "default",
> > + .type = AVMEDIA_TYPE_VIDEO,
> > + .filter_frame = &transpose_vaapi_filter_frame,
> > + .get_video_buffer = get_video_buffer,
> > + .config_props = &transpose_vaapi_vpp_config_input,
> > + },
> > + { NULL }
> > +};
> > +
> > +static const AVFilterPad transpose_vaapi_outputs[] = {
> > + {
> > + .name = "default",
> > + .type = AVMEDIA_TYPE_VIDEO,
> > + .config_props = &transpose_vaapi_vpp_config_output,
> > + },
> > + { NULL }
> > +};
> > +
> > +AVFilter ff_vf_transpose_vaapi = {
> > + .name = "transpose_vaapi",
> > + .description = NULL_IF_CONFIG_SMALL("VAAPI VPP for transpose"),
> > + .priv_size = sizeof(TransposeVAAPIContext),
> > + .init = &transpose_vaapi_init,
> > + .uninit = &transpose_vaapi_vpp_ctx_uninit,
> > + .query_formats = &transpose_vaapi_vpp_query_formats,
> > + .inputs = transpose_vaapi_inputs,
> > + .outputs = transpose_vaapi_outputs,
> > + .priv_class = &transpose_vaapi_class,
> > + .flags_internal = FF_FILTER_FLAG_HWFRAME_AWARE, };
> >
>
> - Mark
Thank you Mark for the review.
- Zach
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list