[FFmpeg-devel] [PATCH v2 11/36] vaapi_encode: Choose profiles dynamically

Xiang, Haihao haihao.xiang at intel.com
Thu Jun 14 07:51:11 EEST 2018


On Wed, 2018-06-13 at 23:27 +0100, Mark Thompson wrote:
> On 12/06/18 08:22, Xiang, Haihao wrote:
> > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote:
> > > Previously there was one fixed choice for each codec (e.g. H.265 -> Main
> > > profile), and using anything else then required an explicit option from
> > > the user.  This changes to selecting the profile based on the input format
> > > and the set of profiles actually supported by the driver (e.g. P010 input
> > > will choose Main 10 profile for H.265 if the driver supports it).
> > > 
> > > The entrypoint and render target format are also chosen dynamically in the
> > > same way, removing those explicit selections from the per-codec code.
> > > ---
> > >  doc/encoders.texi               |   3 +
> > >  libavcodec/vaapi_encode.c       | 271 ++++++++++++++++++++++++++++++++---
> > > ----
> > > -
> > >  libavcodec/vaapi_encode.h       |  43 +++++--
> > >  libavcodec/vaapi_encode_h264.c  |  45 ++-----
> > >  libavcodec/vaapi_encode_h265.c  |  35 ++----
> > >  libavcodec/vaapi_encode_mjpeg.c |  13 +-
> > >  libavcodec/vaapi_encode_mpeg2.c |  36 ++----
> > >  libavcodec/vaapi_encode_vp8.c   |  11 +-
> > >  libavcodec/vaapi_encode_vp9.c   |  34 ++---
> > >  9 files changed, 310 insertions(+), 181 deletions(-)
> > > 
> > > ...
> > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > > index 27ce792fbe..6104470b31 100644
> > > --- a/libavcodec/vaapi_encode.c
> > > +++ b/libavcodec/vaapi_encode.c
> > > @@ -983,70 +983,247 @@ static av_cold void
> > > vaapi_encode_add_global_param(AVCodecContext *avctx,
> > >      ++ctx->nb_global_params;
> > >  }
> > >  
> > > -static av_cold int vaapi_encode_config_attributes(AVCodecContext *avctx)
> > > +typedef struct VAAPIEncodeRTFormat {
> > > +    const char *name;
> > > +    unsigned int value;
> > > +    int depth;
> > > +    int components;
> > 
> > How about adding a prefix of 'nb_' to this field? I think nb_components is
> > more
> > readable. 
> 
> Sure.  It should match the one in VAAPIEncodeProfile, so I'll change it there
> too.
> 
> > > +    int log2_chroma_w;
> > > +    int log2_chroma_h;
> > > +} VAAPIEncodeRTFormat;
> > > +
> > > +static const VAAPIEncodeRTFormat vaapi_encode_rt_formats[] = {
> > > +    { "YUV400",    VA_RT_FORMAT_YUV400,        8, 1,      },
> > > +    { "YUV420",    VA_RT_FORMAT_YUV420,        8, 3, 1, 1 },
> > > +    { "YUV422",    VA_RT_FORMAT_YUV422,        8, 3, 1, 0 },
> > > +    { "YUV444",    VA_RT_FORMAT_YUV444,        8, 3, 0, 0 },
> > > +    { "YUV411",    VA_RT_FORMAT_YUV411,        8, 3, 2, 0 },
> > > +#if VA_CHECK_VERSION(0, 38, 1)
> > > +    { "YUV420_10", VA_RT_FORMAT_YUV420_10BPP, 10, 3, 1, 1 },
> > > +#endif
> > > +};
> > > +
> > > +static const VAEntrypoint vaapi_encode_entrypoints_normal[] = {
> > > +    VAEntrypointEncSlice,
> > > +    VAEntrypointEncPicture,
> > > +#if VA_CHECK_VERSION(0, 39, 2)
> > > +    VAEntrypointEncSliceLP,
> > > +#endif
> > > +    0
> > > +};
> > > +#if VA_CHECK_VERSION(0, 39, 2)
> > > +static const VAEntrypoint vaapi_encode_entrypoints_low_power[] = {
> > > +    VAEntrypointEncSliceLP,
> > > +    0
> > > +};
> > > +#endif
> > > +
> > > +static av_cold int vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
> > >  {
> > > -    VAAPIEncodeContext *ctx = avctx->priv_data;
> > > +    VAAPIEncodeContext      *ctx = avctx->priv_data;
> > > +    VAProfile    *va_profiles    = NULL;
> > > +    VAEntrypoint *va_entrypoints = NULL;
> > >      VAStatus vas;
> > > -    int i, n, err;
> > > -    VAProfile    *profiles    = NULL;
> > > -    VAEntrypoint *entrypoints = NULL;
> > > -    VAConfigAttrib attr[] = {
> > > -        { VAConfigAttribRTFormat         },
> > > -        { VAConfigAttribRateControl      },
> > > -        { VAConfigAttribEncMaxRefFrames  },
> > > -        { VAConfigAttribEncPackedHeaders },
> > > -    };
> > > +    const VAEntrypoint *usable_entrypoints;
> > > +    const VAAPIEncodeProfile *profile;
> > > +    const AVPixFmtDescriptor *desc;
> > > +    VAConfigAttrib rt_format_attr;
> > > +    const VAAPIEncodeRTFormat *rt_format;
> > > +    int i, j, n, depth, err;
> > > +
> > > +
> > > +    if (ctx->low_power) {
> > > +#if VA_CHECK_VERSION(0, 39, 2)
> > > +        usable_entrypoints = vaapi_encode_entrypoints_low_power;
> > > +#else
> > > +        av_log(avctx, AV_LOG_ERROR, "Low-power encoding is not "
> > > +               "supported with this VAAPI version.\n");
> > 
> > Is it possible to report the minimal VAAPI version in the log in case user
> > doesn't know the requirement on vaapi version 0.39.2?
> 
> The VAAPI version is pretty much useless to users (without reading any source
> code, how would you find what VAAPI version 0.39.2 means?).
> 

Ok, I agree it is useless to user.

> Maybe libva version?  That's still not quite right, though, because it's more
> "not supported in this build" (and will continue to not be supported if you
> upgrade libva but don't rebuild with the new headers).
> 

May we ask user to rebuild ffmpeg once user upgrades libva in the log?

> > > +        return AVERROR(EINVAL);
> > > +#endif
> > > +    } else {
> > > +        usable_entrypoints = vaapi_encode_entrypoints_normal;
> > > +    }
> > > +
> > > +    desc = av_pix_fmt_desc_get(ctx->input_frames->sw_format);
> > > +    if (!desc) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Invalid input pixfmt (%d).\n",
> > > +               ctx->input_frames->sw_format);
> > > +        return AVERROR(EINVAL);
> > > +    }
> > > +    depth = desc->comp[0].depth;
> > > +    for (i = 1; i < desc->nb_components; i++) {
> > > +        if (desc->comp[i].depth != depth) {
> > > +            av_log(avctx, AV_LOG_ERROR, "Invalid input pixfmt (%s).\n",
> > > +                   desc->name);
> > > +            return AVERROR(EINVAL);
> > > +        }
> > > +    }
> > > +    av_log(avctx, AV_LOG_VERBOSE, "Input surface format is %s.\n",
> > > +           desc->name);
> > >  
> > >      n = vaMaxNumProfiles(ctx->hwctx->display);
> > > -    profiles = av_malloc_array(n, sizeof(VAProfile));
> > > -    if (!profiles) {
> > > +    va_profiles = av_malloc_array(n, sizeof(VAProfile));
> > > +    if (!va_profiles) {
> > >          err = AVERROR(ENOMEM);
> > >          goto fail;
> > >      }
> > > -    vas = vaQueryConfigProfiles(ctx->hwctx->display, profiles, &n);
> > > +    vas = vaQueryConfigProfiles(ctx->hwctx->display, va_profiles, &n);
> > >      if (vas != VA_STATUS_SUCCESS) {
> > > -        av_log(ctx, AV_LOG_ERROR, "Failed to query profiles: %d (%s).\n",
> > > +        av_log(avctx, AV_LOG_ERROR, "Failed to query profiles: %d
> > > (%s).\n",
> > >                 vas, vaErrorStr(vas));
> > > -        err = AVERROR(ENOSYS);
> > > +        err = AVERROR_EXTERNAL;
> > >          goto fail;
> > >      }
> > > -    for (i = 0; i < n; i++) {
> > > -        if (profiles[i] == ctx->va_profile)
> > > -            break;
> > > +
> > > +    av_assert0(ctx->codec->profiles);
> > > +    for (i = 0; (ctx->codec->profiles[i].av_profile !=
> > > +                 FF_PROFILE_UNKNOWN); i++) {
> > > +        profile = &ctx->codec->profiles[i];
> > > +        if (depth               != profile->depth ||
> > > +            desc->nb_components != profile->components)
> > > +            continue;
> > > +        if (desc->nb_components > 1 &&
> > > +            (desc->log2_chroma_w != profile->log2_chroma_w ||
> > > +             desc->log2_chroma_h != profile->log2_chroma_h))
> > > +            continue;
> > > +        if (avctx->profile != profile->av_profile &&
> > > +            avctx->profile != FF_PROFILE_UNKNOWN)
> > > +            continue;
> > > +
> > > +        for (j = 0; j < n; j++) {
> > > +            if (va_profiles[j] == profile->va_profile)
> > > +                break;
> > > +        }
> > > +        if (j >= n) {
> > > +            av_log(avctx, AV_LOG_VERBOSE, "Matching profile %d is "
> > > +                   "not supported by driver.\n", profile->va_profile);
> > 
> > Is it possible to report the profile string in the log as what you did
> > below?
> 
> The #ifdefs are very nasty, but they seemed merited for the "I chose this one"
> log line.  See below for a different method, not sure it's better.

I prefer the new way although it is not so good on libva < 2. 

> 
> > > +            continue;
> > > +        }
> > > +
> > > +        ctx->profile = profile;
> > > +        break;
> > >      }
> > > -    if (i >= n) {
> > > -        av_log(ctx, AV_LOG_ERROR, "Encoding profile not found (%d).\n",
> > > -               ctx->va_profile);
> > > -        err = AVERROR(ENOSYS);
> > > -        goto fail;
> > > +    if (!ctx->profile) {
> > > +        av_log(avctx, AV_LOG_ERROR, "No usable encoding profile
> > > found.\n");
> > > +        return AVERROR(ENOSYS);
> > 
> > Set err to AVERROR(ENOSYS) then goto fail, otherwise it will result in
> > memory
> > leak.
> 
> Fixed.
> 
> > >      }
> > >  
> > > +    avctx->profile  = profile->av_profile;
> > > +    ctx->va_profile = profile->va_profile;
> > > +#if VA_CHECK_VERSION(1, 0, 0)
> > > +    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI profile %s (%d).\n",
> > > +           vaProfileStr(ctx->va_profile), ctx->va_profile);
> > > +#else
> > > +    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI profile %d.\n",
> > > +           ctx->va_profile);
> > > +#endif
> > > +
> > >      n = vaMaxNumEntrypoints(ctx->hwctx->display);
> > > -    entrypoints = av_malloc_array(n, sizeof(VAEntrypoint));
> > > -    if (!entrypoints) {
> > > +    va_entrypoints = av_malloc_array(n, sizeof(VAEntrypoint));
> > > +    if (!va_entrypoints) {
> > >          err = AVERROR(ENOMEM);
> > >          goto fail;
> > >      }
> > >      vas = vaQueryConfigEntrypoints(ctx->hwctx->display, ctx->va_profile,
> > > -                                   entrypoints, &n);
> > > +                                   va_entrypoints, &n);
> > >      if (vas != VA_STATUS_SUCCESS) {
> > > -        av_log(ctx, AV_LOG_ERROR, "Failed to query entrypoints for "
> > > +        av_log(avctx, AV_LOG_ERROR, "Failed to query entrypoints for "
> > >                 "profile %u: %d (%s).\n", ctx->va_profile,
> > 
> > Log profile string?
> > 
> > >                 vas, vaErrorStr(vas));
> > > -        err = AVERROR(ENOSYS);
> > > +        err = AVERROR_EXTERNAL;
> > >          goto fail;
> > >      }
> > > +
> > >      for (i = 0; i < n; i++) {
> > > -        if (entrypoints[i] == ctx->va_entrypoint)
> > > +        for (j = 0; usable_entrypoints[j]; j++) {
> > > +            if (va_entrypoints[i] == usable_entrypoints[j])
> > > +                break;
> > > +        }
> > > +        if (usable_entrypoints[j])
> > >              break;
> > >      }
> > >      if (i >= n) {
> > > -        av_log(ctx, AV_LOG_ERROR, "Encoding entrypoint not found "
> > > -               "(%d / %d).\n", ctx->va_profile, ctx->va_entrypoint);
> > > +        av_log(avctx, AV_LOG_ERROR, "No usable encoding entrypoint found
> > > "
> > > +               "for profile %d.\n", ctx->va_profile);
> > 
> > Log profile string?
> > 
> > > +        err = AVERROR(ENOSYS);
> > > +        goto fail;
> > > +    }
> > > +
> > > +    ctx->va_entrypoint = va_entrypoints[i];
> > > +#if VA_CHECK_VERSION(1, 0, 0)
> > > +    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI entrypoint %s (%d).\n",
> > > +           vaEntrypointStr(ctx->va_entrypoint), ctx->va_entrypoint);
> > > +#else
> > > +    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI entrypoint %d.\n",
> > > +           ctx->va_entrypoint);
> > > +#endif
> > > +
> > > +    for (i = 0; i < FF_ARRAY_ELEMS(vaapi_encode_rt_formats); i++) {
> > > +        rt_format = &vaapi_encode_rt_formats[i];
> > > +        if (rt_format->depth         == depth &&
> > > +            rt_format->components    == profile->components    &&
> > > +            rt_format->log2_chroma_w == profile->log2_chroma_w &&
> > > +            rt_format->log2_chroma_h == profile->log2_chroma_h)
> > > +            break;
> > > +    }
> > > +    if (i >= FF_ARRAY_ELEMS(vaapi_encode_rt_formats)) {
> > > +        av_log(avctx, AV_LOG_ERROR, "No usable render target format "
> > > +               "found for profile %d entrypoint %d.\n",
> > > +               ctx->va_profile, ctx->va_entrypoint);
> > 
> > Log profile and entrypoint strings?
> > 
> > >          err = AVERROR(ENOSYS);
> > >          goto fail;
> > >      }
> > >  
> > > +    rt_format_attr = (VAConfigAttrib) { VAConfigAttribRTFormat };
> > > +    vas = vaGetConfigAttributes(ctx->hwctx->display,
> > > +                                ctx->va_profile, ctx->va_entrypoint,
> > > +                                &rt_format_attr, 1);
> > > +    if (vas != VA_STATUS_SUCCESS) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Failed to query RT format "
> > > +               "config attribute: %d (%s).\n", vas, vaErrorStr(vas));
> > > +        err = AVERROR_EXTERNAL;
> > > +        goto fail;
> > > +    }
> > > +
> > > +    if (rt_format_attr.value == VA_ATTRIB_NOT_SUPPORTED) {
> > > +        av_log(avctx, AV_LOG_VERBOSE, "RT format config attribute not "
> > > +               "supported by driver: assuming surface RT format %s "
> > > +               "is valid.\n", rt_format->name);
> > 
> > I think it would be better to log it as a warning. 
> 
> I'm not sure what a user would be meant to do with such a warning.  Even if
> the driver doesn't make it queryable, we know what the answer should be and it
> will either work or not after this point with the known value.

User may file an driver issue for adding support for VAConfigAttribRTFormat
query once they see such warning. I think most user will ignore this message if
it is taken as verbose message.

Thanks
Haihao

> 
> > > +    } else if (!(rt_format_attr.value & rt_format->value)) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Surface RT format %s not supported "
> > > +               "by driver for encoding profile %d entrypoint %d.\n",
> > > +               rt_format->name, ctx->va_profile, ctx->va_entrypoint);
> > > +        err = AVERROR(ENOSYS);
> > > +        goto fail;
> > > +    } else {
> > > +        av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI render target "
> > > +               "format %s (%#x).\n", rt_format->name, rt_format->value);
> > > +        ctx->config_attributes[ctx->nb_config_attributes++] =
> > > +            (VAConfigAttrib) {
> > > +            .type  = VAConfigAttribRTFormat,
> > > +            .value = rt_format->value,
> > > +        };
> > > +    }
> > > +
> > > +    err = 0;
> > > +fail:
> > > +    av_freep(&va_profiles);
> > > +    av_freep(&va_entrypoints);
> > > +    return err;
> > > +}
> > > +
> > > ...
> 
> Here's a different way to do the strings.  The result is kindof stupid on
> libva < 2, so I'm not sure I would really argue for it.  Would you prefer
> this?
> 
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 2ed12beb0c..f838ee5bd5 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -1020,6 +1020,7 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>      const AVPixFmtDescriptor *desc;
>      VAConfigAttrib rt_format_attr;
>      const VAAPIEncodeRTFormat *rt_format;
> +    const char *profile_string, *entrypoint_string;
>      int i, j, n, depth, err;
>  
>  
> @@ -1081,6 +1082,12 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>              avctx->profile != FF_PROFILE_UNKNOWN)
>              continue;
>  
> +#if VA_CHECK_VERSION(1, 0, 0)
> +        profile_string = vaProfileStr(profile->va_profile);
> +#else
> +        profile_string = "[no profile names]";
> +#endif
> +
>          for (j = 0; j < n; j++) {
>              if (va_profiles[j] == profile->va_profile)
>                  break;
> @@ -1102,13 +1109,8 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>  
>      avctx->profile  = profile->av_profile;
>      ctx->va_profile = profile->va_profile;
> -#if VA_CHECK_VERSION(1, 0, 0)
>      av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI profile %s (%d).\n",
> -           vaProfileStr(ctx->va_profile), ctx->va_profile);
> -#else
> -    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI profile %d.\n",
> -           ctx->va_profile);
> -#endif
> +           profile_string, ctx->va_profile);
>  
>      n = vaMaxNumEntrypoints(ctx->hwctx->display);
>      va_entrypoints = av_malloc_array(n, sizeof(VAEntrypoint));
> @@ -1120,8 +1122,8 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>                                     va_entrypoints, &n);
>      if (vas != VA_STATUS_SUCCESS) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to query entrypoints for "
> -               "profile %u: %d (%s).\n", ctx->va_profile,
> -               vas, vaErrorStr(vas));
> +               "profile %s (%d): %d (%s).\n", profile_string,
> +               ctx->va_profile, vas, vaErrorStr(vas));
>          err = AVERROR_EXTERNAL;
>          goto fail;
>      }
> @@ -1136,19 +1138,19 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>      }
>      if (i >= n) {
>          av_log(avctx, AV_LOG_ERROR, "No usable encoding entrypoint found "
> -               "for profile %d.\n", ctx->va_profile);
> +               "for profile %s (%d).\n", profile_string, ctx->va_profile);
>          err = AVERROR(ENOSYS);
>          goto fail;
>      }
>  
>      ctx->va_entrypoint = va_entrypoints[i];
>  #if VA_CHECK_VERSION(1, 0, 0)
> -    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI entrypoint %s (%d).\n",
> -           vaEntrypointStr(ctx->va_entrypoint), ctx->va_entrypoint);
> +    entrypoint_string = vaEntrypointStr(ctx->va_entrypoint);
>  #else
> -    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI entrypoint %d.\n",
> -           ctx->va_entrypoint);
> +    entrypoint_string = "[no entrypoint names]";
>  #endif
> +    av_log(avctx, AV_LOG_VERBOSE, "Using VAAPI entrypoint %s (%d).\n",
> +           entrypoint_string, ctx->va_entrypoint);
>  
>      for (i = 0; i < FF_ARRAY_ELEMS(vaapi_encode_rt_formats); i++) {
>          rt_format = &vaapi_encode_rt_formats[i];
> @@ -1160,8 +1162,9 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>      }
>      if (i >= FF_ARRAY_ELEMS(vaapi_encode_rt_formats)) {
>          av_log(avctx, AV_LOG_ERROR, "No usable render target format "
> -               "found for profile %d entrypoint %d.\n",
> -               ctx->va_profile, ctx->va_entrypoint);
> +               "found for profile %s (%d) entrypoint %s (%d).\n",
> +               profile_string, ctx->va_profile,
> +               entrypoint_string, ctx->va_entrypoint);
>          err = AVERROR(ENOSYS);
>          goto fail;
>      }
> @@ -1183,8 +1186,9 @@ static av_cold int
> vaapi_encode_profile_entrypoint(AVCodecContext *avctx)
>                 "is valid.\n", rt_format->name);
>      } else if (!(rt_format_attr.value & rt_format->value)) {
>          av_log(avctx, AV_LOG_ERROR, "Surface RT format %s not supported "
> -               "by driver for encoding profile %d entrypoint %d.\n",
> -               rt_format->name, ctx->va_profile, ctx->va_entrypoint);
> +               "by driver for encoding profile %s (%d) entrypoint %s
> (%d).\n",
> +               rt_format->name, profile_string, ctx->va_profile,
> +               entrypoint_string, ctx->va_entrypoint);
>          err = AVERROR(ENOSYS);
>          goto fail;
>      } else {
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list