[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Hendrik Leppkes
h.leppkes at gmail.com
Mon Aug 26 21:23:54 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.
>
Honoring metadata (or not) and actively dropping identical frames are
quite different things.
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.
- Hendrik
More information about the ffmpeg-devel
mailing list