[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
James Almer
jamrial at gmail.com
Mon Aug 26 02:04:03 EEST 2019
On 8/25/2019 6:46 PM, 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 ?
No, but it would be negligible.
> 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.
What if the filter the user injected is meant to affect each and every
frame in a different way? Think for example a constant fade out. If you
remove the duplicate ones from what's meant to be a several seconds long
completely static scene, would that effect be altered? Between dts x and
dts y there are meant to be 100 frames, but now there's 1.
I'm not sure how libavfilter works here, but if it tries to fill out the
missing frames on its own, then the work of inserting the duplicate
frames for the fade out effect would simply move from lavc to lavfi.
>
> 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 ...
a9dacdeea6168787a142209bd19fdd74aefc9dd6 was committed to reduce timeout
runs in the fuzzer. What that commit did was turn a cfr decoder into a
vfr one. This patch here is not about the fuzzer, no, but it's to fix a
regression introduced by the aforementioned commit, when the last frame
is meant to be a duplicate of the previous one, and it achieves it by
introducing a very convoluted way to precisely pass a new reference to
the previous frame, which was the original behavior.
The output for this specific codec seems to be intended to be 1:1. For
every unique packet passed to the decoder there's a frame meant to be
output from it. The standard compliant behavior would then be a cfr one.
This patch is controversial because, as i mentioned above, it's a very
convoluted and dirty way to output something the decoder was not really
meant to.
The user can choose to get vfr output, or at least should, with the
vsynth cli option. There could also be a new option/flag added to
libavcodec to output such frames or not by setting AV_FRAME_FLAG_DISCARD
on them (thus never returned by receive_frame/decode_video2 since the
generic code looks for it and discards them). Or if that's not an
option, maybe with a new frame flag AV_FRAME_FLAG_DISPOSABLE to let the
user (or even libavfilter) decide what to do with the returned frames in
question.
>
> 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