| Message ID | ae5865d2-3e35-7fe2-ec35-62896d6ce489@jkqxz.net |
|---|---|
| State | Superseded |
| Headers | show |
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
On 14/06/18 05:51, Xiang, Haihao wrote: > 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(-) >>>> >>>> ... >>>> +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? On further thought, I don't think a recommendation is useful here. You end up having to suggest upgrading everything (libva, driver, ffmpeg), and even then it still may not be there (if you are using a driver or codec which doesn't expose this option). So, I think it's most sensible to keep the simple text as originally written. If you have a string opinion then feel free to suggest your own patch for how this should work. (And it should probably be consistent with other similar cases, such as the quality-speed tradeoff option.) >>>> + 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. Ok, I'll change to doing that. >>>> + 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. I don't think end-users aren't going to file bugs of this form for a warning when it works. If it doesn't work then they may file a bug, in which case the verbose logging includes the right information to fix the problem. >>>> + } 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 { As noted above, I've merged this part into the patch. Thanks, - Mark
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 {