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

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

[Hendrik Leppkes]

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.
---
 libavcodec/hevcdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 3aa70e2245..c2451d682e 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -575,7 +575,7 @@ 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;
+        return 1; // skip slice with missing PPS
     }
     if (!sh->first_slice_in_pic_flag &&
         s->ps.pps != (HEVCPPS*)s->ps.pps_list[sh->pps_id]->data) {
-- 
2.33.0.windows.2

_______________________________________________
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] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[James Almer]

On 12/28/2021 10:49 AM, 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.
> ---
>   libavcodec/hevcdec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 3aa70e2245..c2451d682e 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -575,7 +575,7 @@ 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;
> +        return 1; // skip slice with missing PPS

The decoder should IMO also set the output frame's decode_error_flags to 
FF_DECODE_ERROR_DECODE_SLICES in this scenario.

>       }
>       if (!sh->first_slice_in_pic_flag &&
>           s->ps.pps != (HEVCPPS*)s->ps.pps_list[sh->pps_id]->data) {
_______________________________________________
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] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[Hendrik Leppkes]

On Tue, Dec 28, 2021 at 3:26 PM James Almer <jamrial@gmail.com> wrote:
>
> On 12/28/2021 10:49 AM, 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.
> > ---
> >   libavcodec/hevcdec.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index 3aa70e2245..c2451d682e 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -575,7 +575,7 @@ 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;
> > +        return 1; // skip slice with missing PPS
>
> The decoder should IMO also set the output frame's decode_error_flags to
> FF_DECODE_ERROR_DECODE_SLICES in this scenario.
>

There is a variety of error conditions that can lead to a slice being
skipped, not just this one. I can potentially add it here, but it
won't be a very exhaustive flag.
If its also the first slice, the frame will fail to decode entirely as
well, although the point primarily here is to allow parsing of
non-image NALs.

- Hendrik
_______________________________________________
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] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[James Almer]

On 12/28/2021 7:01 PM, Hendrik Leppkes wrote:
> On Tue, Dec 28, 2021 at 3:26 PM James Almer <jamrial@gmail.com> wrote:
>>
>> On 12/28/2021 10:49 AM, 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.
>>> ---
>>>    libavcodec/hevcdec.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
>>> index 3aa70e2245..c2451d682e 100644
>>> --- a/libavcodec/hevcdec.c
>>> +++ b/libavcodec/hevcdec.c
>>> @@ -575,7 +575,7 @@ 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;
>>> +        return 1; // skip slice with missing PPS
>>
>> The decoder should IMO also set the output frame's decode_error_flags to
>> FF_DECODE_ERROR_DECODE_SLICES in this scenario.
>>
> 
> There is a variety of error conditions that can lead to a slice being
> skipped, not just this one. I can potentially add it here, but it
> won't be a very exhaustive flag.
> If its also the first slice, the frame will fail to decode entirely as
> well, although the point primarily here is to allow parsing of
> non-image NALs.

What h264 does is set an internal flag on any kind of slice error at 
assorted places, then set the AVFrame one before returning it based on 
that (Needed because of frame threading).

It's not a blocker in any case. But even if it's just set at this point 
for the time being, it would at least be a start.

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

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

[Hendrik Leppkes]

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;
+        return 1; // skip slice with missing PPS, allowing other NALs to be decoded
     }
     if (!sh->first_slice_in_pic_flag &&
         s->ps.pps != (HEVCPPS*)s->ps.pps_list[sh->pps_id]->data) {
-- 
2.33.0.windows.2

_______________________________________________
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 v2] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[James Almer]

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.

> +        return 1; // skip slice with missing PPS, allowing other NALs to be decoded
>       }
>       if (!sh->first_slice_in_pic_flag &&
>           s->ps.pps != (HEVCPPS*)s->ps.pps_list[sh->pps_id]->data) {
_______________________________________________
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 v2] hevcdec: skip slices with missing PPS instead of skipping the entire packet

[Hendrik Leppkes]

On Thu, Dec 30, 2021 at 4:13 PM James Almer <jamrial@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.
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@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 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@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@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 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@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
_______________________________________________
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".