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

Mark Thompson sw at jkqxz.net
Sun Jun 17 16:05:59 EEST 2018


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


More information about the ffmpeg-devel mailing list