[FFmpeg-devel,8/8] avcodec/nuv: Avoid duplicating frames

Submitted by Michael Niedermayer on Aug. 12, 2019, 7:17 p.m.

Details

Message ID 20190812191708.22608-8-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 12, 2019, 7:17 p.m.
Fixes: Timeout (14sec -> 133ms)
Fixes: 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
Fixes: 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280 (35sec ->0.5sec)

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/nuv.c          | 37 ++++++++++++++++++++++++++++++++++---
 tests/ref/fate/nuv-rtjpeg |  1 -
 2 files changed, 34 insertions(+), 4 deletions(-)

Comments

Paul B Mahol Aug. 14, 2019, 4:02 p.m.
On Mon, Aug 12, 2019 at 9:20 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Fixes: Timeout (14sec -> 133ms)
> Fixes:
> 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
> Fixes:
> 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280
> (35sec ->0.5sec)
>
>
Why? This is bad idea, same like for qtrle and bunch of other cases.



> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/nuv.c          | 37 ++++++++++++++++++++++++++++++++++---
>  tests/ref/fate/nuv-rtjpeg |  1 -
>  2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
> index 75b14bce5b..0952b537b7 100644
> --- a/libavcodec/nuv.c
> +++ b/libavcodec/nuv.c
> @@ -42,6 +42,8 @@ typedef struct NuvContext {
>      unsigned char *decomp_buf;
>      uint32_t lq[64], cq[64];
>      RTJpegContext rtj;
> +    int need_flush;
> +    AVPacket flush_pkt;
>  } NuvContext;
>
>  static const uint8_t fallback_lquant[] = {
> @@ -66,6 +68,12 @@ static const uint8_t fallback_cquant[] = {
>      99, 99, 99, 99, 99, 99, 99, 99
>  };
>
> +static void decode_flush(AVCodecContext *avctx){
> +    NuvContext *s = avctx->priv_data;
> +
> +    s->need_flush = 0;
> +}
> +
>  /**
>   * @brief copy frame data from buffer to AVFrame, handling stride.
>   * @param f destination AVFrame
> @@ -172,6 +180,26 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame,
>          NUV_COPY_LAST     = 'L'
>      } comptype;
>
> +    if (!avpkt->data) {
> +        if (c->need_flush) {
> +            c->need_flush = 0;
> +            if ((ret = ff_reget_buffer(avctx, c->pic)) < 0)
> +                return ret;
> +            c->pic->pkt_pos      = c->flush_pkt.pos;
> +            c->pic->pkt_duration = c->flush_pkt.duration;
> +            c->pic->pkt_dts      = c->flush_pkt.dts;
> +            c->pic->pkt_pts      =
> +            c->pic->pts          = c->flush_pkt.pts;
> +            if ((ret = av_frame_ref(data, c->pic)) < 0)
> +                return ret;
> +            *got_frame = 1;
> +        }
> +        return 0;
> +    }
> +    c->flush_pkt = *avpkt;
> +    c->pic->pkt_dts = c->flush_pkt.dts;
> +
> +
>      if (buf_size < 12) {
>          av_log(avctx, AV_LOG_ERROR, "coded frame too small\n");
>          return AVERROR_INVALIDDATA;
> @@ -204,8 +232,8 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame,
>          }
>          break;
>      case NUV_COPY_LAST:
> -        keyframe = 0;
> -        break;
> +        c->need_flush = 1;
> +        return buf_size;
>      default:
>          keyframe = 1;
>          break;
> @@ -313,6 +341,7 @@ retry:
>      if ((result = av_frame_ref(picture, c->pic)) < 0)
>          return result;
>
> +    c->need_flush = 0;
>      *got_frame = 1;
>      return orig_size;
>  }
> @@ -364,6 +393,8 @@ AVCodec ff_nuv_decoder = {
>      .init           = decode_init,
>      .close          = decode_end,
>      .decode         = decode_frame,
> -    .capabilities   = AV_CODEC_CAP_DR1,
> +    .flush          = decode_flush,
> +    .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>      .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  };
> diff --git a/tests/ref/fate/nuv-rtjpeg b/tests/ref/fate/nuv-rtjpeg
> index b6f3b080dc..0914b985ec 100644
> --- a/tests/ref/fate/nuv-rtjpeg
> +++ b/tests/ref/fate/nuv-rtjpeg
> @@ -6,7 +6,6 @@
>  0,        118,        118,        0,   460800, 0x54aedafe
>  0,        152,        152,        0,   460800, 0xb7aa8b56
>  0,        177,        177,        0,   460800, 0x283ea3b5
> -0,        202,        202,        0,   460800, 0x283ea3b5
>  0,        235,        235,        0,   460800, 0x10e577de
>  0,        269,        269,        0,   460800, 0x4e091ee2
>  0,        302,        302,        0,   460800, 0x2ea88828
> --
> 2.22.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Aug. 14, 2019, 6:06 p.m.
On Wed, Aug 14, 2019 at 06:02:13PM +0200, Paul B Mahol wrote:
> On Mon, Aug 12, 2019 at 9:20 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Fixes: Timeout (14sec -> 133ms)
> > Fixes:
> > 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
> > Fixes:
> > 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280
> > (35sec ->0.5sec)
> >
> >
> Why? This is bad idea, same like for qtrle and bunch of other cases.

This was discussed previously IIRC
Either a codec can be used to turn tiny input to huge number of output frames 
by frame duplication. or
it doesnt do the duplication and rather provides metadata of some sort

In the first case if the next step is a filter it can be slower
In the first case if the next step is a variable fps encoder it can be slower
in the first case it can make a DOS attack less expensive for an attacker

The second case, that is with metadata for example not returning a frame but
relying on timestamps these issues are reduced
thats why i suggest that way but if the community prefers something else then
sure it can be done. But from what i remember the oppinions where mixed on
which way is preferred

thanks

[...]
James Almer Aug. 15, 2019, 5:06 p.m.
On 8/12/2019 4:17 PM, Michael Niedermayer wrote:
> Fixes: Timeout (14sec -> 133ms)
> Fixes: 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
> Fixes: 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280 (35sec ->0.5sec)
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/nuv.c          | 37 ++++++++++++++++++++++++++++++++++---
>  tests/ref/fate/nuv-rtjpeg |  1 -
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
> index 75b14bce5b..0952b537b7 100644
> --- a/libavcodec/nuv.c
> +++ b/libavcodec/nuv.c
> @@ -42,6 +42,8 @@ typedef struct NuvContext {
>      unsigned char *decomp_buf;
>      uint32_t lq[64], cq[64];
>      RTJpegContext rtj;
> +    int need_flush;
> +    AVPacket flush_pkt;
>  } NuvContext;
>  
>  static const uint8_t fallback_lquant[] = {
> @@ -66,6 +68,12 @@ static const uint8_t fallback_cquant[] = {
>      99, 99, 99, 99, 99, 99, 99, 99
>  };
>  
> +static void decode_flush(AVCodecContext *avctx){
> +    NuvContext *s = avctx->priv_data;
> +
> +    s->need_flush = 0;
> +}
> +
>  /**
>   * @brief copy frame data from buffer to AVFrame, handling stride.
>   * @param f destination AVFrame
> @@ -172,6 +180,26 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>          NUV_COPY_LAST     = 'L'
>      } comptype;
>  
> +    if (!avpkt->data) {
> +        if (c->need_flush) {
> +            c->need_flush = 0;
> +            if ((ret = ff_reget_buffer(avctx, c->pic)) < 0)
> +                return ret;
> +            c->pic->pkt_pos      = c->flush_pkt.pos;
> +            c->pic->pkt_duration = c->flush_pkt.duration;
> +            c->pic->pkt_dts      = c->flush_pkt.dts;
> +            c->pic->pkt_pts      =
> +            c->pic->pts          = c->flush_pkt.pts;
> +            if ((ret = av_frame_ref(data, c->pic)) < 0)
> +                return ret;
> +            *got_frame = 1;

Can't this be split off and shared in some way instead of implementing
it for every decoder that needs to remove duplicate frames? Or maybe
handle it in the generic decode.c code.

> +        }
> +        return 0;
> +    }
> +    c->flush_pkt = *avpkt;
> +    c->pic->pkt_dts = c->flush_pkt.dts;
> +
> +
>      if (buf_size < 12) {
>          av_log(avctx, AV_LOG_ERROR, "coded frame too small\n");
>          return AVERROR_INVALIDDATA;
> @@ -204,8 +232,8 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>          }
>          break;
>      case NUV_COPY_LAST:
> -        keyframe = 0;
> -        break;
> +        c->need_flush = 1;
> +        return buf_size;
>      default:
>          keyframe = 1;
>          break;
> @@ -313,6 +341,7 @@ retry:
>      if ((result = av_frame_ref(picture, c->pic)) < 0)
>          return result;
>  
> +    c->need_flush = 0;
>      *got_frame = 1;
>      return orig_size;
>  }
> @@ -364,6 +393,8 @@ AVCodec ff_nuv_decoder = {
>      .init           = decode_init,
>      .close          = decode_end,
>      .decode         = decode_frame,
> -    .capabilities   = AV_CODEC_CAP_DR1,
> +    .flush          = decode_flush,
> +    .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>      .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  };
> diff --git a/tests/ref/fate/nuv-rtjpeg b/tests/ref/fate/nuv-rtjpeg
> index b6f3b080dc..0914b985ec 100644
> --- a/tests/ref/fate/nuv-rtjpeg
> +++ b/tests/ref/fate/nuv-rtjpeg
> @@ -6,7 +6,6 @@
>  0,        118,        118,        0,   460800, 0x54aedafe
>  0,        152,        152,        0,   460800, 0xb7aa8b56
>  0,        177,        177,        0,   460800, 0x283ea3b5
> -0,        202,        202,        0,   460800, 0x283ea3b5
>  0,        235,        235,        0,   460800, 0x10e577de
>  0,        269,        269,        0,   460800, 0x4e091ee2
>  0,        302,        302,        0,   460800, 0x2ea88828
>

Patch hide | download patch | download mbox

diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
index 75b14bce5b..0952b537b7 100644
--- a/libavcodec/nuv.c
+++ b/libavcodec/nuv.c
@@ -42,6 +42,8 @@  typedef struct NuvContext {
     unsigned char *decomp_buf;
     uint32_t lq[64], cq[64];
     RTJpegContext rtj;
+    int need_flush;
+    AVPacket flush_pkt;
 } NuvContext;
 
 static const uint8_t fallback_lquant[] = {
@@ -66,6 +68,12 @@  static const uint8_t fallback_cquant[] = {
     99, 99, 99, 99, 99, 99, 99, 99
 };
 
+static void decode_flush(AVCodecContext *avctx){
+    NuvContext *s = avctx->priv_data;
+
+    s->need_flush = 0;
+}
+
 /**
  * @brief copy frame data from buffer to AVFrame, handling stride.
  * @param f destination AVFrame
@@ -172,6 +180,26 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
         NUV_COPY_LAST     = 'L'
     } comptype;
 
+    if (!avpkt->data) {
+        if (c->need_flush) {
+            c->need_flush = 0;
+            if ((ret = ff_reget_buffer(avctx, c->pic)) < 0)
+                return ret;
+            c->pic->pkt_pos      = c->flush_pkt.pos;
+            c->pic->pkt_duration = c->flush_pkt.duration;
+            c->pic->pkt_dts      = c->flush_pkt.dts;
+            c->pic->pkt_pts      =
+            c->pic->pts          = c->flush_pkt.pts;
+            if ((ret = av_frame_ref(data, c->pic)) < 0)
+                return ret;
+            *got_frame = 1;
+        }
+        return 0;
+    }
+    c->flush_pkt = *avpkt;
+    c->pic->pkt_dts = c->flush_pkt.dts;
+
+
     if (buf_size < 12) {
         av_log(avctx, AV_LOG_ERROR, "coded frame too small\n");
         return AVERROR_INVALIDDATA;
@@ -204,8 +232,8 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
         }
         break;
     case NUV_COPY_LAST:
-        keyframe = 0;
-        break;
+        c->need_flush = 1;
+        return buf_size;
     default:
         keyframe = 1;
         break;
@@ -313,6 +341,7 @@  retry:
     if ((result = av_frame_ref(picture, c->pic)) < 0)
         return result;
 
+    c->need_flush = 0;
     *got_frame = 1;
     return orig_size;
 }
@@ -364,6 +393,8 @@  AVCodec ff_nuv_decoder = {
     .init           = decode_init,
     .close          = decode_end,
     .decode         = decode_frame,
-    .capabilities   = AV_CODEC_CAP_DR1,
+    .flush          = decode_flush,
+    .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS,
+    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
     .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
 };
diff --git a/tests/ref/fate/nuv-rtjpeg b/tests/ref/fate/nuv-rtjpeg
index b6f3b080dc..0914b985ec 100644
--- a/tests/ref/fate/nuv-rtjpeg
+++ b/tests/ref/fate/nuv-rtjpeg
@@ -6,7 +6,6 @@ 
 0,        118,        118,        0,   460800, 0x54aedafe
 0,        152,        152,        0,   460800, 0xb7aa8b56
 0,        177,        177,        0,   460800, 0x283ea3b5
-0,        202,        202,        0,   460800, 0x283ea3b5
 0,        235,        235,        0,   460800, 0x10e577de
 0,        269,        269,        0,   460800, 0x4e091ee2
 0,        302,        302,        0,   460800, 0x2ea88828