[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged

[Baptiste Coudurier]

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.

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