[FFmpeg-devel] [PATCH v2] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[Andreas Rheinhardt]

James Almer:
> 
> 
> On 12/30/2021 12:26 PM, Hendrik Leppkes wrote:
>> On Thu, Dec 30, 2021 at 4:13 PM James Almer <jamrial at gmail.com> wrote:
>>>
>>>
>>>
>>> On 12/30/2021 12:08 PM, Hendrik Leppkes wrote:
>>>> Aborting decoding of the entire packet on a missing PPS can result in
>>>> missing the actual PPS on streams with badly ordered NALs, where the
>>>> SPS/PPS/VPS are stitched to the back of the previous frame, instead of
>>>> the beginning of the next frame.
>>>>
>>>> Instead, skip the undecodable slice, and let the decoder process
>>>> further
>>>> NALs in the same packet.
>>>>
>>>> If this happens on the first slice, the entire frame will be discarded
>>>> later, otherwise on other slices the decode error flag will be set to
>>>> indicate a missing/corrupt slice.
>>>> ---
>>>>    libavcodec/hevcdec.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
>>>> index 3aa70e2245..89381db240 100644
>>>> --- a/libavcodec/hevcdec.c
>>>> +++ b/libavcodec/hevcdec.c
>>>> @@ -575,7 +575,9 @@ static int hls_slice_header(HEVCContext *s)
>>>>        sh->pps_id = get_ue_golomb_long(gb);
>>>>        if (sh->pps_id >= HEVC_MAX_PPS_COUNT ||
>>>> !s->ps.pps_list[sh->pps_id]) {
>>>>            av_log(s->avctx, AV_LOG_ERROR, "PPS id out of range:
>>>> %d\n", sh->pps_id);
>>>> -        return AVERROR_INVALIDDATA;
>>>> +        if (s->ref)
>>>> +            s->ref->frame->decode_error_flags |=
>>>> FF_DECODE_ERROR_DECODE_SLICES;
>>>
>>> Did you check frame threading decoding with tsan after this? Touching
>>> the output frame at this point could result in thread sync races.
>>>
>>> Maybe it's better to implement signaling slice decoding errors in a
>>> separate patch that properly stores such events internally in the
>>> per-thread context, then sets the flag in the output frame before
>>> returning, like it's done in h264.
>>> Like i said it's not a blocker for this fix, so it can be done later.
>>>
>>
>> As far as I can tell, there is no threading issues here. There is no
>> slice-threading here (only through wave-front parallel processing,
>> which comes later), and no other thread would ever read or write the
>> error field. The HEVCContext "s" is per-thread, and s->ref is accessed
>> quite freely by the surrounding code.
> 
> Ok, s->ref doesn't seem to be touched by hevc_update_thread_context(),
> so i guess it's fine.
> 

If I am not mistaken, s->ref points to one of the elements of s->DPB
(see alloc_frame() in hevc_refs.c); and s->DPB is synced in
hevc_update_thread_context(). So I'd be surprised if there were no data
races during the underlying av_frame_ref() call upon frame-threading.

- Andreas