[FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Baptiste Coudurier
baptiste.coudurier at gmail.com
Mon Aug 26 21:43:38 EEST 2019
> On Aug 26, 2019, at 11:23 AM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>
> On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier
> <baptiste.coudurier at gmail.com <mailto: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.
There are no specs for QTRLE, the frames _are_ empty.
How do you know how quicktime behaves ? Both implementations seems valid to me.
> 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.
Arguing about the metadata loss is one thing, CFR output is a different topic IMHO.
—
Baptiste
More information about the ffmpeg-devel
mailing list