[FFmpeg-devel,v8,12/13] avfilter/split_cc: Add split_cc filter for closed caption handling

- 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_textsubs" output.mkv

Signed-off-by: softworkz <softworkz@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

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_textsubs" output.mkv
> 
> Signed-off-by: softworkz <softworkz@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
>  
> +
> +@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:
> +
> +@table @option
> +
> +@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)
> +
> +@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.

> +        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.

> +
> +        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?

> +        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.

> +            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.

> +
> +        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.

> +}
> +
> +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.

> +    }
> +    else
> +        s->next_sub_frame->pts = frame->pts;
> +
> +    ret = ff_filter_frame(outlink0, frame);
> +
> +fail:
> +    av_packet_unref(&pkt);
> +    return ret;
> +}
> +
> +#define OFFSET(x) offsetof(SplitCaptionsContext, x)
> +#define FLAGS (AV_OPT_FLAG_SUBTITLE_PARAM | AV_OPT_FLAG_FILTERING_PARAM)
> +
> +static const AVOption split_cc_options[] = {
> +    { "use_cc_styles",    "Emit closed caption style header", OFFSET(use_cc_styles),  AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS, NULL },
> +    { NULL },
> +};
> +
> +AVFILTER_DEFINE_CLASS(split_cc);
> +
> +static const AVFilterPad inputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_VIDEO,
> +        .filter_frame = filter_frame,
> +        .config_props = config_input,
> +    },
> +};
> +
> +static const AVFilterPad outputs[] = {
> +    {
> +        .name          = "video_passthrough",
> +        .type          = AVMEDIA_TYPE_VIDEO,
> +    },
> +    {
> +        .name          = "subtitles",
> +        .type          = AVMEDIA_TYPE_SUBTITLE,
> +        .request_frame = request_sub_frame,
> +        .config_props  = config_sub_output,
> +    },
> +};
> +
> +const AVFilter ff_sf_split_cc = {
> +    .name          = "split_cc",
> +    .description   = NULL_IF_CONFIG_SMALL("Extract closed-caption (A53) data from video as subtitle stream."),
> +    .init          = init,
> +    .uninit        = uninit,
> +    .query_formats = query_formats,
> +    .priv_size     = sizeof(SplitCaptionsContext),
> +    .priv_class    = &split_cc_class,
> +    FILTER_INPUTS(inputs),
> +    FILTER_OUTPUTS(outputs),
> +};
>

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 03:40
> To: ffmpeg-devel@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@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
> >
> > +
> > +@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:
> > +
> > +@table @option
> > +
> > +@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)
> > +
> > +@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.

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 03:40
>> To: ffmpeg-devel@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@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
>>>
>>> +
>>> +@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:
>>> +
>>> +@table @option
>>> +
>>> +@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)
>>> +
>>> +@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

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 04:18
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
> 
> > 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.)
> 

Good, thanks for the explanation.


> > 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?


