On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer <mich...@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 > > <mich...@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 <mich...@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".