[FFmpeg-devel] [PATCH] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode

[Fu, Linjie]

> From: Fu, Linjie <linjie.fu at intel.com>
> Sent: Friday, March 13, 2020 17:18
> To: ffmpeg-devel at ffmpeg.org
> Cc: Fu, Linjie <linjie.fu at intel.com>
> Subject: [PATCH] lavc/vp9: fix reference frame dimensions check for
> SINGLE_REFERENCE mode
> 
> With the description in frame size with refs semantics (SPEC 7.2.5),
> it is a requirement of bitstream conformance that for at least one
> reference frame has the valid dimensions.
> 
> Modify the check to make sure the decoder works well in
> SINGLE_REFERENCE
> mode that not all reference frames have valid dimensions.
> 
> Assert if invalid reference frame is used in inter_recon.
> 
> One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
> with following reference pool:
> 
> 0.  960x544    LAST    valid
> 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> 3~7  ...     Unused
> 
> Identical logic in libvpx:
> <https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_d
> ecodeframe.c#L736>
> 
> Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> ---
>  libavcodec/vp9.c      | 11 +++++++++--
>  libavcodec/vp9dec.h   |  2 ++
>  libavcodec/vp9recon.c |  4 ++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index 7aaae9b..b9f6ac1 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -794,6 +794,7 @@ static int decode_frame_header(AVCodecContext
> *avctx,
> 
>      /* check reference frames */
>      if (!s->s.h.keyframe && !s->s.h.intraonly) {
> +        int valid_ref_frame = 0;
>          for (i = 0; i < 3; i++) {
>              AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f;
>              int refw = ref->width, refh = ref->height;
> @@ -807,18 +808,24 @@ static int decode_frame_header(AVCodecContext
> *avctx,
>              } else if (refw == w && refh == h) {
>                  s->mvscale[i][0] = s->mvscale[i][1] = 0;
>              } else {
> +                /* Check to make sure at least one of frames that */
> +                /* this frame references has valid dimensions. */
>                  if (w * 2 < refw || h * 2 < refh || w > 16 * refw || h > 16 * refh) {
> -                    av_log(avctx, AV_LOG_ERROR,
> +                    av_log(avctx, AV_LOG_WARNING,
>                             "Invalid ref frame dimensions %dx%d for frame size %dx%d\n",
>                             refw, refh, w, h);
> -                    return AVERROR_INVALIDDATA;
> +                    s->mvscale[i][0] = s->mvscale[i][1] = REF_INVALID_SCALE;
> +                    continue;
>                  }
>                  s->mvscale[i][0] = (refw << 14) / w;
>                  s->mvscale[i][1] = (refh << 14) / h;
>                  s->mvstep[i][0] = 16 * s->mvscale[i][0] >> 14;
>                  s->mvstep[i][1] = 16 * s->mvscale[i][1] >> 14;
>              }
> +            valid_ref_frame++;
>          }
> +        if (!valid_ref_frame)
> +            return AVERROR_INVALIDDATA;
>      }
> 
>      if (s->s.h.keyframe || s->s.h.errorres || (s->s.h.intraonly && s-
> >s.h.resetctx == 3)) {
> diff --git a/libavcodec/vp9dec.h b/libavcodec/vp9dec.h
> index 66573ed..f68fe1b 100644
> --- a/libavcodec/vp9dec.h
> +++ b/libavcodec/vp9dec.h
> @@ -36,6 +36,8 @@
>  #include "vp9dsp.h"
>  #include "vp9shared.h"
> 
> +#define REF_INVALID_SCALE 0xFFFF
> +
>  enum MVJoint {
>      MV_JOINT_ZERO,
>      MV_JOINT_H,
> diff --git a/libavcodec/vp9recon.c b/libavcodec/vp9recon.c
> index 49bb04e..0611d05 100644
> --- a/libavcodec/vp9recon.c
> +++ b/libavcodec/vp9recon.c
> @@ -572,6 +572,10 @@ static av_always_inline void inter_recon(VP9TileData
> *td, int bytesperpixel)
>      VP9Block *b = td->b;
>      int row = td->row, col = td->col;
> 
> +    /* assert if mvscale factor is marked as invalid in the reference frame size
> check */
> +    if (s->mvscale[b->ref[0]][0] == REF_INVALID_SCALE || (b->comp && s-
> >mvscale[b->ref[1]][0] ==REF_INVALID_SCALE))
> +        av_assert0(0 && "Reference frame has invalid dimensions");
> +
>      if (s->mvscale[b->ref[0]][0] || (b->comp && s->mvscale[b->ref[1]][0])) {
>          if (bytesperpixel == 1) {
>              inter_pred_scaled_8bpp(td);
> --

Find out some comments towards similar patch in[1]:

> What does libvpx do with the "invalid" refs?
Libvpx marks the mv scale factors as REF_INVALID_SCALE for these invalid refs. And while in
dec_build_inter_predictors_sb(), the scale factor would be checked.

In this kind of cases, the inter frame is in SINGLE_REFERENCE mode, and it only referenced the
"valid" frame, and the "invalid" refs are actually ignored for both libvpx and ffvp9.

> What do hardware decoders do?
Since it's available for ffvp9 as well, nothing special is needed for hardware decoder about the
Reference management.

> What does the spec formally require
Checked the spec, however didn't find how the invalid reference frame should be coped with
in compound reference mode.(According to the code in libvpx, decoder would report error if they are
actually used)

Hope this could answer the questions, please help to review.

[1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2019-July/246404.html

- Linjie