diff mbox

[FFmpeg-devel] Fixing HW accelerated decoders hanging or throwing error in h263dec

Message ID MN2PR12MB3357DAEE2E8B66F703A4B673A9D90@MN2PR12MB3357.namprd12.prod.outlook.com
State New
Headers show

Commit Message

Stefan Schoenefeld Aug. 2, 2019, 9:18 a.m. UTC 
Hi all,

Recently we encountered an issue when decoding a h.263 file:

FFmpeg will freeze when decoding h.263 video with NVDEC. Turns out this is not directly related to NVDEC but is a problem that shows with several other HW decoders like VDPAU, though the exact kind of error is different (either error messages or freezing[1]). The root cause is that ff_thread_finish_setup() is called twice per frame from ff_h263_decode_frame(). This is not supported by ff_thread_finish_setup() and specifically checked for and warned against in the functions code. The issue is also specific to hw accelerated decoding only as the second call to ff_thread_finish_setup() is only issued when hw acceleration is on. The fix is simple: add a check that the first call is only send when hw acceleration is off, and the second call only when hw acceleration is on (see attached patch). This works fine as far as I was able to test with vdpau and nvdec/nvcuvid hw decoding. The patch also adds NVDEC to the hw config list if available.

I also noticed a secondary issue when browsing through the code which is that, according to documentation, ff_thread_finish_setup() should only be called if the codec implements update_thread_context(), which h263dec does not. The patch does not address this and I'm not sure any action needs to be taken here at all.

Thanks,
Stefan

[1] This is depending on whether or not the hw decoder sets the  HWACCEL_CAPS_ASYNC_SAFE flag

From 0620ee777a8ba98145bb4781e30a77687c97dbf8 Mon Sep 17 00:00:00 2001
From: Stefan Schoenefeld <sschoenefeld@nvidia.com>
Date: Fri, 2 Aug 2019 10:52:22 +0200
Subject: [PATCH] Fixing HW accelerated decoders hanging or throwing error
messages in h263dec

---
libavcodec/h263dec.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

     HWACCEL_VAAPI(h263),
#endif
+#if CONFIG_MPEG4_NVDEC_HWACCEL
+    HWACCEL_NVDEC(mpeg4),
+#endif
#if CONFIG_MPEG4_VDPAU_HWACCEL
     HWACCEL_VDPAU(mpeg4),
#endif
--
2.7.4


NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Comments

Timo Rothenpieler Aug. 2, 2019, 4:09 p.m. UTC | #1 
On 02.08.2019 11:18, Stefan Schoenefeld wrote:
> Hi all,
> 
> Recently we encountered an issue when decoding a h.263 file:
> 
> FFmpeg will freeze when decoding h.263 video with NVDEC. Turns out this is not directly related to NVDEC but is a problem that shows with several other HW decoders like VDPAU, though the exact kind of error is different (either error messages or freezing[1]). The root cause is that ff_thread_finish_setup() is called twice per frame from ff_h263_decode_frame(). This is not supported by ff_thread_finish_setup() and specifically checked for and warned against in the functions code. The issue is also specific to hw accelerated decoding only as the second call to ff_thread_finish_setup() is only issued when hw acceleration is on. The fix is simple: add a check that the first call is only send when hw acceleration is off, and the second call only when hw acceleration is on (see attached patch). This works fine as far as I was able to test with vdpau and nvdec/nvcuvid hw decoding. The patch also adds NVDEC to the hw config list if available.
> 
> I also noticed a secondary issue when browsing through the code which is that, according to documentation, ff_thread_finish_setup() should only be called if the codec implements update_thread_context(), which h263dec does not. The patch does not address this and I'm not sure any action needs to be taken here at all.
> 
> Thanks,
> Stefan
> 
> [1] This is depending on whether or not the hw decoder sets the  HWACCEL_CAPS_ASYNC_SAFE flag
> 
>  From 0620ee777a8ba98145bb4781e30a77687c97dbf8 Mon Sep 17 00:00:00 2001
> From: Stefan Schoenefeld <sschoenefeld@nvidia.com>
> Date: Fri, 2 Aug 2019 10:52:22 +0200
> Subject: [PATCH] Fixing HW accelerated decoders hanging or throwing error
> messages in h263dec
> 
> ---
> libavcodec/h263dec.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
> index 6f001f6..8ee844e 100644
> --- a/libavcodec/h263dec.c
> +++ b/libavcodec/h263dec.c
> @@ -614,7 +614,7 @@ retry:
>       if ((ret = ff_mpv_frame_start(s, avctx)) < 0)
>           return ret;
> -    if (!s->divx_packed)
> +    if (!s->divx_packed && !avctx->hwaccel)
>           ff_thread_finish_setup(avctx);

This seems correct to me, but I'd like another pair of eyes to look over it.

