From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id A69C84580C for ; Mon, 24 Apr 2023 08:26:59 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2F45468BCBC; Mon, 24 Apr 2023 11:26:56 +0300 (EEST) Received: from w4.tutanota.de (w4.tutanota.de [81.3.6.165]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C601768A8C0 for ; Mon, 24 Apr 2023 11:26:49 +0300 (EEST) Received: from tutadb.w10.tutanota.de (unknown [192.168.1.10]) by w4.tutanota.de (Postfix) with ESMTP id 5DEA91060164 for ; Mon, 24 Apr 2023 08:26:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1682324809; s=s1; d=lynne.ee; h=From:From:To:To:Subject:Subject:Content-Description:Content-ID:Content-Type:Content-Type:Content-Transfer-Encoding:Content-Transfer-Encoding:Cc:Date:Date:In-Reply-To:In-Reply-To:MIME-Version:MIME-Version:Message-ID:Message-ID:Reply-To:References:References:Sender; bh=hAV8JMIlUkyXXpil/2K+DIHKRmWl3osG5d80SldehNM=; b=XC0Kt/Sw+Jwo1880Jpd9eJHr7JCwFYhHrz2O2CLpWHnJ2wPL0sNoYtM7Gw3cnjsl RmhNZfFwOee/Qk5AYJ9im2Oj15udxZSSalan0V0UUaRwIevzP8uUjMEDx8FmoMBxjN3 IFGQ9pUzU9e3Z/oVjAJBDx6k5WyytPFLbnV7hC1Dn10gVAeIPjsgDvU07OYDjADPY+7 svnd4kaTr7OS82p4JnQuP4+sGDAaUdWnKiLm0egjx3X54MVASclTECmL0Rk6JoLSOwx 0wFg5cqh0eoTdaemdfR4ZubtOZix9c9tqOM7QoC8SMzgInRs9rqG/SueslAb5zklFCW z3Ho1iWHeA== Date: Mon, 24 Apr 2023 10:26:49 +0200 (CEST) From: Lynne To: FFmpeg development discussions and patches Message-ID: In-Reply-To: <168216731222.3843.7711457127740646229@lain.khirnov.net> References: <168172351803.3843.891431528072089458@lain.khirnov.net> <168216731222.3843.7711457127740646229@lain.khirnov.net> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 31/92] Vulkan patchset part 1 - common code changes X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Apr 22, 2023, 14:42 by anton@khirnov.net: > Quoting Lynne (2023-04-19 16:39:28) > >> Apr 17, 2023, 11:25 by anton@khirnov.net: >> >> > Quoting Lynne (2023-03-14 07:33:43) >> > >> >> From 6e2dfc44e50798264eb16bc2dcabfdbf2fbac2d7 Mon Sep 17 00:00:00 2001 >> >> From: Lynne >> >> Date: Thu, 15 Dec 2022 01:06:52 +0100 >> >> Subject: [PATCH 24/92] hwconfig: add a new HWACCEL_CAP_THREAD_SAFE for >> >> threadsafe hwaccels >> >> >> >> Vulkan is fully threadsafe and stateless, so we can benefit from this. >> >> --- >> >> libavcodec/hwconfig.h | 1 + >> >> libavcodec/pthread_frame.c | 2 +- >> >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/libavcodec/hwconfig.h b/libavcodec/hwconfig.h >> >> index 721424912c..e6b78f0160 100644 >> >> --- a/libavcodec/hwconfig.h >> >> +++ b/libavcodec/hwconfig.h >> >> @@ -24,6 +24,7 @@ >> >> >> >> >> >> #define HWACCEL_CAP_ASYNC_SAFE (1 << 0) >> >> +#define HWACCEL_CAP_THREAD_SAFE (1 << 1) >> >> >> >> >> >> typedef struct AVCodecHWConfigInternal { >> >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >> >> index 74864e19c5..c096287233 100644 >> >> --- a/libavcodec/pthread_frame.c >> >> +++ b/libavcodec/pthread_frame.c >> >> @@ -204,7 +204,7 @@ static attribute_align_arg void *frame_worker_thread(void *arg) >> >> >> >> /* if the previous thread uses hwaccel then we take the lock to ensure >> >> * the threads don't run concurrently */ >> >> - if (avctx->hwaccel) { >> >> + if (avctx->hwaccel && !(avctx->hwaccel->caps_internal & HWACCEL_CAP_THREAD_SAFE)) { >> >> >> > >> > A major problem here is that hwaccel_priv_data exists in only a single >> > instance shared across all the worker threads. If you want frame-threaded >> > hwaccels, you'll need to add some mechanism for synchronizing it between >> > threads. >> > >> > I really wonder how does your code survive all the races involved. >> > >> >> It works because hwaccel_priv_data is read-only. No need to synchronize >> anything, the contexts Vulkan creates are defined as driver-synchronized, >> and all the info a frame needs is provided when decoding it, rather than >> kept as a state. >> So I don't see a problem here. >> > > From a quick glance, at least vk_av1_free_frame_priv() modifies this > context, which seems highly unsafe. > Our AV1 decoder isn't threaded yet. I've removed the threadsafe flag from the hwaccel, Dave is currently looking into whether it's possible to remove the frame id. > This really seems like a disaster waiting to happen, there should be > some stronger guarantees that nobody will modify this context when they > shouldn't. > It's an opt-in flag, that's more than enough. _______________________________________________ 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".