| Message ID | 6301fb24-1d5d-b069-a773-0defdc2086df@ycbcr.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/pthread_frame: update the main avctx from the current, ThreadContext | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| andriy/commit_msg_armv7_RPi4 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
On Sat, Jul 09, 2022 at 08:45:31AM +0200, Steve Lhomme wrote: > Patch attached in the email. > > In some cases, the submit packet can result in configurations changes of the > hardware decoders. The previous HW context is then freed and a new one > created. That context is supposed to move up to the main context and the > other threads. > > It appears that when more than 2 frame threads are involved, the new context > doesn't move in the right place (or rather at the right time). And it can > create a crash when reusing the old HW context. This patch fixes the issue I > could reproduce in VLC with DXVA decoding. > > I have no idea if this is correct or the side effects induced by this. It > seems the right thing to do. Keeping the previous call exhibits the issue. > It seems odd the other existing thread context are not updated with the > current hwaccel_priv_data. But maybe they are updated later from the "main" > thread context, in which case this patch seems solid. > pthread_frame.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > 2274e52382008f403c7bf52f76d608d2a56ef859 0001-avcodec-pthread_frame-update-the-main-avctx-from-the.patch > From e8abeeff92f5d7b3b553acdb7595d40153cbec1e Mon Sep 17 00:00:00 2001 > From: Steve Lhomme <robux4@ycbcr.xyz> > Date: Fri, 8 Jul 2022 11:49:27 +0200 > Subject: [PATCH] avcodec/pthread_frame: update the main avctx from the current > ThreadContext > > After a submit_decoder() the hwaccel_priv_data may have changed in that avctx. > > Doing it after the "next available frame" loop will likely point to the > hwaccel_priv_data from another PerThreadContext which had the old value which > might have been freed, if the submit_decoder() resulting in a format change. > --- > libavcodec/pthread_frame.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) It would be nice if a solution to this would make it in 5.1 thx [...]
On Mon, Jul 18, 2022 at 08:56:32PM +0200, Michael Niedermayer wrote: > On Sat, Jul 09, 2022 at 08:45:31AM +0200, Steve Lhomme wrote: > > Patch attached in the email. > > > > In some cases, the submit packet can result in configurations changes of the > > hardware decoders. The previous HW context is then freed and a new one > > created. That context is supposed to move up to the main context and the > > other threads. > > > > It appears that when more than 2 frame threads are involved, the new context > > doesn't move in the right place (or rather at the right time). And it can > > create a crash when reusing the old HW context. This patch fixes the issue I > > could reproduce in VLC with DXVA decoding. > > > > I have no idea if this is correct or the side effects induced by this. It > > seems the right thing to do. Keeping the previous call exhibits the issue. > > It seems odd the other existing thread context are not updated with the > > current hwaccel_priv_data. But maybe they are updated later from the "main" > > thread context, in which case this patch seems solid. > > > pthread_frame.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > 2274e52382008f403c7bf52f76d608d2a56ef859 0001-avcodec-pthread_frame-update-the-main-avctx-from-the.patch > > From e8abeeff92f5d7b3b553acdb7595d40153cbec1e Mon Sep 17 00:00:00 2001 > > From: Steve Lhomme <robux4@ycbcr.xyz> > > Date: Fri, 8 Jul 2022 11:49:27 +0200 > > Subject: [PATCH] avcodec/pthread_frame: update the main avctx from the current > > ThreadContext > > > > After a submit_decoder() the hwaccel_priv_data may have changed in that avctx. > > > > Doing it after the "next available frame" loop will likely point to the > > hwaccel_priv_data from another PerThreadContext which had the old value which > > might have been freed, if the submit_decoder() resulting in a format change. > > --- > > libavcodec/pthread_frame.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > It would be nice if a solution to this would make it in 5.1 noone ? i guess the fix will then be in 5.1.1 is there any note / warning that should be included in the release notes or something about this open issue ? thx [...]
Why are you not resubmitting your original patch that stops copying hwaccel_priv_data to the user-facing context? It seemed more correct to me, since the user-facing context should never see any hwaccel data. Or does it not fix the issue fully? For a more proper fix, we probably want to bundle hwaccel+state+uninit to a single refcounted thing.
Hi, On 2022-08-02 16:19, Anton Khirnov wrote: > Why are you not resubmitting your original patch that stops copying > hwaccel_priv_data to the user-facing context? > > It seemed more correct to me, since the user-facing context should never > see any hwaccel data. Or does it not fix the issue fully? The original patch is not fixing it properly. With that patch and avcodec-threads > 1, the uninit of the hardware decoder is not called anymore. So it will replace a crash fix with a (big) resource leak. For reference, this it the patch we're talking about https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg94274.html > For a more proper fix, we probably want to bundle hwaccel+state+uninit > to a single refcounted thing. > > -- > Anton Khirnov > _______________________________________________ > 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".
On Fri, Aug 19, 2022 at 10:07:54AM +0200, Steve Lhomme wrote: > Hi, > > On 2022-08-02 16:19, Anton Khirnov wrote: > > Why are you not resubmitting your original patch that stops copying > > hwaccel_priv_data to the user-facing context? > > > > It seemed more correct to me, since the user-facing context should never > > see any hwaccel data. Or does it not fix the issue fully? > > The original patch is not fixing it properly. With that patch and > avcodec-threads > 1, the uninit of the hardware decoder is not called > anymore. So it will replace a crash fix with a (big) resource leak. > > For reference, this it the patch we're talking about > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg94274.html Should we maybe add something to the release notes if this bug is unfixed in the next minor release ? thx [...]
Quoting Steve Lhomme (2022-08-19 10:07:54) > Hi, > > On 2022-08-02 16:19, Anton Khirnov wrote: > > Why are you not resubmitting your original patch that stops copying > > hwaccel_priv_data to the user-facing context? > > > > It seemed more correct to me, since the user-facing context should never > > see any hwaccel data. Or does it not fix the issue fully? > > The original patch is not fixing it properly. With that patch and > avcodec-threads > 1, the uninit of the hardware decoder is not called > anymore. So it will replace a crash fix with a (big) resource leak. > > For reference, this it the patch we're talking about > https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg94274.html A problem with this patch is that currently the user-facing codec context will contain values corresponding to the last output frame. You're changing it to contain values corresponding to the thread a new packet was submitted to, which is wrong. I'll try to write a better patch, could you tell me exactly how to reproduce the crash?
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 8faea75a49..eff09c3510 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -529,6 +529,8 @@ int ff_thread_decode_frame(AVCodecContext *avctx, if (err) goto finish; + update_context_from_thread(avctx, p->avctx, 1); + /* * If we're still receiving the initial packets, don't return a frame. */ @@ -578,8 +580,6 @@ int ff_thread_decode_frame(AVCodecContext *avctx, if (finished >= avctx->thread_count) finished = 0; } while (!avpkt->size && !*got_picture_ptr && err >= 0 && finished != fctx->next_finished); - update_context_from_thread(avctx, p->avctx, 1); - if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0; fctx->next_finished = finished;
Patch attached in the email. In some cases, the submit packet can result in configurations changes of the hardware decoders. The previous HW context is then freed and a new one created. That context is supposed to move up to the main context and the other threads. It appears that when more than 2 frame threads are involved, the new context doesn't move in the right place (or rather at the right time). And it can create a crash when reusing the old HW context. This patch fixes the issue I could reproduce in VLC with DXVA decoding. I have no idea if this is correct or the side effects induced by this. It seems the right thing to do. Keeping the previous call exhibits the issue. It seems odd the other existing thread context are not updated with the current hwaccel_priv_data. But maybe they are updated later from the "main" thread context, in which case this patch seems solid. From e8abeeff92f5d7b3b553acdb7595d40153cbec1e Mon Sep 17 00:00:00 2001 From: Steve Lhomme <robux4@ycbcr.xyz> Date: Fri, 8 Jul 2022 11:49:27 +0200 Subject: [PATCH] avcodec/pthread_frame: update the main avctx from the current ThreadContext After a submit_decoder() the hwaccel_priv_data may have changed in that avctx. Doing it after the "next available frame" loop will likely point to the hwaccel_priv_data from another PerThreadContext which had the old value which might have been freed, if the submit_decoder() resulting in a format change. --- libavcodec/pthread_frame.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)