diff mbox series

[FFmpeg-devel] hwcontext_vulkan: workaround MoltenVK's bug which leads to segmentation fault

Message ID tencent_5C530E884D33E1B36BD5C8A9124EA5C54905@qq.com
State New
Headers show
Series [FFmpeg-devel] hwcontext_vulkan: workaround MoltenVK's bug which leads to segmentation fault | expand

Checks

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
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished

Commit Message

Zhao Zhili Jan. 27, 2022, 8:11 a.m. UTC 
MoltenVK doesn't reset instance pointer when CreateInstance() failed,
then DestroyInstance() leads to segmentation fault. MoltenVK's bug
has been fixed by [1], which doesn't available on homebrew yet.
Regardless MoltenVK's bug, we shouldn't call DestroyInstance()
in the case of CreateInstance() failed, so reset instance making
sense.

[1] https://github.com/KhronosGroup/MoltenVK/commit/86a1fbdb8
---
 libavutil/hwcontext_vulkan.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Wu, Jianhua Jan. 27, 2022, 8:18 a.m. UTC | #1 
Zhao Zhili wrote:
> Sent: Thursday, January 27, 2022 4:11 PM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Zhao Zhili <quinkblack@foxmail.com>
> Subject: [FFmpeg-devel] [PATCH] hwcontext_vulkan: workaround
> MoltenVK's bug which leads to segmentation fault
> 
> MoltenVK doesn't reset instance pointer when CreateInstance() failed, then
> DestroyInstance() leads to segmentation fault. MoltenVK's bug has been
> fixed by [1], which doesn't available on homebrew yet.
> Regardless MoltenVK's bug, we shouldn't call DestroyInstance() in the case
> of CreateInstance() failed, so reset instance making sense.
> 
> [1] https://github.com/KhronosGroup/MoltenVK/commit/86a1fbdb8
> ---
>  libavutil/hwcontext_vulkan.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index 2e219511c9..ac8e00003a 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -719,6 +719,8 @@ 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));
> +        /* Workaround MoltenVK's bug which doesn't reset instance pointer.
> */
> +        hwctx->inst = (VkInstance) { 0 };

Hi,

It's no need to use the explicit cast and use hwctx->inst = VK_NULL_HANDLE instead, which is the null context defined by Vulkan spec.

Thanks,
Jianhua
Zhao Zhili Jan. 27, 2022, 8:42 a.m. UTC | #2 
> On Jan 27, 2022, at 4:18 PM, Wu, Jianhua <jianhua.wu-at-intel.com@ffmpeg.org> wrote:
> 
> Zhao Zhili wrote:
>> Sent: Thursday, January 27, 2022 4:11 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Zhao Zhili <quinkblack@foxmail.com>
>> Subject: [FFmpeg-devel] [PATCH] hwcontext_vulkan: workaround
>> MoltenVK's bug which leads to segmentation fault
>> 
>> MoltenVK doesn't reset instance pointer when CreateInstance() failed, then
>> DestroyInstance() leads to segmentation fault. MoltenVK's bug has been
>> fixed by [1], which doesn't available on homebrew yet.
>> Regardless MoltenVK's bug, we shouldn't call DestroyInstance() in the case
>> of CreateInstance() failed, so reset instance making sense.
>> 
>> [1] https://github.com/KhronosGroup/MoltenVK/commit/86a1fbdb8
>> ---
>> libavutil/hwcontext_vulkan.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>> index 2e219511c9..ac8e00003a 100644
>> --- a/libavutil/hwcontext_vulkan.c
>> +++ b/libavutil/hwcontext_vulkan.c
>> @@ -719,6 +719,8 @@ 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));
>> +        /* Workaround MoltenVK's bug which doesn't reset instance pointer.
>> */
>> +        hwctx->inst = (VkInstance) { 0 };
> 
> Hi,
> 
> It's no need to use the explicit cast and use hwctx->inst = VK_NULL_HANDLE instead, which is the null context defined by Vulkan spec.

I didn’t notice that, thanks! Patch v2:

http://ffmpeg.org/pipermail/ffmpeg-devel/2022-January/292181.html

> 
> Thanks,
> Jianhua
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index 2e219511c9..ac8e00003a 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -719,6 +719,8 @@  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));
+        /* Workaround MoltenVK's bug which doesn't reset instance pointer. */
+        hwctx->inst = (VkInstance) { 0 };
         err = AVERROR_EXTERNAL;
         goto fail;
     }