Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
> > > Lets check H264 > claim "They do not have a timestamp" > immedeatly after the pic_struct field which tells you about the repeating > behavior there is a loop for each repeated value with a timestamp. > This timestamp is lost, if at CFR one can call it that way. > > about "Every encoded frame should produce a decoded frame" > quote about this timestamp from the h264 spec > "The contents of the clock timestamp syntax elements indicate a time of > origin, capture, or alternative ideal display." > This is a simple misunderstanding of the difference between timecode and timestamp. I point you to numerous publications about the difference. > "origin, capture" implies that the repeated frame can have come from > a real input packet because otherwise it wouldnt have a capture or origin > time. > So an encoder could use this as a way to represent some input frames > And in that case only a decoder that follows these repeats exactly would > have input == output > which again brings us full circle as this matches what we see in qtrle > repeats > You conflate presentation (with repeated fields/frame) and decoding which are not the same thing. Kieran ___ 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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
> > "time of origin, capture" is clearly a timecode, not a timestamp in > the sense we're talking about here (plus that the bitstream codes it > in hours/minutes/seconds). I expect you know the difference. > If these timecodes are considered useful it would be trivial to expose > them from the decoder too, since they are already being parsed and > stored. > These timecodes are already outputted as side data. As Nev says they have *nothing* to do with presentation, they are just metadata. Kieran ___ 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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 9:38 PM Michael Niedermayer
wrote:
>
> On Mon, Aug 26, 2019 at 01:45:55PM +0200, Hendrik Leppkes wrote:
> > On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer
> > wrote:
> > >
> > > On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> > > > On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> > > > wrote:
> > > > >
> > > > > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > > > > 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
> > > > > > >>> ---
> > > > > > >>> 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, >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(>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(>g) & 0x3FFF;
> > > > > > >>> @@ -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(>g);
> > > > > > >>> bytestream2_skip(>g, 2);
> > > > > > >>> height = bytestream2_get_be16(>g);
> > > > > > >>> bytestream2_skip(>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,
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 01:45:55PM +0200, Hendrik Leppkes wrote:
> On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer
> wrote:
> >
> > On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> > > On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> > > wrote:
> > > >
> > > > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > > > 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
> > > > > >>> ---
> > > > > >>> 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, >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(>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(>g) & 0x3FFF;
> > > > > >>> @@ -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(>g);
> > > > > >>> bytestream2_skip(>g, 2);
> > > > > >>> height = bytestream2_get_be16(>g);
> > > > > >>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
> On Aug 26, 2019, at 11:23 AM, Hendrik Leppkes wrote:
>
> On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier
> mailto:baptiste.coudur...@gmail.com>> wrote:
>>
>> Hey guys,
>>
>>
>>> On Aug 26, 2019, at 9:25 AM, James Almer wrote:
>>>
>>> On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
On Mon, Aug 26, 2019 at 1:18 AM 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
> ---
> 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, >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
>bytestream2_skip(>g, 2);
>height = bytestream2_get_be16(>g);
>bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier
wrote:
>
> Hey guys,
>
>
> > On Aug 26, 2019, at 9:25 AM, James Almer wrote:
> >
> > On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
> >> On Mon, Aug 26, 2019 at 1:18 AM 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
> >>> ---
> >>> 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, >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(>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(>g) & 0x3FFF;
> >>> @@ -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(>g);
> >>> bytestream2_skip(>g, 2);
> >>> height = bytestream2_get_be16(>g);
> >>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 7:47 PM Baptiste Coudurier <
baptiste.coudur...@gmail.com> wrote:
> Hey guys,
>
>
> > On Aug 26, 2019, at 9:25 AM, James Almer wrote:
> >
> > On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
> >> On Mon, Aug 26, 2019 at 1:18 AM 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
> >>> ---
> >>> 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, >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(>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(>g) & 0x3FFF;
> >>> @@ -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(>g);
> >>> bytestream2_skip(>g, 2);
> >>> height = bytestream2_get_be16(>g);
> >>> bytestream2_skip(>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.
> >
> >
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Hey guys,
> On Aug 26, 2019, at 9:25 AM, James Almer wrote:
>
> On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
>> On Mon, Aug 26, 2019 at 1:18 AM 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
>>> ---
>>> 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, >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(>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(>g) & 0x3FFF;
>>> @@ -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(>g);
>>> bytestream2_skip(>g, 2);
>>> height = bytestream2_get_be16(>g);
>>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On 8/26/2019 11:35 AM, Hendrik Leppkes wrote:
> On Mon, Aug 26, 2019 at 1:18 AM 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
>> ---
>> 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, >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(>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(>g) & 0x3FFF;
>> @@ -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(>g);
>> bytestream2_skip(>g, 2);
>> height = bytestream2_get_be16(>g);
>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 1:18 AM 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
> ---
> 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, >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On 8/26/2019 4:32 AM, Michael Niedermayer wrote:
> On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
>> 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
> ---
> 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, >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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.
>
> ok, lets see some actual numbers
>
> This comparission is about fixing ticket 7880 (just saying so we dont loose
> track of what we compare here)
>
> this qtrle patchset which fixes the last frame
> time ./ffmpeg -i clock.mov -vf scale=1920x1080 -qscale 2 -y test-new.avi
> real
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer
wrote:
>
> On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> > On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> > wrote:
> > >
> > > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > > 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
> > > > >>> ---
> > > > >>> 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, >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(>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(>g) & 0x3FFF;
> > > > >>> @@ -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(>g);
> > > > >>> bytestream2_skip(>g, 2);
> > > > >>> height = bytestream2_get_be16(>g);
> > > > >>> bytestream2_skip(>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
> > > >
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 11:09 AM Michael Niedermayer
wrote:
> On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> > On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> > wrote:
> > >
> > > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > > 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
> > > > >>> ---
> > > > >>> 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, >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(>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(>g) & 0x3FFF;
> > > > >>> @@ -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(>g);
> > > > >>> bytestream2_skip(>g, 2);
> > > > >>> height = bytestream2_get_be16(>g);
> > > > >>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 09:51:45AM +0200, Hendrik Leppkes wrote:
> On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
> wrote:
> >
> > On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > > 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
> > > >>> ---
> > > >>> 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, >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(>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(>g) & 0x3FFF;
> > > >>> @@ -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(>g);
> > > >>> bytestream2_skip(>g, 2);
> > > >>> height = bytestream2_get_be16(>g);
> > > >>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 9:51 AM Paul B Mahol wrote:
>
>
> On Mon, Aug 26, 2019 at 9:37 AM Michael Niedermayer
> wrote:
>
>> On Sun, Aug 25, 2019 at 11:43:42PM -0300, James Almer wrote:
>> > 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
>> > > ---
>> > > 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, >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(>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(>g) & 0x3FFF;
>> > > @@ -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(>g);
>> > > bytestream2_skip(>g, 2);
>> > > height = bytestream2_get_be16(>g);
>> > > bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 9:37 AM Michael Niedermayer
wrote:
> On Sun, Aug 25, 2019 at 11:43:42PM -0300, James Almer wrote:
> > 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
> > > ---
> > > 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, >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(>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(>g) & 0x3FFF;
> > > @@ -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(>g);
> > > bytestream2_skip(>g, 2);
> > > height = bytestream2_get_be16(>g);
> > > bytestream2_skip(>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
>
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Mon, Aug 26, 2019 at 9:33 AM Michael Niedermayer
wrote:
>
> On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> > 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
> > >>> ---
> > >>> 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, >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(>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(>g) & 0x3FFF;
> > >>> @@ -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(>g);
> > >>> bytestream2_skip(>g, 2);
> > >>> height = bytestream2_get_be16(>g);
> > >>> bytestream2_skip(>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.
>
> ok, lets see some actual numbers
>
> This
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote: > 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: [...] > > 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. If you run a filter that requires a minimum number of frames per second then you will need to ensure that to be there. Theres a wide range of reasons why your input might not have frames there. So unless you only work with filter chains designed for a single input container and codec simply assuming that there would be 25 fps at least might once in a while bite you with no frame drop patches. you might even with a single container and codec be hit by a 1fps or lower slide show once in a while thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting. signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Sun, Aug 25, 2019 at 11:43:42PM -0300, James Almer wrote:
> 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
> > ---
> > 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, >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(>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(>g) & 0x3FFF;
> > @@ -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(>g);
> > bytestream2_skip(>g, 2);
> > height = bytestream2_get_be16(>g);
> > bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Sun, Aug 25, 2019 at 08:04:03PM -0300, James Almer wrote:
> 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
> >>> ---
> >>> 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, >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(>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(>g) & 0x3FFF;
> >>> @@ -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(>g);
> >>> bytestream2_skip(>g, 2);
> >>> height = bytestream2_get_be16(>g);
> >>> bytestream2_skip(>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.
ok, lets see some actual numbers
This comparission is about fixing ticket 7880 (just saying so we dont loose
track of what we compare here)
this qtrle patchset which fixes the last frame
time ./ffmpeg -i clock.mov -vf scale=1920x1080 -qscale 2 -y test-new.avi
real0m0.233s
user0m0.404s
sys 0m0.036s
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On Sun, Aug 25, 2019 at 11:37:02PM -0300, James Almer wrote:
> On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> > Fixes: Ticket7880
> >
> > Signed-off-by: Michael Niedermayer
> > ---
> > 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;
>
> This same effect can be achieved by instead checking for
> s->frame->data[0] != NULL after adding a AVCodec.flush() callback that
> unrefs s->frame (Which should be added regardless of this patch).
>
> That way you wont need to add need_flush to AVCodecInternal.
need_flush in common code was done to avoid haveing the same code in
every decoder as shown in the previous iteration of the patchset
Avoiding the need_flush completely by clearing the internal frame
might not work completly because there are more cases than this
1. seek (clear the last frame is possible so here it works)
2. output a frame (here we cannot clear the frame as it may be used by the next
frame, still this is the same case, if a frame was just output
theres no need to flush at the end so we also need need_flush = 0
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Never trust a computer, one day, it may think you are the virus. -- Compn
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
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
> ---
> 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, >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> Fixes: Ticket7880
>
> Signed-off-by: Michael Niedermayer
> ---
> 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;
This same effect can be achieved by instead checking for
s->frame->data[0] != NULL after adding a AVCodec.flush() callback that
unrefs s->frame (Which should be added regardless of this patch).
That way you wont need to add need_flush to AVCodecInternal.
> +ret = ff_setup_buffered_frame_for_return(avctx, data, s->frame,
> >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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
>
___
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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
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
---
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, >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(>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(>g) & 0x3FFF;
@@ -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(>g);
bytestream2_skip(>g, 2);
height = bytestream2_get_be16(>g);
bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
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
>>> ---
>>> 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, >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(>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(>g) & 0x3FFF;
>>> @@ -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(>g);
>>> bytestream2_skip(>g, 2);
>>> height = bytestream2_get_be16(>g);
>>> bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
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
> > > ---
> > > 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, >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(>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(>g) & 0x3FFF;
> > > @@ -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(>g);
> > > bytestream2_skip(>g, 2);
> > > height = bytestream2_get_be16(>g);
> > > bytestream2_skip(>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
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
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
> > ---
> > 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, >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(>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(>g) & 0x3FFF;
> > @@ -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(>g);
> > bytestream2_skip(>g, 2);
> > height = bytestream2_get_be16(>g);
> > bytestream2_skip(>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 ...
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> Fixes: Ticket7880
>
> Signed-off-by: Michael Niedermayer
> ---
> 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,
> >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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.
___
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".
Re: [FFmpeg-devel] [PATCH 5/6] avcodec/qtrle: return last frame even if unchanged
Very ugly non-solution. NAK.
On Sat, Aug 24, 2019 at 8:27 PM Michael Niedermayer
wrote:
> Fixes: Ticket7880
>
> Signed-off-by: Michael Niedermayer
> ---
> 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, >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(>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(>g) & 0x3FFF;
> @@ -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(>g);
> bytestream2_skip(>g, 2);
> height = bytestream2_get_be16(>g);
> bytestream2_skip(>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
> --
> 2.23.0
>
> ___
> 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".
___
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".
