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

[Michael Niedermayer]

On Sun, Aug 25, 2019 at 01:22:22PM -0300, James Almer wrote:
> On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> > Fixes: Ticket7880
> > 
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  libavcodec/qtrle.c        | 30 ++++++++++++++++++++++++++----
> >  tests/ref/fate/qtrle-8bit |  1 +
> >  2 files changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> > index 2c29547e5a..c22a1a582d 100644
> > --- a/libavcodec/qtrle.c
> > +++ b/libavcodec/qtrle.c
> > @@ -45,6 +45,7 @@ typedef struct QtrleContext {
> >  
> >      GetByteContext g;
> >      uint32_t pal[256];
> > +    AVPacket flush_pkt;
> >  } QtrleContext;
> >  
> >  #define CHECK_PIXEL_PTR(n)                                                            \
> > @@ -454,11 +455,27 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
> >      int has_palette = 0;
> >      int ret, size;
> >  
> > +    if (!avpkt->data) {
> > +        if (avctx->internal->need_flush) {
> > +            avctx->internal->need_flush = 0;
> > +            ret = ff_setup_buffered_frame_for_return(avctx, data, s->frame, &s->flush_pkt);
> > +            if (ret < 0)
> > +                return ret;
> > +            *got_frame = 1;
> > +        }
> > +        return 0;
> > +    }
> > +    s->flush_pkt = *avpkt;
> > +    s->frame->pkt_dts = s->flush_pkt.dts;
> > +
> >      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) {
> > +        avctx->internal->need_flush = 1;
> >          return avpkt->size;
> > +    }
> > +    avctx->internal->need_flush = 0;
> >  
> >      /* start after the chunk size */
> >      size = bytestream2_get_be32(&s->g) & 0x3FFFFFFF;
> > @@ -471,14 +488,18 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
> >  
> >      /* if a header is present, fetch additional decoding parameters */
> >      if (header & 0x0008) {
> > -        if (avpkt->size < 14)
> > +        if (avpkt->size < 14) {
> > +            avctx->internal->need_flush = 1;
> >              return avpkt->size;
> > +        }
> >          start_line = bytestream2_get_be16(&s->g);
> >          bytestream2_skip(&s->g, 2);
> >          height     = bytestream2_get_be16(&s->g);
> >          bytestream2_skip(&s->g, 2);
> > -        if (height > s->avctx->height - start_line)
> > +        if (height > s->avctx->height - start_line) {
> > +            avctx->internal->need_flush = 1;
> >              return avpkt->size;
> > +        }
> >      } else {
> >          start_line = 0;
> >          height     = s->avctx->height;
> > @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> >      .init           = qtrle_decode_init,
> >      .close          = qtrle_decode_end,
> >      .decode         = qtrle_decode_frame,
> > -    .capabilities   = AV_CODEC_CAP_DR1,
> > +    .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS | FF_CODEC_CAP_SETS_PKT_POS,
> > +    .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
> 
> Following what i said in the nuv patch, do you still experience timeouts
> with the current codebase, or even if you revert commit
> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> existing frame buffer shouldn't be expensive anymore for the fuzzer
> after my ref counting changes to target_dec_fuzzer.c
> 
> This is a very ugly solution to a problem that was already solved when
> reference counting was introduced. Returning duplicate frames to achieve
> cfr in decoders where it's expected shouldn't affect performance.

Maybe i should ask this backward to make it clearer what this is trying
to fix.

Consider a patch that would return every frame twice with the same ref
of course.
Would you consider this to have 0 performance impact ?
if every filter following needs to process frames twice 2x CPU load
and after the filters references would also not be the same anymore
the following encoder would encoder 2x as many frames 2x CPU load,
bigger file lower quality per bitrate. Also playback of the resulting
file would require more cpu time as it has more frames.

I think that would be considered a very bad patch for its performance
impact.
So if we do the opposite of removing duplicates why is this so
controversal ?

This is not about the fuzzer at all ...

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190825/02c1f2cc/attachment.sig>