[FFmpeg-devel] [PATCH V3 2/6] lavfi: use common VPP infrastructure for vf_scale_vaapi.

[Mark Thompson]

On 18/01/18 05:18, Jun Zhao wrote:
> 
> From d157fdbffebd07066b1a857398e1067615f908b3 Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao at intel.com>
> Date: Mon, 8 Jan 2018 16:02:35 +0800
> Subject: [PATCH V3 2/6] lavfi: use common VPP infrastructure for
>  vf_scale_vaapi.
> 
> Use the common VPP infrastructure re-work vf_scale_vaapi.
> 
> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
> ---
>  libavfilter/Makefile         |   2 +-
>  libavfilter/vf_scale_vaapi.c | 353 +++++--------------------------------------
>  2 files changed, 41 insertions(+), 314 deletions(-)
> 
> ...
>  
>  static av_cold int scale_vaapi_init(AVFilterContext *avctx)
>  {
> -    ScaleVAAPIContext *ctx = avctx->priv;
> +    VAAPIVPPContext *vpp_ctx = avctx->priv;
> +    ScaleVAAPIContext *ctx   = (ScaleVAAPIContext *)vpp_ctx->priv_data;

The extra indirection means this is reading ctx before it's set.  E.g. try:

$ gdb --args ./ffmpeg_g -y -hwaccel vaapi -hwaccel_device /dev/dri/renderD129 -hwaccel_output_format vaapi -i input.mp4 -vf 'scale_vaapi=format=invalid' -f null -
...
Thread 1 "ffmpeg_g" received signal SIGSEGV, Segmentation fault.
0x0000555556927515 in format_line (avcl=0x55555843b950, level=16, fmt=0x555556984f69 "Invalid output format.\n", vl=0x7fffffffcfb0, part=0x7fffffffbf20, print_prefix=0x55555708c708 <print_prefix>, type=0x7fffffffbb18) at src/libavutil/log.c:258
258                 AVClass** parent = *(AVClass ***) (((uint8_t *) avcl) +
(gdb) bt
#0  0x0000555556927515 in format_line (avcl=0x55555843b950, level=16, fmt=0x555556984f69 "Invalid output format.\n", vl=0x7fffffffcfb0, part=0x7fffffffbf20, print_prefix=0x55555708c708 <print_prefix>, type=0x7fffffffbb18) at src/libavutil/log.c:258
#1  0x00005555569278da in av_log_default_callback (ptr=0x55555843b950, level=16, fmt=0x555556984f69 "Invalid output format.\n", vl=0x7fffffffcfb0) at src/libavutil/log.c:320
#2  0x0000555556927d05 in av_vlog (avcl=0x55555843b950, level=16, fmt=0x555556984f69 "Invalid output format.\n", vl=0x7fffffffcfb0) at src/libavutil/log.c:377
#3  0x0000555556927cc4 in av_log (avcl=0x55555843b950, level=16, fmt=0x555556984f69 "Invalid output format.\n") at src/libavutil/log.c:369
#4  0x0000555555844bac in scale_vaapi_init (avctx=0x55555836ad80) at src/libavfilter/vf_scale_vaapi.c:151


I still think having VAAPIVPPContext as the first element of ScaleVAAPIContext would be cleaner.


>  
> -    ctx->va_config  = VA_INVALID_ID;
> -    ctx->va_context = VA_INVALID_ID;
> -    ctx->valid_ids  = 1;
> +    vaapi_vpp_ctx_init(vpp_ctx);
> +    vpp_ctx->pipeline_uninit = vaapi_vpp_pipeline_uninit;
>  
>      if (ctx->output_format_string) {
> -        ctx->output_format = av_get_pix_fmt(ctx->output_format_string);
> -        if (ctx->output_format == AV_PIX_FMT_NONE) {
> +        vpp_ctx->output_format = av_get_pix_fmt(ctx->output_format_string);
> +        if (vpp_ctx->output_format == AV_PIX_FMT_NONE) {
>              av_log(ctx, AV_LOG_ERROR, "Invalid output format.\n");
>              return AVERROR(EINVAL);
>          }
>      } else {
>          // Use the input format once that is configured.
> -        ctx->output_format = AV_PIX_FMT_NONE;
> +        vpp_ctx->output_format = AV_PIX_FMT_NONE;
>      }
>  
>      return 0;
>  }
>  
> -static av_cold void scale_vaapi_uninit(AVFilterContext *avctx)
> -{
> -    ScaleVAAPIContext *ctx = avctx->priv;
> -
> -    if (ctx->valid_ids)
> -        scale_vaapi_pipeline_uninit(ctx);
> -
> -    av_buffer_unref(&ctx->input_frames_ref);
> -    av_buffer_unref(&ctx->output_frames_ref);
> -    av_buffer_unref(&ctx->device_ref);
> -}
> -
> -
> -#define OFFSET(x) offsetof(ScaleVAAPIContext, x)
> +#define OFFSET(x) (offsetof(VAAPIVPPContext, priv_data) + \
> +                   offsetof(ScaleVAAPIContext, x))
>  #define FLAGS (AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM)
>  static const AVOption scale_vaapi_options[] = {
>      { "w", "Output video width",
> @@ -441,19 +172,14 @@ static const AVOption scale_vaapi_options[] = {
>      { NULL },
>  };
>  
> -static const AVClass scale_vaapi_class = {
> -    .class_name = "scale_vaapi",
> -    .item_name  = av_default_item_name,
> -    .option     = scale_vaapi_options,
> -    .version    = LIBAVUTIL_VERSION_INT,
> -};
> +AVFILTER_DEFINE_CLASS(scale_vaapi);
>  
>  static const AVFilterPad scale_vaapi_inputs[] = {
>      {
>          .name         = "default",
>          .type         = AVMEDIA_TYPE_VIDEO,
>          .filter_frame = &scale_vaapi_filter_frame,
> -        .config_props = &scale_vaapi_config_input,
> +        .config_props = &vaapi_vpp_config_input,
>      },
>      { NULL }
>  };
> @@ -470,10 +196,11 @@ static const AVFilterPad scale_vaapi_outputs[] = {
>  AVFilter ff_vf_scale_vaapi = {
>      .name          = "scale_vaapi",
>      .description   = NULL_IF_CONFIG_SMALL("Scale to/from VAAPI surfaces."),
> -    .priv_size     = sizeof(ScaleVAAPIContext),
> +    .priv_size     = (sizeof(VAAPIVPPContext) +
> +                      sizeof(ScaleVAAPIContext)),
>      .init          = &scale_vaapi_init,
> -    .uninit        = &scale_vaapi_uninit,
> -    .query_formats = &scale_vaapi_query_formats,
> +    .uninit        = &vaapi_vpp_ctx_uninit,
> +    .query_formats = &vaapi_vpp_query_formats,
>      .inputs        = scale_vaapi_inputs,
>      .outputs       = scale_vaapi_outputs,
>      .priv_class    = &scale_vaapi_class,
> -- 
> 2.14.1
> 

Otherwise LGTM.

- Mark