On Mon, Aug 26, 2019 at 1:18 AM James Almer <jamr...@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 <mich...@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).
- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
