> On Aug 26, 2019, at 11:23 AM, Hendrik Leppkes <h.lepp...@gmail.com> wrote: > > On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier > <baptiste.coudur...@gmail.com <mailto:baptiste.coudur...@gmail.com>> wrote: >> >> Hey guys, >> >> >>> On Aug 26, 2019, at 9:25 AM, James Almer <jamr...@gmail.com> wrote: >>> >>> On 8/26/2019 11:35 AM, Hendrik Leppkes wrote: >>>> On Mon, Aug 26, 2019 at 1:18 AM James Almer <jamr...@gmail.com> wrote: >>>>> >>>>> On 8/25/2019 7:14 PM, Michael Niedermayer wrote: >>>>>> On Sun, Aug 25, 2019 at 11:46:36PM +0200, 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 ? >>>>>>> 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 ... >>>>>> >>>>>> Also about the implementation itself. >>>>>> This can of course be done in countless other ways >>>>>> for example one can probably detect the duplicate ref somewhere in common >>>>>> code and then optionally drop the frames. >>>>> >>>>> This is one of the suggestions i made in the email sent a few minutes >>>>> ago, yes. Based on a user set option, either dropping the frames in >>>>> generic code by flagging them as discard, or flagging them as >>>>> "disposable" and letting the library user (external applications, ffmpeg >>>>> cli, libavfilter, etc) decide what to do with them. >>>>> >>>> >>>> Flagging identical repeat frames as "disposable" seems like a good >>>> idea to me. "discard" imho doesn't fit, since it has a specific >>>> meaning of "should be discarded", while the semantics of "disposable" >>>> would fit this use-case (ie. this frame is valid and you can keep it, >>>> but you could also drop it if you favor performance). >>> >>> Ok, just sent patchset to signal frames as disposable, with qtrle as the >>> first decoder to implement it as a PoC. >>> >>> What's missing is making some "vfr" setting in the ffmpeg cli look for >>> it in frames to effectively drop them before instead of passing them to >>> filters or encoders, at the user's request. >>> Suggestions or patches for that welcome. >> >> IMHO a decoder should output according to the specifications. >> FFmpeg, a while ago, chose to disagree and ignore features like “repeat >> first field” in mpeg codecs >> and instead signal it so the user/application would do it. >> In that spirit, it is understandable that QTRLE decoder behaves the same way >> for consistency reasons. >> > > Honoring metadata (or not) and actively dropping identical frames are > quite different things.
There are no specs for QTRLE, the frames _are_ empty. How do you know how quicktime behaves ? Both implementations seems valid to me. > As explained in an earlier mail already, the repeat metadata is > maintained perfectly as metadata, the information about the dropped > frames here is flat out lost. Arguing about the metadata loss is one thing, CFR output is a different topic IMHO. — Baptiste _______________________________________________ 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".
