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 9A318442DD for ; Mon, 5 Sep 2022 05:42:30 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6E1DB68B6CC; Mon, 5 Sep 2022 08:42:28 +0300 (EEST) Received: from mout-p-202.mailbox.org (mout-p-202.mailbox.org [80.241.56.172]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 17A9768B156 for ; Mon, 5 Sep 2022 08:42:21 +0300 (EEST) Received: from smtp1.mailbox.org (smtp1.mailbox.org [IPv6:2001:67c:2050:b231:465::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-202.mailbox.org (Postfix) with ESMTPS id 4MLcpz49hfz9sS0 for ; Mon, 5 Sep 2022 07:42:19 +0200 (CEST) Message-ID: <489f21e8-34fa-3c83-cd85-99308233bc30@ycbcr.xyz> Date: Mon, 5 Sep 2022 07:42:17 +0200 MIME-Version: 1.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220902205932.17883-1-anton@khirnov.net> From: Steve Lhomme In-Reply-To: <20220902205932.17883-1-anton@khirnov.net> X-Rspamd-Queue-Id: 4MLcpz49hfz9sS0 Subject: Re: [FFmpeg-devel] [PATCH] lavc/pthread_frame: avoid leaving stale hwaccel state in worker threads 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Hi Anton, On 2022-09-02 22:59, Anton Khirnov wrote: > This state is not refcounted, so make sure it always has a well-defined > owner. > --- > Steve, could you please test this? I can confirm it doesn't leak the context and plays correctly. It also doesn't crash ;) > --- > libavcodec/pthread_frame.c | 37 ++++++++++++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 08a6f98898..9b44e2e698 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -148,6 +148,10 @@ typedef struct FrameThreadContext { > * Set for the first N packets, where N is the number of threads. > * While it is set, ff_thread_en/decode_frame won't return any results. > */ > + > + const AVHWAccel *stash_hwaccel; > + void *stash_hwaccel_context; > + void *stash_hwaccel_priv; > } FrameThreadContext; > > #if FF_API_THREAD_SAFE_CALLBACKS > @@ -228,9 +232,17 @@ FF_ENABLE_DEPRECATION_WARNINGS > ff_thread_finish_setup(avctx); > > if (p->hwaccel_serializing) { > + /* wipe hwaccel state to avoid stale pointers lying around; > + * the state was transferred to FrameThreadContext in > + * ff_thread_finish_setup(), so nothing is leaked */ > + avctx->hwaccel = NULL; > + avctx->hwaccel_context = NULL; > + avctx->internal->hwaccel_priv_data = NULL; > + > p->hwaccel_serializing = 0; > pthread_mutex_unlock(&p->parent->hwaccel_mutex); > } > + av_assert0(!avctx->hwaccel); > > if (p->async_serializing) { > p->async_serializing = 0; > @@ -294,9 +306,6 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src, > dst->color_range = src->color_range; > dst->chroma_sample_location = src->chroma_sample_location; > > - dst->hwaccel = src->hwaccel; > - dst->hwaccel_context = src->hwaccel_context; > - > dst->sample_rate = src->sample_rate; > dst->sample_fmt = src->sample_fmt; > #if FF_API_OLD_CHANNEL_LAYOUT > @@ -309,8 +318,6 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (err < 0) > return err; > > - dst->internal->hwaccel_priv_data = src->internal->hwaccel_priv_data; > - > if (!!dst->hw_frames_ctx != !!src->hw_frames_ctx || > (dst->hw_frames_ctx && dst->hw_frames_ctx->data != src->hw_frames_ctx->data)) { > av_buffer_unref(&dst->hw_frames_ctx); > @@ -450,6 +457,12 @@ static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx, > pthread_mutex_unlock(&p->mutex); > return err; > } > + > + /* transfer hwaccel state stashed from previous thread, if any */ > + av_assert0(!p->avctx->hwaccel); > + FFSWAP(const AVHWAccel*, p->avctx->hwaccel, fctx->stash_hwaccel); > + FFSWAP(void*, p->avctx->hwaccel_context, fctx->stash_hwaccel_context); > + FFSWAP(void*, p->avctx->internal->hwaccel_priv_data, fctx->stash_hwaccel_priv); > } > > av_packet_unref(p->avpkt); > @@ -655,6 +668,13 @@ void ff_thread_finish_setup(AVCodecContext *avctx) { > async_lock(p->parent); > } > > + /* save hwaccel state for passing to the next thread; > + * this is done here so that this worker thread can wipe its own hwaccel > + * state after decoding, without requiring synchronization */ > + p->parent->stash_hwaccel = avctx->hwaccel; > + p->parent->stash_hwaccel_context = avctx->hwaccel_context; > + p->parent->stash_hwaccel_priv = avctx->internal->hwaccel_priv_data; > + > pthread_mutex_lock(&p->progress_mutex); > if(atomic_load(&p->state) == STATE_SETUP_FINISHED){ > av_log(avctx, AV_LOG_WARNING, "Multiple ff_thread_finish_setup() calls\n"); > @@ -761,6 +781,13 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count) > av_freep(&fctx->threads); > ff_pthread_free(fctx, thread_ctx_offsets); > > + /* if we have stashed hwaccel state, move it to the user-facing context, > + * so it will be freed in avcodec_close() */ > + av_assert0(!avctx->hwaccel); > + FFSWAP(const AVHWAccel*, avctx->hwaccel, fctx->stash_hwaccel); > + FFSWAP(void*, avctx->hwaccel_context, fctx->stash_hwaccel_context); > + FFSWAP(void*, avctx->internal->hwaccel_priv_data, fctx->stash_hwaccel_priv); > + > av_freep(&avctx->internal->thread_ctx); > } > > -- > 2.35.1 > _______________________________________________ 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".