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

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

> But I can take it out if there is any remaining concern. In reality,
> its quite likely that the entire frame is undecodable in this scenario
> anyway, and the change is only to possibly recover decoding of future
> frames - not partially decode this one, although that might be a bonus
> in some rare cases.
> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".