diff mbox series

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

Message ID tencent_A8311C468B86661DD09F26DB27DA2ABAD306@qq.com
State New
Headers show
Series [FFmpeg-devel,v2] 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:38 a.m. UTC 
MoltenVK doesn't reset instance 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

Lynne Jan. 27, 2022, 9:07 a.m. UTC | #1 
27 Jan 2022, 09:38 by quinkblack@foxmail.com:

> MoltenVK doesn't reset instance 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..d3bd37a6a7 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. */
> +        hwctx->inst = VK_NULL_HANDLE;
>  err = AVERROR_EXTERNAL;
>  goto fail;
>  }
>

Not a fan of working around this implementation bug.
Couldn't homebrew just update?
Zhao Zhili Jan. 27, 2022, 9:43 a.m. UTC | #2 
> On Jan 27, 2022, at 5:07 PM, Lynne <dev@lynne.ee> wrote:
> 
> 27 Jan 2022, 09:38 by quinkblack@foxmail.com:
> 
>> MoltenVK doesn't reset instance 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..d3bd37a6a7 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. */
>> +        hwctx->inst = VK_NULL_HANDLE;
>> err = AVERROR_EXTERNAL;
>> goto fail;
>> }
>> 
> 
> Not a fan of working around this implementation bug.
> Couldn't homebrew just update?

I don’t have strong opinion on the patch, feel free to drop it.

It’s a cheap workaround and it takes a lot of time for me to figure
out the crash. Other people may confront the same issue, then the
patch can be seen as a reference.

> _______________________________________________
> 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..d3bd37a6a7 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. */
+        hwctx->inst = VK_NULL_HANDLE;
         err = AVERROR_EXTERNAL;
         goto fail;
     }