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 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, &s->flush_pkt);
> >>> +if (ret < 0)
> >>> +return ret;
> >>> +*got_frame = 1;
> >>> +}
> >>> +return 0;
> >>> +}
> >>> +s->flush_pkt = *avpkt;
> >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> >>> +
> >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >>>
> >>> /* check if this frame is even supposed to change */
> >>> -if (avpkt->size < 8)
> >>> +if (avpkt->size < 8) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> +avctx->internal->need_flush = 0;
> >>>
> >>> /* start after the chunk size */
> >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> height = bytestream2_get_be16(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> -if (height > s->avctx->height - start_line)
> >>> +if (height > s->avctx->height - start_line) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> } else {
> >>> start_line = 0;
> >>> height = s->avctx->height;
> >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> >>> .init = qtrle_decode_init,
> >>> .close = qtrle_decode_end,
> >>> .decode = qtrle_decode_frame,
> >>> -.capabilities = AV_CODEC_CAP_DR1,
> >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> >>> FF_CODEC_CAP_SETS_PKT_POS,
> >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> >>> };
> >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> >>> index 27bb8aad71..39a03b7b6c 100644
> >>> --- a/tests/ref/fate/qtrle-8bit
> >>> +++ b/tests/ref/fate/qtrle-8bit
> >>> @@ -61,3 +61,4 @@
> >>> 0,160,160,1, 921600, 0xcfd6ad2b
> >>> 0,163,163,1, 921600, 0x3b372379
> >>> 0,165,165,1, 921600, 0x36f245f5
> >>> +0,166,166,1, 921600, 0x36f245f5
> >>
> >> Following what i said in the nuv patch, do you still experience timeouts
> >> with the current codebase, or even if you revert commit
> >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> >> after my ref counting changes to target_dec_fuzzer.c
> >>
> >> This is a very ugly solution to a problem that was already solved when
> >> reference counting was introduced. Returning duplicate frames to achieve
> >> cfr in decoders where it's expected shouldn't affect performance.
> >
> > Maybe i should ask this backward to make it clearer what this is trying
> > to fix.
> >
> > Consider a patch that would return every frame twice with the same ref
> > of course.
> > Would you consider this to have 0 performance impact ?
>
> No, but it would be negligible.
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
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, &s->flush_pkt);
> > +if (ret < 0)
> > +return ret;
> > +*got_frame = 1;
> > +}
> > +return 0;
> > +}
> > +s->flush_pkt = *avpkt;
> > +s->frame->pkt_dts = s->flush_pkt.dts;
> > +
> > bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >
> > /* check if this frame is even supposed to change */
> > -if (avpkt->size < 8)
> > +if (avpkt->size < 8) {
> > +avctx->internal->need_flush = 1;
> > return avpkt->size;
> > +}
> > +avctx->internal->need_flush = 0;
> >
> > /* start after the chunk size */
> > size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > bytestream2_skip(&s->g, 2);
> > height = bytestream2_get_be16(&s->g);
> > bytestream2_skip(&s->g, 2);
> > -if (height > s->avctx->height - start_line)
> > +if (height > s->avctx->height - start_line) {
> > +avctx->internal->need_flush = 1;
> > return avpkt->size;
> > +}
> > } else {
> > start_line = 0;
> > height = s->avctx->height;
> > @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > .init = qtrle_decode_init,
> > .close = qtrle_decode_end,
> > .decode = qtrle_decode_frame,
> > -.capabilities = AV_CODEC_CAP_DR1,
> > +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > FF_CODEC_CAP_SETS_PKT_POS,
> > +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > };
> > diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > index 27bb8aad71..39a03b7b6c 100644
> > --- a/tests/ref/fate/qtrle-8bit
> > +++ b/tests/ref/fate/qtrle-8bit
> > @@ -61,3 +61,4 @@
> > 0,160,160,1, 921600, 0xcfd6ad2b
> > 0,163,163,1, 921600, 0x3b372379
> > 0,165,165,1, 921600, 0x36f245f5
> > +0,166,166,1, 921600, 0x36f245f5
>
> Following what i said in the nuv patch, do you still experience timeouts
> with the current codebase, or even if you revert commit
> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> existing frame buffer shouldn't be expensive anymore for the fuzzer
> after my ref counting changes to target_dec_fuzzer.c
>
> This is a very ugly solution to a problem that was already solved when
> reference counting was introduced. Returning duplicate frames to achieve
> cfr in decoders where it's expected shouldn't affect performance.
> >>>
> >>> Maybe i should ask this backward to make it clearer what this is trying
> >>> to fix.
> >>>
> >>> Consider a patch that would return every frame twice with the same ref
> >>> of course.
> >>> Would you consider this to have 0 performance impact ?
>
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 6/6] avcodec/nuv: Avoid duplicating frames
On Sun, Aug 25, 2019 at 11:35 PM Michael Niedermayer
wrote:
> On Sun, Aug 25, 2019 at 01:05:35PM -0300, James Almer wrote:
> > On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> > > Testcase:
> 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
> > > Testcase:
> 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280
> > >
> > > Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer
> > > ---
> > > libavcodec/nuv.c | 25 +
> > > tests/ref/fate/nuv-rtjpeg | 1 -
> > > 2 files changed, 21 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
> > > index 75b14bce5b..39479d2389 100644
> > > --- a/libavcodec/nuv.c
> > > +++ b/libavcodec/nuv.c
> > > @@ -42,6 +42,7 @@ typedef struct NuvContext {
> > > unsigned char *decomp_buf;
> > > uint32_t lq[64], cq[64];
> > > RTJpegContext rtj;
> > > +AVPacket flush_pkt;
> > > } NuvContext;
> > >
> > > static const uint8_t fallback_lquant[] = {
> > > @@ -172,6 +173,20 @@ static int decode_frame(AVCodecContext *avctx,
> void *data, int *got_frame,
> > > NUV_COPY_LAST = 'L'
> > > } comptype;
> > >
> > > +if (!avpkt->data) {
> > > +if (avctx->internal->need_flush) {
> > > +avctx->internal->need_flush = 0;
> > > +ret = ff_setup_buffered_frame_for_return(avctx, data,
> c->pic, &c->flush_pkt);
> > > +if (ret < 0)
> > > +return ret;
> > > +*got_frame = 1;
> > > +}
> > > +return 0;
> > > +}
> > > +c->flush_pkt = *avpkt;
> > > +c->pic->pkt_dts = c->flush_pkt.dts;
> > > +
> > > +
> > > if (buf_size < 12) {
> > > av_log(avctx, AV_LOG_ERROR, "coded frame too small\n");
> > > return AVERROR_INVALIDDATA;
> > > @@ -204,8 +219,8 @@ static int decode_frame(AVCodecContext *avctx,
> void *data, int *got_frame,
> > > }
> > > break;
> > > case NUV_COPY_LAST:
> > > -keyframe = 0;
> > > -break;
> > > +avctx->internal->need_flush = 1;
> > > +return buf_size;
> > > default:
> > > keyframe = 1;
> > > break;
> > > @@ -313,6 +328,7 @@ retry:
> > > if ((result = av_frame_ref(picture, c->pic)) < 0)
> > > return result;
> > >
> > > +avctx->internal->need_flush = 0;
> > > *got_frame = 1;
> > > return orig_size;
> > > }
> > > @@ -364,6 +380,7 @@ AVCodec ff_nuv_decoder = {
> > > .init = decode_init,
> > > .close = decode_end,
> > > .decode = decode_frame,
> > > -.capabilities = AV_CODEC_CAP_DR1,
> > > -.caps_internal = FF_CODEC_CAP_INIT_CLEANUP,
> > > +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS |
> > > + FF_CODEC_CAP_INIT_CLEANUP,
> > > };
> > > diff --git a/tests/ref/fate/nuv-rtjpeg b/tests/ref/fate/nuv-rtjpeg
> > > index b6f3b080dc..0914b985ec 100644
> > > --- a/tests/ref/fate/nuv-rtjpeg
> > > +++ b/tests/ref/fate/nuv-rtjpeg
> > > @@ -6,7 +6,6 @@
> > > 0,118,118,0, 460800, 0x54aedafe
> > > 0,152,152,0, 460800, 0xb7aa8b56
> > > 0,177,177,0, 460800, 0x283ea3b5
> > > -0,202,202,0, 460800, 0x283ea3b5
> >
> > I haven't been following these cfr -> vfr patches too closely, but why
> > remove the duplicate frames (thus changing the expected compliant
> > behavior) instead of ensuring the duplicate ones are made with no
> > performance penalty by reusing the buffer reference from the previous
> frame?
>
> Because a filter or encoder or whatever follows the decoder would then
> process
> them multiple times unneccessarily.
>
Incorrect. This "extra" as you call processing is necessary.
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Observe your enemies, for they first find out your faults. -- Antisthenes
> ___
> 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".
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, &s->flush_pkt);
> > >>> +if (ret < 0)
> > >>> +return ret;
> > >>> +*got_frame = 1;
> > >>> +}
> > >>> +return 0;
> > >>> +}
> > >>> +s->flush_pkt = *avpkt;
> > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > >>> +
> > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > >>>
> > >>> /* check if this frame is even supposed to change */
> > >>> -if (avpkt->size < 8)
> > >>> +if (avpkt->size < 8) {
> > >>> +avctx->internal->need_flush = 1;
> > >>> return avpkt->size;
> > >>> +}
> > >>> +avctx->internal->need_flush = 0;
> > >>>
> > >>> /* start after the chunk size */
> > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > >>> bytestream2_skip(&s->g, 2);
> > >>> height = bytestream2_get_be16(&s->g);
> > >>> bytestream2_skip(&s->g, 2);
> > >>> -if (height > s->avctx->height - start_line)
> > >>> +if (height > s->avctx->height - start_line) {
> > >>> +avctx->internal->need_flush = 1;
> > >>> return avpkt->size;
> > >>> +}
> > >>> } else {
> > >>> start_line = 0;
> > >>> height = s->avctx->height;
> > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > >>> .init = qtrle_decode_init,
> > >>> .close = qtrle_decode_end,
> > >>> .decode = qtrle_decode_frame,
> > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > >>> FF_CODEC_CAP_SETS_PKT_POS,
> > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > >>> };
> > >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > >>> index 27bb8aad71..39a03b7b6c 100644
> > >>> --- a/tests/ref/fate/qtrle-8bit
> > >>> +++ b/tests/ref/fate/qtrle-8bit
> > >>> @@ -61,3 +61,4 @@
> > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > >>> 0,163,163,1, 921600, 0x3b372379
> > >>> 0,165,165,1, 921600, 0x36f245f5
> > >>> +0,166,166,1, 921600, 0x36f245f5
> > >>
> > >> Following what i said in the nuv patch, do you still experience timeouts
> > >> with the current codebase, or even if you revert commit
> > >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> > >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> > >> after my ref counting changes to target_dec_fuzzer.c
> > >>
> > >> This is a very ugly solution to a problem that was already solved when
> > >> reference counting was introduced. Returning duplicate frames to achieve
> > >> cfr in decoders where it's expected shouldn't affect performance.
> > >
> > > Maybe i should ask this backward to make it clearer what this is trying
> > > to fix.
> > >
> > > Consider a patch that would return every frame twice with the same ref
> > > of course.
> > > Would you consider this to have 0 performance impact ?
> >
> > No, but it would be negligible.
>
> ok, lets see some actu
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, &s->flush_pkt);
> > > +if (ret < 0)
> > > +return ret;
> > > +*got_frame = 1;
> > > +}
> > > +return 0;
> > > +}
> > > +s->flush_pkt = *avpkt;
> > > +s->frame->pkt_dts = s->flush_pkt.dts;
> > > +
> > > bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > >
> > > /* check if this frame is even supposed to change */
> > > -if (avpkt->size < 8)
> > > +if (avpkt->size < 8) {
> > > +avctx->internal->need_flush = 1;
> > > return avpkt->size;
> > > +}
> > > +avctx->internal->need_flush = 0;
> > >
> > > /* start after the chunk size */
> > > size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > bytestream2_skip(&s->g, 2);
> > > height = bytestream2_get_be16(&s->g);
> > > bytestream2_skip(&s->g, 2);
> > > -if (height > s->avctx->height - start_line)
> > > +if (height > s->avctx->height - start_line) {
> > > +avctx->internal->need_flush = 1;
> > > return avpkt->size;
> > > +}
> > > } else {
> > > start_line = 0;
> > > height = s->avctx->height;
> > > @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > .init = qtrle_decode_init,
> > > .close = qtrle_decode_end,
> > > .decode = qtrle_decode_frame,
> > > -.capabilities = AV_CODEC_CAP_DR1,
> > > +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> > > +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > };
> > > diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > > index 27bb8aad71..39a03b7b6c 100644
> > > --- a/tests/ref/fate/qtrle-8bit
> > > +++ b/tests/ref/fate/qtrle-8bit
> > > @@ -61,3 +61,4 @@
> > > 0,160,160,1, 921600, 0xcfd6ad2b
> > > 0,163,163,1, 921600, 0x3b372379
> > > 0,165,165,1, 921600, 0x36f245f5
> > > +0,166,166,1, 921600, 0x36f245f5
> >
> > Following what i said in the nuv patch, do you still experience
> timeouts
> > with the current codebase, or even if you revert commit
> > a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> > existing frame buffer shouldn't be expensive anymore for the fuzzer
> > after my ref counting changes to target_dec_fuzzer.c
> >
> > This is a very ugly solution to a problem that was already solved
> when
> > reference counting was introduced. Returning duplicate frames to
> achieve
> > cfr in decoders where it's expected shouldn't affect performance.
> > >>>
> > >>> Maybe i should ask this backward to make it clearer wha
Re: [FFmpeg-devel] [PATCH] DVB EPG decoder
Okay, thanks
I will patch that
Le sam. 24 août 2019 à 20:09, Marton Balint a écrit :
>
>
> On Fri, 23 Aug 2019, Anthony Delannoy wrote:
>
> >> I think we should only merge the part of this patchset which makes the EIT
> >> available as a data stream. Parsing the whole EIT or dumping the data as
> >> ASCII is not libavcodec's or libavutil's job.
> >
> > The EPG decoder does not change the table's data, it just store them
> > and it happens to
> > contains text sometimes.
> > Some utilites functions I made in libavutil/dvb can convert those raw
> > data in text
> > description(s) for certain descriptors.
> >
> >> Also there is no such concept in libavcodec as a data decoder, if
> >> something happens to
> >> work with avcodec_send_packet/avcodec_receive_frame that is mostly luck I
> >> believe.
> >
> > avcodec_send_packet and avcodec_receive_frame both call
> > AVCodec::receive_frame if it is
> > implemented. That's why my implementation of the EPG decoder does
> > contain this function.
> >
> > For now my test scripts consists to:
> > ```
> > 99 if (st->codecpar->codec_id != AV_CODEC_ID_EPG)
> > 100 goto free_pkt;
> > 101
> > 102 ret = avcodec_send_packet(dec_ctx, &pkt);
> > ...
> > 112 while (1) {
> > 113 ret = avcodec_receive_frame(dec_ctx, frame);
> > 114 if (ret < 0)
> > 115 break;
> > 116
> > 117 for (int i = 0; i < frame->nb_side_data; i++) {
> > 118 AVFrameSideData *sd = frame->side_data[i];
> > 119 if (sd && sd->type == AV_FRAME_DATA_EPG_TABLE) {
> > 120 EPGTable *table = sd->data;
> > 121 av_epg_show_table(table, AV_LOG_WARNING);
> > 122 }
> > 123 }
> > 124 av_frame_unref(frame);
> > 125 }
> > 126
> > 127 free_pkt:
> > 128 av_packet_unref(&pkt);
> > ```
> > It works as intended and permits to decode EPGTable without issues, I
> > tried on multiple channels.
> >
> > I wanted to permit the table decoding and not just an EPG data stream
> > to permit easy reading
> > and in the future easy modification before encoding EPG back.
> >
> >> I am also not sure if we should add the EIT PID to all programs, that
> >> would mess up the direct relation between a PMT and an AVProgram, and we
> >> probably don't want that. So I'd rather see the EIT data stream as a
> >> standalone PID separate from the programs.
> >
> > I'm not an expert but I think each service/program contains a PMT
> > table and all others. So one
> > EPG stream (if available) for each service.
> > That's what I understood from the ETSI EN 300 468 V1.16.1
>
> The EPG stream is a single stream in the whole TS, it is on a single PID.
> The PMTs do not reference the EIT PID, therefore we should not make the
> EPG stream part of the programs, even if the EPG stream can contain the
> schedule of the programs.
>
> Regards,
> Marton
> ___
> 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".
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, &s->flush_pkt);
>> > > +if (ret < 0)
>> > > +return ret;
>> > > +*got_frame = 1;
>> > > +}
>> > > +return 0;
>> > > +}
>> > > +s->flush_pkt = *avpkt;
>> > > +s->frame->pkt_dts = s->flush_pkt.dts;
>> > > +
>> > > bytestream2_init(&s->g, avpkt->data, avpkt->size);
>> > >
>> > > /* check if this frame is even supposed to change */
>> > > -if (avpkt->size < 8)
>> > > +if (avpkt->size < 8) {
>> > > +avctx->internal->need_flush = 1;
>> > > return avpkt->size;
>> > > +}
>> > > +avctx->internal->need_flush = 0;
>> > >
>> > > /* start after the chunk size */
>> > > size = bytestream2_get_be32(&s->g) & 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(&s->g);
>> > > bytestream2_skip(&s->g, 2);
>> > > height = bytestream2_get_be16(&s->g);
>> > > bytestream2_skip(&s->g, 2);
>> > > -if (height > s->avctx->height - start_line)
>> > > +if (height > s->avctx->height - start_line) {
>> > > +avctx->internal->need_flush = 1;
>> > > return avpkt->size;
>> > > +}
>> > > } else {
>> > > start_line = 0;
>> > > height = s->avctx->height;
>> > > @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
>> > > .init = qtrle_decode_init,
>> > > .close = qtrle_decode_end,
>> > > .decode = qtrle_decode_frame,
>> > > -.capabilities = AV_CODEC_CAP_DR1,
>> > > +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
>> FF_CODEC_CAP_SETS_PKT_POS,
>> > > +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>> > > };
>> > > diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
>> > > index 27bb8aad71..39a03b7b6c 100644
>> > > --- a/tests/ref/fate/qtrle-8bit
>> > > +++ b/tests/ref/fate/qtrle-8bit
>> > > @@ -61,3 +61,4 @@
>> > > 0,160,160,1, 921600, 0xcfd6ad2b
>> > > 0,163,163,1, 921600, 0x3b372379
>> > > 0,165,165,1, 921600, 0x36f245f5
>> > > +0,166,166,1, 921600, 0x36f245f5
>> >
>> > Following what i said in the nuv patch, do you still experience
>> timeouts
>> > with the current codebase, or even if you revert commit
>> > a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to
>> an
>> > existing frame buffer shouldn't be expensive anymore for the fuzzer
>> > after my ref counting changes to target_dec_fuzzer.c
>> >
>> > This is a very ugly solution to a problem that was already solved
>> when
>> > reference counting was introduced. Returnin
Re: [FFmpeg-devel] [PATCH 4/5] avcodec/alac: Check for bps of 0
On Sun, Aug 25, 2019 at 12:06:30PM -0300, James Almer wrote:
> On 8/25/2019 4:32 AM, Michael Niedermayer wrote:
> > On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote:
> >> On 8/8/2019 8:23 PM, Michael Niedermayer wrote:
> >>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
> >>> Fixes:
> >>> 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440
> >>>
> >>> Found-by: continuous fuzzing process
> >>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer
> >>> ---
> >>> libavcodec/alac.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> >>> index 6086e2caa8..1196925aa7 100644
> >>> --- a/libavcodec/alac.c
> >>> +++ b/libavcodec/alac.c
> >>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx,
> >>> AVFrame *frame, int ch_index,
> >>>
> >>> alac->extra_bits = get_bits(&alac->gb, 2) << 3;
> >>> bps = alac->sample_size - alac->extra_bits + channels - 1;
> >>> -if (bps > 32U) {
> >>> +if (bps > 32 || bps < 1) {
> >>> avpriv_report_missing_feature(avctx, "bps %d", bps);
> >>> return AVERROR_PATCHWELCOME;
> >>
> >> bps 0 (or negative) is obviously a broken file,
> >
> > id say very likely a broken file, yes
> >
> >
> >> so asking for a sample
> >> is pointless. Just return invalid data in those cases, and leave this
> >> check for > 32.
> >
> > thats a few lines more code, for an error code and different/no message
> > its a bit difficult to guess where people prefer the extra code to be
> > correct and where they prefer somewhat incorrect solutions to minimize
> > fuzzer found bugfixes.
>
> I don't know who this was aimed to, but afaik i don't ask or don't
> intend to ask for "incorrect solutions" for fuzzer found issues.
no, of course not, but in some reviews people complained along the
lines of there being too much fuzzer found fixes/checks.
> I simply stated that asking for a sample in a situation where it's known
> that the sample is simply a broken/corrupt one makes no sense and can
> end up wasting a user's time for bothering to submit it. The extra lines
> to properly abort in such cases are justified.
yes, your request was fine, sorry if it appeared that i meant something
else
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
You can kill me, but you cannot change the truth.
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 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, &s->flush_pkt);
> > > >>> +if (ret < 0)
> > > >>> +return ret;
> > > >>> +*got_frame = 1;
> > > >>> +}
> > > >>> +return 0;
> > > >>> +}
> > > >>> +s->flush_pkt = *avpkt;
> > > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > > >>> +
> > > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > > >>>
> > > >>> /* check if this frame is even supposed to change */
> > > >>> -if (avpkt->size < 8)
> > > >>> +if (avpkt->size < 8) {
> > > >>> +avctx->internal->need_flush = 1;
> > > >>> return avpkt->size;
> > > >>> +}
> > > >>> +avctx->internal->need_flush = 0;
> > > >>>
> > > >>> /* start after the chunk size */
> > > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > >>> bytestream2_skip(&s->g, 2);
> > > >>> height = bytestream2_get_be16(&s->g);
> > > >>> bytestream2_skip(&s->g, 2);
> > > >>> -if (height > s->avctx->height - start_line)
> > > >>> +if (height > s->avctx->height - start_line) {
> > > >>> +avctx->internal->need_flush = 1;
> > > >>> return avpkt->size;
> > > >>> +}
> > > >>> } else {
> > > >>> start_line = 0;
> > > >>> height = s->avctx->height;
> > > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > >>> .init = qtrle_decode_init,
> > > >>> .close = qtrle_decode_end,
> > > >>> .decode = qtrle_decode_frame,
> > > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > > >>> FF_CODEC_CAP_SETS_PKT_POS,
> > > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > >>> };
> > > >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > > >>> index 27bb8aad71..39a03b7b6c 100644
> > > >>> --- a/tests/ref/fate/qtrle-8bit
> > > >>> +++ b/tests/ref/fate/qtrle-8bit
> > > >>> @@ -61,3 +61,4 @@
> > > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > > >>> 0,163,163,1, 921600, 0x3b372379
> > > >>> 0,165,165,1, 921600, 0x36f245f5
> > > >>> +0,166,166,1, 921600, 0x36f245f5
> > > >>
> > > >> Following what i said in the nuv patch, do you still experience
> > > >> timeouts
> > > >> with the current codebase, or even if you revert commit
> > > >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> > > >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> > > >> after my ref counting changes to target_dec_fuzzer.c
> > > >>
> > > >> This is a very ugly solution to a problem that was already solved when
> > > >> reference counting was introduced. Returning duplicate frames to
> > > >> achieve
> > > >> cfr in decoders where it's expected shouldn't affect performance.
> > > >
> >
Re: [FFmpeg-devel] [PATCH 6/6] avcodec/nuv: Avoid duplicating frames
On Mon, Aug 26, 2019 at 09:53:46AM +0200, Paul B Mahol wrote:
> On Sun, Aug 25, 2019 at 11:35 PM Michael Niedermayer
> wrote:
>
> > On Sun, Aug 25, 2019 at 01:05:35PM -0300, James Almer wrote:
> > > On 8/24/2019 3:18 PM, Michael Niedermayer wrote:
> > > > Testcase:
> > 14843/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5661969614372864
> > > > Testcase:
> > 16257/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_NUV_fuzzer-5769175464673280
> > > >
> > > > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer
> > > > ---
> > > > libavcodec/nuv.c | 25 +
> > > > tests/ref/fate/nuv-rtjpeg | 1 -
> > > > 2 files changed, 21 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
> > > > index 75b14bce5b..39479d2389 100644
> > > > --- a/libavcodec/nuv.c
> > > > +++ b/libavcodec/nuv.c
> > > > @@ -42,6 +42,7 @@ typedef struct NuvContext {
> > > > unsigned char *decomp_buf;
> > > > uint32_t lq[64], cq[64];
> > > > RTJpegContext rtj;
> > > > +AVPacket flush_pkt;
> > > > } NuvContext;
> > > >
> > > > static const uint8_t fallback_lquant[] = {
> > > > @@ -172,6 +173,20 @@ static int decode_frame(AVCodecContext *avctx,
> > void *data, int *got_frame,
> > > > NUV_COPY_LAST = 'L'
> > > > } comptype;
> > > >
> > > > +if (!avpkt->data) {
> > > > +if (avctx->internal->need_flush) {
> > > > +avctx->internal->need_flush = 0;
> > > > +ret = ff_setup_buffered_frame_for_return(avctx, data,
> > c->pic, &c->flush_pkt);
> > > > +if (ret < 0)
> > > > +return ret;
> > > > +*got_frame = 1;
> > > > +}
> > > > +return 0;
> > > > +}
> > > > +c->flush_pkt = *avpkt;
> > > > +c->pic->pkt_dts = c->flush_pkt.dts;
> > > > +
> > > > +
> > > > if (buf_size < 12) {
> > > > av_log(avctx, AV_LOG_ERROR, "coded frame too small\n");
> > > > return AVERROR_INVALIDDATA;
> > > > @@ -204,8 +219,8 @@ static int decode_frame(AVCodecContext *avctx,
> > void *data, int *got_frame,
> > > > }
> > > > break;
> > > > case NUV_COPY_LAST:
> > > > -keyframe = 0;
> > > > -break;
> > > > +avctx->internal->need_flush = 1;
> > > > +return buf_size;
> > > > default:
> > > > keyframe = 1;
> > > > break;
> > > > @@ -313,6 +328,7 @@ retry:
> > > > if ((result = av_frame_ref(picture, c->pic)) < 0)
> > > > return result;
> > > >
> > > > +avctx->internal->need_flush = 0;
> > > > *got_frame = 1;
> > > > return orig_size;
> > > > }
> > > > @@ -364,6 +380,7 @@ AVCodec ff_nuv_decoder = {
> > > > .init = decode_init,
> > > > .close = decode_end,
> > > > .decode = decode_frame,
> > > > -.capabilities = AV_CODEC_CAP_DR1,
> > > > -.caps_internal = FF_CODEC_CAP_INIT_CLEANUP,
> > > > +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > > +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > FF_CODEC_CAP_SETS_PKT_POS |
> > > > + FF_CODEC_CAP_INIT_CLEANUP,
> > > > };
> > > > diff --git a/tests/ref/fate/nuv-rtjpeg b/tests/ref/fate/nuv-rtjpeg
> > > > index b6f3b080dc..0914b985ec 100644
> > > > --- a/tests/ref/fate/nuv-rtjpeg
> > > > +++ b/tests/ref/fate/nuv-rtjpeg
> > > > @@ -6,7 +6,6 @@
> > > > 0,118,118,0, 460800, 0x54aedafe
> > > > 0,152,152,0, 460800, 0xb7aa8b56
> > > > 0,177,177,0, 460800, 0x283ea3b5
> > > > -0,202,202,0, 460800, 0x283ea3b5
> > >
> > > I haven't been following these cfr -> vfr patches too closely, but why
> > > remove the duplicate frames (thus changing the expected compliant
> > > behavior) instead of ensuring the duplicate ones are made with no
> > > performance penalty by reusing the buffer reference from the previous
> > frame?
> >
> > Because a filter or encoder or whatever follows the decoder would then
> > process
> > them multiple times unneccessarily.
> >
>
> Incorrect. This "extra" as you call processing is necessary.
no
if your final output is vfr and for that you drop duplicate frames (be that
in the decoder or filter)
you would get the exact same output as when you drop in the decoder
just that by earlier droping you avoid some processing.
If the frames where marked by some flag as duplicate or if they refer to
the same memory then finding them becomes easier and there is less
processing. (this was suggested by james and myself but sofar others
have not confirmed they would agree to that)
Either a flag or using the same ref for the duplicate frames would
require changes as well. The current ref fixes only work with the
fuzzer IIRC
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mpegtsenc: add support for setting PCR interval for VBR streams
Hi Marton, ‐‐‐ Original Message ‐‐‐ On Friday, 23 de August de 2019 23:13, Marton Balint wrote: > On Fri, 23 Aug 2019, Marton Balint wrote: > > > On Fri, 16 Aug 2019, Andreas Håkon wrote: > > > > > Hi Marton, > > > Very good work with your series of patches on the mpegtsenc! > > > > > A simple aesthetic comment: > > > Please, change this to... > > > /* By default, for VBR we select the highest multiple of frame duration > > > which is less than 100 ms. */ > > > > Ok, changed locally. > > Will apply patchset soon. > > Applied patchset. > Great! I'm testing it (last patchset) with my Interleaved MUX patch and it seems to play well. https://patchwork.ffmpeg.org/patch/14589/ So, please, can you start reviewing it? Regards. A.H. --- ___ 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 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, &s->flush_pkt);
> > > > >>> +if (ret < 0)
> > > > >>> +return ret;
> > > > >>> +*got_frame = 1;
> > > > >>> +}
> > > > >>> +return 0;
> > > > >>> +}
> > > > >>> +s->flush_pkt = *avpkt;
> > > > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > > > >>> +
> > > > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > > > >>>
> > > > >>> /* check if this frame is even supposed to change */
> > > > >>> -if (avpkt->size < 8)
> > > > >>> +if (avpkt->size < 8) {
> > > > >>> +avctx->internal->need_flush = 1;
> > > > >>> return avpkt->size;
> > > > >>> +}
> > > > >>> +avctx->internal->need_flush = 0;
> > > > >>>
> > > > >>> /* start after the chunk size */
> > > > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > > >>> bytestream2_skip(&s->g, 2);
> > > > >>> height = bytestream2_get_be16(&s->g);
> > > > >>> bytestream2_skip(&s->g, 2);
> > > > >>> -if (height > s->avctx->height - start_line)
> > > > >>> +if (height > s->avctx->height - start_line) {
> > > > >>> +avctx->internal->need_flush = 1;
> > > > >>> return avpkt->size;
> > > > >>> +}
> > > > >>> } else {
> > > > >>> start_line = 0;
> > > > >>> height = s->avctx->height;
> > > > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > > >>> .init = qtrle_decode_init,
> > > > >>> .close = qtrle_decode_end,
> > > > >>> .decode = qtrle_decode_frame,
> > > > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > > > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> > > > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > > >>> };
> > > > >>> diff --git a/tests/ref/fate/qtrle-8bit
> b/tests/ref/fate/qtrle-8bit
> > > > >>> index 27bb8aad71..39a03b7b6c 100644
> > > > >>> --- a/tests/ref/fate/qtrle-8bit
> > > > >>> +++ b/tests/ref/fate/qtrle-8bit
> > > > >>> @@ -61,3 +61,4 @@
> > > > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > > > >>> 0,163,163,1, 921600, 0x3b372379
> > > > >>> 0,165,165,1, 921600, 0x36f245f5
> > > > >>> +0,166,166,1, 921600, 0x36f245f5
> > > > >>
> > > > >> Following what i said in the nuv patch, do you still experience
> timeouts
> > > > >> with the current codebase, or even if you revert commit
> > > > >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to
> an
> > > > >> existing frame buffer shouldn't be expensive anymore for the
> fuzzer
> > > > >> after my ref counting changes to target_dec_fuzzer.c
> > > > >>
> > > > >> This is a very ugly solution to a problem that was already solved
> when
>
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, &s->flush_pkt);
> > > > >>> +if (ret < 0)
> > > > >>> +return ret;
> > > > >>> +*got_frame = 1;
> > > > >>> +}
> > > > >>> +return 0;
> > > > >>> +}
> > > > >>> +s->flush_pkt = *avpkt;
> > > > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > > > >>> +
> > > > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > > > >>>
> > > > >>> /* check if this frame is even supposed to change */
> > > > >>> -if (avpkt->size < 8)
> > > > >>> +if (avpkt->size < 8) {
> > > > >>> +avctx->internal->need_flush = 1;
> > > > >>> return avpkt->size;
> > > > >>> +}
> > > > >>> +avctx->internal->need_flush = 0;
> > > > >>>
> > > > >>> /* start after the chunk size */
> > > > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > > >>> bytestream2_skip(&s->g, 2);
> > > > >>> height = bytestream2_get_be16(&s->g);
> > > > >>> bytestream2_skip(&s->g, 2);
> > > > >>> -if (height > s->avctx->height - start_line)
> > > > >>> +if (height > s->avctx->height - start_line) {
> > > > >>> +avctx->internal->need_flush = 1;
> > > > >>> return avpkt->size;
> > > > >>> +}
> > > > >>> } else {
> > > > >>> start_line = 0;
> > > > >>> height = s->avctx->height;
> > > > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > > >>> .init = qtrle_decode_init,
> > > > >>> .close = qtrle_decode_end,
> > > > >>> .decode = qtrle_decode_frame,
> > > > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > > > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > > > >>> FF_CODEC_CAP_SETS_PKT_POS,
> > > > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > > >>> };
> > > > >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > > > >>> index 27bb8aad71..39a03b7b6c 100644
> > > > >>> --- a/tests/ref/fate/qtrle-8bit
> > > > >>> +++ b/tests/ref/fate/qtrle-8bit
> > > > >>> @@ -61,3 +61,4 @@
> > > > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > > > >>> 0,163,163,1, 921600, 0x3b372379
> > > > >>> 0,165,165,1, 921600, 0x36f245f5
> > > > >>> +0,166,166,1, 921600, 0x36f245f5
> > > > >>
> > > > >> Following what i said in the nuv patch, do you still experience
> > > > >> timeouts
> > > > >> with the current codebase, or even if you revert commit
> > > > >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> > > > >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> > > > >> after my ref counting changes to target_d
Re: [FFmpeg-devel] libavformat/mpegtsenc: new interleaved mux mode [v3]
Hi, ‐‐‐ Original Message ‐‐‐ On Monday, 19 de August de 2019 21:16, Andreas Håkon wrote: > Hi, > > This is the third version of my patch for an "interleaved MPEG-TS muxer". > This new version includes all recommendations and rebases the fix of the > incorrect PCR with multiple programs (fixed in collaboration with Marton > Balint). > > Supersedes: https://patchwork.ffmpeg.org/patch/13745/ > > How to check it: > > (Note: I use for all the tests the file > https://samples.ffmpeg.org/HDTV/bshi01.tp > ) > > - To check the new interlaced mode you can perform this other test: > > $ ffmpeg-patched -y -loglevel verbose -i bshi01.tp \ > -map "i:0x100" -c:0 copy \ > -map "i:0x110" -c:a:0 mp2 -ac:0 2 -ar:0 48000 -ab:0 384k \ > -map "i:0x130" -c:2 copy \ > -map "i:0x110" -c:3 copy \ > -map "i:0x100" -c:4 copy \ > -program title=Prog1:st=0:st=1:st=2 \ > -program title=Prog2:st=3:st=4 \ > -f mpegts -muxrate 44M -mpegts_extra_mux 1 bshi01-mode1.ts > > $ ffmpeg-patched -y -loglevel verbose -i bshi01.tp \ > -map "i:0x100" -c:0 copy \ > -map "i:0x110" -c:a:0 mp2 -ac:0 2 -ar:0 48000 -ab:0 384k \ > -map "i:0x130" -c:2 copy \ > -map "i:0x110" -c:3 copy \ > -map "i:0x100" -c:4 copy \ > -program title=Prog1:st=0:st=1:st=2 \ > -program title=Prog2:st=3:st=4 \ > -f mpegts -muxrate 44M -mpegts_extra_mux 0 bshi01-mode0.ts > > --- To understand what this patch is doing, see these screenshots about the test files "bshi01-mode0.ts" and "bshi01-mode1.ts": - Current muxing: https://trac.ffmpeg.org/attachment/ticket/8096/MODE-0.PNG - New interleaved muxing mode: https://trac.ffmpeg.org/attachment/ticket/8096/MODE-1.PNG See also this example of a professional muxer: https://trac.ffmpeg.org/attachment/ticket/8096/MPTS.PNG I hope this patch will be reviewed soon. Regards. A.H. --- ___ 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 1/2] avformat/http: add ff_http_get_shutdown_status api for check the status of shutdown
On Sun, Aug 25, 2019 at 22:53:19 +0800, Steven Liu wrote: Nitpicking: > + * Get the HTTP shurdown reponse status, be used after http_shutdown. ^^ Typos: shutdown, response. Moritz ___ 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/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, &s->flush_pkt);
> +if (ret < 0)
> +return ret;
> +*got_frame = 1;
> +}
> +return 0;
> +}
> +s->flush_pkt = *avpkt;
> +s->frame->pkt_dts = s->flush_pkt.dts;
> +
> bytestream2_init(&s->g, avpkt->data, avpkt->size);
>
> /* check if this frame is even supposed to change */
> -if (avpkt->size < 8)
> +if (avpkt->size < 8) {
> +avctx->internal->need_flush = 1;
> return avpkt->size;
> +}
> +avctx->internal->need_flush = 0;
>
> /* start after the chunk size */
> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> bytestream2_skip(&s->g, 2);
> height = bytestream2_get_be16(&s->g);
> bytestream2_skip(&s->g, 2);
> -if (height > s->avctx->height - start_line)
> +if (height > s->avctx->height - start_line) {
> +avctx->internal->need_flush = 1;
> return avpkt->size;
> +}
> } else {
> start_line = 0;
> height = s->avctx->height;
> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> .init = qtrle_decode_init,
> .close = qtrle_decode_end,
> .decode = qtrle_decode_frame,
> -.capabilities = AV_CODEC_CAP_DR1,
> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> };
> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> index 27bb8aad71..39a03b7b6c 100644
> --- a/tests/ref/fate/qtrle-8bit
> +++ b/tests/ref/fate/qtrle-8bit
> @@ -61,3 +61,4 @@
> 0,160,160,1, 921600, 0xcfd6ad2b
> 0,163,163,1, 921600, 0x3b372379
> 0,165,165,1, 921600, 0x36f245f5
> +0,166,166,1, 921600, 0x36f245f5
Following what i said in the nuv patch, do you still experience timeouts
with the current codebase, or even if you revert commit
a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
existing frame buffer shouldn't be expensive anymore for the fuzzer
after my ref counting changes to target_dec_fuzzer.c
This is a very ugly solution to a problem that was already solved when
reference counting was introduced. Returning duplicate frames to achieve
cfr in decoders where it's expected shouldn't affect performance.
>>>
>>> Maybe i should ask this backward to make it clearer what this is trying
>>> to fix.
>>>
>>> Consider a patch that would return every frame twice with the same ref
>>> of course.
>>> Would you consider this to have 0 performance impact ?
>>
>> No, but it would be negligible.
>
> 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
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, &s->flush_pkt);
> +if (ret < 0)
> +return ret;
> +*got_frame = 1;
> +}
> +return 0;
> +}
> +s->flush_pkt = *avpkt;
> +s->frame->pkt_dts = s->flush_pkt.dts;
> +
> bytestream2_init(&s->g, avpkt->data, avpkt->size);
>
> /* check if this frame is even supposed to change */
> -if (avpkt->size < 8)
> +if (avpkt->size < 8) {
> +avctx->internal->need_flush = 1;
> return avpkt->size;
> +}
> +avctx->internal->need_flush = 0;
>
> /* start after the chunk size */
> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> bytestream2_skip(&s->g, 2);
> height = bytestream2_get_be16(&s->g);
> bytestream2_skip(&s->g, 2);
> -if (height > s->avctx->height - start_line)
> +if (height > s->avctx->height - start_line) {
> +avctx->internal->need_flush = 1;
> return avpkt->size;
> +}
> } else {
> start_line = 0;
> height = s->avctx->height;
> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> .init = qtrle_decode_init,
> .close = qtrle_decode_end,
> .decode = qtrle_decode_frame,
> -.capabilities = AV_CODEC_CAP_DR1,
> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> };
> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> index 27bb8aad71..39a03b7b6c 100644
> --- a/tests/ref/fate/qtrle-8bit
> +++ b/tests/ref/fate/qtrle-8bit
> @@ -61,3 +61,4 @@
> 0,160,160,1, 921600, 0xcfd6ad2b
> 0,163,163,1, 921600, 0x3b372379
> 0,165,165,1, 921600, 0x36f245f5
> +0,166,166,1, 921600, 0x36f245f5
> >>>
> >>> Following what i said in the nuv patch, do you still experience timeouts
> >>> with the current codebase, or even if you revert commit
> >>> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> >>> existing frame buffer shouldn't be expensive anymore for the fuzzer
> >>> after my ref counting changes to target_dec_fuzzer.c
> >>>
> >>> This is a very ugly solution to a problem that was already solved when
> >>> reference counting was introduced. Returning duplicate frames to achieve
> >>> cfr in decoders where it's expected shouldn't affect performance.
> >>
> >> Maybe i should ask this backward to make it clearer what this is trying
> >> to fix.
> >>
> >> Consider a patch that would return every frame twice with the same ref
> >> of course.
> >> Would you consider this to have 0 performance impact ?
> >> if every filter following needs to process frames twice 2x CPU load
> >> and after the filters references would also not be the same anymore
> >> the following encoder woul
[FFmpeg-devel] [PATCH 2/3][RFC] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"
This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6.
This patch effectively made the decoder output vfr content out of samples
where cfr is expected.
Addresses ticket #7880.
Signed-off-by: James Almer
---
libavcodec/qtrle.c| 12 ++---
tests/ref/fate/qtrle-8bit | 109 ++
2 files changed, 115 insertions(+), 6 deletions(-)
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index 2c29547e5a..7367f8688d 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -455,10 +455,12 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
int ret, size;
bytestream2_init(&s->g, avpkt->data, avpkt->size);
+if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
+return ret;
/* check if this frame is even supposed to change */
if (avpkt->size < 8)
-return avpkt->size;
+goto done;
/* start after the chunk size */
size = bytestream2_get_be32(&s->g) & 0x3FFF;
@@ -472,20 +474,17 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
/* if a header is present, fetch additional decoding parameters */
if (header & 0x0008) {
if (avpkt->size < 14)
-return avpkt->size;
+goto done;
start_line = bytestream2_get_be16(&s->g);
bytestream2_skip(&s->g, 2);
height = bytestream2_get_be16(&s->g);
bytestream2_skip(&s->g, 2);
if (height > s->avctx->height - start_line)
-return avpkt->size;
+goto done;
} else {
start_line = 0;
height = s->avctx->height;
}
-if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
-return ret;
-
row_ptr = s->frame->linesize[0] * start_line;
switch (avctx->bits_per_coded_sample) {
@@ -546,6 +545,7 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
memcpy(s->frame->data[1], s->pal, AVPALETTE_SIZE);
}
+done:
if ((ret = av_frame_ref(data, s->frame)) < 0)
return ret;
*got_frame = 1;
diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
index 27bb8aad71..8da113d83e 100644
--- a/tests/ref/fate/qtrle-8bit
+++ b/tests/ref/fate/qtrle-8bit
@@ -4,60 +4,169 @@
#dimensions 0: 640x480
#sar 0: 0/1
0, 0, 0,1, 921600, 0x1492e3ed
+0, 1, 1,1, 921600, 0x1492e3ed
+0, 2, 2,1, 921600, 0x1492e3ed
0, 3, 3,1, 921600, 0x23ef4fc7
+0, 4, 4,1, 921600, 0x23ef4fc7
0, 5, 5,1, 921600, 0xe406d4be
+0, 6, 6,1, 921600, 0xe406d4be
+0, 7, 7,1, 921600, 0xe406d4be
0, 8, 8,1, 921600, 0x62b8b5a1
+0, 9, 9,1, 921600, 0x62b8b5a1
0, 10, 10,1, 921600, 0x7d8ba674
+0, 11, 11,1, 921600, 0x7d8ba674
+0, 12, 12,1, 921600, 0x7d8ba674
0, 13, 13,1, 921600, 0xfe666be7
+0, 14, 14,1, 921600, 0xfe666be7
0, 15, 15,1, 921600, 0x721baec0
+0, 16, 16,1, 921600, 0x721baec0
+0, 17, 17,1, 921600, 0x721baec0
0, 18, 18,1, 921600, 0xc237180a
+0, 19, 19,1, 921600, 0xc237180a
0, 20, 20,1, 921600, 0xf03a7482
+0, 21, 21,1, 921600, 0xf03a7482
+0, 22, 22,1, 921600, 0xf03a7482
0, 23, 23,1, 921600, 0x5612a391
+0, 24, 24,1, 921600, 0x5612a391
0, 25, 25,1, 921600, 0x9dbcc46a
+0, 26, 26,1, 921600, 0x9dbcc46a
+0, 27, 27,1, 921600, 0x9dbcc46a
0, 28, 28,1, 921600, 0xa128a5d5
+0, 29, 29,1, 921600, 0xa128a5d5
0, 30, 30,1, 921600, 0x63e0025c
+0, 31, 31,1, 921600, 0x63e0025c
+0, 32, 32,1, 921600, 0x63e0025c
0, 33, 33,1, 921600, 0x262359ed
+0, 34, 34,1, 921600, 0x262359ed
0, 35, 35,1, 921600, 0x343688e8
+0, 36, 36,1, 921600, 0x343688e8
+0, 37, 37,1, 921600, 0x343688e8
+0, 38, 38,1, 921600, 0x343688e8
+0, 39, 39,1, 921600, 0x343688e8
+0, 40, 40,1, 921600, 0x343688e8
+0, 41, 41,1, 921600, 0x343688e8
+0, 42, 42,1, 921600, 0x343688e8
+0, 43, 43,1, 921600, 0x343688e8
+0, 44, 44,1, 921600, 0x343688e8
0, 45, 45,1, 921600, 0xe4b29d57
+0, 46, 4
[FFmpeg-devel] [PATCH 3/3][RFC] avcodec/qtrle: signal duplicate frames as disposable
Signed-off-by: James Almer
---
Maybe we could also add an AV_CODEC_CAP_DISPOSABLE_FRAMES capability
to lavc and use it on relevant decoders like this one, to let the user
know to expect frames with this flag?
Although if lavc generic code doesn't do anything different for decoders
setting it, then i guess it's superfluous.
libavcodec/qtrle.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index 7367f8688d..bf3daf26e1 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -453,14 +453,17 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
int height, row_ptr;
int has_palette = 0;
int ret, size;
+int drop = 0;
bytestream2_init(&s->g, avpkt->data, avpkt->size);
if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
return ret;
/* check if this frame is even supposed to change */
-if (avpkt->size < 8)
+if (avpkt->size < 8) {
+drop = 1;
goto done;
+}
/* start after the chunk size */
size = bytestream2_get_be32(&s->g) & 0x3FFF;
@@ -473,14 +476,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) {
+drop = 1;
goto done;
+}
start_line = bytestream2_get_be16(&s->g);
bytestream2_skip(&s->g, 2);
height = bytestream2_get_be16(&s->g);
bytestream2_skip(&s->g, 2);
-if (height > s->avctx->height - start_line)
+if (height > s->avctx->height - start_line) {
+drop = 1;
goto done;
+}
} else {
start_line = 0;
height = s->avctx->height;
@@ -548,6 +555,8 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
done:
if ((ret = av_frame_ref(data, s->frame)) < 0)
return ret;
+if (drop)
+((AVFrame *)data)->flags |= AV_FRAME_FLAG_DISPOSABLE;
*got_frame = 1;
/* always report that the buffer was completely consumed */
--
2.22.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] [PATCH 1/3][RFC] avutil/frame: add AV_FRAME_FLAG_DISPOSABLE
Used to signal frames that can be safely discarded without losing
any picture data, side data, or metadata other than timing info.
Signed-off-by: James Almer
---
This implements the "disposable frame" solution to allow library
users to drop duplicate frames before further processing if desired,
instead of forcing decoders to output vfr content when cfr is coded
in the bitstream.
doc/APIchanges | 3 +++
libavutil/frame.h | 5 +
libavutil/version.h | 2 +-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/doc/APIchanges b/doc/APIchanges
index 682b67aa25..b28d702bae 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@ libavutil: 2017-10-21
API changes, most recent first:
+2019-08-xx - xx - lavu 58.34.100 - avframe.h
+ Add AV_FRAME_FLAG_DISPOSABLE
+
2019-08-xx - xx - lavf 58.31.101 - avio.h
4K limit removed from avio_printf.
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 5d3231e7bb..e1bf8795d2 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -522,6 +522,11 @@ typedef struct AVFrame {
* A flag to mark the frames which need to be decoded, but shouldn't be output.
*/
#define AV_FRAME_FLAG_DISCARD (1 << 2)
+/**
+ * A flag to indicate frames that can be discarded by the encoder. I.e. frames
+ * that are an exact duplicate of the previous one.
+ */
+#define AV_FRAME_FLAG_DISPOSABLE(1 << 3)
/**
* @}
*/
diff --git a/libavutil/version.h b/libavutil/version.h
index ecc6a7c9e2..658a508284 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
*/
#define LIBAVUTIL_VERSION_MAJOR 56
-#define LIBAVUTIL_VERSION_MINOR 33
+#define LIBAVUTIL_VERSION_MINOR 34
#define LIBAVUTIL_VERSION_MICRO 100
#define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
--
2.22.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".
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, &s->flush_pkt);
>> +if (ret < 0)
>> +return ret;
>> +*got_frame = 1;
>> +}
>> +return 0;
>> +}
>> +s->flush_pkt = *avpkt;
>> +s->frame->pkt_dts = s->flush_pkt.dts;
>> +
>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
>>
>> /* check if this frame is even supposed to change */
>> -if (avpkt->size < 8)
>> +if (avpkt->size < 8) {
>> +avctx->internal->need_flush = 1;
>> return avpkt->size;
>> +}
>> +avctx->internal->need_flush = 0;
>>
>> /* start after the chunk size */
>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
>> bytestream2_skip(&s->g, 2);
>> height = bytestream2_get_be16(&s->g);
>> bytestream2_skip(&s->g, 2);
>> -if (height > s->avctx->height - start_line)
>> +if (height > s->avctx->height - start_line) {
>> +avctx->internal->need_flush = 1;
>> return avpkt->size;
>> +}
>> } else {
>> start_line = 0;
>> height = s->avctx->height;
>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
>> .init = qtrle_decode_init,
>> .close = qtrle_decode_end,
>> .decode = qtrle_decode_frame,
>> -.capabilities = AV_CODEC_CAP_DR1,
>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
>> FF_CODEC_CAP_SETS_PKT_POS,
>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>> };
>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
>> index 27bb8aad71..39a03b7b6c 100644
>> --- a/tests/ref/fate/qtrle-8bit
>> +++ b/tests/ref/fate/qtrle-8bit
>> @@ -61,3 +61,4 @@
>> 0,160,160,1, 921600, 0xcfd6ad2b
>> 0,163,163,1, 921600, 0x3b372379
>> 0,165,165,1, 921600, 0x36f245f5
>> +0,166,166,1, 921600, 0x36f245f5
>
> Following what i said in the nuv patch, do you still experience timeouts
> with the current codebase, or even if you revert commit
> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> existing frame buffer shouldn't be expensive anymore for the fuzzer
> after my ref counting changes to target_dec_fuzzer.c
>
> This is a very ugly solution to a problem that was already solved when
> reference counting was introduced. Returning duplicate frames to achieve
> cfr in decoders where it's expected shouldn't affect performance.
Maybe i should ask this backward to make it clearer what this is trying
to fix.
Consider a patch that would return every frame twice with the same ref
of course.
Would you consider this to have 0 performance impact ?
if every filter following needs to process frames twice 2x CPU load
and after the filters references would also not b
[FFmpeg-devel] [PATCH 4/3][RFC] avcodec/qtrle: call ff_reget_buffer() only when the picture data is going to change
ff_reget_buffer() will attempt to create a writable copy of the frame,
which is not needed when the decoder intends to return a reference to
the same buffer as the previous frame.
Should reduce data copy, hopefully achieving a similar speed up as
a9dacdeea6168787a142209bd19fdd74aefc9dd6 without dropping frames.
Signed-off-by: James Almer
---
And this hopefully works around the other side of the issue.
libavcodec/qtrle.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index bf3daf26e1..3255c64063 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -456,8 +456,6 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
int drop = 0;
bytestream2_init(&s->g, avpkt->data, avpkt->size);
-if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
-return ret;
/* check if this frame is even supposed to change */
if (avpkt->size < 8) {
@@ -492,6 +490,9 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
start_line = 0;
height = s->avctx->height;
}
+if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
+return ret;
+
row_ptr = s->frame->linesize[0] * start_line;
switch (avctx->bits_per_coded_sample) {
@@ -553,6 +554,13 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
}
done:
+if (drop) {
+// ff_reget_buffer() isn't needed when frames don't change, so just
update
+// frame props.
+ret = ff_decode_frame_props(avctx, s->frame);
+if (ret < 0)
+return ret;
+}
if ((ret = av_frame_ref(data, s->frame)) < 0)
return ret;
if (drop)
--
2.22.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".
Re: [FFmpeg-devel] [PATCH 4/5] avcodec/alac: Check for bps of 0
Hi Paul,
> On Aug 25, 2019, at 12:37 AM, Paul B Mahol wrote:
>
> On Sun, Aug 25, 2019 at 9:33 AM Michael Niedermayer
> wrote:
>
>> On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote:
>>> On 8/8/2019 8:23 PM, Michael Niedermayer wrote:
Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
Fixes:
>> 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440
Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer
---
libavcodec/alac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/alac.c b/libavcodec/alac.c
index 6086e2caa8..1196925aa7 100644
--- a/libavcodec/alac.c
+++ b/libavcodec/alac.c
@@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx,
>> AVFrame *frame, int ch_index,
alac->extra_bits = get_bits(&alac->gb, 2) << 3;
bps = alac->sample_size - alac->extra_bits + channels - 1;
-if (bps > 32U) {
+if (bps > 32 || bps < 1) {
avpriv_report_missing_feature(avctx, "bps %d", bps);
return AVERROR_PATCHWELCOME;
>>>
>>> bps 0 (or negative) is obviously a broken file,
>>
>> id say very likely a broken file, yes
>>
>>
>>> so asking for a sample
>>> is pointless. Just return invalid data in those cases, and leave this
>>> check for > 32.
>>
>> thats a few lines more code, for an error code and different/no message
>> its a bit difficult to guess where people prefer the extra code to be
>> correct and where they prefer somewhat incorrect solutions to minimize
>> fuzzer found bugfixes.
>>
>
> If you dislike what people prefer when reviewing, perhaps you should stop
> sending patches :-)
This remark sounds rude and disrespectful to me.
Please refrain from making remarks like this one in the future.
—
Baptiste
___
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 4/5] avcodec/alac: Check for bps of 0
On Mon, Aug 26, 2019 at 7:45 PM Baptiste Coudurier <
baptiste.coudur...@gmail.com> wrote:
> Hi Paul,
>
>
> > On Aug 25, 2019, at 12:37 AM, Paul B Mahol wrote:
> >
> > On Sun, Aug 25, 2019 at 9:33 AM Michael Niedermayer
>
> > wrote:
> >
> >> On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote:
> >>> On 8/8/2019 8:23 PM, Michael Niedermayer wrote:
> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
> Fixes:
> >>
> 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440
>
> Found-by: continuous fuzzing process
> >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/alac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> index 6086e2caa8..1196925aa7 100644
> --- a/libavcodec/alac.c
> +++ b/libavcodec/alac.c
> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx,
> >> AVFrame *frame, int ch_index,
>
> alac->extra_bits = get_bits(&alac->gb, 2) << 3;
> bps = alac->sample_size - alac->extra_bits + channels - 1;
> -if (bps > 32U) {
> +if (bps > 32 || bps < 1) {
> avpriv_report_missing_feature(avctx, "bps %d", bps);
> return AVERROR_PATCHWELCOME;
> >>>
> >>> bps 0 (or negative) is obviously a broken file,
> >>
> >> id say very likely a broken file, yes
> >>
> >>
> >>> so asking for a sample
> >>> is pointless. Just return invalid data in those cases, and leave this
> >>> check for > 32.
> >>
> >> thats a few lines more code, for an error code and different/no message
> >> its a bit difficult to guess where people prefer the extra code to be
> >> correct and where they prefer somewhat incorrect solutions to minimize
> >> fuzzer found bugfixes.
> >>
> >
> > If you dislike what people prefer when reviewing, perhaps you should stop
> > sending patches :-)
>
> This remark sounds rude and disrespectful to me.
> Please refrain from making remarks like this one in the future.
>
I do not see how this can be rude and disrespectful to you or anyone else.
>
> —
> Baptiste
> ___
> 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".
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, &s->flush_pkt);
>>> +if (ret < 0)
>>> +return ret;
>>> +*got_frame = 1;
>>> +}
>>> +return 0;
>>> +}
>>> +s->flush_pkt = *avpkt;
>>> +s->frame->pkt_dts = s->flush_pkt.dts;
>>> +
>>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
>>>
>>> /* check if this frame is even supposed to change */
>>> -if (avpkt->size < 8)
>>> +if (avpkt->size < 8) {
>>> +avctx->internal->need_flush = 1;
>>> return avpkt->size;
>>> +}
>>> +avctx->internal->need_flush = 0;
>>>
>>> /* start after the chunk size */
>>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
>>> bytestream2_skip(&s->g, 2);
>>> height = bytestream2_get_be16(&s->g);
>>> bytestream2_skip(&s->g, 2);
>>> -if (height > s->avctx->height - start_line)
>>> +if (height > s->avctx->height - start_line) {
>>> +avctx->internal->need_flush = 1;
>>> return avpkt->size;
>>> +}
>>> } else {
>>> start_line = 0;
>>> height = s->avctx->height;
>>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
>>> .init = qtrle_decode_init,
>>> .close = qtrle_decode_end,
>>> .decode = qtrle_decode_frame,
>>> -.capabilities = AV_CODEC_CAP_DR1,
>>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
>>> FF_CODEC_CAP_SETS_PKT_POS,
>>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
>>> };
>>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
>>> index 27bb8aad71..39a03b7b6c 100644
>>> --- a/tests/ref/fate/qtrle-8bit
>>> +++ b/tests/ref/fate/qtrle-8bit
>>> @@ -61,3 +61,4 @@
>>> 0,160,160,1, 921600, 0xcfd6ad2b
>>> 0,163,163,1, 921600, 0x3b372379
>>> 0,165,165,1, 921600, 0x36f245f5
>>> +0,166,166,1, 921600, 0x36f245f5
>>
>> Following what i said in the nuv patch, do you still experience timeouts
>> with the current codebase, or even if you revert commit
>> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
>> existing frame buffer shouldn't be expensive anymore for the fuzzer
>> after my ref counting changes to target_dec_fuzzer.c
>>
>> This is a very ugly solution to a problem that was already solved when
>> reference counting was introduced. Returning duplicate frames to achieve
>> cfr in decoders where it's expected shouldn't affect performance.
>
> Maybe i should ask this backward to make it clearer what this is trying
> to fix.
>
> Consider a patch that would return every frame twice with the same ref
> of course.
> Would you consider
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, &s->flush_pkt);
> >>> +if (ret < 0)
> >>> +return ret;
> >>> +*got_frame = 1;
> >>> +}
> >>> +return 0;
> >>> +}
> >>> +s->flush_pkt = *avpkt;
> >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> >>> +
> >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >>>
> >>> /* check if this frame is even supposed to change */
> >>> -if (avpkt->size < 8)
> >>> +if (avpkt->size < 8) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> +avctx->internal->need_flush = 0;
> >>>
> >>> /* start after the chunk size */
> >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> height = bytestream2_get_be16(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> -if (height > s->avctx->height - start_line)
> >>> +if (height > s->avctx->height - start_line) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> } else {
> >>> start_line = 0;
> >>> height = s->avctx->height;
> >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> >>> .init = qtrle_decode_init,
> >>> .close = qtrle_decode_end,
> >>> .decode = qtrle_decode_frame,
> >>> -.capabilities = AV_CODEC_CAP_DR1,
> >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> >>> };
> >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> >>> index 27bb8aad71..39a03b7b6c 100644
> >>> --- a/tests/ref/fate/qtrle-8bit
> >>> +++ b/tests/ref/fate/qtrle-8bit
> >>> @@ -61,3 +61,4 @@
> >>> 0,160,160,1, 921600, 0xcfd6ad2b
> >>> 0,163,163,1, 921600, 0x3b372379
> >>> 0,165,165,1, 921600, 0x36f245f5
> >>> +0,166,166,1, 921600, 0x36f245f5
> >>
> >> Following what i said in the nuv patch, do you still experience
> timeouts
> >> with the current codebase, or even if you revert commit
> >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> >> after my ref counting changes to target_dec_fuzzer.c
> >>
> >> This is a very ugly solution to a problem that was already solved
> when
> >> reference counting was introduced. Returning duplicate frames to
> achieve
> >> cfr in decoders where it's expected shouldn't affect performa
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, &s->flush_pkt);
> >>> +if (ret < 0)
> >>> +return ret;
> >>> +*got_frame = 1;
> >>> +}
> >>> +return 0;
> >>> +}
> >>> +s->flush_pkt = *avpkt;
> >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> >>> +
> >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> >>>
> >>> /* check if this frame is even supposed to change */
> >>> -if (avpkt->size < 8)
> >>> +if (avpkt->size < 8) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> +avctx->internal->need_flush = 0;
> >>>
> >>> /* start after the chunk size */
> >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> height = bytestream2_get_be16(&s->g);
> >>> bytestream2_skip(&s->g, 2);
> >>> -if (height > s->avctx->height - start_line)
> >>> +if (height > s->avctx->height - start_line) {
> >>> +avctx->internal->need_flush = 1;
> >>> return avpkt->size;
> >>> +}
> >>> } else {
> >>> start_line = 0;
> >>> height = s->avctx->height;
> >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> >>> .init = qtrle_decode_init,
> >>> .close = qtrle_decode_end,
> >>> .decode = qtrle_decode_frame,
> >>> -.capabilities = AV_CODEC_CAP_DR1,
> >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> >>> FF_CODEC_CAP_SETS_PKT_POS,
> >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> >>> };
> >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> >>> index 27bb8aad71..39a03b7b6c 100644
> >>> --- a/tests/ref/fate/qtrle-8bit
> >>> +++ b/tests/ref/fate/qtrle-8bit
> >>> @@ -61,3 +61,4 @@
> >>> 0,160,160,1, 921600, 0xcfd6ad2b
> >>> 0,163,163,1, 921600, 0x3b372379
> >>> 0,165,165,1, 921600, 0x36f245f5
> >>> +0,166,166,1, 921600, 0x36f245f5
> >>
> >> Following what i said in the nuv patch, do you still experience
> >> timeouts
> >> with the current codebase, or even if you revert commit
> >> a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
> >> existing frame buffer shouldn't be expensive anymore for the fuzzer
> >> after my ref counting changes to target_dec_fuzzer.c
> >>
> >> This is a very ugly solution to a problem that was already solved when
> >> reference counting was introduced. Returning duplicate frames to
> >> achieve
> >>
[FFmpeg-devel] [PATCH] avcodec/qtrle: add a flush() callback
The reference frame isn't valid after seeking
Signed-off-by: James Almer
---
libavcodec/qtrle.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index 3255c64063..1021986f01 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -571,6 +571,14 @@ done:
return avpkt->size;
}
+static void qtrle_decode_flush(AVCodecContext *avctx)
+{
+QtrleContext *s = avctx->priv_data;
+
+memset(s->pal, 0, sizeof(s->pal));
+av_frame_unref(s->frame);
+}
+
static av_cold int qtrle_decode_end(AVCodecContext *avctx)
{
QtrleContext *s = avctx->priv_data;
@@ -589,5 +597,6 @@ AVCodec ff_qtrle_decoder = {
.init = qtrle_decode_init,
.close = qtrle_decode_end,
.decode = qtrle_decode_frame,
+.flush = qtrle_decode_flush,
.capabilities = AV_CODEC_CAP_DR1,
};
--
2.22.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".
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, &s->flush_pkt);
> +if (ret < 0)
> +return ret;
> +*got_frame = 1;
> +}
> +return 0;
> +}
> +s->flush_pkt = *avpkt;
> +s->frame->pkt_dts = s->flush_pkt.dts;
> +
>bytestream2_init(&s->g, avpkt->data, avpkt->size);
>
>/* check if this frame is even supposed to change */
> -if (avpkt->size < 8)
> +if (avpkt->size < 8) {
> +avctx->internal->need_flush = 1;
>return avpkt->size;
> +}
> +avctx->internal->need_flush = 0;
>
>/* start after the chunk size */
>size = bytestream2_get_be32(&s->g) & 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(&s->g);
>bytestream2_skip(&s->g, 2);
>height = bytestream2_get_be16(&s->g);
>bytestream2_skip(&s->g, 2);
> -if (height > s->avctx->height - start_line)
> +if (height > s->avctx->height - start_line) {
> +avctx->internal->need_flush = 1;
>return avpkt->size;
> +}
>} else {
>start_line = 0;
>height = s->avctx->height;
> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
>.init = qtrle_decode_init,
>.close = qtrle_decode_end,
>.decode = qtrle_decode_frame,
> -.capabilities = AV_CODEC_CAP_DR1,
> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_SETS_PKT_POS,
> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> };
> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> index 27bb8aad71..39a03b7b6c 100644
> --- a/tests/ref/fate/qtrle-8bit
> +++ b/tests/ref/fate/qtrle-8bit
> @@ -61,3 +61,4 @@
> 0,160,160,1, 921600, 0xcfd6ad2b
> 0,163,163,1, 921600, 0x3b372379
> 0,165,165,1, 921600, 0x36f245f5
> +0,166,166,1, 921600, 0x36f245f5
Following what i said in the nuv patch, do you still experience
timeouts
with the current codebase, or even if you revert commit
a9dacdeea6168787a142209bd19fdd74aefc9dd6? Creating a reference to an
existing frame buffer shouldn't be expensive anymore for the fuzzer
after my ref counting changes to target_dec_fuzzer.c
This is a very ugly solution to a problem that was already solved when
[FFmpeg-devel] [PATCH 4/3 v2][RFC] avcodec/qtrle: call ff_reget_buffer() only when the picture data is going to change
ff_reget_buffer() will attempt to create a writable copy of the frame,
which is not needed when the decoder intends to return a reference to
the same buffer as the previous frame.
Should reduce data copy, hopefully achieving a similar speed up as
a9dacdeea6168787a142209bd19fdd74aefc9dd6 without dropping frames.
Signed-off-by: James Almer
---
Now ensuring there's a previous frame available to reuse.
libavcodec/qtrle.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
index bf3daf26e1..edfbef7f28 100644
--- a/libavcodec/qtrle.c
+++ b/libavcodec/qtrle.c
@@ -456,8 +456,6 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
int drop = 0;
bytestream2_init(&s->g, avpkt->data, avpkt->size);
-if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
-return ret;
/* check if this frame is even supposed to change */
if (avpkt->size < 8) {
@@ -492,6 +490,9 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
start_line = 0;
height = s->avctx->height;
}
+if ((ret = ff_reget_buffer(avctx, s->frame)) < 0)
+return ret;
+
row_ptr = s->frame->linesize[0] * start_line;
switch (avctx->bits_per_coded_sample) {
@@ -553,6 +554,15 @@ static int qtrle_decode_frame(AVCodecContext *avctx,
}
done:
+if (!s->frame->data[0])
+return AVERROR_INVALIDDATA;
+if (drop) {
+// ff_reget_buffer() isn't needed when frames don't change, so just
update
+// frame props.
+ret = ff_decode_frame_props(avctx, s->frame);
+if (ret < 0)
+return ret;
+}
if ((ret = av_frame_ref(data, s->frame)) < 0)
return ret;
if (drop)
--
2.22.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".
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, &s->flush_pkt);
> > > > > >>> +if (ret < 0)
> > > > > >>> +return ret;
> > > > > >>> +*got_frame = 1;
> > > > > >>> +}
> > > > > >>> +return 0;
> > > > > >>> +}
> > > > > >>> +s->flush_pkt = *avpkt;
> > > > > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > > > > >>> +
> > > > > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > > > > >>>
> > > > > >>> /* check if this frame is even supposed to change */
> > > > > >>> -if (avpkt->size < 8)
> > > > > >>> +if (avpkt->size < 8) {
> > > > > >>> +avctx->internal->need_flush = 1;
> > > > > >>> return avpkt->size;
> > > > > >>> +}
> > > > > >>> +avctx->internal->need_flush = 0;
> > > > > >>>
> > > > > >>> /* start after the chunk size */
> > > > > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > > > >>> bytestream2_skip(&s->g, 2);
> > > > > >>> height = bytestream2_get_be16(&s->g);
> > > > > >>> bytestream2_skip(&s->g, 2);
> > > > > >>> -if (height > s->avctx->height - start_line)
> > > > > >>> +if (height > s->avctx->height - start_line) {
> > > > > >>> +avctx->internal->need_flush = 1;
> > > > > >>> return avpkt->size;
> > > > > >>> +}
> > > > > >>> } else {
> > > > > >>> start_line = 0;
> > > > > >>> height = s->avctx->height;
> > > > > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > > > >>> .init = qtrle_decode_init,
> > > > > >>> .close = qtrle_decode_end,
> > > > > >>> .decode = qtrle_decode_frame,
> > > > > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > > > > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > > > > >>> FF_CODEC_CAP_SETS_PKT_POS,
> > > > > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > > > >>> };
> > > > > >>> diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit
> > > > > >>> index 27bb8aad71..39a03b7b6c 100644
> > > > > >>> --- a/tests/ref/fate/qtrle-8bit
> > > > > >>> +++ b/tests/ref/fate/qtrle-8bit
> > > > > >>> @@ -61,3 +61,4 @@
> > > > > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > > > > >>> 0,163,163,1, 921600, 0x3b372379
> > > > > >>> 0,165,165,1, 921600, 0x36f245f5
> > > > > >>> +0,166,166,1, 921600, 0x36f245f5
> > > > > >>
> > > > > >> Following what i said in the nuv patch, do you still experience
> > > > > >> timeouts
Re: [FFmpeg-devel] [PATCH 1/3][RFC] avutil/frame: add AV_FRAME_FLAG_DISPOSABLE
On Mon, Aug 26, 2019 at 01:17:25PM -0300, James Almer wrote:
> Used to signal frames that can be safely discarded without losing
> any picture data, side data, or metadata other than timing info.
>
> Signed-off-by: James Almer
> ---
> This implements the "disposable frame" solution to allow library
> users to drop duplicate frames before further processing if desired,
> instead of forcing decoders to output vfr content when cfr is coded
> in the bitstream.
>
> doc/APIchanges | 3 +++
> libavutil/frame.h | 5 +
> libavutil/version.h | 2 +-
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 682b67aa25..b28d702bae 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
>
> API changes, most recent first:
>
> +2019-08-xx - xx - lavu 58.34.100 - avframe.h
> + Add AV_FRAME_FLAG_DISPOSABLE
> +
> 2019-08-xx - xx - lavf 58.31.101 - avio.h
>4K limit removed from avio_printf.
>
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 5d3231e7bb..e1bf8795d2 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -522,6 +522,11 @@ typedef struct AVFrame {
> * A flag to mark the frames which need to be decoded, but shouldn't be
> output.
> */
> #define AV_FRAME_FLAG_DISCARD (1 << 2)
> +/**
> + * A flag to indicate frames that can be discarded by the encoder. I.e.
> frames
> + * that are an exact duplicate of the previous one.
> + */
... exact duplicate of the previous one, except its timestamp and duration.
maybe AV_FRAME_FLAG_DUPLICATE or AV_FRAME_FLAG_REPEATED would be clearer ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri
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 1/3][RFC] avutil/frame: add AV_FRAME_FLAG_DISPOSABLE
On 8/26/2019 5:20 PM, Michael Niedermayer wrote:
> On Mon, Aug 26, 2019 at 01:17:25PM -0300, James Almer wrote:
>> Used to signal frames that can be safely discarded without losing
>> any picture data, side data, or metadata other than timing info.
>>
>> Signed-off-by: James Almer
>> ---
>> This implements the "disposable frame" solution to allow library
>> users to drop duplicate frames before further processing if desired,
>> instead of forcing decoders to output vfr content when cfr is coded
>> in the bitstream.
>>
>> doc/APIchanges | 3 +++
>> libavutil/frame.h | 5 +
>> libavutil/version.h | 2 +-
>> 3 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 682b67aa25..b28d702bae 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
>>
>> API changes, most recent first:
>>
>> +2019-08-xx - xx - lavu 58.34.100 - avframe.h
>> + Add AV_FRAME_FLAG_DISPOSABLE
>> +
>> 2019-08-xx - xx - lavf 58.31.101 - avio.h
>>4K limit removed from avio_printf.
>>
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 5d3231e7bb..e1bf8795d2 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -522,6 +522,11 @@ typedef struct AVFrame {
>> * A flag to mark the frames which need to be decoded, but shouldn't be
>> output.
>> */
>> #define AV_FRAME_FLAG_DISCARD (1 << 2)
>> +/**
>> + * A flag to indicate frames that can be discarded by the encoder. I.e.
>> frames
>> + * that are an exact duplicate of the previous one.
>> + */
>
> ... exact duplicate of the previous one, except its timestamp and duration.
>
> maybe AV_FRAME_FLAG_DUPLICATE or AV_FRAME_FLAG_REPEATED would be clearer ?
It might, but i wanted to use the same name as the AVPacket flag defined
in avcodec.h. If duplicate or repeated is preferred then i'll change it.
I could also instead change the doxy to state it signals that the frame
is disposable in general and not just because it's a duplicate of a
previous one, even if for now that's the only usecase for it.
>
> thx
>
> [...]
>
>
> ___
> 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".
Re: [FFmpeg-devel] [PATCH 1/3][RFC] avutil/frame: add AV_FRAME_FLAG_DISPOSABLE
On Mon, Aug 26, 2019 at 05:23:22PM -0300, James Almer wrote:
> On 8/26/2019 5:20 PM, Michael Niedermayer wrote:
> > On Mon, Aug 26, 2019 at 01:17:25PM -0300, James Almer wrote:
> >> Used to signal frames that can be safely discarded without losing
> >> any picture data, side data, or metadata other than timing info.
> >>
> >> Signed-off-by: James Almer
> >> ---
> >> This implements the "disposable frame" solution to allow library
> >> users to drop duplicate frames before further processing if desired,
> >> instead of forcing decoders to output vfr content when cfr is coded
> >> in the bitstream.
> >>
> >> doc/APIchanges | 3 +++
> >> libavutil/frame.h | 5 +
> >> libavutil/version.h | 2 +-
> >> 3 files changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index 682b67aa25..b28d702bae 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> >>
> >> API changes, most recent first:
> >>
> >> +2019-08-xx - xx - lavu 58.34.100 - avframe.h
> >> + Add AV_FRAME_FLAG_DISPOSABLE
> >> +
> >> 2019-08-xx - xx - lavf 58.31.101 - avio.h
> >>4K limit removed from avio_printf.
> >>
> >> diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> index 5d3231e7bb..e1bf8795d2 100644
> >> --- a/libavutil/frame.h
> >> +++ b/libavutil/frame.h
> >> @@ -522,6 +522,11 @@ typedef struct AVFrame {
> >> * A flag to mark the frames which need to be decoded, but shouldn't be
> >> output.
> >> */
> >> #define AV_FRAME_FLAG_DISCARD (1 << 2)
> >> +/**
> >> + * A flag to indicate frames that can be discarded by the encoder. I.e.
> >> frames
> >> + * that are an exact duplicate of the previous one.
> >> + */
> >
> > ... exact duplicate of the previous one, except its timestamp and duration.
> >
> > maybe AV_FRAME_FLAG_DUPLICATE or AV_FRAME_FLAG_REPEATED would be clearer ?
>
> It might, but i wanted to use the same name as the AVPacket flag defined
> in avcodec.h. If duplicate or repeated is preferred then i'll change it.
wouldnt this overload the meaning of "discard" ?
where the AVPacket flag does not neccesarily indicate identical data
but iam fine with any name really, the other names was just a thought that
came to my mind when reading the patch ...
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
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] lavc: Add DICOM decoder
On Sun, Aug 25, 2019 at 03:20:13 +0530, Shivam wrote:
> The patch contains DICOM decoder. I have improved the code as suggested.
Hi Shivan,
nice job.
First of all, all your samples now appear to demux and decode properly.
I didn't get a chance to look at their output, but I guess you have
that covered.
Most of my review remarks have been integrated. You did miss
some which I believe are essential:
> +static int extract_ed(AVCodecContext *avctx)
> +{
> +DICOMopts *dcmopts = avctx->priv_data;
> +uint8_t *ed = avctx->extradata;
> +int ed_size = avctx->extradata_size;
> +const uint8_t **b = &ed;
> +
> +dcmopts->interpret = 0x02; // Set defaults
> +dcmopts->slope = 1;
> +dcmopts->intcpt = 0;
> +dcmopts->pixpad = 1 << 31;
> +dcmopts->pixrep = 0;
> +
> +if (ed_size < DECODER_ED_SIZE + AV_INPUT_BUFFER_PADDING_SIZE)
> +return -1;
This return value isn't used anywhere (or ignored). That's fine if
that's intentional, but a bit confusing for review.
> +static uint8_t apply_transform(int64_t val, int64_t bitmask, int pix_pad,
> + int slope, int intercept, int w, int l, int
> interpret)
Indentation is now off in the second quoted line.
[...]
> +// DICOM MONOCHROME1 and MONOCHROME2 decoder
> +static int decode_mono( AVCodecContext *avctx,
> +const uint8_t *buf,
> +AVFrame *p)
There's still a space too much here, after "decode_mono(", and the
subsequent two lines need to adjust as well.
[...]
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index e70ebc0c70..b4b79ef63a 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
> #include "libavutil/version.h"
>
> #define LIBAVCODEC_VERSION_MAJOR 58
> -#define LIBAVCODEC_VERSION_MINOR 55
> +#define LIBAVCODEC_VERSION_MINOR 56
> #define LIBAVCODEC_VERSION_MICRO 101
When bumping MINOR, you need to reset MICRO to 100. But this part
doesn't apply anymore anyway, since MICRO has changed on master since.
You may need to rebase, but this will likely also be done by whoever
pushes to master once your patch is acknowledged.
Apart from that, I still get a memory leak when decoding 17.dcm
(with just "valgrind ./ffmpeg_g -i DICOM-samples/17.dcm").
Cheers,
Moritz
___
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] lavf: Add DICOM demuxer
On Sun, Aug 25, 2019 at 03:22:02 +0530, Shivam wrote:
> The patch contains DICOM demuxer. I have improved the code as suggested.
Second part of my review:
> From: Shivam Goyal <1998.goyal.shi...@gmail.com>
> Date: Sun, 25 Aug 2019 02:57:35 +0530
> Subject: [PATCH] lavf: Add DICOM demuxer
>
> ---
> Changelog| 1 +
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 1 +
> libavformat/dicom.h | 109 ++
> libavformat/dicomdec.c | 617 +
> libavformat/dicomdict.c | 711 +++
> libavformat/version.h| 2 +-
> 7 files changed, 1441 insertions(+), 1 deletion(-)
> create mode 100644 libavformat/dicom.h
> create mode 100644 libavformat/dicomdec.c
> create mode 100644 libavformat/dicomdict.c
You still need to document the options in doc/*.texi.
> diff --git a/Changelog b/Changelog
> index 52096eed0e..5e5a8c5c6c 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -5,6 +5,7 @@ version :
> - v360 filter
> - Intel QSV-accelerated MJPEG decoding
> - Intel QSV-accelerated VP9 decoding
> +- DICOM demuxer
Here, this patch doesn't apply in top of the DICOM decoder, even though
it requires the decoder, because the decoder patch already adds another
line to the Changelog.
> --- /dev/null
> +++ b/libavformat/dicomdec.c
[...]
> +static void free_seq(DataElement *de) {
> +int i = 0;
> +DataElement *seq_data = de->bytes;
> +for(; i < MAX_UNDEF_LENGTH; ++i) {
BTW, ffmpeg prefers the "i++" style.
[...]
> +// detects transfer syntax
> +static TransferSyntax get_transfer_sytax (const char *ts) {
There's still a typo in the name of the function, sytax -> syntax.
Please fix.
[...]
> +static int set_multiframe_data(AVFormatContext *s, AVStream* st, DataElement
> *de)
> +{
The return value still is always 0 and isn't being used for anything.
(I'm saying, this function could return void, unless it can be expanded
to something more later.)
> +DICOMContext *dicom = s->priv_data;
> +
> +if (de->GroupNumber != MF_GR_NB)
> +return 0;
> +
> +switch (de->ElementNumber) {
> +case 0x1063: // frame time
> +dicom->delay = conv_DS(de);
> +dicom->duration = dicom->delay * dicom->nb_frames;
> +break;
> +}
Again, here, I expect this to be a switch/case with one case only if it
can be expanded later, i.e. de->ElementNumber has multiple meanings
which aren't covered here.
> +return 0;
> +}
> +
> +
> +static int read_de_metainfo(AVFormatContext *s, DataElement *de)
> +{
[...]
> +if (de->VL < 0)
> +return AVERROR_INVALIDDATA;
> +if (de->VL != UNDEFINED_VL && de->VL % 2)
> +av_log(s, AV_LOG_WARNING,"Data Element Value length: %"PRIi64" can't
> be odd\n", de->VL);
^
Still missing a space here.
> +return bytes_read;
> +}
> +
> +static int read_de(AVFormatContext *s, DataElement *de)
> +{
> +int ret;
> +uint32_t len = de->VL;
> +de->bytes = av_malloc(len);
> +ret = avio_read(s->pb, de->bytes, len);
> +return ret;
> +}
> +
> +static int read_implicit_seq_item(AVFormatContext *s, DataElement *de)
> +{
> +int ret, f = -2, i = 0;
> +uint8_t *bytes = de->bytes;
> +
> +bytes = av_malloc(MAX_UNDEF_LENGTH);
You're still not checking the return value and returning an error on
failure.
> +for(; i < MAX_UNDEF_LENGTH; ++i) {
ffmpeg prefers the "i++" style.
[...]
> +static int read_seq(AVFormatContext *s, DataElement *de) {
> +int i = 0, ret;
> +DICOMContext *dicom = s->priv_data;
> +DataElement *seq_data = av_malloc_array(MAX_SEQ_LENGTH,
> sizeof(DataElement));
> +
> +de->bytes = seq_data;
> +dicom->inseq = 1;
> +for (;i < MAX_SEQ_LENGTH; ++i) {
ffmpeg prefers the "i++" style. (And missing a space after the first
semicolon.)
> +seq_data[i].bytes = NULL;
> +seq_data[i].desc = NULL;
> +seq_data[i].is_found = 0;
> +read_de_metainfo(s, seq_data + i);
> +if (seq_data[i].GroupNumber == SEQ_GR_NB
> +&& seq_data[i].ElementNumber == SEQ_DEL_EL_NB) {
> +ret = i;
> +break;
> +}
> +if (seq_data[i].VL == UNDEFINED_VL)
> +ret = read_implicit_seq_item(s, seq_data + i);
I believe these array elements are still not freed.
> +else
> +ret = read_de(s, seq_data + i);
> +if (ret < 0)
> +break;
> +}
> +
> +dicom->inseq = 0;
> +return ret;
> +}
[...]
> +static int dicom_read_header(AVFormatContext *s)
> +{
> +AVIOContext *pb = s->pb;
> +AVDictionary **m = &s->metadata;
> +DICOMContext *dicom = s->priv_data;
> +DataElement *de;
> +char *key, *value;
> +uint32_t header_size, bytes_read = 0;
> +int ret;
> +
> +ret = avio_skip(pb, DICOM_PREAMBLE_SIZE + DICOM_PREFIX_SIZE);
> +if (ret < 0)
> +return ret;
> +dicom->inheader = 1;
> +de = alloc_de();
> +if (!de)
>
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, &s->flush_pkt);
> > > > > > >>> +if (ret < 0)
> > > > > > >>> +return ret;
> > > > > > >>> +*got_frame = 1;
> > > > > > >>> +}
> > > > > > >>> +return 0;
> > > > > > >>> +}
> > > > > > >>> +s->flush_pkt = *avpkt;
> > > > > > >>> +s->frame->pkt_dts = s->flush_pkt.dts;
> > > > > > >>> +
> > > > > > >>> bytestream2_init(&s->g, avpkt->data, avpkt->size);
> > > > > > >>>
> > > > > > >>> /* check if this frame is even supposed to change */
> > > > > > >>> -if (avpkt->size < 8)
> > > > > > >>> +if (avpkt->size < 8) {
> > > > > > >>> +avctx->internal->need_flush = 1;
> > > > > > >>> return avpkt->size;
> > > > > > >>> +}
> > > > > > >>> +avctx->internal->need_flush = 0;
> > > > > > >>>
> > > > > > >>> /* start after the chunk size */
> > > > > > >>> size = bytestream2_get_be32(&s->g) & 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(&s->g);
> > > > > > >>> bytestream2_skip(&s->g, 2);
> > > > > > >>> height = bytestream2_get_be16(&s->g);
> > > > > > >>> bytestream2_skip(&s->g, 2);
> > > > > > >>> -if (height > s->avctx->height - start_line)
> > > > > > >>> +if (height > s->avctx->height - start_line) {
> > > > > > >>> +avctx->internal->need_flush = 1;
> > > > > > >>> return avpkt->size;
> > > > > > >>> +}
> > > > > > >>> } else {
> > > > > > >>> start_line = 0;
> > > > > > >>> height = s->avctx->height;
> > > > > > >>> @@ -572,5 +593,6 @@ AVCodec ff_qtrle_decoder = {
> > > > > > >>> .init = qtrle_decode_init,
> > > > > > >>> .close = qtrle_decode_end,
> > > > > > >>> .decode = qtrle_decode_frame,
> > > > > > >>> -.capabilities = AV_CODEC_CAP_DR1,
> > > > > > >>> +.caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> > > > > > >>> FF_CODEC_CAP_SETS_PKT_POS,
> > > > > > >>> +.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
> > > > > > >>> };
> > > > > > >>> diff --git a/tests/ref/fate/qtrle-8bit
> > > > > > >>> b/tests/ref/fate/qtrle-8bit
> > > > > > >>> index 27bb8aad71..39a03b7b6c 100644
> > > > > > >>> --- a/tests/ref/fate/qtrle-8bit
> > > > > > >>> +++ b/tests/ref/fate/qtrle-8bit
> > > > > > >>> @@ -61,3 +61,4 @@
> > > > > > >>> 0,160,160,1, 921600, 0xcfd6ad2b
> > > > > > >>> 0,163,163,
[FFmpeg-devel] [PATCH] [cbs_h2645]: Used av_realloc instead of av_malloc
Follow the description of av_realloc, the memory needs to be allocated by av_realloc. --- libavcodec/cbs_h2645.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 69ea6dc6bb..8da8421e47 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -1457,7 +1457,7 @@ static int cbs_h2645_assemble_fragment(CodedBitstreamContext *ctx, max_size += 3 + frag->units[i].data_size * 3 / 2; } -data = av_malloc(max_size + AV_INPUT_BUFFER_PADDING_SIZE); +data = av_realloc(NULL, max_size + AV_INPUT_BUFFER_PADDING_SIZE); if (!data) return AVERROR(ENOMEM); -- 2.23.0.187.g17f5b7556c-goog ___ 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
> > > 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 v2 0/2] AltiVec/VSX fixes in swscale
On Sat, Aug 24, 2019, at 19:23, Lauri Kasanen wrote: > Hi, > > I approve of this series, but being in the middle of a move, I can't > test it. Alright, thanks. It's not urgent (distros that have this broken can patch locally for the time being), but would be nice to go through it once you have time. Thanks > > - Lauri > ___ > 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".
Re: [FFmpeg-devel] [PATCH 1/3][RFC] avutil/frame: add AV_FRAME_FLAG_DISPOSABLE
On 8/26/2019 5:30 PM, Michael Niedermayer wrote:
> On Mon, Aug 26, 2019 at 05:23:22PM -0300, James Almer wrote:
>> On 8/26/2019 5:20 PM, Michael Niedermayer wrote:
>>> On Mon, Aug 26, 2019 at 01:17:25PM -0300, James Almer wrote:
Used to signal frames that can be safely discarded without losing
any picture data, side data, or metadata other than timing info.
Signed-off-by: James Almer
---
This implements the "disposable frame" solution to allow library
users to drop duplicate frames before further processing if desired,
instead of forcing decoders to output vfr content when cfr is coded
in the bitstream.
doc/APIchanges | 3 +++
libavutil/frame.h | 5 +
libavutil/version.h | 2 +-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/doc/APIchanges b/doc/APIchanges
index 682b67aa25..b28d702bae 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@ libavutil: 2017-10-21
API changes, most recent first:
+2019-08-xx - xx - lavu 58.34.100 - avframe.h
+ Add AV_FRAME_FLAG_DISPOSABLE
+
2019-08-xx - xx - lavf 58.31.101 - avio.h
4K limit removed from avio_printf.
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 5d3231e7bb..e1bf8795d2 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -522,6 +522,11 @@ typedef struct AVFrame {
* A flag to mark the frames which need to be decoded, but shouldn't be
output.
*/
#define AV_FRAME_FLAG_DISCARD (1 << 2)
+/**
+ * A flag to indicate frames that can be discarded by the encoder. I.e.
frames
+ * that are an exact duplicate of the previous one.
+ */
>>>
>>> ... exact duplicate of the previous one, except its timestamp and duration.
>>>
>>> maybe AV_FRAME_FLAG_DUPLICATE or AV_FRAME_FLAG_REPEATED would be clearer ?
>>
>> It might, but i wanted to use the same name as the AVPacket flag defined
>> in avcodec.h. If duplicate or repeated is preferred then i'll change it.
>
> wouldnt this overload the meaning of "discard" ?
> where the AVPacket flag does not neccesarily indicate identical data
> but iam fine with any name really, the other names was just a thought that
> came to my mind when reading the patch ...
AV_PKT_FLAG_DISPOSABLE is currently used to signal packets with encoded
frames a decoder can safely drop without processing. Right now it's
being used only for non-ref B frames, but the doxy allows it to be
implemented for other use cases as well.
With AV_FRAME_FLAG_DISPOSABLE, the idea would be signaling a frame that
can in theory be dropped without affecting whatever the encoder, filter
or player handling them will output.
So ideally, both "disposable" flags, frame and packet, would be defined
in a generic enough way to essentially mean "a frame/packet that can be
dropped without putting the ongoing process at risk".
If we instead use "duplicate" or "repeated" here, then we're defining a
much bigger constrain regarding what the flag can be used for.
And both "discard" flags are currently pretty much internal flags used
by lavc's generic code to drop a frame after being decoded, ensuring
it's never output.
>
> thx
>
> [...]
>
>
> ___
> 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".
[FFmpeg-devel] [PATCH] avfilter/vf_delogo: support expr in delogo filter
Signed-off-by: Steven Liu
---
libavfilter/vf_delogo.c | 72 ++---
1 file changed, 68 insertions(+), 4 deletions(-)
diff --git a/libavfilter/vf_delogo.c b/libavfilter/vf_delogo.c
index 065d093641..b50699fb4d 100644
--- a/libavfilter/vf_delogo.c
+++ b/libavfilter/vf_delogo.c
@@ -31,10 +31,52 @@
#include "libavutil/imgutils.h"
#include "libavutil/opt.h"
#include "libavutil/pixdesc.h"
+#include "libavutil/eval.h"
#include "avfilter.h"
#include "formats.h"
#include "internal.h"
#include "video.h"
+static const char * const var_names[] = {
+"x",
+"y",
+"w",
+"h",
+"n",///< number of frame
+"t",///< timestamp expressed in seconds
+NULL
+};
+
+enum var_name {
+VAR_X,
+VAR_Y,
+VAR_W,
+VAR_H,
+VAR_N,
+VAR_T,
+VAR_VARS_NB
+};
+#define TS2T(ts, tb) ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts) * av_q2d(tb))
+
+static int set_expr(AVExpr **pexpr, const char *expr, const char *option, void
*log_ctx)
+{
+int ret;
+AVExpr *old = NULL;
+
+if (*pexpr)
+old = *pexpr;
+ret = av_expr_parse(pexpr, expr, var_names, NULL, NULL, NULL, NULL, 0,
log_ctx);
+if (ret < 0) {
+av_log(log_ctx, AV_LOG_ERROR,
+ "Error when parsing the expression '%s' for %s\n",
+ expr, option);
+*pexpr = old;
+return ret;
+}
+
+av_expr_free(old);
+return 0;
+}
+
/**
* Apply a simple delogo algorithm to the image in src and put the
@@ -156,16 +198,19 @@ static void apply_delogo(uint8_t *dst, int dst_linesize,
typedef struct DelogoContext {
const AVClass *class;
int x, y, w, h, band, show;
+char *x_expr, *y_expr, *w_expr, *h_expr;
+AVExpr *x_pexpr, *y_pexpr, *w_pexpr, *h_pexpr;
+double var_values[VAR_VARS_NB];
} DelogoContext;
#define OFFSET(x) offsetof(DelogoContext, x)
#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
static const AVOption delogo_options[]= {
-{ "x","set logo x position", OFFSET(x),AV_OPT_TYPE_INT, {
.i64 = -1 }, -1, INT_MAX, FLAGS },
-{ "y","set logo y position", OFFSET(y),AV_OPT_TYPE_INT, {
.i64 = -1 }, -1, INT_MAX, FLAGS },
-{ "w","set logo width",OFFSET(w),AV_OPT_TYPE_INT, {
.i64 = -1 }, -1, INT_MAX, FLAGS },
-{ "h","set logo height", OFFSET(h),AV_OPT_TYPE_INT, {
.i64 = -1 }, -1, INT_MAX, FLAGS },
+{ "x","set logo x position", OFFSET(x_expr),
AV_OPT_TYPE_STRING, { .str = "-1" }, CHAR_MIN, CHAR_MAX, FLAGS },
+{ "y","set logo y position", OFFSET(y_expr),
AV_OPT_TYPE_STRING, { .str = "-1" }, CHAR_MIN, CHAR_MAX, FLAGS },
+{ "w","set logo width",OFFSET(w_expr),
AV_OPT_TYPE_STRING, { .i64 = "-1" }, CHAR_MIN, CHAR_MAX, FLAGS },
+{ "h","set logo height", OFFSET(h_expr),
AV_OPT_TYPE_STRING, { .i64 = "-1" }, CHAR_MIN, CHAR_MAX, FLAGS },
#if LIBAVFILTER_VERSION_MAJOR < 7
/* Actual default value for band/t is 1, set in init */
{ "band", "set delogo area band size", OFFSET(band), AV_OPT_TYPE_INT, {
.i64 = 0 }, 0, INT_MAX, FLAGS },
@@ -194,6 +239,18 @@ static int query_formats(AVFilterContext *ctx)
static av_cold int init(AVFilterContext *ctx)
{
DelogoContext *s = ctx->priv;
+int ret = 0;
+
+if ((ret = set_expr(&s->x_pexpr, s->x_expr, "x", ctx)) < 0 ||
+(ret = set_expr(&s->y_pexpr, s->y_expr, "y", ctx)) < 0 ||
+(ret = set_expr(&s->w_pexpr, s->w_expr, "w", ctx)) < 0 ||
+(ret = set_expr(&s->h_pexpr, s->h_expr, "h", ctx)) < 0 )
+return ret;
+
+s->x = av_expr_eval(s->x_pexpr, s->var_values, s);
+s->y = av_expr_eval(s->y_pexpr, s->var_values, s);
+s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
+s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
#define CHECK_UNSET_OPT(opt)\
if (s->opt == -1) {\
@@ -252,6 +309,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
int plane;
AVRational sar;
+s->var_values[VAR_N] = inlink->frame_count_out;
+s->var_values[VAR_T] = TS2T(in->pts, inlink->time_base);
+s->x = av_expr_eval(s->x_pexpr, s->var_values, s);
+s->y = av_expr_eval(s->y_pexpr, s->var_values, s);
+s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
+s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
+
if (av_frame_is_writable(in)) {
direct = 1;
out = in;
--
2.17.2 (Apple Git-113)
___
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 V5] avcodec/libvpxenc: add ROI-based encoding support for VP8/VP9 support
Hi,
On Thu, Aug 22, 2019 at 5:56 PM Guo, Yejun wrote:
>
> example command line to verify it:
> ./ffmpeg -i input.stream -vf addroi=0:0:iw/3:ih/3:-0.8 -c:v libvpx -b:v 2M
> tmp.webm
>
> Signed-off-by: Guo, Yejun
> ---
> libavcodec/libvpxenc.c | 194
> +
> 1 file changed, 194 insertions(+)
>
Looks OK overall, just a few minor comments and you should bump the
micro version number for this change.
> [...]
> +
> +segment_mapping[mapping_index] = segment_id + 1;
> +roi_map->delta_q[segment_id] = delta_q;
> +segment_id++;
> +}
> +}
> +
> +
This line can go.
> [...]
> +
> +av_assert0(!roi_supported);
> +if (!ctx->roi_warned) {
> +ctx->roi_warned = 1;
> +av_log(avctx, AV_LOG_WARNING, "ROI is not supported, please upgrade
> libvpx to version >= 1.8.1, "
> + "and you might need to build ffmpeg
> again.\n");
This could be "...to version >= 1.8.1. You may need to rebuild ffmpeg.\n"
> [...]
> +
> +if (sd) {
> +if (avctx->codec_id == AV_CODEC_ID_VP8)
> +vp8_encode_set_roi(avctx, frame->width, frame->height, sd);
> +else if (avctx->codec_id == AV_CODEC_ID_VP9)
This is the only other option, it can just be 'else'. Other checks in
the code assume this already.
> +vp9_encode_set_roi(avctx, frame->width, frame->height, sd);
> +}
> }
___
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] avcodec/qtrle: add a flush() callback
LGTM
On Mon, Aug 26, 2019 at 8:28 PM James Almer wrote:
> The reference frame isn't valid after seeking
>
> Signed-off-by: James Almer
> ---
> libavcodec/qtrle.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c
> index 3255c64063..1021986f01 100644
> --- a/libavcodec/qtrle.c
> +++ b/libavcodec/qtrle.c
> @@ -571,6 +571,14 @@ done:
> return avpkt->size;
> }
>
> +static void qtrle_decode_flush(AVCodecContext *avctx)
> +{
> +QtrleContext *s = avctx->priv_data;
> +
> +memset(s->pal, 0, sizeof(s->pal));
> +av_frame_unref(s->frame);
> +}
> +
> static av_cold int qtrle_decode_end(AVCodecContext *avctx)
> {
> QtrleContext *s = avctx->priv_data;
> @@ -589,5 +597,6 @@ AVCodec ff_qtrle_decoder = {
> .init = qtrle_decode_init,
> .close = qtrle_decode_end,
> .decode = qtrle_decode_frame,
> +.flush = qtrle_decode_flush,
> .capabilities = AV_CODEC_CAP_DR1,
> };
> --
> 2.22.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".
Re: [FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based encoding support for VP8/VP9 support
> -Original Message-
> From: James Zern [mailto:jz...@google.com]
> Sent: Tuesday, August 27, 2019 12:03 PM
> To: FFmpeg development discussions and patches
> Cc: Guo, Yejun
> Subject: Re: [FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based
> encoding support for VP8/VP9 support
>
> Hi,
>
> On Thu, Aug 22, 2019 at 5:56 PM Guo, Yejun wrote:
> >
> > example command line to verify it:
> > ./ffmpeg -i input.stream -vf addroi=0:0:iw/3:ih/3:-0.8 -c:v libvpx -b:v 2M
> tmp.webm
> >
> > Signed-off-by: Guo, Yejun
> > ---
> > libavcodec/libvpxenc.c | 194
> +
> > 1 file changed, 194 insertions(+)
> >
>
> Looks OK overall, just a few minor comments and you should bump the
> micro version number for this change.
thanks,
do you mean bump LIBAVCODEC_VERSION_MICRO in file libavcodec/version.h? thanks.
>
> > [...]
> > +
> > +segment_mapping[mapping_index] = segment_id + 1;
> > +roi_map->delta_q[segment_id] = delta_q;
> > +segment_id++;
> > +}
> > +}
> > +
> > +
>
> This line can go.
do you mean the next line av_assert0 can be removed? thanks.
>
> > [...]
> > +
> > +av_assert0(!roi_supported);
> > +if (!ctx->roi_warned) {
> > +ctx->roi_warned = 1;
> > +av_log(avctx, AV_LOG_WARNING, "ROI is not supported, please
> upgrade libvpx to version >= 1.8.1, "
> > + "and you might need to build
> ffmpeg again.\n");
>
> This could be "...to version >= 1.8.1. You may need to rebuild ffmpeg.\n"
thanks, will change.
>
> > [...]
> > +
> > +if (sd) {
> > +if (avctx->codec_id == AV_CODEC_ID_VP8)
> > +vp8_encode_set_roi(avctx, frame->width,
> frame->height, sd);
> > +else if (avctx->codec_id == AV_CODEC_ID_VP9)
>
> This is the only other option, it can just be 'else'. Other checks in
> the code assume this already.
ok, will only keep 'else'
>
> > +vp9_encode_set_roi(avctx, frame->width,
> frame->height, sd);
> > +}
> > }
___
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".
