[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Paul B Mahol
onemda at gmail.com
Mon Aug 26 20:50:07 EEST 2019
On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier <
baptiste.coudurier at gmail.com> wrote:
> Hey guys,
>
>
> > On Aug 26, 2019, at 9:25 AM, James Almer <jamrial at gmail.com> wrote:
> >
> > On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
> >> On Mon, Aug 26, 2019 at 1:18 AM James Almer <jamrial at 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 <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 ...
> >>>>
> >>>> 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.
>
There is no any consistency at all, qtrle is not repeating any fields.
>
> Now, I would not be opposed to change decoders to follow specifications
> and actually repeat the fields
> instead of relying on the users of libavcodec to do it, but obviously this
> is a much bigger undertaking.
>
> —
> Baptiste
> _______________________________________________
> 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