[FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()

[Fu, Linjie]

> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Fu,
> Linjie
> Sent: Thursday, May 21, 2020 14:38
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Xu, Guangxin <guangxin.xu at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of
> find_ref_idx()
> 
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Fu,
> > Linjie
> > Sent: Monday, May 18, 2020 15:17
> > To: ffmpeg-devel at ffmpeg.org
> > Cc: Xu, Guangxin <guangxin.xu at intel.com>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of
> > find_ref_idx()
> >
> > > From: Fu, Linjie <linjie.fu at intel.com>
> > > Sent: Tuesday, May 12, 2020 21:44
> > > To: ffmpeg-devel at ffmpeg.org
> > > Cc: Xu, Guangxin <guangxin.xu at intel.com>; Fu, Linjie
> <linjie.fu at intel.com>
> > > Subject: [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()
> > >
> > > From: Xu Guangxin <guangxin.xu at intel.com>
> > >
> > > Currently find_ref_idx() would trigger 2 scans in DPB to find the
> > > requested POC:
> > > 1. Firstly, ignore MSB of ref->poc and search for the requested POC;
> > > 2. Secondly, compare the entire ref->poc with requested POC;
> > >
> > > For long term reference, we are able to only check LSB if MSB is not
> > > presented(e.g. delta_poc_msb_present_flag == 0). However, for short
> > > term reference, we should never ignore poc's MSB and it should be
> > > kind of bit-exact. (Details in 8.3.2)
> > >
> > > Otherwise this leads to decoding failures like:
> > > [hevc @ 0x5638f4328600] Error constructing the frame RPS.
> > > [hevc @ 0x5638f4328600] Error parsing NAL unit #2.
> > > [hevc @ 0x5638f4338a80] Could not find ref with POC 21
> > > Error while decoding stream #0:0: Invalid data found when processing
> input
> > >
> > > Search the requested POC based on whether MSB is used, and avoid
> > > the 2-times scan for DPB buffer. This benefits both native HEVC
> > > decoder and integrated HW decoders.
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> > > ---
> > >
> > > Since it's kind of difficult to generate an identical bitstream for
> > > fate or test, I'd like to elaborate more about one of the failures:
> > >
> > >     requested POC = 5;
> > >     LtMask = (1 << 4) - 1 = 15;
> > >     ref[0]->poc = 21; // unexpected ref for poc = 5 (short term)
> > >     ref[1]->poc = 5;  // expected ref for poc = 5 (short term)
> > >
> > > Hence find_ref_idx() would wrongly return a ref with poc = 21, which
> > > leads to the decoding error.
> > >
> > >  libavcodec/hevc_refs.c | 38 ++++++++++++++++++++------------------
> > >  1 file changed, 20 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> > > index 7870a72..73aa6d8 100644
> > > --- a/libavcodec/hevc_refs.c
> > > +++ b/libavcodec/hevc_refs.c
> > > @@ -358,24 +358,26 @@ int ff_hevc_slice_rpl(HEVCContext *s)
> > >      return 0;
> > >  }
> > >
> > > -static HEVCFrame *find_ref_idx(HEVCContext *s, int poc)
> > > +static HEVCFrame *find_ref_idx(HEVCContext *s, int poc, uint8_t
> > use_msb)
> > >  {
> > >      int i;
> > > -    int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> > >
> > > -    for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > -        HEVCFrame *ref = &s->DPB[i];
> > > -        if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > > -            if ((ref->poc & LtMask) == poc)
> > > -                return ref;
> > > +    if (use_msb) {
> > > +        for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > +            HEVCFrame *ref = &s->DPB[i];
> > > +            if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > > +                if (ref->poc == poc)
> > > +                    return ref;
> > > +            }
> > >          }
> > > -    }
> > > -
> > > -    for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > -        HEVCFrame *ref = &s->DPB[i];
> > > -        if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> > > -            if (ref->poc == poc || (ref->poc & LtMask) == poc)
> > > -                return ref;
> > > +    } else {
> > > +        int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> > > +        for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > +            HEVCFrame *ref = &s->DPB[i];
> > > +            if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> > > +                if ((ref->poc & LtMask) == poc)
> > > +                    return ref;
> > > +            }
> > >          }
> > >      }
> > >
> > > @@ -427,9 +429,9 @@ static HEVCFrame
> > > *generate_missing_ref(HEVCContext *s, int poc)
> > >
> > >  /* add a reference with the given poc to the list and mark it as used in
> DPB
> > > */
> > >  static int add_candidate_ref(HEVCContext *s, RefPicList *list,
> > > -                             int poc, int ref_flag)
> > > +                             int poc, int ref_flag, uint8_t use_msb)
> > >  {
> > > -    HEVCFrame *ref = find_ref_idx(s, poc);
> > > +    HEVCFrame *ref = find_ref_idx(s, poc, use_msb);
> > >
> > >      if (ref == s->ref || list->nb_refs >= HEVC_MAX_REFS)
> > >          return AVERROR_INVALIDDATA;
> > > @@ -485,7 +487,7 @@ int ff_hevc_frame_rps(HEVCContext *s)
> > >          else
> > >              list = ST_CURR_AFT;
> > >
> > > -        ret = add_candidate_ref(s, &rps[list], poc,
> > > HEVC_FRAME_FLAG_SHORT_REF);
> > > +        ret = add_candidate_ref(s, &rps[list], poc,
> > > HEVC_FRAME_FLAG_SHORT_REF, 1);
> > >          if (ret < 0)
> > >              goto fail;
> > >      }
> > > @@ -495,7 +497,7 @@ int ff_hevc_frame_rps(HEVCContext *s)
> > >          int poc  = long_rps->poc[i];
> > >          int list = long_rps->used[i] ? LT_CURR : LT_FOLL;
> > >
> > > -        ret = add_candidate_ref(s, &rps[list], poc,
> > > HEVC_FRAME_FLAG_LONG_REF);
> > > +        ret = add_candidate_ref(s, &rps[list], poc,
> > > HEVC_FRAME_FLAG_LONG_REF, long_rps->poc_msb_present[i]);
> > >          if (ret < 0)
> > >              goto fail;
> > >      }
> > > --
> > > 2.7.4
> >
> > Ping for this patch set.
> > Verified with 1000 + decoding conformance cases including 8/10 bit, no
> > regression observed.
> >
> 
> Also see descriptions in chapter 2.4.3 in <High Efficiency Video Coding>[1]:
> 
> "Since it is possible to have more than one picture with the same POC LSB
> value in the DPB, there is an optional
> possibility to also signal POC MSB information for long-term pictures."
> 
> This patch is to fix the potential " more than one picture with the same POC
> LSB value in the DPB" issue.
> 
> Ping for comments. (Would like to apply the set after this weekend,  if no
> objections)
> 
> - Linjie
> 
> [1] < http://what-when-how.com/Tutorial/topic-397pct9eq3/High-Efficiency-
> Video-Coding-HEVC-51.html>

Applied, thx.

- Linjie