| Message ID | 1683237740-32743-3-git-send-email-dheitmueller@ltnglobal.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Closed Caption FIFO | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On Thu, May 04, 2023 at 06:02:16PM -0400, Devin Heitmueller wrote: > The existing implementation made an attempt to remove duplicate > captions if increasing the framerate, but made no attempt to > handle reducing the framerate, nor did it rewrite the caption > payloads to have the appropriate cc_count (e.g. the cc_count needs > to change from 20 to 10 when going from 1080i59 to 720p59 and > vice-versa). > > Make use of the new ccfifo mechanism to ensure that caption data > is properly preserved. > > Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> > --- > libavfilter/vf_fps.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) breaks fate make -j32 fate-filter-fps-down-eof-pass ... ==28512== Invalid read of size 8 ==28512== at 0x11754A0: av_fifo_freep2 (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x5A40F3: ff_ccfifo_freep (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x2570D1: uninit (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x34C1E2: avfilter_free (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x34DCC3: avfilter_graph_free (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x2FCD32: init_complex_filtergraph (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x30F5F5: ffmpeg_parse_options (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== by 0x2F1254: main (in /home/michael/ffmpeg-git/ffmpeg/ffmpeg_g) ==28512== Address 0x0 is not stack'd, malloc'd or (recently) free'd [...]
Hi Michael, Thanks for testing. On Fri, May 5, 2023 at 10:10 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > breaks fate > > make -j32 fate-filter-fps-down-eof-pass Yeah, this was actually introduced in the last series due to removal of the NULL checks per Anton's review of the patch. We need to put one of the checks back in because while it's safe to call av_freep(*ccp) when it contains a NULL pointer it isn't safe to dereference that pointer when freeing the associated FIFOs vis av_fifo_freep2(). I'll include a fix in the next patch series. Regards, Devin
diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c index 051d278..824e7a1 100644 --- a/libavfilter/vf_fps.c +++ b/libavfilter/vf_fps.c @@ -34,6 +34,7 @@ #include "libavutil/mathematics.h" #include "libavutil/opt.h" #include "avfilter.h" +#include "ccfifo.h" #include "filters.h" #include "internal.h" @@ -85,6 +86,7 @@ typedef struct FPSContext { AVFrame *frames[2]; ///< buffered frames int frames_count; ///< number of buffered frames + AVCCFifo *cc_fifo; ///< closed captions int64_t next_pts; ///< pts of the next frame to output @@ -165,6 +167,7 @@ static av_cold void uninit(AVFilterContext *ctx) frame = shift_frame(ctx, s); av_frame_free(&frame); } + ff_ccfifo_freep(&s->cc_fifo); av_log(ctx, AV_LOG_VERBOSE, "%d frames in, %d frames out; %d frames dropped, " "%d frames duplicated.\n", s->frames_in, s->frames_out, s->drop, s->dup); @@ -210,6 +213,11 @@ static int config_props(AVFilterLink* outlink) s->in_pts_off, s->out_pts_off, s->start_time); } + if (!(s->cc_fifo = ff_ccfifo_alloc(outlink->frame_rate, ctx))) { + av_log(ctx, AV_LOG_ERROR, "Failure to setup CC FIFO queue\n"); + return AVERROR(ENOMEM); + } + av_log(ctx, AV_LOG_VERBOSE, "fps=%d/%d\n", outlink->frame_rate.num, outlink->frame_rate.den); return 0; @@ -242,6 +250,7 @@ static int read_frame(AVFilterContext *ctx, FPSContext *s, AVFilterLink *inlink, av_log(ctx, AV_LOG_DEBUG, "Read frame with in pts %"PRId64", out pts %"PRId64"\n", in_pts, frame->pts); + ff_ccfifo_extract(s->cc_fifo, frame); s->frames[s->frames_count++] = frame; s->frames_in++; @@ -289,7 +298,7 @@ static int write_frame(AVFilterContext *ctx, FPSContext *s, AVFilterLink *outlin if (!frame) return AVERROR(ENOMEM); // Make sure Closed Captions will not be duplicated - av_frame_remove_side_data(s->frames[0], AV_FRAME_DATA_A53_CC); + ff_ccfifo_inject(s->cc_fifo, frame); frame->pts = s->next_pts++; frame->duration = 1;
The existing implementation made an attempt to remove duplicate captions if increasing the framerate, but made no attempt to handle reducing the framerate, nor did it rewrite the caption payloads to have the appropriate cc_count (e.g. the cc_count needs to change from 20 to 10 when going from 1080i59 to 720p59 and vice-versa). Make use of the new ccfifo mechanism to ensure that caption data is properly preserved. Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavfilter/vf_fps.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)