[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
James Almer
jamrial at gmail.com
Mon Aug 26 05:43:42 EEST 2019
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.
> , or flagging them as
> "disposable" and letting the library user (external applications, ffmpeg
> cli, libavfilter, etc) decide what to do with them.
This one would also help making the cli's -vsync vfr option more useful,
i think.
>
>> Or it could be done in a filter, that would then only help applications
>> using libavfilter too though, ...
>>
>> Iam not arguing in favor of this specific implementation, rather that
>> these frames should not be processed multiple times
>> Also the terse NAK comments this sort of patches receive and did receive
>> do not motivate one very much to spend alot of time doing a really
>> perfect design ...
>
> That's why i'm not the one writing such replies, and instead is trying
> to help coming up with a nicer way to solve this that doesn't involve
> unconditionally altering the output of decoders.
>
>>
>> Thanks
>>
>> [...]
>>
>>
>> _______________________________________________
>> 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