[FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc filter for closed caption handling
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Wed Sep 22 05:17:32 EEST 2021
Soft Works:
>
>
>> -----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?
>
The init functions are only called from avfilter_init_dict() and
avfilter_init_str() (the latter actually calls the former). Both of
these require the context to be uninitialized initially. If a user calls
these functions with an already initialized AVFilterContext, they are
responsible for the consequences (it's undefined behaviour, most likely
leaks).
(I have to admit that I don't know whether it is documented whether one
is allowed to call these functions again after the first try failed. But
I know that the whole codebase assumes the answer to this to be "no", so
you may presume it, too.)
>>
>>> + 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?
So the frames are supposed to be never forwarded to the user? Or is the
user supposed to stop using these frames after the filter's lifetime ended?
- Andreas
More information about the ffmpeg-devel
mailing list