[FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Steve Lhomme]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

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.

[-- Attachment #2: 0001-avcodec-pthread_frame-update-the-main-avctx-from-the.patch --]
[-- Type: text/plain, Size: 1489 bytes --]

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(-)

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;
-- 
2.27.0.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2203 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2415 bytes --]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Anton Khirnov]

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.

-- 
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Steve Lhomme]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1083 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: update the main avctx from the current, ThreadContext

[Anton Khirnov]

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?

-- 
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".