diff mbox series

[FFmpeg-devel,RFC] lavc/hevcdec: add invalid for skip_frame to skip invalid nalus before IRAP

Message ID 20200814103543.23695-1-linjie.justin.fu@gmail.com
State New
Headers show
Series [FFmpeg-devel,RFC] lavc/hevcdec: add invalid for skip_frame to skip invalid nalus before IRAP
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Linjie Fu Aug. 14, 2020, 10:35 a.m. UTC
Add "-skip_frame invalid" option to allow user to request decoder to skip
invalid nalus before an IRAP.

This would benefit decoding pipeline of bitstreams who didn't start from
an IRAP frame. NULL pointer pointing to missing reference may lead to
unexpected hang issues[1] in sub-level like hardware decoding.

Fixing the hang in driver is the first correct thing. Besides, adding a check
in nalu parsing and skip frames until we got the first IRAP would be another
good choice to be more robust for error tolerance.

Also this needs worker thread to update the skip_frames field in time.

[1] https://github.com/intel/media-driver/issues/992

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
Request for comments:
The purpose is to allow decoder to skip frames until an IRAP has arrived, however
not sure whether we already had this in ffmpeg, hence submit a patch and
request for comments.

Skip logic identical for AVDISCARD_NONKEY in decode_nal_unit().

 libavcodec/avcodec.h       | 1 +
 libavcodec/hevcdec.c       | 5 ++++-
 libavcodec/options_table.h | 1 +
 libavcodec/pthread_frame.c | 1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

Comments

Mark Thompson Aug. 14, 2020, 6:23 p.m. UTC | #1
On 14/08/2020 11:35, Linjie Fu wrote:
> Add "-skip_frame invalid" option to allow user to request decoder to skip
> invalid nalus before an IRAP.
> 
> This would benefit decoding pipeline of bitstreams who didn't start from
> an IRAP frame. NULL pointer pointing to missing reference may lead to
> unexpected hang issues[1] in sub-level like hardware decoding.
> 
> Fixing the hang in driver is the first correct thing. Besides, adding a check
> in nalu parsing and skip frames until we got the first IRAP would be another
> good choice to be more robust for error tolerance.
> 
> Also this needs worker thread to update the skip_frames field in time.
> 
> [1] https://github.com/intel/media-driver/issues/992

H.265 has a standard mechanism for dealing with missing reference pictures, which is defined in 8.3.3.  That doesn't directly apply for the case here (it's intended for RASL pictures), but a conformant decoder must be implementing it somewhere - can the Intel driver use that?

Trying to eliminate this case entirely is not going to work - being able to splice intra-refresh streams at any point depends on sensible behaviour for missing references, and streams containing errors due to packet loss can always happen.

> 
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
> Request for comments:
> The purpose is to allow decoder to skip frames until an IRAP has arrived, however
> not sure whether we already had this in ffmpeg, hence submit a patch and
> request for comments.
> 
> Skip logic identical for AVDISCARD_NONKEY in decode_nal_unit(). >
>   libavcodec/avcodec.h       | 1 +
>   libavcodec/hevcdec.c       | 5 ++++-
>   libavcodec/options_table.h | 1 +
>   libavcodec/pthread_frame.c | 1 +
>   4 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index c91b2fd169..376d7f4d6d 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -233,6 +233,7 @@ enum AVDiscard{
>       AVDISCARD_BIDIR   = 16, ///< discard all bidirectional frames
>       AVDISCARD_NONINTRA= 24, ///< discard all non intra frames
>       AVDISCARD_NONKEY  = 32, ///< discard all frames except keyframes
> +    AVDISCARD_INVALID = 33, ///< discard invalid packets like NALs before IRAP

IMO INVALID is not a good name to use here - such packets are perfectly valid in cases like intra-refresh.

"before" would need to be clarified - RADL frames can definitely be thought of as "before" the associated IRAP picture, but this wouldn't be discarding them.

Also, it should probably avoid using H.265-specific terminology for the explanation in the generic header.

>       AVDISCARD_ALL     = 48, ///< discard all
>   };
>   
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index b77df8d89f..78658fd537 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -539,8 +539,11 @@ static int hls_slice_header(HEVCContext *s)
>               ff_hevc_clear_refs(s);
>       }
>       sh->no_output_of_prior_pics_flag = 0;
> -    if (IS_IRAP(s))
> +    if (IS_IRAP(s)) {
>           sh->no_output_of_prior_pics_flag = get_bits1(gb);
> +        if (s->avctx->skip_frame == AVDISCARD_INVALID)
> +            s->avctx->skip_frame = AVDISCARD_DEFAULT;
> +    }
>   
>       sh->pps_id = get_ue_golomb_long(gb);
>       if (sh->pps_id >= HEVC_MAX_PPS_COUNT || !s->ps.pps_list[sh->pps_id]) {
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index 1d0db1b5a4..d52bce5908 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -310,6 +310,7 @@ static const AVOption avcodec_options[] = {
>   {"bidir"           , "discard all bidirectional frames",    0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_BIDIR   }, INT_MIN, INT_MAX, V|D, "avdiscard"},
>   {"nokey"           , "discard all frames except keyframes", 0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_NONKEY  }, INT_MIN, INT_MAX, V|D, "avdiscard"},
>   {"nointra"         , "discard all frames except I frames",  0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_NONINTRA}, INT_MIN, INT_MAX, V|D, "avdiscard"},
> +{"invalid"         , "discard NALUs before IRAP",           0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_INVALID }, INT_MIN, INT_MAX, V|D, "avdiscard"},
>   {"all"             , "discard all frames",                  0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_ALL     }, INT_MIN, INT_MAX, V|D, "avdiscard"},
>   {"bidir_refine", "refine the two motion vectors used in bidirectional macroblocks", OFFSET(bidir_refine), AV_OPT_TYPE_INT, {.i64 = 1 }, 0, 4, V|E},
>   #if FF_API_PRIVATE_OPT
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 3255aa9337..302e6149ac 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -259,6 +259,7 @@ static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,
>   
>           dst->has_b_frames = src->has_b_frames;
>           dst->idct_algo    = src->idct_algo;
> +        dst->skip_frame   = src->skip_frame;
>   
>           dst->bits_per_coded_sample = src->bits_per_coded_sample;
>           dst->sample_aspect_ratio   = src->sample_aspect_ratio;
> 

- Mark
diff mbox series

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index c91b2fd169..376d7f4d6d 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -233,6 +233,7 @@  enum AVDiscard{
     AVDISCARD_BIDIR   = 16, ///< discard all bidirectional frames
     AVDISCARD_NONINTRA= 24, ///< discard all non intra frames
     AVDISCARD_NONKEY  = 32, ///< discard all frames except keyframes
+    AVDISCARD_INVALID = 33, ///< discard invalid packets like NALs before IRAP
     AVDISCARD_ALL     = 48, ///< discard all
 };
 
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index b77df8d89f..78658fd537 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -539,8 +539,11 @@  static int hls_slice_header(HEVCContext *s)
             ff_hevc_clear_refs(s);
     }
     sh->no_output_of_prior_pics_flag = 0;
-    if (IS_IRAP(s))
+    if (IS_IRAP(s)) {
         sh->no_output_of_prior_pics_flag = get_bits1(gb);
+        if (s->avctx->skip_frame == AVDISCARD_INVALID)
+            s->avctx->skip_frame = AVDISCARD_DEFAULT;
+    }
 
     sh->pps_id = get_ue_golomb_long(gb);
     if (sh->pps_id >= HEVC_MAX_PPS_COUNT || !s->ps.pps_list[sh->pps_id]) {
diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index 1d0db1b5a4..d52bce5908 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -310,6 +310,7 @@  static const AVOption avcodec_options[] = {
 {"bidir"           , "discard all bidirectional frames",    0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_BIDIR   }, INT_MIN, INT_MAX, V|D, "avdiscard"},
 {"nokey"           , "discard all frames except keyframes", 0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_NONKEY  }, INT_MIN, INT_MAX, V|D, "avdiscard"},
 {"nointra"         , "discard all frames except I frames",  0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_NONINTRA}, INT_MIN, INT_MAX, V|D, "avdiscard"},
+{"invalid"         , "discard NALUs before IRAP",           0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_INVALID }, INT_MIN, INT_MAX, V|D, "avdiscard"},
 {"all"             , "discard all frames",                  0, AV_OPT_TYPE_CONST, {.i64 = AVDISCARD_ALL     }, INT_MIN, INT_MAX, V|D, "avdiscard"},
 {"bidir_refine", "refine the two motion vectors used in bidirectional macroblocks", OFFSET(bidir_refine), AV_OPT_TYPE_INT, {.i64 = 1 }, 0, 4, V|E},
 #if FF_API_PRIVATE_OPT
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 3255aa9337..302e6149ac 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -259,6 +259,7 @@  static int update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,
 
         dst->has_b_frames = src->has_b_frames;
         dst->idct_algo    = src->idct_algo;
+        dst->skip_frame   = src->skip_frame;
 
         dst->bits_per_coded_sample = src->bits_per_coded_sample;
         dst->sample_aspect_ratio   = src->sample_aspect_ratio;