[FFmpeg-devel,09/12] avcodec/pafvideo: Only clear frame when it was written to

Submitted by Michael Niedermayer on Sept. 25, 2019, 8:38 p.m.

Details

Message ID 20190925203858.27870-9-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 25, 2019, 8:38 p.m.
This avoids unneeded operations and makes the code faster.

Fixes: Timeout
Fixes: 15724/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PAF_VIDEO_fuzzer-5750842205929472 (12sec -> 9sec)
Fixes: 17625/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PAF_VIDEO_fuzzer-5640515311108096 (16sec -> 4sec)

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/pafvideo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Paul B Mahol Sept. 26, 2019, 8:04 a.m.
Why this does not set dirty for all decoding cases?

On 9/25/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> This avoids unneeded operations and makes the code faster.
>
> Fixes: Timeout
> Fixes:
> 15724/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PAF_VIDEO_fuzzer-5750842205929472
> (12sec -> 9sec)
> Fixes:
> 17625/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PAF_VIDEO_fuzzer-5640515311108096
> (16sec -> 4sec)
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/pafvideo.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/pafvideo.c b/libavcodec/pafvideo.c
> index 6b4771cbce..07fa05caf8 100644
> --- a/libavcodec/pafvideo.c
> +++ b/libavcodec/pafvideo.c
> @@ -55,6 +55,7 @@ typedef struct PAFVideoDecContext {
>
>      int current_frame;
>      uint8_t *frame[4];
> +    int dirty[4];
>      int frame_size;
>      int video_size;
>
> @@ -187,6 +188,7 @@ static int decode_0(PAFVideoDecContext *c, uint8_t *pkt,
> uint8_t code)
>              j      = bytestream2_get_le16(&c->gb) + offset;
>              if (bytestream2_get_bytes_left(&c->gb) < (j - offset) * 16)
>                  return AVERROR_INVALIDDATA;
> +            c->dirty[page] = 1;
>              do {
>                  offset++;
>                  if (dst + 3 * c->width + 4 > dend)
> @@ -329,9 +331,13 @@ static int paf_video_decode(AVCodecContext *avctx, void
> *data,
>          c->pic->palette_has_changed = 1;
>      }
>
> +    c->dirty[c->current_frame] = 1;
>      if (code & 0x20)
> -        for (i = 0; i < 4; i++)
> -            memset(c->frame[i], 0, c->frame_size);
> +        for (i = 0; i < 4; i++) {
> +            if (c->dirty[i])
> +                memset(c->frame[i], 0, c->frame_size);
> +            c->dirty[i] = 0;
> +        }
>
>      switch (code & 0x0F) {
>      case 0:
> --
> 2.23.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 Sept. 26, 2019, 9:18 p.m.
On Thu, Sep 26, 2019 at 10:04:44AM +0200, Paul B Mahol wrote:
> Why this does not set dirty for all decoding cases?

dirty is set for c->current_frame, so cases which write only
into that do not need an explicit case.

But maybe i forgot something ?

Thanks

[...]
Michael Niedermayer Oct. 16, 2019, 4:14 p.m.
On Thu, Sep 26, 2019 at 11:18:37PM +0200, Michael Niedermayer wrote:
> On Thu, Sep 26, 2019 at 10:04:44AM +0200, Paul B Mahol wrote:
> > Why this does not set dirty for all decoding cases?
> 
> dirty is set for c->current_frame, so cases which write only
> into that do not need an explicit case.
> 
> But maybe i forgot something ?

do you see any remaining issues or can i apply it ?

thx

[...]
Paul B Mahol Oct. 16, 2019, 4:27 p.m.
On 10/16/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Thu, Sep 26, 2019 at 11:18:37PM +0200, Michael Niedermayer wrote:
>> On Thu, Sep 26, 2019 at 10:04:44AM +0200, Paul B Mahol wrote:
>> > Why this does not set dirty for all decoding cases?
>>
>> dirty is set for c->current_frame, so cases which write only
>> into that do not need an explicit case.
>>
>> But maybe i forgot something ?
>
> do you see any remaining issues or can i apply it ?
>

Nope.

> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
>
Michael Niedermayer Oct. 16, 2019, 4:48 p.m.
On Wed, Oct 16, 2019 at 06:27:10PM +0200, Paul B Mahol wrote:
> On 10/16/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Thu, Sep 26, 2019 at 11:18:37PM +0200, Michael Niedermayer wrote:
> >> On Thu, Sep 26, 2019 at 10:04:44AM +0200, Paul B Mahol wrote:
> >> > Why this does not set dirty for all decoding cases?
> >>
> >> dirty is set for c->current_frame, so cases which write only
> >> into that do not need an explicit case.
> >>
> >> But maybe i forgot something ?
> >
> > do you see any remaining issues or can i apply it ?
> >
> 
> Nope.

ill assume that the "nope" referes to "remaining issues" and not the apply part
and thus will apply in a few days

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/pafvideo.c b/libavcodec/pafvideo.c
index 6b4771cbce..07fa05caf8 100644
--- a/libavcodec/pafvideo.c
+++ b/libavcodec/pafvideo.c
@@ -55,6 +55,7 @@  typedef struct PAFVideoDecContext {
 
     int current_frame;
     uint8_t *frame[4];
+    int dirty[4];
     int frame_size;
     int video_size;
 
@@ -187,6 +188,7 @@  static int decode_0(PAFVideoDecContext *c, uint8_t *pkt, uint8_t code)
             j      = bytestream2_get_le16(&c->gb) + offset;
             if (bytestream2_get_bytes_left(&c->gb) < (j - offset) * 16)
                 return AVERROR_INVALIDDATA;
+            c->dirty[page] = 1;
             do {
                 offset++;
                 if (dst + 3 * c->width + 4 > dend)
@@ -329,9 +331,13 @@  static int paf_video_decode(AVCodecContext *avctx, void *data,
         c->pic->palette_has_changed = 1;
     }
 
+    c->dirty[c->current_frame] = 1;
     if (code & 0x20)
-        for (i = 0; i < 4; i++)
-            memset(c->frame[i], 0, c->frame_size);
+        for (i = 0; i < 4; i++) {
+            if (c->dirty[i])
+                memset(c->frame[i], 0, c->frame_size);
+            c->dirty[i] = 0;
+        }
 
     switch (code & 0x0F) {
     case 0: