diff mbox

[FFmpeg-devel] avcodec/qtrle: return last frame even if unchanged

Message ID 20190515184011.22733-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer May 15, 2019, 6:40 p.m. UTC 
Fixes: Ticket7880

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/qtrle.c        | 27 +++++++++++++++++++++++++--
 tests/ref/fate/qtrle-8bit |  1 +
 2 files changed, 26 insertions(+), 2 deletions(-)

Comments

Hendrik Leppkes May 15, 2019, 8:53 p.m. UTC | #1 
On Wed, May 15, 2019 at 8:41 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> Fixes: Ticket7880
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/qtrle.c        | 27 +++++++++++++++++++++++++--
>  tests/ref/fate/qtrle-8bit |  1 +
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> index 2c29547e5a..11226cb842 100644
> --- a/libavcodec/qtrle.c
> +++ b/libavcodec/qtrle.c
> @@ -45,6 +45,7 @@ typedef struct QtrleContext {
>
>      GetByteContext g;
>      uint32_t pal[256];
> +    int need_flush;
>  } QtrleContext;
>
>  #define CHECK_PIXEL_PTR(n)                                                            \
> @@ -444,6 +445,12 @@ static av_cold int qtrle_decode_init(AVCodecContext *avctx)
>      return 0;
>  }
>
> +static void qtrle_flush(AVCodecContext *avctx){
> +    QtrleContext *s = avctx->priv_data;
> +
> +    s->need_flush = 0;
> +}
> +
>  static int qtrle_decode_frame(AVCodecContext *avctx,
>                                void *data, int *got_frame,
>                                AVPacket *avpkt)
> @@ -454,11 +461,26 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
>      int has_palette = 0;
>      int ret, size;
>
> +    if (!avpkt->data) {
> +        if (s->need_flush) {
> +            s->need_flush = 0;
> +            if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
> +                return ret;
> +            if ((ret = av_frame_ref(data, s->frame)) < 0)
> +                return ret;
> +            *got_frame = 1;
> +        }
> +        return 0;
> +    }
> +
>      bytestream2_init(&s->g, avpkt->data, avpkt->size);
>
>      /* check if this frame is even supposed to change */
> -    if (avpkt->size < 8)
> +    if (avpkt->size < 8) {
> +        s->need_flush = 1;
>          return avpkt->size;
> +    }
> +    s->need_flush = 0;
>
>      /* start after the chunk size */
>      size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
> @@ -572,5 +594,6 @@ AVCodec ff_qtrle_decoder = {
>      .init           = qtrle_decode_init,
>      .close          = qtrle_decode_end,
>      .decode         = qtrle_decode_frame,
> -    .capabilities   = AV_CODEC_CAP_DR1,
> +    .flush          = qtrle_flush,
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>  };
> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> index 27bb8aad71..39a03b7b6c 100644
> --- a/tests/ref/fate/qtrle-8bit
> +++ b/tests/ref/fate/qtrle-8bit
> @@ -61,3 +61,4 @@
>  0,        160,        160,        1,   921600, 0xcfd6ad2b
>  0,        163,        163,        1,   921600, 0x3b372379
>  0,        165,        165,        1,   921600, 0x36f245f5
> +0,        166,        166,        1,   921600, 0x36f245f5

Does this actually work if there is a gap between the last frame and
the previously changed frame?
I'm not sure where it would save the proper timestamp.

- Hendrik
Michael Niedermayer May 15, 2019, 9:48 p.m. UTC | #2 
On Wed, May 15, 2019 at 10:53:25PM +0200, Hendrik Leppkes wrote:
> On Wed, May 15, 2019 at 8:41 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > Fixes: Ticket7880
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/qtrle.c        | 27 +++++++++++++++++++++++++--
> >  tests/ref/fate/qtrle-8bit |  1 +
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> > index 2c29547e5a..11226cb842 100644
> > --- a/libavcodec/qtrle.c
> > +++ b/libavcodec/qtrle.c
> > @@ -45,6 +45,7 @@ typedef struct QtrleContext {
> >
> >      GetByteContext g;
> >      uint32_t pal[256];
> > +    int need_flush;
> >  } QtrleContext;
> >
> >  #define CHECK_PIXEL_PTR(n)                                                            \
> > @@ -444,6 +445,12 @@ static av_cold int qtrle_decode_init(AVCodecContext *avctx)
> >      return 0;
> >  }
> >
> > +static void qtrle_flush(AVCodecContext *avctx){
> > +    QtrleContext *s = avctx->priv_data;
> > +
> > +    s->need_flush = 0;
> > +}
> > +
> >  static int qtrle_decode_frame(AVCodecContext *avctx,
> >                                void *data, int *got_frame,
> >                                AVPacket *avpkt)
> > @@ -454,11 +461,26 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
> >      int has_palette = 0;
> >      int ret, size;
> >
> > +    if (!avpkt->data) {
> > +        if (s->need_flush) {
> > +            s->need_flush = 0;
> > +            if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
> > +                return ret;
> > +            if ((ret = av_frame_ref(data, s->frame)) < 0)
> > +                return ret;
> > +            *got_frame = 1;
> > +        }
> > +        return 0;
> > +    }
> > +
> >      bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >
> >      /* check if this frame is even supposed to change */
> > -    if (avpkt->size < 8)
> > +    if (avpkt->size < 8) {
> > +        s->need_flush = 1;
> >          return avpkt->size;
> > +    }
> > +    s->need_flush = 0;
> >
> >      /* start after the chunk size */
> >      size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
> > @@ -572,5 +594,6 @@ AVCodec ff_qtrle_decoder = {
> >      .init           = qtrle_decode_init,
> >      .close          = qtrle_decode_end,
> >      .decode         = qtrle_decode_frame,
> > -    .capabilities   = AV_CODEC_CAP_DR1,
> > +    .flush          = qtrle_flush,
> > +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> >  };
> > diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > index 27bb8aad71..39a03b7b6c 100644
> > --- a/tests/ref/fate/qtrle-8bit
> > +++ b/tests/ref/fate/qtrle-8bit
> > @@ -61,3 +61,4 @@
> >  0,        160,        160,        1,   921600, 0xcfd6ad2b
> >  0,        163,        163,        1,   921600, 0x3b372379
> >  0,        165,        165,        1,   921600, 0x36f245f5
> > +0,        166,        166,        1,   921600, 0x36f245f5
> 
> Does this actually work if there is a gap between the last frame and
> the previously changed frame?
> I'm not sure where it would save the proper timestamp.

i had tested it with this:
make -j12 && ./ffmpeg -i test.mov test%d.jpg

and it turned a case with a single frame and 10 skiped ones into 11 frames
so i assume it works. Do you have a case where it does not work ?

thx

[...]
Marton Balint May 16, 2019, 7:29 p.m. UTC | #3 
On Wed, 15 May 2019, Michael Niedermayer wrote:

> Fixes: Ticket7880
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavcodec/qtrle.c        | 27 +++++++++++++++++++++++++--
> tests/ref/fate/qtrle-8bit |  1 +
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> index 2c29547e5a..11226cb842 100644
> --- a/libavcodec/qtrle.c
> +++ b/libavcodec/qtrle.c
> @@ -45,6 +45,7 @@ typedef struct QtrleContext {
>
>     GetByteContext g;
>     uint32_t pal[256];
> +    int need_flush;
> } QtrleContext;
> 
> #define CHECK_PIXEL_PTR(n)                                                            \
> @@ -444,6 +445,12 @@ static av_cold int qtrle_decode_init(AVCodecContext *avctx)
>     return 0;
> }
> 
> +static void qtrle_flush(AVCodecContext *avctx){
> +    QtrleContext *s = avctx->priv_data;
> +
> +    s->need_flush = 0;
> +}
> +
> static int qtrle_decode_frame(AVCodecContext *avctx,
>                               void *data, int *got_frame,
>                               AVPacket *avpkt)
> @@ -454,11 +461,26 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
>     int has_palette = 0;
>     int ret, size;
> 
> +    if (!avpkt->data) {
> +        if (s->need_flush) {
> +            s->need_flush = 0;
> +            if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
> +                return ret;
> +            if ((ret = av_frame_ref(data, s->frame)) < 0)
> +                return ret;
> +            *got_frame = 1;
> +        }
> +        return 0;
> +    }
> +
>     bytestream2_init(&s->g, avpkt->data, avpkt->size);
>
>     /* check if this frame is even supposed to change */
> -    if (avpkt->size < 8)
> +    if (avpkt->size < 8) {
> +        s->need_flush = 1;

You probably have to set need_flush for the other two cases where you 
return avpkt->size for undersized packets.

>         return avpkt->size;
> +    }
> +    s->need_flush = 0;
>
>     /* start after the chunk size */
>     size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
> @@ -572,5 +594,6 @@ AVCodec ff_qtrle_decoder = {
>     .init           = qtrle_decode_init,
>     .close          = qtrle_decode_end,
>     .decode         = qtrle_decode_frame,
> -    .capabilities   = AV_CODEC_CAP_DR1,
> +    .flush          = qtrle_flush,
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> };
> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> index 27bb8aad71..39a03b7b6c 100644
> --- a/tests/ref/fate/qtrle-8bit
> +++ b/tests/ref/fate/qtrle-8bit
> @@ -61,3 +61,4 @@
> 0,        160,        160,        1,   921600, 0xcfd6ad2b
> 0,        163,        163,        1,   921600, 0x3b372379
> 0,        165,        165,        1,   921600, 0x36f245f5
> +0,        166,        166,        1,   921600, 0x36f245f5
> --

I tested your patch and it looks like pkt_dts and pkt_pos is lost for the 
last frame. Having no pkt_dts can be an issue later, because pkt_pts is 
already deprecated.

Regards,
Marton
Michael Niedermayer May 30, 2019, 2:06 p.m. UTC | #4 
On Thu, May 16, 2019 at 09:29:26PM +0200, Marton Balint wrote:
> 
> On Wed, 15 May 2019, Michael Niedermayer wrote:
> 
> >Fixes: Ticket7880
> >
> >Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >---
> >libavcodec/qtrle.c        | 27 +++++++++++++++++++++++++--
> >tests/ref/fate/qtrle-8bit |  1 +
> >2 files changed, 26 insertions(+), 2 deletions(-)
> >
> >diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> >index 2c29547e5a..11226cb842 100644
> >--- a/libavcodec/qtrle.c
> >+++ b/libavcodec/qtrle.c
> >@@ -45,6 +45,7 @@ typedef struct QtrleContext {
> >
> >    GetByteContext g;
> >    uint32_t pal[256];
> >+    int need_flush;
> >} QtrleContext;
> >
> >#define CHECK_PIXEL_PTR(n)                                                            \
> >@@ -444,6 +445,12 @@ static av_cold int qtrle_decode_init(AVCodecContext *avctx)
> >    return 0;
> >}
> >
> >+static void qtrle_flush(AVCodecContext *avctx){
> >+    QtrleContext *s = avctx->priv_data;
> >+
> >+    s->need_flush = 0;
> >+}
> >+
> >static int qtrle_decode_frame(AVCodecContext *avctx,
> >                              void *data, int *got_frame,
> >                              AVPacket *avpkt)
> >@@ -454,11 +461,26 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
> >    int has_palette = 0;
> >    int ret, size;
> >
> >+    if (!avpkt->data) {
> >+        if (s->need_flush) {
> >+            s->need_flush = 0;
> >+            if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
> >+                return ret;
> >+            if ((ret = av_frame_ref(data, s->frame)) < 0)
> >+                return ret;
> >+            *got_frame = 1;
> >+        }
> >+        return 0;
> >+    }
> >+
> >    bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >
> >    /* check if this frame is even supposed to change */
> >-    if (avpkt->size < 8)
> >+    if (avpkt->size < 8) {
> >+        s->need_flush = 1;
> 
> You probably have to set need_flush for the other two cases where you return
> avpkt->size for undersized packets.
> 
> >        return avpkt->size;
> >+    }
> >+    s->need_flush = 0;
> >
> >    /* start after the chunk size */
> >    size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
> >@@ -572,5 +594,6 @@ AVCodec ff_qtrle_decoder = {
> >    .init           = qtrle_decode_init,
> >    .close          = qtrle_decode_end,
> >    .decode         = qtrle_decode_frame,
> >-    .capabilities   = AV_CODEC_CAP_DR1,
> >+    .flush          = qtrle_flush,
> >+    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> >};
> >diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> >index 27bb8aad71..39a03b7b6c 100644
> >--- a/tests/ref/fate/qtrle-8bit
> >+++ b/tests/ref/fate/qtrle-8bit
> >@@ -61,3 +61,4 @@
> >0,        160,        160,        1,   921600, 0xcfd6ad2b
> >0,        163,        163,        1,   921600, 0x3b372379
> >0,        165,        165,        1,   921600, 0x36f245f5
> >+0,        166,        166,        1,   921600, 0x36f245f5
> >--
> 
> I tested your patch and it looks like pkt_dts and pkt_pos is lost for the
> last frame. Having no pkt_dts can be an issue later, because pkt_pts is
> already deprecated.

new patch posted

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index 2c29547e5a..11226cb842 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -45,6 +45,7 @@  typedef struct QtrleContext {
 
     GetByteContext g;
     uint32_t pal[256];
+    int need_flush;
 } QtrleContext;
 
 #define CHECK_PIXEL_PTR(n)                                                            \
@@ -444,6 +445,12 @@  static av_cold int qtrle_decode_init(AVCodecContext *avctx)
     return 0;
 }
 
+static void qtrle_flush(AVCodecContext *avctx){
+    QtrleContext *s = avctx->priv_data;
+
+    s->need_flush = 0;
+}
+
 static int qtrle_decode_frame(AVCodecContext *avctx,
                               void *data, int *got_frame,
                               AVPacket *avpkt)
@@ -454,11 +461,26 @@  static int qtrle_decode_frame(AVCodecContext *avctx,
     int has_palette = 0;
     int ret, size;
 
+    if (!avpkt->data) {
+        if (s->need_flush) {
+            s->need_flush = 0;
+            if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
+                return ret;
+            if ((ret = av_frame_ref(data, s->frame)) < 0)
+                return ret;
+            *got_frame = 1;
+        }
+        return 0;
+    }
+
     bytestream2_init(&s->g, avpkt->data, avpkt->size);
 
     /* check if this frame is even supposed to change */
-    if (avpkt->size < 8)
+    if (avpkt->size < 8) {
+        s->need_flush = 1;
         return avpkt->size;
+    }
+    s->need_flush = 0;
 
     /* start after the chunk size */
     size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
@@ -572,5 +594,6 @@  AVCodec ff_qtrle_decoder = {
     .init           = qtrle_decode_init,
     .close          = qtrle_decode_end,
     .decode         = qtrle_decode_frame,
-    .capabilities   = AV_CODEC_CAP_DR1,
+    .flush          = qtrle_flush,
+    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
 };
diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
index 27bb8aad71..39a03b7b6c 100644
--- a/tests/ref/fate/qtrle-8bit
+++ b/tests/ref/fate/qtrle-8bit
@@ -61,3 +61,4 @@ 
 0,        160,        160,        1,   921600, 0xcfd6ad2b
 0,        163,        163,        1,   921600, 0x3b372379
 0,        165,        165,        1,   921600, 0x36f245f5
+0,        166,        166,        1,   921600, 0x36f245f5