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

[Mark Thompson]

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