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