[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Paul B Mahol
onemda at gmail.com
Mon Aug 26 11:28:28 EEST 2019
On Mon, Aug 26, 2019 at 9:51 AM Paul B Mahol <onemda at gmail.com> wrote:
>
>
> On Mon, Aug 26, 2019 at 9:37 AM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
>
>> On Sun, Aug 25, 2019 at 11:43:42PM -0300, James Almer wrote:
>> > On 8/25/2019 8:18 PM, James Almer 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
>> >
>> > Of course, this has the same issue as the regression this patch is
>> > trying to solve. The last frame, being a duplicate, would also be
>> > discarded by the generic code.
>>
>> yes, so we need to keep a reference to the last frame and return that at
>> the end.
>> can certainly be done, i do not think it will get any other reaction
>> than a single line "NAK".
>> I mean if a single added variable to common code is rejected, this will
>> need more changes in common code and API
>> we need to first find a clear consensus what to do before its done
>> so whoever does it doesnt waste his time ...
>>
>
>
> Sure, there is other very large opposition but they are as usual very
> silent.
> Do you remember your fraps codec change which broke it in same way as
> qtrle.
>
Actually, it was not your change, but Reimar change. Which is little
puzzling why so much before all other changes that follow same agenda.
>
More information about the ffmpeg-devel
mailing list