[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Paul B Mahol
onemda at gmail.com
Mon Aug 26 13:21:48 EEST 2019
On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer <michael at niedermayer.cc>
wrote:
> On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> > On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> > <michael at niedermayer.cc> wrote:
> > >
> > > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > > On 8/25/2019 6:46 PM, Michael Niedermayer wrote:
> > > > > 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 ?
> > > >
> > > > No, but it would be negligible.
> > >
> > > ok, lets see some actual numbers
> > >
> > > This comparission is about fixing ticket 7880 (just saying so we dont
> loose
> > > track of what we compare here)
> > >
> > > this qtrle patchset which fixes the last frame
> > > time ./ffmpeg -i clock.mov -vf scale=1920x1080 -qscale 2 -y
> test-new.avi
> > > real 0m0.233s
> > > user 0m0.404s
> > > sys 0m0.036s
> > > -rw-r----- 1 michael michael 769212 Aug 26 08:38 test-new.avi
> > > time ./ffmpeg -i test-new.avi -f null -
> > > real 0m0.095s
> > > user 0m0.131s
> > > sys 0m0.038s
> > >
> > > The alternative of reverting the code that discards duplicate frames
> > > (this i believe is what the people opposing this patchset here want)
> > > git revert a9dacdeea6168787a142209bd19fdd74aefc9dd6
> > > time ./ffmpeg -i clock.mov -vf scale=1920x1080 -qscale 2 -y test.avi
> > > real 0m1.208s
> > > user 0m2.866s
> > > sys 0m0.168s
> > > -rw-r----- 1 michael michael 3274430 Aug 26 08:44 test.avi
> > > time ./ffmpeg -i test.avi -f null -
> > > real 0m0.185s
> > > user 0m0.685s
> > > sys 0m0.076s
> > >
> > > https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket7880/clock.mov
> > >
> > > As we can see the solution preferred by the opposition to this patchset
> > > will make the code 6 times slower and result in 4 times higher bitrate
> for
> > > the same quality.
> > > I would say this is not "negligible"
> > > you know this is not 0.6%, its not 6% its not 60% it is 600%
> > >
> > > I do not understand why we have a discussion here about this.
> > > Do we want 600% slower code ?
> > > Do our users want 600% slower code ?
> > >
> > > I do not want 600% slower code
> > >
> >
> > Speed at the expense of correctness should not even be an argument.
>
> and here we are back to square 0 because for all mainstream codecs
> we drop duplicates and fail this correctness.
>
So you basically telling now that all mainstream codecs in FFmpeg do not
follow specification?
> And noone wants to change that no matter which side of the argument one
> is on.
>
>
> > You seem to not value at all the correctness of proper consistent CFR
> > output, while many of us others do. The need for CFR output cannot be
> > explained away with performance numbers.
>
> being correct in the "CFR" sense and still being fast is possible, these
> are not exclusive.
> But we lack consensus to go that direction as well as exactly how as there
> are several possibilities
>
> Thanks
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are too smart to engage in politics are punished by being
> governed by those who are dumber. -- Plato
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list