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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
