Message ID | 20211124041154.1090109-4-jianhua.wu@intel.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2,1/4] avutil/vulkan_functions: add EnumerateInstanceLayerProperties | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
24 Nov 2021, 05:11 by jianhua.wu@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.
Lynne: Sent: 2021年11月24日 18:36 To: FFmpeg development discussions and patches<mailto:ffmpeg-devel@ffmpeg.org> Subject: Re: [FFmpeg-devel] [PATCH v2 4/4] avutil/hwcontext_vulkan: fully support customizable validation layers 24 Nov 2021, 05:11 by jianhua.wu@intel.com: >> /* 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? > If the creation fails or something exception, assign them here to ensure that they could be released in the vulkan_device_free() just like releasing by a de-constructor, and it's no need to write more codes to free them in this function. If the context creation failed, the vulkan_device_free() will be called immediately, so they would not keep for a long time. > > I've pushed patches 2 and 3, just squash patch 1 and 4 (this one) and > resubmit with the changes I mentioned. > Okay. No problem. I’ll resubmit it. Thanks, Jianhua
diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c index 644ed947f8..75f9f90d70 100644 --- a/libavutil/hwcontext_vulkan.c +++ b/libavutil/hwcontext_vulkan.c @@ -146,6 +146,13 @@ typedef struct AVVkFrameInternal { } \ } while(0) +#define RELEASE_PROPS(props, count) \ + if (props) { \ + for (int i = 0; i < count; i++) \ + av_free((void *)((props)[i])); \ + av_free((void *)props); \ + } + static const struct { enum AVPixelFormat pixfmt; const VkFormat vkfmts[4]; @@ -511,15 +518,101 @@ static int check_extensions(AVHWDeviceContext *ctx, int dev, AVDictionary *opts, return 0; fail: - if (extension_names) - for (int i = 0; i < extensions_found; i++) - av_free((void *)extension_names[i]); - av_free(extension_names); + RELEASE_PROPS(extension_names, extensions_found); av_free(user_exts_str); av_free(sup_ext); return err; } +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; + + 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); + 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); + } 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; if (err < 0) - return err; + goto fail; if (debug_mode) { - static const char *layers[] = { "VK_LAYER_KHRONOS_validation" }; - inst_props.ppEnabledLayerNames = layers; - inst_props.enabledLayerCount = FF_ARRAY_ELEMS(layers); + err = check_validation_layers(ctx, opts, &inst_props.ppEnabledLayerNames, + &inst_props.enabledLayerCount); + if (err) + goto fail; } /* Try to create the instance */ @@ -574,16 +670,14 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts) if (ret != VK_SUCCESS) { av_log(ctx, AV_LOG_ERROR, "Instance creation failure: %s\n", vk_ret2str(ret)); - for (int i = 0; i < inst_props.enabledExtensionCount; i++) - av_free((void *)inst_props.ppEnabledExtensionNames[i]); - av_free((void *)inst_props.ppEnabledExtensionNames); - return AVERROR_EXTERNAL; + err = AVERROR_EXTERNAL; + goto fail; } err = ff_vk_load_functions(ctx, vk, p->extensions, 1, 0); if (err < 0) { av_log(ctx, AV_LOG_ERROR, "Unable to load instance functions!\n"); - return err; + goto fail; } if (debug_mode) { @@ -604,10 +698,11 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts) hwctx->alloc, &p->debug_ctx); } - hwctx->enabled_inst_extensions = inst_props.ppEnabledExtensionNames; - hwctx->nb_enabled_inst_extensions = inst_props.enabledExtensionCount; + err = 0; - return 0; +fail: + RELEASE_PROPS(inst_props.ppEnabledLayerNames, inst_props.enabledLayerCount); + return err; } typedef struct VulkanDeviceSelection { @@ -1163,13 +1258,8 @@ static void vulkan_device_free(AVHWDeviceContext *ctx) if (p->libvulkan) dlclose(p->libvulkan); - for (int i = 0; i < hwctx->nb_enabled_inst_extensions; i++) - av_free((void *)hwctx->enabled_inst_extensions[i]); - av_free((void *)hwctx->enabled_inst_extensions); - - for (int i = 0; i < hwctx->nb_enabled_dev_extensions; i++) - av_free((void *)hwctx->enabled_dev_extensions[i]); - av_free((void *)hwctx->enabled_dev_extensions); + RELEASE_PROPS(hwctx->enabled_inst_extensions, hwctx->nb_enabled_inst_extensions); + RELEASE_PROPS(hwctx->enabled_dev_extensions, hwctx->nb_enabled_dev_extensions); } static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
Validation layer is an indispensable part of developing on Vulkan. The following commands is on how to enable validation layers: ffmpeg -init_hw_device vulkan=0,debug=1,validation_layers=VK_LAYER_LUNARG_monitor+VK_LAYER_LUNARG_api_dump Signed-off-by: Wu Jianhua <jianhua.wu@intel.com> --- libavutil/hwcontext_vulkan.c | 136 +++++++++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 23 deletions(-)