diff mbox series

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

Message ID 1584438009-23765-1-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series [FFmpeg-devel,v3] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie March 17, 2020, 9:40 a.m. UTC
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.

Check and error out 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_decodeframe.c#L736>

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
[v3]: replace assert with check/return, tested in both multi frame/slice mode

 libavcodec/vp9.c      | 18 ++++++++++++++++--
 libavcodec/vp9dec.h   |  5 +++++
 libavcodec/vp9recon.c | 10 ++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

Comments

Carl Eugen Hoyos March 17, 2020, 3:04 p.m. UTC | #1
Am Di., 17. März 2020 um 10:45 Uhr schrieb Linjie Fu <linjie.fu@intel.com>:
>
> 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.
>
> Check and error out if invalid reference frame is used in inter_recon.

I strongly suspect we only want to error out if explode > default.

Not necessarily related to this patch:
In general, FFmpeg should not unnecessarily copy the behaviour
of other libraries.

Carl Eugen
Fu, Linjie March 18, 2020, 1:58 a.m. UTC | #2
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Carl Eugen Hoyos
> Sent: Tuesday, March 17, 2020 23:04
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v3] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> Am Di., 17. März 2020 um 10:45 Uhr schrieb Linjie Fu <linjie.fu@intel.com>:
> >
> > 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.
> >
> > Check and error out if invalid reference frame is used in inter_recon.
> 
> I strongly suspect we only want to error out if explode > default.
> 
> Not necessarily related to this patch:
> In general, FFmpeg should not unnecessarily copy the behaviour
> of other libraries.

Yes, and IMHO it depends on whether such behavior it's valuable and
suitable for FFmpeg.

(In this case, libvpx uses longjmp to error out when block-level error happens,
which seems not suitable/allowed in FFmpeg)

Thanks for the review.

- Linjie
diff mbox series

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 7aaae9b..17a4f7d 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,17 +808,25 @@  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) {
+            av_log(avctx, AV_LOG_ERROR, "No valid reference frame is found, bitstream not supported\n");
+            return AVERROR_INVALIDDATA;
         }
     }
 
@@ -1670,6 +1679,11 @@  FF_ENABLE_DEPRECATION_WARNINGS
     } while (s->pass++ == 1);
     ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
 
+    if (s->td->error_info < 0) {
+        av_log(avctx, AV_LOG_ERROR, "Failed to decode tile data\n");
+        return s->td->error_info;
+    }
+
 finish:
     // ref frame setup
     for (i = 0; i < 8; i++) {
diff --git a/libavcodec/vp9dec.h b/libavcodec/vp9dec.h
index 66573ed..f2585c3 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,
@@ -217,6 +219,9 @@  struct VP9TileData {
     struct { int x, y; } min_mv, max_mv;
     int16_t *block_base, *block, *uvblock_base[2], *uvblock[2];
     uint8_t *eob_base, *uveob_base[2], *eob, *uveob[2];
+
+    // error message
+    int error_info;
 };
 
 void ff_vp9_fill_mv(VP9TileData *td, VP56mv *mv, int mode, int sb);
diff --git a/libavcodec/vp9recon.c b/libavcodec/vp9recon.c
index 49bb04e..9a4e7c7 100644
--- a/libavcodec/vp9recon.c
+++ b/libavcodec/vp9recon.c
@@ -572,6 +572,16 @@  static av_always_inline void inter_recon(VP9TileData *td, int bytesperpixel)
     VP9Block *b = td->b;
     int row = td->row, col = td->col;
 
+    if (s->mvscale[b->ref[0]][0] == REF_INVALID_SCALE ||
+        (b->comp && s->mvscale[b->ref[1]][0] == REF_INVALID_SCALE)) {
+        if (!s->td->error_info) {
+            s->td->error_info = AVERROR_INVALIDDATA;
+            av_log(NULL, AV_LOG_ERROR, "Bitstream not supported, "
+                                       "reference frame has invalid dimensions\n");
+        }
+        return;
+    }
+
     if (s->mvscale[b->ref[0]][0] || (b->comp && s->mvscale[b->ref[1]][0])) {
         if (bytesperpixel == 1) {
             inter_pred_scaled_8bpp(td);