[FFmpeg-devel] [PATCH v2 4/4] avutil/hwcontext_vulkan: fully support customizable validation layers

[Lynne]

24 Nov 2021, 05:11 by jianhua.wu at intel.com: 

> +static int check_validation_layers(AVHWDeviceContext *ctx, AVDictionary *opts,
> +                                   const char * const **dst, uint32_t *num)
> +{
> +    static const char default_layer[] = { "VK_LAYER_KHRONOS_validation" };
> +
> +    int found = 0, err = 0;
> +    VulkanDevicePriv *priv = ctx->internal->priv;
> +    FFVulkanFunctions *vk = &priv->vkfn;
> +
> +    uint32_t sup_layer_count;
> +    VkLayerProperties *sup_layers;
> +
> +    AVDictionaryEntry *user_layers;
> +    char *user_layers_str, *save, *token;
> +
> +    const char **enabled_layers = NULL;
> +    uint32_t enabled_layers_count = 0;
> +
> +    user_layers = av_dict_get(opts, "validation_layers", NULL, 0);
> +    if (!user_layers)
> +        return 0;
>

With this change, unless users specify another validation layer,
then not even the default layer will get activated. That's not desirable.

This is how this should work:
 - If `debug=1` is specified, enable the standard validation layer extension.
 - If `validation_layers` is specified, without any `debug`, enable just the layers specified in the list, and if the standard validation is amongst them, enable debug mode to load its callback properly.
 - If `debug=1` and `validation_layers` is specified, enable the standard validation layer regardless of whether it's included in validation_layers, and enable the rest of the layers.
 - If `debug=0` and `validation_layers` is specified, enable no layers at all.

If any layer enabled, including the standard validation layer, is not supported, error out and stop initialization.


> +    user_layers_str = av_strdup(user_layers->value);
> +    if (!user_layers_str) {
> +        err = AVERROR(EINVAL);
> +        goto fail;
> +    }
> +
> +    vk->EnumerateInstanceLayerProperties(&sup_layer_count, NULL);
> +    sup_layers = av_malloc_array(sup_layer_count, sizeof(VkLayerProperties));
> +    if (!sup_layers)
> +        return AVERROR(ENOMEM);
>

You leak `user_layers_str` on error.


> +    vk->EnumerateInstanceLayerProperties(&sup_layer_count, sup_layers);
> +
> +    av_log(ctx, AV_LOG_VERBOSE, "Supported validation layers:\n");
> +    for (int i = 0; i < sup_layer_count; i++) {
> +        av_log(ctx, AV_LOG_VERBOSE, "\t%s\n", sup_layers[i].layerName);
> +        if (!strcmp(default_layer, sup_layers[i].layerName))
> +            found = 1;
> +    }
> +
> +    if (!found) {
> +        av_log(ctx, AV_LOG_ERROR, "Default layer\"%s\" isn't supported. Please "
> +               "check if vulkan-validation-layers installed\n", default_layer);
>

Return an error here to stop initialization.


> +    } else {
> +        av_log(ctx, AV_LOG_VERBOSE,
> +               "Default validation layer %s is enabled\n", default_layer);
> +        ADD_VAL_TO_LIST(enabled_layers, enabled_layers_count, default_layer);
> +    }
> +
> +    token = av_strtok(user_layers_str, "+", &save);
> +    while (token) {
> +        found = 0;
> +        if (!strcmp(default_layer, token)) {
> +            token = av_strtok(NULL, "+", &save);
> +            continue;
> +        }
> +        for (int j = 0; j < sup_layer_count; j++) {
> +            if (!strcmp(token, sup_layers[j].layerName)) {
> +                found = 1;
> +                break;
> +            }
> +        }
> +        if (found) {
> +            av_log(ctx, AV_LOG_VERBOSE, "Requested Validation Layer: %s\n", token);
> +            ADD_VAL_TO_LIST(enabled_layers, enabled_layers_count, token);
> +        } else {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "Validation Layer \"%s\" not support.\n", token);
> +            err = AVERROR(EINVAL);
> +            goto fail;
> +        }
> +        token = av_strtok(NULL, "+", &save);
> +    }
> +
> +    *dst = enabled_layers;
> +    *num = enabled_layers_count;
> +
> +    av_free(sup_layers);
> +    av_free(user_layers_str);
> +    return 0;
> +
> +fail:
> +    RELEASE_PROPS(enabled_layers, enabled_layers_count);
> +    av_free(sup_layers);
> +    av_free(user_layers_str);
> +    return err;
> +}
> +
>  /* Creates a VkInstance */
>  static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts)
>  {
> @@ -558,13 +651,16 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts)
>  /* Check for present/missing extensions */
>  err = check_extensions(ctx, 0, opts, &inst_props.ppEnabledExtensionNames,
>  &inst_props.enabledExtensionCount, debug_mode);
> +    hwctx->enabled_inst_extensions = inst_props.ppEnabledExtensionNames;
> +    hwctx->nb_enabled_inst_extensions = inst_props.enabledExtensionCount;
>

Why did you move that assignment?

I've pushed patches 2 and 3, just squash patch 1 and 4 (this one) and
resubmit with the changes I mentioned.