| Message ID | 20230317200941.3936-5-dheitmueller@ltnglobal.com |
|---|---|
| State | New |
| Headers | show |
| Series | Properly handle CEA-708 caption data when transcoding | expand |
Hi Devin, Am Fr., 17. März 2023 um 21:10 Uhr schrieb Devin Heitmueller < devin.heitmueller@ltnglobal.com>: > Because the interlacing filter halves the effective framerate, we > need to ensure that no CEA-708 data is lost as frames are merged. > Not all modes of tinterlace halve the frame rate. mergex2 and pad keep it, interlacex2 even doubles it. > Make use of the new ccfifo mechanism to ensure that caption data > is properly preserved as frames pass through the filter. > > Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> > --- > libavfilter/tinterlace.h | 2 ++ > libavfilter/vf_tinterlace.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/libavfilter/tinterlace.h b/libavfilter/tinterlace.h > index 37b6c10c08..30574c2ebf 100644 > --- a/libavfilter/tinterlace.h > +++ b/libavfilter/tinterlace.h > @@ -30,6 +30,7 @@ > #include "libavutil/bswap.h" > #include "libavutil/opt.h" > #include "libavutil/pixdesc.h" > +#include "libavutil/ccfifo.h" > #include "drawutils.h" > #include "avfilter.h" > > @@ -77,6 +78,7 @@ typedef struct TInterlaceContext { > const AVPixFmtDescriptor *csp; > void (*lowpass_line)(uint8_t *dstp, ptrdiff_t width, const uint8_t > *srcp, > ptrdiff_t mref, ptrdiff_t pref, int clip_max); > + AVCCFifo *cc_fifo; > } TInterlaceContext; > > void ff_tinterlace_init_x86(TInterlaceContext *interlace); > diff --git a/libavfilter/vf_tinterlace.c b/libavfilter/vf_tinterlace.c > index 032629279a..260386a889 100644 > --- a/libavfilter/vf_tinterlace.c > +++ b/libavfilter/vf_tinterlace.c > @@ -291,6 +291,9 @@ static int config_out_props(AVFilterLink *outlink) > #endif > } > > + if (!(tinterlace->cc_fifo = av_ccfifo_alloc(&outlink->frame_rate, > ctx))) > + av_log(ctx, AV_LOG_VERBOSE, "Failure to setup CC FIFO queue. > Captions will be passed through\n"); > + > av_log(ctx, AV_LOG_VERBOSE, "mode:%d filter:%s h:%d -> h:%d\n", > tinterlace->mode, > (tinterlace->flags & TINTERLACE_FLAG_CVLPF) ? "complex" : > (tinterlace->flags & TINTERLACE_FLAG_VLPF) ? "linear" : "off", > @@ -375,6 +378,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > tinterlace->cur = tinterlace->next; > tinterlace->next = picref; > > + av_ccfifo_extract(tinterlace->cc_fifo, picref); > + > cur = tinterlace->cur; > next = tinterlace->next; > /* we need at least two frames */ > @@ -390,6 +395,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > if (!out) > return AVERROR(ENOMEM); > av_frame_copy_props(out, cur); > + av_ccfifo_inject(tinterlace->cc_fifo, out); > out->height = outlink->h; > out->interlaced_frame = 1; > out->top_field_first = 1; > @@ -423,6 +429,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > Are the drop modes missing? Would it perhaps be clearer to put av_ccfifo_inject before ff_filter_frame? > if (!out) > return AVERROR(ENOMEM); > av_frame_copy_props(out, cur); > + av_ccfifo_inject(tinterlace->cc_fifo, out); > out->height = outlink->h; > out->sample_aspect_ratio = av_mul_q(cur->sample_aspect_ratio, > av_make_q(2, 1)); > > @@ -459,6 +466,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > if (!out) > return AVERROR(ENOMEM); > av_frame_copy_props(out, cur); > + av_ccfifo_inject(tinterlace->cc_fifo, out); > out->interlaced_frame = 1; > out->top_field_first = tff; > > @@ -481,6 +489,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > out = av_frame_clone(cur); > if (!out) > return AVERROR(ENOMEM); > + av_ccfifo_inject(tinterlace->cc_fifo, out); > out->interlaced_frame = 1; > if (cur->pts != AV_NOPTS_VALUE) > out->pts = cur->pts*2; > @@ -495,6 +504,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame > *picref) > if (!out) > return AVERROR(ENOMEM); > av_frame_copy_props(out, next); > + av_ccfifo_inject(tinterlace->cc_fifo, out); > out->interlaced_frame = 1; > out->top_field_first = !tff; > > -- > Regards, Thomas
On Sat, Mar 18, 2023 at 1:12 PM Thomas Mundt <tmundt75@gmail.com> wrote:
> Not all modes of tinterlace halve the frame rate. mergex2 and pad keep it, interlacex2 even doubles it.
Sorry, I should have been more clear with the patch description in
that I realize that interlacing doesn't necessarily halve the
framerate, depending on the mode. That said, I was pretty sure I got
it working with all the code paths, but if you think I've missed one
or more then please let me know. Or if you have an alternative patch
which you think is cleaner for the tinterlace filter, I would be happy
to review it.
Thanks,
Devin
Am So., 19. März 2023 um 00:22 Uhr schrieb Devin Heitmueller < devin.heitmueller@ltnglobal.com>: > On Sat, Mar 18, 2023 at 1:12 PM Thomas Mundt <tmundt75@gmail.com> wrote: > > Not all modes of tinterlace halve the frame rate. mergex2 and pad keep > it, interlacex2 even doubles it. > > Sorry, I should have been more clear with the patch description in > that I realize that interlacing doesn't necessarily halve the > framerate, depending on the mode. That said, I was pretty sure I got > it working with all the code paths, but if you think I've missed one > or more then please let me know. Or if you have an alternative patch > which you think is cleaner for the tinterlace filter, I would be happy > to review it. > > I haven't worked with closed captions yet and also don't have the expertise to test them reliably. I just would like to roughly understand as maintainer of the tinterlace filter how your patch works in order not to break the functionality in future changes. Looking through it, I wonder for example why av_ccfifo_inject is set for the interleave modes and not for the drop modes. Under which conditions must av_ccfifo_inject be set? Whenever a frame is created at the output of the filter, or are there other conditions? Regards, Thomas
On Sun, Mar 19, 2023 at 12:50 PM Thomas Mundt <tmundt75@gmail.com> wrote: > Looking through it, I wonder for example why av_ccfifo_inject is set for the interleave modes and not for the drop modes. > Under which conditions must av_ccfifo_inject be set? Whenever a frame is created at the output of the filter, or are there other conditions? I'll have to double check the logic in tinterlace. The intention is that whenever a frame arrives on the input we should be calling extract, and whenever we generate a frame on the output we should be calling inject. If I've missed a case, then I will have to take care of that. Devin
diff --git a/libavfilter/tinterlace.h b/libavfilter/tinterlace.h index 37b6c10c08..30574c2ebf 100644 --- a/libavfilter/tinterlace.h +++ b/libavfilter/tinterlace.h @@ -30,6 +30,7 @@ #include "libavutil/bswap.h" #include "libavutil/opt.h" #include "libavutil/pixdesc.h" +#include "libavutil/ccfifo.h" #include "drawutils.h" #include "avfilter.h" @@ -77,6 +78,7 @@ typedef struct TInterlaceContext { const AVPixFmtDescriptor *csp; void (*lowpass_line)(uint8_t *dstp, ptrdiff_t width, const uint8_t *srcp, ptrdiff_t mref, ptrdiff_t pref, int clip_max); + AVCCFifo *cc_fifo; } TInterlaceContext; void ff_tinterlace_init_x86(TInterlaceContext *interlace); diff --git a/libavfilter/vf_tinterlace.c b/libavfilter/vf_tinterlace.c index 032629279a..260386a889 100644 --- a/libavfilter/vf_tinterlace.c +++ b/libavfilter/vf_tinterlace.c @@ -291,6 +291,9 @@ static int config_out_props(AVFilterLink *outlink) #endif } + if (!(tinterlace->cc_fifo = av_ccfifo_alloc(&outlink->frame_rate, ctx))) + av_log(ctx, AV_LOG_VERBOSE, "Failure to setup CC FIFO queue. Captions will be passed through\n"); + av_log(ctx, AV_LOG_VERBOSE, "mode:%d filter:%s h:%d -> h:%d\n", tinterlace->mode, (tinterlace->flags & TINTERLACE_FLAG_CVLPF) ? "complex" : (tinterlace->flags & TINTERLACE_FLAG_VLPF) ? "linear" : "off", @@ -375,6 +378,8 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) tinterlace->cur = tinterlace->next; tinterlace->next = picref; + av_ccfifo_extract(tinterlace->cc_fifo, picref); + cur = tinterlace->cur; next = tinterlace->next; /* we need at least two frames */ @@ -390,6 +395,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) if (!out) return AVERROR(ENOMEM); av_frame_copy_props(out, cur); + av_ccfifo_inject(tinterlace->cc_fifo, out); out->height = outlink->h; out->interlaced_frame = 1; out->top_field_first = 1; @@ -423,6 +429,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) if (!out) return AVERROR(ENOMEM); av_frame_copy_props(out, cur); + av_ccfifo_inject(tinterlace->cc_fifo, out); out->height = outlink->h; out->sample_aspect_ratio = av_mul_q(cur->sample_aspect_ratio, av_make_q(2, 1)); @@ -459,6 +466,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) if (!out) return AVERROR(ENOMEM); av_frame_copy_props(out, cur); + av_ccfifo_inject(tinterlace->cc_fifo, out); out->interlaced_frame = 1; out->top_field_first = tff; @@ -481,6 +489,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) out = av_frame_clone(cur); if (!out) return AVERROR(ENOMEM); + av_ccfifo_inject(tinterlace->cc_fifo, out); out->interlaced_frame = 1; if (cur->pts != AV_NOPTS_VALUE) out->pts = cur->pts*2; @@ -495,6 +504,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) if (!out) return AVERROR(ENOMEM); av_frame_copy_props(out, next); + av_ccfifo_inject(tinterlace->cc_fifo, out); out->interlaced_frame = 1; out->top_field_first = !tff;
Because the interlacing filter halves the effective framerate, we need to ensure that no CEA-708 data is lost as frames are merged. Make use of the new ccfifo mechanism to ensure that caption data is properly preserved as frames pass through the filter. Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavfilter/tinterlace.h | 2 ++ libavfilter/vf_tinterlace.c | 10 ++++++++++ 2 files changed, 12 insertions(+)