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

[Philip Langdale]

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 at 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