The subtitle_header is global and is constant across all frames.
A resulting media file has only a (= the) single "subtitle header".
(it's not like a "header for each subtitle line or event").

softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 04:18
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
>> filter for closed caption handling
>>
>>> 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?
> 
> 
> The subtitle_header is global and is constant across all frames.
> A resulting media file has only a (= the) single "subtitle header".
> (it's not like a "header for each subtitle line or event").
> 

Who ensures that it is constant across all frames? What happens if one
mixes frames from two different sources? And who owns the subtitle header?

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 04:43
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 04:18
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> split_cc
> >> filter for closed caption handling
> >>
> >>> 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?
> >
> >
> > The subtitle_header is global and is constant across all frames.
> > A resulting media file has only a (= the) single "subtitle header".
> > (it's not like a "header for each subtitle line or event").
> >
> 
> Who ensures that it is constant across all frames? What happens if one
> mixes frames from two different sources? And who owns the subtitle header?

In cases when the subtitle stream originates from a decoder and frames 
are submitted into the filtergraph, ffmpeg.c makes a copy of it (field 
of AVCodecContext) and stores it in InputStream->subtitle_heartbeat->subtitle header 
and stores the pointer to this in each subtitle frame.
It gets freed during ffmpeg uninit.

In cases when the subtitle stream originates from a filter like split_cc
or graphicsubs2text, the filter creates the header and owns it, and stores 
the reference in each subtitle frame it produces.
It gets freed during filter uninit.

Relevant is the subtitle_header of the first frame that arrives at a target.
A target can be an encoder at the end of the filtergraph or other filters
like overlay_textsubs. 

Any filters in-between may use the subtitle_header for reference when 
performing certain manipulations, but are not allowed (meant to) modify it.

softworkz


 first sub

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 04:43
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
>> filter for closed caption handling
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>>>> Rheinhardt
>>>> Sent: Wednesday, 22 September 2021 04:18
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
>> split_cc
>>>> filter for closed caption handling
>>>>
>>>>> 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?
>>>
>>>
>>> The subtitle_header is global and is constant across all frames.
>>> A resulting media file has only a (= the) single "subtitle header".
>>> (it's not like a "header for each subtitle line or event").
>>>
>>
>> Who ensures that it is constant across all frames? What happens if one
>> mixes frames from two different sources? And who owns the subtitle header?
> 
> In cases when the subtitle stream originates from a decoder and frames 
> are submitted into the filtergraph, ffmpeg.c makes a copy of it (field 
> of AVCodecContext) and stores it in InputStream->subtitle_heartbeat->subtitle header 
> and stores the pointer to this in each subtitle frame.
> It gets freed during ffmpeg uninit.
> 
> In cases when the subtitle stream originates from a filter like split_cc
> or graphicsubs2text, the filter creates the header and owns it, and stores 
> the reference in each subtitle frame it produces.
> It gets freed during filter uninit.
> 
> Relevant is the subtitle_header of the first frame that arrives at a target.
> A target can be an encoder at the end of the filtergraph or other filters
> like overlay_textsubs. 
> 
> Any filters in-between may use the subtitle_header for reference when 
> performing certain manipulations, but are not allowed (meant to) modify it.
> 

If a frame or packet is refcounted, the lifetime of the data contained
(or rather referenced) by it is indefinite; it survives closing a
demuxer/encoder (for packets) or a decoder (for frames). I might have
trouble making sense of the timestamp if I close the demuxer and the
AVStream (with the timebase) it came from, but I still have the data.

This is not so with this field; it is supposed to be not modified by the
owner of an AVFrame, yet it is not even a const char*. Worse, it's
lifetime is tied in a completely unclear way to something else. This is
just not going to work.

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 05:50
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 04:43
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> split_cc
> >> filter for closed caption handling
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas
> >>>> Rheinhardt
> >>>> Sent: Wednesday, 22 September 2021 04:18
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> >> split_cc
> >>>> filter for closed caption handling
> >>>>
> >>>>> 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?
> >>>
> >>>
> >>> The subtitle_header is global and is constant across all frames.
> >>> A resulting media file has only a (= the) single "subtitle header".
> >>> (it's not like a "header for each subtitle line or event").
> >>>
> >>
> >> Who ensures that it is constant across all frames? What happens if one
> >> mixes frames from two different sources? And who owns the subtitle header?
> >
> > In cases when the subtitle stream originates from a decoder and frames
> > are submitted into the filtergraph, ffmpeg.c makes a copy of it (field
> > of AVCodecContext) and stores it in InputStream->subtitle_heartbeat-
> >subtitle header
> > and stores the pointer to this in each subtitle frame.
> > It gets freed during ffmpeg uninit.
> >
> > In cases when the subtitle stream originates from a filter like split_cc
> > or graphicsubs2text, the filter creates the header and owns it, and stores
> > the reference in each subtitle frame it produces.
> > It gets freed during filter uninit.
> >
> > Relevant is the subtitle_header of the first frame that arrives at a
> target.
> > A target can be an encoder at the end of the filtergraph or other filters
> > like overlay_textsubs.
> >
> > Any filters in-between may use the subtitle_header for reference when
> > performing certain manipulations, but are not allowed (meant to) modify it.
> >
> 
> If a frame or packet is refcounted, the lifetime of the data contained
> (or rather referenced) by it is indefinite; it survives closing a
> demuxer/encoder (for packets) or a decoder (for frames).

That's why it's strduped to InputStream->subtitle_hearbeat which is 
freed only on ffmpeg shutdown.

> This is not so with this field; it is supposed to be not modified by the
> owner of an AVFrame, yet it is not even a const char*. Worse, it's
> lifetime is tied in a completely unclear way to something else. This is
> just not going to work.

I was unsure how this would be considered. I'm sometimes having a hard
time to guess what's considered good or bad. At some places, code is
strongly relying on assumptions and conventions and could easily fail
if any of those is not fulfilled, and at other places/situations
there are rather strict constraints imposed even when it's unlikely 
that these won't be missed.
 
Anyway - focusing on that issue - I'd change the subtitle header 
string to be carried in a refcounted buffer instead,
would that be ok?

softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 05:50
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
>> filter for closed caption handling
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>>>> Rheinhardt
>>>> Sent: Wednesday, 22 September 2021 04:43
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
>> split_cc
>>>> filter for closed caption handling
>>>>
>>>> Soft Works:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas
>>>>>> Rheinhardt
>>>>>> Sent: Wednesday, 22 September 2021 04:18
>>>>>> To: ffmpeg-devel@ffmpeg.org
>>>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
>>>> split_cc
>>>>>> filter for closed caption handling
>>>>>>
>>>>>>> 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?
>>>>>
>>>>>
>>>>> The subtitle_header is global and is constant across all frames.
>>>>> A resulting media file has only a (= the) single "subtitle header".
>>>>> (it's not like a "header for each subtitle line or event").
>>>>>
>>>>
>>>> Who ensures that it is constant across all frames? What happens if one
>>>> mixes frames from two different sources? And who owns the subtitle header?
>>>
>>> In cases when the subtitle stream originates from a decoder and frames
>>> are submitted into the filtergraph, ffmpeg.c makes a copy of it (field
>>> of AVCodecContext) and stores it in InputStream->subtitle_heartbeat-
>>> subtitle header
>>> and stores the pointer to this in each subtitle frame.
>>> It gets freed during ffmpeg uninit.
>>>
>>> In cases when the subtitle stream originates from a filter like split_cc
>>> or graphicsubs2text, the filter creates the header and owns it, and stores
>>> the reference in each subtitle frame it produces.
>>> It gets freed during filter uninit.
>>>
>>> Relevant is the subtitle_header of the first frame that arrives at a
>> target.
>>> A target can be an encoder at the end of the filtergraph or other filters
>>> like overlay_textsubs.
>>>
>>> Any filters in-between may use the subtitle_header for reference when
>>> performing certain manipulations, but are not allowed (meant to) modify it.
>>>
>>
>> If a frame or packet is refcounted, the lifetime of the data contained
>> (or rather referenced) by it is indefinite; it survives closing a
>> demuxer/encoder (for packets) or a decoder (for frames).
> 
> That's why it's strduped to InputStream->subtitle_hearbeat which is 
> freed only on ffmpeg shutdown.
> 
>> This is not so with this field; it is supposed to be not modified by the
>> owner of an AVFrame, yet it is not even a const char*. Worse, it's
>> lifetime is tied in a completely unclear way to something else. This is
>> just not going to work.
> 
> I was unsure how this would be considered. I'm sometimes having a hard
> time to guess what's considered good or bad. At some places, code is
> strongly relying on assumptions and conventions and could easily fail
> if any of those is not fulfilled, and at other places/situations
> there are rather strict constraints imposed even when it's unlikely 
> that these won't be missed.

Ownership needs to be absolutely clear or everything will crash (or leak).

>  
> Anyway - focusing on that issue - I'd change the subtitle header 
> string to be carried in a refcounted buffer instead,
> would that be ok?
> 

It would solve the ownership issues; unfortunately at the cost of an
allocation more for av_frame_ref, av_frame_copy_props etc. But that is
not your fault. But it does not solve the problem of ensuring that the
header that is supposed not to change does in fact not change.
The other route would be to treat it like an AVStream's extradata, i.e.
like something that is by design not part of a single AVPacket/AVFrame,
but belongs to something else that is associated with these
packets/frames. How to know where to find this other thing is up to each
piece of code.

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 06:55
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 05:50
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> split_cc
> >> filter for closed caption handling
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas
> >>>> Rheinhardt
> >>>> Sent: Wednesday, 22 September 2021 04:43
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> >> split_cc
> >>>> filter for closed caption handling
> >>>>
> >>>> Soft Works:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas
> >>>>>> Rheinhardt
> >>>>>> Sent: Wednesday, 22 September 2021 04:18
> >>>>>> To: ffmpeg-devel@ffmpeg.org
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> >>>> split_cc
> >>>>>> filter for closed caption handling
> >>>>>>
> >>>>>>> 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?
> >>>>>
> >>>>>
> >>>>> The subtitle_header is global and is constant across all frames.
> >>>>> A resulting media file has only a (= the) single "subtitle header".
> >>>>> (it's not like a "header for each subtitle line or event").
> >>>>>
> >>>>
> >>>> Who ensures that it is constant across all frames? What happens if one
> >>>> mixes frames from two different sources? And who owns the subtitle
> header?
> >>>
> >>> In cases when the subtitle stream originates from a decoder and frames
> >>> are submitted into the filtergraph, ffmpeg.c makes a copy of it (field
> >>> of AVCodecContext) and stores it in InputStream->subtitle_heartbeat-
> >>> subtitle header
> >>> and stores the pointer to this in each subtitle frame.
> >>> It gets freed during ffmpeg uninit.
> >>>
> >>> In cases when the subtitle stream originates from a filter like split_cc
> >>> or graphicsubs2text, the filter creates the header and owns it, and
> stores
> >>> the reference in each subtitle frame it produces.
> >>> It gets freed during filter uninit.
> >>>
> >>> Relevant is the subtitle_header of the first frame that arrives at a
> >> target.
> >>> A target can be an encoder at the end of the filtergraph or other filters
> >>> like overlay_textsubs.
> >>>
> >>> Any filters in-between may use the subtitle_header for reference when
> >>> performing certain manipulations, but are not allowed (meant to) modify
> it.
> >>>
> >>
> >> If a frame or packet is refcounted, the lifetime of the data contained
> >> (or rather referenced) by it is indefinite; it survives closing a
> >> demuxer/encoder (for packets) or a decoder (for frames).
> >
> > That's why it's strduped to InputStream->subtitle_hearbeat which is
> > freed only on ffmpeg shutdown.
> >
> >> This is not so with this field; it is supposed to be not modified by the
> >> owner of an AVFrame, yet it is not even a const char*. Worse, it's
> >> lifetime is tied in a completely unclear way to something else. This is
> >> just not going to work.
> >
> > I was unsure how this would be considered. I'm sometimes having a hard
> > time to guess what's considered good or bad. At some places, code is
> > strongly relying on assumptions and conventions and could easily fail
> > if any of those is not fulfilled, and at other places/situations
> > there are rather strict constraints imposed even when it's unlikely
> > that these won't be missed.
> 
> Ownership needs to be absolutely clear or everything will crash (or leak).
> 
> >
> > Anyway - focusing on that issue - I'd change the subtitle header
> > string to be carried in a refcounted buffer instead,
> > would that be ok?
> >
> 
> It would solve the ownership issues; unfortunately at the cost of an
> allocation more for av_frame_ref, av_frame_copy_props etc. But that is
> not your fault. But it does not solve the problem of ensuring that the
> header that is supposed not to change does in fact not change.

Every language has its upsides and downsides, and in the case of C, 
options are limited for enforcing things like that. 
But everybody knows that. There are thousands of similar things 
which a C API can't prevent a user from doing and in order to know 
what is allowed to be done and what not, it is required to read docs, 
source code or example code.

So I wonder why there would be a reason to be concerned about 
specifically this subtitle_header string. The worst case is that
an API consumer doesn't read docs or source annotations, starts to
modify that field and then: he will notice that it doesn't have any 
effect and stop modifying it.

On the other hand it's clear that it's neither nice to copy over this
from frame-to-frame nor does it make sense from a logical perspective
for this to be carried as a field of a frame.

> The other route would be to treat it like an AVStream's extradata, i.e.
> like something that is by design not part of a single AVPacket/AVFrame,
> but belongs to something else that is associated with these
> packets/frames. How to know where to find this other thing is up to each
> piece of code.

That makes more sense of course, to have a single global "object"
containing per-stream data and have each frame carry a reference to 
that instead.

If such thing would be available, I would use it of course...

softworkz