[FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc filter for closed caption handling
Soft Works
softworkz at hotmail.com
Wed Sep 22 05:03:41 EEST 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 03:40
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
>
> Soft Works:
> > - split_cc {V -> VS)
> > Extract closed-caption (A53) data from video
> > frames as subtitle Frames
> >
> > ffmpeg -y -loglevel verbose -i
> "https://streams.videolan.org/streams/ts/CC/NewsStream-608-ac3.ts" -
> filter_complex
> "[0:v]split_cc[vid1],textmod=mode=remove_chars:find='@',[vid1]overlay_textsub
> s" output.mkv
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> > doc/filters.texi | 25 ++++
> > libavfilter/Makefile | 1 +
> > libavfilter/allfilters.c | 1 +
> > libavfilter/sf_split_cc.c | 272 ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 299 insertions(+)
> > create mode 100644 libavfilter/sf_split_cc.c
> >
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 2d3dcdd7e6..da463e2cc1 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -25441,6 +25441,31 @@ ffmpeg -i INPUT -filter_complex
> "show_speaker=format=colon:style='@{\\c&HDD0000&
> > @end example
> > @end itemize
> >
> > +
> > + at section split_cc
> > +
> > +Split-out closed-caption/A53 subtitles from video frame side data.
> > +
> > +This filter provides an input and an output for video frames, which are
> just passed through without modification.
> > +The second out provides subtitle frames which are extracted from video
> frame side data.
> > +
> > +Inputs:
> > +- 0: Video
> > +
> > +Outputs:
> > +- 0: Video (same as input)
> > +- 1: Subtitles [text]
> > +
> > +It accepts the following parameters:
> > +
> > + at table @option
> > +
> > + at item use_cc_styles
> > +Emit closed caption style header. This will make closed captions look like
> on normal TV devices.
> > +(white font on black background rectangles)
> > +
> > + at end table
> > +
> > @section textsub2video
> >
> > Converts text subtitles to video frames.
> > diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> > index 309c404bf7..39abf6d2a6 100644
> > --- a/libavfilter/Makefile
> > +++ b/libavfilter/Makefile
> > @@ -540,6 +540,7 @@ OBJS-$(CONFIG_NULLSINK_FILTER) +=
> vsink_nullsink.o
> > OBJS-$(CONFIG_CENSOR_FILTER) += sf_textmod.o
> > OBJS-$(CONFIG_SHOW_SPEAKER_FILTER) += sf_textmod.o
> > OBJS-$(CONFIG_TEXTMOD_FILTER) += sf_textmod.o
> > +OBJS-$(CONFIG_SPLIT_CC_FILTER) += sf_split_cc.o
> > OBJS-$(CONFIG_STRIPSTYLES_FILTER) += sf_stripstyles.o
> >
> > # multimedia filters
> > diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> > index e2e4deea22..77c6379302 100644
> > --- a/libavfilter/allfilters.c
> > +++ b/libavfilter/allfilters.c
> > @@ -528,6 +528,7 @@ extern const AVFilter ff_avf_showwavespic;
> > extern const AVFilter ff_vaf_spectrumsynth;
> > extern const AVFilter ff_sf_censor;
> > extern const AVFilter ff_sf_show_speaker;
> > +extern const AVFilter ff_sf_split_cc;
> > extern const AVFilter ff_sf_stripstyles;
> > extern const AVFilter ff_sf_textmod;
> > extern const AVFilter ff_svf_graphicsub2video;
> > diff --git a/libavfilter/sf_split_cc.c b/libavfilter/sf_split_cc.c
> > new file mode 100644
> > index 0000000000..e0ba5d4bcd
> > --- /dev/null
> > +++ b/libavfilter/sf_split_cc.c
> > @@ -0,0 +1,272 @@
> > +/*
> > + * Copyright (c) 2021 softworkz
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
> 1301 USA
> > + */
> > +
> > +/**
> > + * @file
> > + * subtitle filter for splitting out closed-caption/A53 subtitles from
> video frame side data
> > + */
> > +
> > +#include <libavcodec/ass.h>
> > +
> > +#include "libavutil/avassert.h"
> > +#include "libavutil/opt.h"
> > +#include "avfilter.h"
> > +#include "internal.h"
> > +#include "subtitles.h"
> > +#include "libavcodec/avcodec.h"
> > +
> > +typedef struct SplitCaptionsContext {
> > + const AVClass *class;
> > + enum AVSubtitleType format;
> > + AVCodecContext *cc_dec;
> > + int eof;
> > + AVFrame *next_sub_frame;
> > + int new_frame;
> > + char *subtile_header;
>
> I presume it is supposed to be subtitle_header
>
> > + int use_cc_styles;
> > +} SplitCaptionsContext;
> > +
> > +static int init(AVFilterContext *ctx)
> > +{
> > + SplitCaptionsContext *s = ctx->priv;
> > +
> > + if (!s->cc_dec) {
>
> Unnecessary: When init is called, only AVOpt-enabled fields (in your
> case: the class pointer and use_cc_styles) are not zero; the rest of the
> context is zeroed. This includes cc_dec.
I wasn't sure whether it is guaranteed that init doesn't get called
twice. Probably it is?
>
> > + AVDictionary *options = NULL;
> > + int ret;
> > + const AVCodec *codec = avcodec_find_decoder(AV_CODEC_ID_EIA_608);
> > + if (!codec) {
> > + av_log(ctx, AV_LOG_ERROR, "failed to find EIA-608/708
> decoder\n");
> > + return AVERROR_DECODER_NOT_FOUND;
> > + }
>
> If a component relies on another component to be usable, you should add
> a dependency to configure.
I had this from vf_subtitle, but in that case it doesn't fully depend
on the decoder it tries to find.
Fine - I'll make it statically dependant.
>
> > +
> > + if (!((s->cc_dec = avcodec_alloc_context3(codec)))) {
> > + av_log(ctx, AV_LOG_ERROR, "failed to allocate EIA-608/708
> decoder\n");
> > + return AVERROR(ENOMEM);
> > + }
> > +
> > + av_dict_set(&options, "sub_text_format", "ass", 0);
>
> Isn't this deprecated and unused?
Correct, this can be dropped.
>
> > + av_dict_set(&options, "real_time", "0", 0);
> > +
> > + if ((ret = avcodec_open2(s->cc_dec, codec, &options)) < 0) {
> > + av_log(ctx, AV_LOG_ERROR, "failed to open EIA-608/708 decoder:
> %i\n", ret);
>
> Potential leak of options here. The easiest way to fix it is by setting
> the option directly: av_opt_set_int(s->cc_dec, "real_time", 0,
> AV_OPT_SEARCH_CHILDREN)
> But actually you do not need to set the option at all, as it is the
> default value.
This can be dropped as well. Initially I had set it to 1, that's
where it came from.
>
> > + return ret;
> > + }
> > +
> > + if (s->cc_dec->subtitle_header && s->use_cc_styles)
> > + s->subtile_header = av_strdup((char *)s->cc_dec-
> >subtitle_header);
>
> Unchecked allocation. Furthermore, this string leaks if the allocation
> succeeds; finally, it is currently unused, but I guess this is a WIP patch.
It's pretty complete already, but an option to choose a certain
set of default styles is planned.
>
> > +
> > + av_dict_free(&options);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void uninit(AVFilterContext *ctx)
> > +{
> > + SplitCaptionsContext *s = ctx->priv;
> > + if (s->next_sub_frame)
> > + av_frame_unref(s->next_sub_frame);
>
> Leak. You must call av_frame_free(&s->next_sub_frame), as the AVFrame
> itself would leak otherwise (av_frame_unref() only frees its contents,
> not the frame). Notice that we typically do this unconditionally and do
> not check beforehand, as av_frame_free() takes care not to free an
> inexistent frame.
Yup makes sense, thanks.
> > +
> > +static int config_input(AVFilterLink *link)
> > +{
> > + const SplitCaptionsContext *context = link->dst->priv;
> > +
> > + if (context->cc_dec)
>
> Unnecessary: init has been called before and set cc_dec (if it failed,
> config_input is not called at all).
>
> > + context->cc_dec->pkt_timebase = link->time_base;
> > +
> > + return 0;
> > +}
> > +
> > +static int query_formats(AVFilterContext *ctx)
> > +{
> > + AVFilterFormats *formats;
> > + AVFilterLink *inlink0 = ctx->inputs[0];
> > + AVFilterLink *outlink0 = ctx->outputs[0];
> > + AVFilterLink *outlink1 = ctx->outputs[1];
> > + static const enum AVSubtitleType subtitle_fmts[] = {
> AV_SUBTITLE_FMT_ASS, AV_SUBTITLE_FMT_NB };
> > + int ret;
> > +
> > + /* set input0 video formats */
> > + formats = ff_all_formats(AVMEDIA_TYPE_VIDEO);
> > + if ((ret = ff_formats_ref(formats, &inlink0->outcfg.formats)) < 0)
> > + return ret;
> > +
> > + /* set output0 video formats */
> > + if ((ret = ff_formats_ref(formats, &outlink0->incfg.formats)) < 0)
> > + return ret;
> > +
> > + /* set output1 subtitle formats */
> > + formats = ff_make_format_list(subtitle_fmts);
> > + if ((ret = ff_formats_ref(formats, &outlink1->incfg.formats)) < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int config_sub_output(AVFilterLink *outlink)
> > +{
> > + const AVFilterLink *inlink = outlink->src->inputs[0];
> > +
> > + outlink->frame_rate = inlink->frame_rate;
> > + outlink->time_base = inlink->time_base;
> > + outlink->format = AV_SUBTITLE_FMT_ASS;
> > +
> > + return 0;
> > +}
> > +
> > +static int request_sub_frame(AVFilterLink *outlink)
> > +{
> > + SplitCaptionsContext *s = outlink->src->priv;
> > +
> > + if (s->eof)
> > + return AVERROR_EOF;
> > +
> > + if (s->next_sub_frame && s->new_frame) {
> > + AVFrame *out;
> > + s->next_sub_frame->pts++;
> > +
> > + out = av_frame_clone(s->next_sub_frame);
> > + if (!out)
> > + return AVERROR(ENOMEM);
> > +
> > + out->subtitle_pts = av_rescale_q(s->next_sub_frame->pts, outlink-
> >time_base, AV_TIME_BASE_Q);
> > + s->new_frame = 0;
> > +
> > + return ff_filter_frame(outlink, out);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> > +{
> > + AVFrameSideData *sd;
> > + SplitCaptionsContext *s = inlink->dst->priv;
> > + AVFilterLink *outlink0 = inlink->dst->outputs[0];
> > + AVFilterLink *outlink1 = inlink->dst->outputs[1];
> > + AVPacket pkt = {0};
>
> sizeof(AVPacket) being part of the ABI is deprecated, i.e. stack packets
> are supposed to be removed and definitely not added.
>
> > + AVFrame *sub_out;
> > +
> > + int ret;
> > +
> > + outlink0->format = inlink->format;
> > +
> > + if (!frame)
> > + return AVERROR(ENOMEM);
>
> Nonsense check: The frame passed to a filter_frame function is never
> NULL. (Unless you added weird semantics for subtitles.)
>
> > +
> > + sd = av_frame_get_side_data(frame, AV_FRAME_DATA_A53_CC);
> > +
> > + if (sd) {
> > + int got_output = 0;
> > +
> > + av_new_packet(&pkt, (int)sd->size);
>
> Unchecked allocation.
>
> > + if (!((pkt.buf = av_buffer_ref(sd->buf))))
>
> If the above allocation was successful, it leaks here.
>
> > + return AVERROR(ENOMEM);
> > +
> > + pkt.data = sd->data;
> > + pkt.size = (int)sd->size;
> > + pkt.pts = frame->pts;
> > +
> > + sub_out = ff_get_subtitles_buffer(outlink1, AV_SUBTITLE_FMT_ASS);
> > + if (!sub_out)
> > + return AVERROR(ENOMEM);
>
> pkt leaks here
>
> > +
> > + sub_out->subtitle_header = (char*)s->cc_dec->subtitle_header;
> > +
> > + ret = avcodec_decode_subtitle3(s->cc_dec, sub_out, &got_output,
> &pkt);
> > +
> > + if (ret < 0)
> > + goto fail;
> > +
> > + if (got_output) {
> > + sub_out->pts = frame->pts;
> > + av_frame_unref(s->next_sub_frame);
> > + s->next_sub_frame = sub_out;
>
> You are leaking next_sub_frame here.
>
> > + s->new_frame = 1;
> > +
> > + ret = request_sub_frame(outlink1);
> > + if (ret < 0)
> > + return ret;
>
> pkt leaks here.
>
> > + }
> > + else
>
> Please put your else on the same line as the closing braces (it would
> waste too much space otherwise).
>
> > + av_frame_free(&sub_out);
> > + }
> > +
> > + if (!s->next_sub_frame) {
> > + s->next_sub_frame = ff_get_subtitles_buffer(outlink1, outlink1-
> >format);
>
> Unchecked allocation.
>
> > + s->next_sub_frame->subtitle_end_time = 1000;
> > + s->next_sub_frame->pts = frame->pts;
> > + s->new_frame = 1;
> > + s->next_sub_frame->subtitle_header = (char*)s->cc_dec-
> >subtitle_header;
>
> The lifetime of this string ends when this filter is uninitialized; is
> there a possibility that it ends up outside (via the av_frame_clone in
> request_sub_frame)? Then this is not ok.
This is a good question, as to whether it is safe to assume that the
filter's lifetime doesn't end before the last frame has been processed.
I thought it would be?
Thanks again,
softworkz
PS: The unresponded notes are all justified, will fix them.
More information about the ffmpeg-devel
mailing list