>       if (avctx->hwaccel) {
> @@ -747,6 +747,9 @@ const AVCodecHWConfigInternal *ff_h263_hw_config_list[] = {
> #if CONFIG_H263_VAAPI_HWACCEL
>       HWACCEL_VAAPI(h263),
> #endif
> +#if CONFIG_MPEG4_NVDEC_HWACCEL
> +    HWACCEL_NVDEC(mpeg4),
> +#endif

Probably cleaner to split this into its own patch, but otherwise OK.

> #if CONFIG_MPEG4_VDPAU_HWACCEL
>       HWACCEL_VDPAU(mpeg4),
> #endif
> --
> 2.7.4
>
Philip Langdale Aug. 3, 2019, 6:56 a.m. UTC | #2 
On 2019-08-03 00:09, Timo Rothenpieler wrote:
> On 02.08.2019 11:18, Stefan Schoenefeld wrote:
>> Hi all,
>> 
>> Recently we encountered an issue when decoding a h.263 file:
>> 
>> FFmpeg will freeze when decoding h.263 video with NVDEC. Turns out 
>> this is not directly related to NVDEC but is a problem that shows with 
>> several other HW decoders like VDPAU, though the exact kind of error 
>> is different (either error messages or freezing[1]). The root cause is 
>> that ff_thread_finish_setup() is called twice per frame from 
>> ff_h263_decode_frame(). This is not supported by 
>> ff_thread_finish_setup() and specifically checked for and warned 
>> against in the functions code. The issue is also specific to hw 
>> accelerated decoding only as the second call to 
>> ff_thread_finish_setup() is only issued when hw acceleration is on. 
>> The fix is simple: add a check that the first call is only send when 
>> hw acceleration is off, and the second call only when hw acceleration 
>> is on (see attached patch). This works fine as far as I was able to 
>> test with vdpau and nvdec/nvcuvid hw decoding. The patch also adds 
>> NVDEC to the hw config list if available.
>> 
>> I also noticed a secondary issue when browsing through the code which 
>> is that, according to documentation, ff_thread_finish_setup() should 
>> only be called if the codec implements update_thread_context(), which 
>> h263dec does not. The patch does not address this and I'm not sure any 
>> action needs to be taken here at all.
>> 
>> Thanks,
>> Stefan
>> 
>> [1] This is depending on whether or not the hw decoder sets the  
>> HWACCEL_CAPS_ASYNC_SAFE flag
>> 
>>  From 0620ee777a8ba98145bb4781e30a77687c97dbf8 Mon Sep 17 00:00:00 
>> 2001
>> From: Stefan Schoenefeld <sschoenefeld@nvidia.com>
>> Date: Fri, 2 Aug 2019 10:52:22 +0200
>> Subject: [PATCH] Fixing HW accelerated decoders hanging or throwing 
>> error
>> messages in h263dec
>> 
>> ---
>> libavcodec/h263dec.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
>> index 6f001f6..8ee844e 100644
>> --- a/libavcodec/h263dec.c
>> +++ b/libavcodec/h263dec.c
>> @@ -614,7 +614,7 @@ retry:
>>       if ((ret = ff_mpv_frame_start(s, avctx)) < 0)
>>           return ret;
>> -    if (!s->divx_packed)
>> +    if (!s->divx_packed && !avctx->hwaccel)
>>           ff_thread_finish_setup(avctx);
> 
> This seems correct to me, but I'd like another pair of eyes to look 
> over it.

Seems fine to me as well.

> 
>>       if (avctx->hwaccel) {
>> @@ -747,6 +747,9 @@ const AVCodecHWConfigInternal 
>> *ff_h263_hw_config_list[] = {
>> #if CONFIG_H263_VAAPI_HWACCEL
>>       HWACCEL_VAAPI(h263),
>> #endif
>> +#if CONFIG_MPEG4_NVDEC_HWACCEL
>> +    HWACCEL_NVDEC(mpeg4),
>> +#endif
> 
> Probably cleaner to split this into its own patch, but otherwise OK.
> 
>> #if CONFIG_MPEG4_VDPAU_HWACCEL
>>       HWACCEL_VDPAU(mpeg4),
>> #endif

Thanks for fixing this. I was always confused about whether h.263 was 
really supported by
the hardware or not. Can you provide a link to a reference sample that 
fails without this
change and succeeds with it?

--phil
Timo Rothenpieler Aug. 4, 2019, 2:26 p.m. UTC | #3 
On 02.08.2019 11:18, Stefan Schoenefeld wrote:
> Hi all,
> 
> Recently we encountered an issue when decoding a h.263 file:
> 
> FFmpeg will freeze when decoding h.263 video with NVDEC. Turns out this is not directly related to NVDEC but is a problem that shows with several other HW decoders like VDPAU, though the exact kind of error is different (either error messages or freezing[1]). The root cause is that ff_thread_finish_setup() is called twice per frame from ff_h263_decode_frame(). This is not supported by ff_thread_finish_setup() and specifically checked for and warned against in the functions code. The issue is also specific to hw accelerated decoding only as the second call to ff_thread_finish_setup() is only issued when hw acceleration is on. The fix is simple: add a check that the first call is only send when hw acceleration is off, and the second call only when hw acceleration is on (see attached patch). This works fine as far as I was able to test with vdpau and nvdec/nvcuvid hw decoding. The patch also adds NVDEC to the hw config list if available.
> 
> I also noticed a secondary issue when browsing through the code which is that, according to documentation, ff_thread_finish_setup() should only be called if the codec implements update_thread_context(), which h263dec does not. The patch does not address this and I'm not sure any action needs to be taken here at all.
> 
> Thanks,
> Stefan
> 
> [1] This is depending on whether or not the hw decoder sets the  HWACCEL_CAPS_ASYNC_SAFE flag

Split in two, applied and backported to relevant branches.
Stefan Schoenefeld Aug. 7, 2019, 8:58 a.m. UTC | #4 
Hi Phil,

Unfortunately I cannot share the sample we use internally, but it should be reproducible with any h.263/mpeg-4 encoded file & hw accelerated decoding.

Thanks
Stefan

-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Philip Langdale

Sent: Saturday, August 3, 2019 08:57
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] Fixing HW accelerated decoders hanging or throwing error in h263dec

On 2019-08-03 00:09, Timo Rothenpieler wrote:
> On 02.08.2019 11:18, Stefan Schoenefeld wrote:

>> Hi all,

>> 

>> Recently we encountered an issue when decoding a h.263 file:

>> 

>> FFmpeg will freeze when decoding h.263 video with NVDEC. Turns out 

>> this is not directly related to NVDEC but is a problem that shows 

>> with several other HW decoders like VDPAU, though the exact kind of 

>> error is different (either error messages or freezing[1]). The root 

>> cause is that ff_thread_finish_setup() is called twice per frame from 

>> ff_h263_decode_frame(). This is not supported by

>> ff_thread_finish_setup() and specifically checked for and warned 

>> against in the functions code. The issue is also specific to hw 

>> accelerated decoding only as the second call to

>> ff_thread_finish_setup() is only issued when hw acceleration is on. 

>> The fix is simple: add a check that the first call is only send when 

>> hw acceleration is off, and the second call only when hw acceleration 

>> is on (see attached patch). This works fine as far as I was able to 

>> test with vdpau and nvdec/nvcuvid hw decoding. The patch also adds 

>> NVDEC to the hw config list if available.

>> 

>> I also noticed a secondary issue when browsing through the code which 

>> is that, according to documentation, ff_thread_finish_setup() should 

>> only be called if the codec implements update_thread_context(), which 

>> h263dec does not. The patch does not address this and I'm not sure 

>> any action needs to be taken here at all.

>> 

>> Thanks,

>> Stefan

>> 

>> [1] This is depending on whether or not the hw decoder sets the 

>> HWACCEL_CAPS_ASYNC_SAFE flag

>> 

>>  From 0620ee777a8ba98145bb4781e30a77687c97dbf8 Mon Sep 17 00:00:00

>> 2001

>> From: Stefan Schoenefeld <sschoenefeld@nvidia.com>

>> Date: Fri, 2 Aug 2019 10:52:22 +0200

>> Subject: [PATCH] Fixing HW accelerated decoders hanging or throwing 

>> error messages in h263dec

>> 

>> ---

>> libavcodec/h263dec.c | 5 ++++-

>> 1 file changed, 4 insertions(+), 1 deletion(-)

>> 

>> diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c index 

>> 6f001f6..8ee844e 100644

>> --- a/libavcodec/h263dec.c

>> +++ b/libavcodec/h263dec.c

>> @@ -614,7 +614,7 @@ retry:

>>       if ((ret = ff_mpv_frame_start(s, avctx)) < 0)

>>           return ret;

>> -    if (!s->divx_packed)

>> +    if (!s->divx_packed && !avctx->hwaccel)

>>           ff_thread_finish_setup(avctx);

> 

> This seems correct to me, but I'd like another pair of eyes to look 

> over it.


Seems fine to me as well.

> 

>>       if (avctx->hwaccel) {

>> @@ -747,6 +747,9 @@ const AVCodecHWConfigInternal 

>> *ff_h263_hw_config_list[] = { #if CONFIG_H263_VAAPI_HWACCEL

>>       HWACCEL_VAAPI(h263),

>> #endif

>> +#if CONFIG_MPEG4_NVDEC_HWACCEL

>> +    HWACCEL_NVDEC(mpeg4),

>> +#endif

> 

> Probably cleaner to split this into its own patch, but otherwise OK.

> 

>> #if CONFIG_MPEG4_VDPAU_HWACCEL

>>       HWACCEL_VDPAU(mpeg4),

>> #endif


Thanks for fixing this. I was always confused about whether h.263 was really supported by the hardware or not. Can you provide a link to a reference sample that fails without this change and succeeds with it?

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

NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
diff mbox

Patch

diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
index 6f001f6..8ee844e 100644
--- a/libavcodec/h263dec.c
+++ b/libavcodec/h263dec.c
@@ -614,7 +614,7 @@  retry:
     if ((ret = ff_mpv_frame_start(s, avctx)) < 0)
         return ret;
-    if (!s->divx_packed)
+    if (!s->divx_packed && !avctx->hwaccel)
         ff_thread_finish_setup(avctx);
     if (avctx->hwaccel) {
@@ -747,6 +747,9 @@  const AVCodecHWConfigInternal *ff_h263_hw_config_list[] = {
#if CONFIG_H263_VAAPI_HWACCEL