[FFmpeg-devel,v8,11/13] avfilter/stripstyles: Add stripstyles filter

- stripstyles {S -> S)
  Remove all inline styles from subtitle events

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/Makefile         |   1 +
 libavfilter/allfilters.c     |   1 +
 libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 libavfilter/sf_stripstyles.c

Soft Works:
> - stripstyles {S -> S)
>   Remove all inline styles from subtitle events
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  libavfilter/Makefile         |   1 +
>  libavfilter/allfilters.c     |   1 +
>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 libavfilter/sf_stripstyles.c
> 
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
>  
>  # multimedia filters
>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index fc72858d18..e2e4deea22 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_stripstyles;
>  extern const AVFilter ff_sf_textmod;
>  extern const AVFilter ff_svf_graphicsub2video;
>  extern const AVFilter ff_svf_textsub2video;
> diff --git a/libavfilter/sf_stripstyles.c b/libavfilter/sf_stripstyles.c
> new file mode 100644
> index 0000000000..15c00e73b1
> --- /dev/null
> +++ b/libavfilter/sf_stripstyles.c
> @@ -0,0 +1,211 @@
> +/*
> + * 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
> + * text subtitle filter which removes inline-styles from subtitles
> + */
> +
> +#include <libavcodec/ass.h>

This header and the API in it is not public.

> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/opt.h"
> +#include "avfilter.h"
> +#include "internal.h"
> +#include "libavcodec/avcodec.h"
> +#include "libavcodec/ass_split.h"
> +
> +typedef struct StripStylesContext {
> +    const AVClass *class;
> +    enum AVSubtitleType format;
> +    int remove_animated;
> +} StripStylesContext;
> +
> +typedef struct DialogContext {
> +    const AVClass *class;

An AVClass is for options and logging (and would actually need an
AVClass, which you didn't define); you are not using it for that, so
just remove this.

> +    const StripStylesContext* ss_ctx;
> +    AVBPrint buffer;
> +    int drawing_scale;
> +    int is_animated;
> +} DialogContext;
> +
> +static void dialog_text_cb(void *priv, const char *text, int len)
> +{
> +    DialogContext *s = priv;
> +    if (!s->drawing_scale && (!s->is_animated || !s->ss_ctx->remove_animated))
> +        av_bprint_append_data(&s->buffer, text, len);
> +}
> +
> +static void dialog_new_line_cb(void *priv, int forced)
> +{
> +    DialogContext *s = priv;
> +    if (!s->drawing_scale && !s->is_animated)
> +        av_bprint_append_data(&s->buffer, forced ? "\\N" : "\\n", 2);
> +}
> +
> +static void dialog_drawing_mode_cb(void *priv, int scale)
> +{
> +    DialogContext *s = priv;
> +    s->drawing_scale = scale;
> +}
> +
> +static void dialog_animate_cb(void *priv, int t1, int t2, int accel, char *style)
> +{
> +    DialogContext *s = priv;
> +    s->is_animated = 1;
> +}
> +
> +static void dialog_move_cb(void *priv, int x1, int y1, int x2, int y2, int t1, int t2)
> +{
> +    DialogContext *s = priv;
> +    if (t1 >= 0 || t2 >= 0)
> +        s->is_animated = 1;
> +}
> +
> +static const ASSCodesCallbacks dialog_callbacks = {
> +    .text             = dialog_text_cb,
> +    .new_line         = dialog_new_line_cb,
> +    .drawing_mode     = dialog_drawing_mode_cb,
> +    .animate          = dialog_animate_cb,
> +    .move             = dialog_move_cb,
> +};
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    AVFilterFormats *formats;
> +    AVFilterLink *inlink = ctx->inputs[0];
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    static const enum AVSubtitleType subtitle_fmts[] = { AV_SUBTITLE_FMT_ASS, AV_SUBTITLE_FMT_NONE };
> +    int ret;
> +
> +    /* set input subtitle format */
> +    formats = ff_make_format_list(subtitle_fmts);
> +    if ((ret = ff_formats_ref(formats, &inlink->outcfg.formats)) < 0)
> +        return ret;
> +
> +    /* set output video format */
> +    if ((ret = ff_formats_ref(formats, &outlink->incfg.formats)) < 0)
> +        return ret;
> +
> +    return 0;
> +}
> +
> +static char *ass_get_line(int readorder, int layer, const char *style,
> +                        const char *speaker, const char *effect, const char *text)
> +{
> +    return av_asprintf("%d,%d,%s,%s,0,0,0,%s,%s",
> +                       readorder, layer, style ? style : "Default",
> +                       speaker ? speaker : "", effect, text);
> +}
> +
> +static char *process_dialog(StripStylesContext *s, const char *ass_line)
> +{
> +    ASSDialog *dialog = ff_ass_split_dialog(NULL, ass_line);
> +    char *result = NULL, *text;
> +    DialogContext dlg_ctx = { 0 };
> +
> +    if (!dialog)
> +        return NULL;
> +
> +    dlg_ctx.ss_ctx = s;
> +
> +    av_bprint_init(&dlg_ctx.buffer, 0, AV_BPRINT_SIZE_UNLIMITED);
> +
> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog->text);

This function is in libavcodec and is not public; it is not available
when using shared linking. The same goes for all the other ff_ass_*
functions. Generally, the ff-prefix denotes something that is library
internal, but is not static, i.e. not confined to one translation unit.

> +
> +    av_bprint_finalize(&dlg_ctx.buffer, &text);
> +
> +    if (text && strlen(text) > 0)

The AVBPrint API actually keeps track of the length of the buffer for
you: dlg_ctx.buffer.len. And anyway, checking whether a string has
positive strlen can simply be done by "*text != '\0'" (or to "*text").
Notice that an AVBPrint can be truncated on allocation failure. This
needs to be checked (and often isn't).

> +        result = ass_get_line(dialog->readorder, dialog->layer, dialog->style, dialog->name, dialog->effect, text);
> +
> +    av_free(text);

This is not how the AVBPrint is supposed to be used; you are wasting the
small string-optimization.

> +    ff_ass_free_dialog(&dialog);
> +
> +    return result;
> +}
> +
> +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> +{
> +    StripStylesContext *s = inlink->dst->priv;
> +    AVFilterLink *outlink = inlink->dst->outputs[0];
> +
> +    outlink->format = inlink->format;
> +
> +    av_frame_make_writable(frame);
> +    if (!frame)
> +        return AVERROR(ENOMEM);

This is complete nonsense: C uses call-by-value, so
av_frame_make_writable just can't modify the pointer at all even if it
wanted. Instead it returns a negative value on error.
(And you are also leaking frame.)

> +
> +    for (unsigned i = 0; i < frame->num_subtitle_areas; i++) {
> +
> +        AVSubtitleArea *area = frame->subtitle_areas[i];
> +
> +        if (area->ass) {
> +            char *tmp = area->ass;
> +            area->ass = process_dialog(s, area->ass);
> +
> +            if (area->ass) {
> +                av_log(inlink->dst, AV_LOG_DEBUG, "original: %s\n", tmp);
> +                av_log(inlink->dst, AV_LOG_DEBUG, "stripped: %s\n", area->ass);
> +            }
> +            else
> +                area->ass = av_strdup("");

Unchecked allocation.

> +
> +            av_free(tmp);
> +        }
> +    }
> +
> +    return ff_filter_frame(outlink, frame);
> +}
> +
> +#define OFFSET(x) offsetof(StripStylesContext, x)
> +#define FLAGS (AV_OPT_FLAG_SUBTITLE_PARAM | AV_OPT_FLAG_FILTERING_PARAM)
> +
> +static const AVOption stripstyles_options[] = {
> +    { "remove_animated", "remove animated text (default: yes)",   OFFSET(remove_animated),  AV_OPT_TYPE_BOOL, {.i64 = 1 }, 0, 1, FLAGS, 0 },
> +    { NULL },
> +};
> +
> +AVFILTER_DEFINE_CLASS(stripstyles);
> +
> +static const AVFilterPad inputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_SUBTITLE,
> +        .filter_frame = filter_frame,
> +    },
> +};
> +
> +static const AVFilterPad outputs[] = {
> +    {
> +        .name          = "default",
> +        .type          = AVMEDIA_TYPE_SUBTITLE,
> +    },
> +};
> +
> +const AVFilter ff_sf_stripstyles = {
> +    .name          = "stripstyles",
> +    .description   = NULL_IF_CONFIG_SMALL("Strip subtitle inline styles"),
> +    .query_formats = query_formats,
> +    .priv_size     = sizeof(StripStylesContext),
> +    .priv_class    = &stripstyles_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:06
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> stripstyles filter
> 
> Soft Works:
> > - stripstyles {S -> S)
> >   Remove all inline styles from subtitle events
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  libavfilter/Makefile         |   1 +
> >  libavfilter/allfilters.c     |   1 +
> >  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 213 insertions(+)
> >  create mode 100644 libavfilter/sf_stripstyles.c
> >
> > diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> > index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
> >
> >  # multimedia filters
> >  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
> > diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> > index fc72858d18..e2e4deea22 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_stripstyles;
> >  extern const AVFilter ff_sf_textmod;
> >  extern const AVFilter ff_svf_graphicsub2video;
> >  extern const AVFilter ff_svf_textsub2video;
> > diff --git a/libavfilter/sf_stripstyles.c b/libavfilter/sf_stripstyles.c
> > new file mode 100644
> > index 0000000000..15c00e73b1
> > --- /dev/null
> > +++ b/libavfilter/sf_stripstyles.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * 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
> > + * text subtitle filter which removes inline-styles from subtitles
> > + */
> > +
> > +#include <libavcodec/ass.h>
> 
> This header and the API in it is not public.

I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
are accessing non-public symbols from each other?


> > +
> > +#include "libavutil/avassert.h"
> > +#include "libavutil/opt.h"
> > +#include "avfilter.h"
> > +#include "internal.h"
> > +#include "libavcodec/avcodec.h"
> > +#include "libavcodec/ass_split.h"
> > +
> > +typedef struct StripStylesContext {
> > +    const AVClass *class;
> > +    enum AVSubtitleType format;
> > +    int remove_animated;
> > +} StripStylesContext;
> > +
> > +typedef struct DialogContext {
> > +    const AVClass *class;
> 
> An AVClass is for options and logging (and would actually need an
> AVClass, which you didn't define); you are not using it for that, so
> just remove this.

No I can't, because this is being passed to the 
ff_ass_split_override_codes which expects an AVClass at the first 
position of the struct.



> > +    const StripStylesContext* ss_ctx;
> > +    AVBPrint buffer;
> > +    int drawing_scale;
> > +    int is_animated;
> > +} DialogContext;
> > +
> > +static void dialog_text_cb(void *priv, const char *text, int len)
> > +{
> > +    DialogContext *s = priv;
> > +    if (!s->drawing_scale && (!s->is_animated || !s->ss_ctx-
> >remove_animated))
> > +        av_bprint_append_data(&s->buffer, text, len);
> > +}
> > +
> > +static void dialog_new_line_cb(void *priv, int forced)
> > +{
> > +    DialogContext *s = priv;
> > +    if (!s->drawing_scale && !s->is_animated)
> > +        av_bprint_append_data(&s->buffer, forced ? "\\N" : "\\n", 2);
> > +}
> > +
> > +static void dialog_drawing_mode_cb(void *priv, int scale)
> > +{
> > +    DialogContext *s = priv;
> > +    s->drawing_scale = scale;
> > +}
> > +
> > +static void dialog_animate_cb(void *priv, int t1, int t2, int accel, char
> *style)
> > +{
> > +    DialogContext *s = priv;
> > +    s->is_animated = 1;
> > +}
> > +
> > +static void dialog_move_cb(void *priv, int x1, int y1, int x2, int y2, int
> t1, int t2)
> > +{
> > +    DialogContext *s = priv;
> > +    if (t1 >= 0 || t2 >= 0)
> > +        s->is_animated = 1;
> > +}
> > +
> > +static const ASSCodesCallbacks dialog_callbacks = {
> > +    .text             = dialog_text_cb,
> > +    .new_line         = dialog_new_line_cb,
> > +    .drawing_mode     = dialog_drawing_mode_cb,
> > +    .animate          = dialog_animate_cb,
> > +    .move             = dialog_move_cb,
> > +};
> > +
> > +static int query_formats(AVFilterContext *ctx)
> > +{
> > +    AVFilterFormats *formats;
> > +    AVFilterLink *inlink = ctx->inputs[0];
> > +    AVFilterLink *outlink = ctx->outputs[0];
> > +    static const enum AVSubtitleType subtitle_fmts[] = {
> AV_SUBTITLE_FMT_ASS, AV_SUBTITLE_FMT_NONE };
> > +    int ret;
> > +
> > +    /* set input subtitle format */
> > +    formats = ff_make_format_list(subtitle_fmts);
> > +    if ((ret = ff_formats_ref(formats, &inlink->outcfg.formats)) < 0)
> > +        return ret;
> > +
> > +    /* set output video format */
> > +    if ((ret = ff_formats_ref(formats, &outlink->incfg.formats)) < 0)
> > +        return ret;
> > +
> > +    return 0;
> > +}
> > +
> > +static char *ass_get_line(int readorder, int layer, const char *style,
> > +                        const char *speaker, const char *effect, const
> char *text)
> > +{
> > +    return av_asprintf("%d,%d,%s,%s,0,0,0,%s,%s",
> > +                       readorder, layer, style ? style : "Default",
> > +                       speaker ? speaker : "", effect, text);
> > +}
> > +
> > +static char *process_dialog(StripStylesContext *s, const char *ass_line)
> > +{
> > +    ASSDialog *dialog = ff_ass_split_dialog(NULL, ass_line);
> > +    char *result = NULL, *text;
> > +    DialogContext dlg_ctx = { 0 };
> > +
> > +    if (!dialog)
> > +        return NULL;
> > +
> > +    dlg_ctx.ss_ctx = s;
> > +
> > +    av_bprint_init(&dlg_ctx.buffer, 0, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
> >text);
> 
> This function is in libavcodec and is not public; it is not available
> when using shared linking. The same goes for all the other ff_ass_*
> functions. Generally, the ff-prefix denotes something that is library
> internal, but is not static, i.e. not confined to one translation unit.

I thought this could be ensured by having something like this in 
configure:

overlay_textsubs_filter_deps="avcodec libass"


If not, what would you suggest. Duplicate the ass code in avfilter,
move it to avutil and make it public, or any other idea?


> > +
> > +    av_bprint_finalize(&dlg_ctx.buffer, &text);
> > +
> > +    if (text && strlen(text) > 0)
> 
> The AVBPrint API actually keeps track of the length of the buffer for
> you: dlg_ctx.buffer.len. And anyway, checking whether a string has
> positive strlen can simply be done by "*text != '\0'" (or to "*text").

Even though I try to adapt to a given code style as much as possible, 
I generally prefer code that can be quickly read and understood, as long
as it's not inside a performance critical path where every single CPU 
op counts. The difference to your suggestion is small, but it still
takes an additional "brain op" when reading code :-)

> Notice that an AVBPrint can be truncated on allocation failure. This
> needs to be checked (and often isn't).
> 
> > +        result = ass_get_line(dialog->readorder, dialog->layer, dialog-
> >style, dialog->name, dialog->effect, text);
> > +
> > +    av_free(text);
> 
> This is not how the AVBPrint is supposed to be used; you are wasting the
> small string-optimization.

OK. Good.

> > +    ff_ass_free_dialog(&dialog);
> > +
> > +    return result;
> > +}
> > +
> > +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> > +{
> > +    StripStylesContext *s = inlink->dst->priv;
> > +    AVFilterLink *outlink = inlink->dst->outputs[0];
> > +
> > +    outlink->format = inlink->format;
> > +
> > +    av_frame_make_writable(frame);
> > +    if (!frame)
> > +        return AVERROR(ENOMEM);
> 
> This is complete nonsense: C uses call-by-value, so
> av_frame_make_writable just can't modify the pointer at all even if it
> wanted. Instead it returns a negative value on error.
> (And you are also leaking frame.)
> 
> > +
> > +    for (unsigned i = 0; i < frame->num_subtitle_areas; i++) {
> > +
> > +        AVSubtitleArea *area = frame->subtitle_areas[i];
> > +
> > +        if (area->ass) {
> > +            char *tmp = area->ass;
> > +            area->ass = process_dialog(s, area->ass);
> > +
> > +            if (area->ass) {
> > +                av_log(inlink->dst, AV_LOG_DEBUG, "original: %s\n", tmp);
> > +                av_log(inlink->dst, AV_LOG_DEBUG, "stripped: %s\n", area-
> >ass);
> > +            }
> > +            else
> > +                area->ass = av_strdup("");
> 
> Unchecked allocation.

NULL would still be an acceptable value for area->ass, so I didn't 
bother to check, but I can add that for keeping all aligned and consistent.

Thanks a lot for reviewing,
softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 03:06
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>> stripstyles filter
>>
>> Soft Works:
>>> - stripstyles {S -> S)
>>>   Remove all inline styles from subtitle events
>>>
>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>> ---
>>>  libavfilter/Makefile         |   1 +
>>>  libavfilter/allfilters.c     |   1 +
>>>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 213 insertions(+)
>>>  create mode 100644 libavfilter/sf_stripstyles.c
>>>
>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
>>>
>>>  # multimedia filters
>>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>>> index fc72858d18..e2e4deea22 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_stripstyles;
>>>  extern const AVFilter ff_sf_textmod;
>>>  extern const AVFilter ff_svf_graphicsub2video;
>>>  extern const AVFilter ff_svf_textsub2video;
>>> diff --git a/libavfilter/sf_stripstyles.c b/libavfilter/sf_stripstyles.c
>>> new file mode 100644
>>> index 0000000000..15c00e73b1
>>> --- /dev/null
>>> +++ b/libavfilter/sf_stripstyles.c
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * 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
>>> + * text subtitle filter which removes inline-styles from subtitles
>>> + */
>>> +
>>> +#include <libavcodec/ass.h>
>>
>> This header and the API in it is not public.
> 
> I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
> are accessing non-public symbols from each other?
> 

Yes, there are via avpriv functions. Yet this is strongly discouraged,
so the bar for new ones is pretty high.

> 
>>> +
>>> +#include "libavutil/avassert.h"
>>> +#include "libavutil/opt.h"
>>> +#include "avfilter.h"
>>> +#include "internal.h"
>>> +#include "libavcodec/avcodec.h"
>>> +#include "libavcodec/ass_split.h"
>>> +
>>> +typedef struct StripStylesContext {
>>> +    const AVClass *class;
>>> +    enum AVSubtitleType format;
>>> +    int remove_animated;
>>> +} StripStylesContext;
>>> +
>>> +typedef struct DialogContext {
>>> +    const AVClass *class;
>>
>> An AVClass is for options and logging (and would actually need an
>> AVClass, which you didn't define); you are not using it for that, so
>> just remove this.
> 
> No I can't, because this is being passed to the 
> ff_ass_split_override_codes which expects an AVClass at the first 
> position of the struct.
> 
> 

Why? The docs don't say so. And the opaque priv pointer is never used
for logging. (Or did I miss something?)

> 
>>> +    const StripStylesContext* ss_ctx;
>>> +    AVBPrint buffer;
>>> +    int drawing_scale;
>>> +    int is_animated;
>>> +} DialogContext;
>>> +
>>> +static void dialog_text_cb(void *priv, const char *text, int len)
>>> +{
>>> +    DialogContext *s = priv;
>>> +    if (!s->drawing_scale && (!s->is_animated || !s->ss_ctx-
>>> remove_animated))
>>> +        av_bprint_append_data(&s->buffer, text, len);
>>> +}
>>> +
>>> +static void dialog_new_line_cb(void *priv, int forced)
>>> +{
>>> +    DialogContext *s = priv;
>>> +    if (!s->drawing_scale && !s->is_animated)
>>> +        av_bprint_append_data(&s->buffer, forced ? "\\N" : "\\n", 2);
>>> +}
>>> +
>>> +static void dialog_drawing_mode_cb(void *priv, int scale)
>>> +{
>>> +    DialogContext *s = priv;
>>> +    s->drawing_scale = scale;
>>> +}
>>> +
>>> +static void dialog_animate_cb(void *priv, int t1, int t2, int accel, char
>> *style)
>>> +{
>>> +    DialogContext *s = priv;
>>> +    s->is_animated = 1;
>>> +}
>>> +
>>> +static void dialog_move_cb(void *priv, int x1, int y1, int x2, int y2, int
>> t1, int t2)
>>> +{
>>> +    DialogContext *s = priv;
>>> +    if (t1 >= 0 || t2 >= 0)
>>> +        s->is_animated = 1;
>>> +}
>>> +
>>> +static const ASSCodesCallbacks dialog_callbacks = {
>>> +    .text             = dialog_text_cb,
>>> +    .new_line         = dialog_new_line_cb,
>>> +    .drawing_mode     = dialog_drawing_mode_cb,
>>> +    .animate          = dialog_animate_cb,
>>> +    .move             = dialog_move_cb,
>>> +};
>>> +
>>> +static int query_formats(AVFilterContext *ctx)
>>> +{
>>> +    AVFilterFormats *formats;
>>> +    AVFilterLink *inlink = ctx->inputs[0];
>>> +    AVFilterLink *outlink = ctx->outputs[0];
>>> +    static const enum AVSubtitleType subtitle_fmts[] = {
>> AV_SUBTITLE_FMT_ASS, AV_SUBTITLE_FMT_NONE };
>>> +    int ret;
>>> +
>>> +    /* set input subtitle format */
>>> +    formats = ff_make_format_list(subtitle_fmts);
>>> +    if ((ret = ff_formats_ref(formats, &inlink->outcfg.formats)) < 0)
>>> +        return ret;
>>> +
>>> +    /* set output video format */
>>> +    if ((ret = ff_formats_ref(formats, &outlink->incfg.formats)) < 0)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static char *ass_get_line(int readorder, int layer, const char *style,
>>> +                        const char *speaker, const char *effect, const
>> char *text)
>>> +{
>>> +    return av_asprintf("%d,%d,%s,%s,0,0,0,%s,%s",
>>> +                       readorder, layer, style ? style : "Default",
>>> +                       speaker ? speaker : "", effect, text);
>>> +}
>>> +
>>> +static char *process_dialog(StripStylesContext *s, const char *ass_line)
>>> +{
>>> +    ASSDialog *dialog = ff_ass_split_dialog(NULL, ass_line);
>>> +    char *result = NULL, *text;
>>> +    DialogContext dlg_ctx = { 0 };
>>> +
>>> +    if (!dialog)
>>> +        return NULL;
>>> +
>>> +    dlg_ctx.ss_ctx = s;
>>> +
>>> +    av_bprint_init(&dlg_ctx.buffer, 0, AV_BPRINT_SIZE_UNLIMITED);
>>> +
>>> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
>>> text);
>>
>> This function is in libavcodec and is not public; it is not available
>> when using shared linking. The same goes for all the other ff_ass_*
>> functions. Generally, the ff-prefix denotes something that is library
>> internal, but is not static, i.e. not confined to one translation unit.
> 
> I thought this could be ensured by having something like this in 
> configure:
> 
> overlay_textsubs_filter_deps="avcodec libass"
> 

This won't work (as it won't make libavcodec export these functions).

> 
> If not, what would you suggest. Duplicate the ass code in avfilter,
> move it to avutil and make it public, or any other idea?
> 

Making it public? You know that your 10/13 would not be possible if this
were public? (And it would also not work if it were avpriv; at least not
without some versioning uglyness.)
I don't like the idea of duplicating the code. The only vague suggestion
that I have is that this might be more suitable for a bitstream filter.

> 
>>> +
>>> +    av_bprint_finalize(&dlg_ctx.buffer, &text);
>>> +
>>> +    if (text && strlen(text) > 0)
>>
>> The AVBPrint API actually keeps track of the length of the buffer for
>> you: dlg_ctx.buffer.len. And anyway, checking whether a string has
>> positive strlen can simply be done by "*text != '\0'" (or to "*text").
> 
> Even though I try to adapt to a given code style as much as possible, 
> I generally prefer code that can be quickly read and understood, as long
> as it's not inside a performance critical path where every single CPU 
> op counts. The difference to your suggestion is small, but it still
> takes an additional "brain op" when reading code :-)
> 
>> Notice that an AVBPrint can be truncated on allocation failure. This
>> needs to be checked (and often isn't).
>>
>>> +        result = ass_get_line(dialog->readorder, dialog->layer, dialog-
>>> style, dialog->name, dialog->effect, text);
>>> +
>>> +    av_free(text);
>>
>> This is not how the AVBPrint is supposed to be used; you are wasting the
>> small string-optimization.
> 
> OK. Good.
> 
>>> +    ff_ass_free_dialog(&dialog);
>>> +
>>> +    return result;
>>> +}
>>> +
>>> +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>>> +{
>>> +    StripStylesContext *s = inlink->dst->priv;
>>> +    AVFilterLink *outlink = inlink->dst->outputs[0];
>>> +
>>> +    outlink->format = inlink->format;
>>> +
>>> +    av_frame_make_writable(frame);
>>> +    if (!frame)
>>> +        return AVERROR(ENOMEM);
>>
>> This is complete nonsense: C uses call-by-value, so
>> av_frame_make_writable just can't modify the pointer at all even if it
>> wanted. Instead it returns a negative value on error.
>> (And you are also leaking frame.)
>>
>>> +
>>> +    for (unsigned i = 0; i < frame->num_subtitle_areas; i++) {
>>> +
>>> +        AVSubtitleArea *area = frame->subtitle_areas[i];
>>> +
>>> +        if (area->ass) {
>>> +            char *tmp = area->ass;
>>> +            area->ass = process_dialog(s, area->ass);
>>> +
>>> +            if (area->ass) {
>>> +                av_log(inlink->dst, AV_LOG_DEBUG, "original: %s\n", tmp);
>>> +                av_log(inlink->dst, AV_LOG_DEBUG, "stripped: %s\n", area-
>>> ass);
>>> +            }
>>> +            else
>>> +                area->ass = av_strdup("");
>>
>> Unchecked allocation.
> 
> NULL would still be an acceptable value for area->ass, so I didn't 
> bother to check, but I can add that for keeping all aligned and consistent.
> 
> Thanks a lot for reviewing,
> softworkz
> 
>

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 03:58
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> stripstyles filter
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 03:06
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >> stripstyles filter
> >>
> >> Soft Works:
> >>> - stripstyles {S -> S)
> >>>   Remove all inline styles from subtitle events
> >>>
> >>> Signed-off-by: softworkz <softworkz@hotmail.com>
> >>> ---
> >>>  libavfilter/Makefile         |   1 +
> >>>  libavfilter/allfilters.c     |   1 +
> >>>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 213 insertions(+)
> >>>  create mode 100644 libavfilter/sf_stripstyles.c
> >>>
> >>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> >>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
> >>>
> >>>  # multimedia filters
> >>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
> >>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> >>> index fc72858d18..e2e4deea22 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_stripstyles;
> >>>  extern const AVFilter ff_sf_textmod;
> >>>  extern const AVFilter ff_svf_graphicsub2video;
> >>>  extern const AVFilter ff_svf_textsub2video;
> >>> diff --git a/libavfilter/sf_stripstyles.c b/libavfilter/sf_stripstyles.c
> >>> new file mode 100644
> >>> index 0000000000..15c00e73b1
> >>> --- /dev/null
> >>> +++ b/libavfilter/sf_stripstyles.c
> >>> @@ -0,0 +1,211 @@
> >>> +/*
> >>> + * 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
> >>> + * text subtitle filter which removes inline-styles from subtitles
> >>> + */
> >>> +
> >>> +#include <libavcodec/ass.h>
> >>
> >> This header and the API in it is not public.
> >
> > I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
> > are accessing non-public symbols from each other?
> >
> 
> Yes, there are via avpriv functions. Yet this is strongly discouraged,
> so the bar for new ones is pretty high.
> 
> >
> >>> +
> >>> +#include "libavutil/avassert.h"
> >>> +#include "libavutil/opt.h"
> >>> +#include "avfilter.h"
> >>> +#include "internal.h"
> >>> +#include "libavcodec/avcodec.h"
> >>> +#include "libavcodec/ass_split.h"
> >>> +
> >>> +typedef struct StripStylesContext {
> >>> +    const AVClass *class;
> >>> +    enum AVSubtitleType format;
> >>> +    int remove_animated;
> >>> +} StripStylesContext;
> >>> +
> >>> +typedef struct DialogContext {
> >>> +    const AVClass *class;
> >>
> >> An AVClass is for options and logging (and would actually need an
> >> AVClass, which you didn't define); you are not using it for that, so
> >> just remove this.
> >
> > No I can't, because this is being passed to the
> > ff_ass_split_override_codes which expects an AVClass at the first
> > position of the struct.
> >
> >
> 
> Why? The docs don't say so. And the opaque priv pointer is never used
> for logging. (Or did I miss something?)

It crashed at some point, I don't remember where exactly, but the first
member of the struct got overwritten and invalidated. Then I realized that
it was assumed to have an AVClass.

> 
> >
> >>> +    const StripStylesContext* ss_ctx;
> >>> +    AVBPrint buffer;
> >>> +    int drawing_scale;
> >>> +    int is_animated;
> >>> +} DialogContext;
> >>> +
> >>> +static void dialog_text_cb(void *priv, const char *text, int len)
> >>> +{
> >>> +    DialogContext *s = priv;
> >>> +    if (!s->drawing_scale && (!s->is_animated || !s->ss_ctx-
> >>> remove_animated))
> >>> +        av_bprint_append_data(&s->buffer, text, len);
> >>> +}
> >>> +
> >>> +static void dialog_new_line_cb(void *priv, int forced)
> >>> +{
> >>> +    DialogContext *s = priv;
> >>> +    if (!s->drawing_scale && !s->is_animated)
> >>> +        av_bprint_append_data(&s->buffer, forced ? "\\N" : "\\n", 2);
> >>> +}
> >>> +
> >>> +static void dialog_drawing_mode_cb(void *priv, int scale)
> >>> +{
> >>> +    DialogContext *s = priv;
> >>> +    s->drawing_scale = scale;
> >>> +}
> >>> +
> >>> +static void dialog_animate_cb(void *priv, int t1, int t2, int accel,
> char
> >> *style)
> >>> +{
> >>> +    DialogContext *s = priv;
> >>> +    s->is_animated = 1;
> >>> +}
> >>> +
> >>> +static void dialog_move_cb(void *priv, int x1, int y1, int x2, int y2,
> int
> >> t1, int t2)
> >>> +{
> >>> +    DialogContext *s = priv;
> >>> +    if (t1 >= 0 || t2 >= 0)
> >>> +        s->is_animated = 1;
> >>> +}
> >>> +
> >>> +static const ASSCodesCallbacks dialog_callbacks = {
> >>> +    .text             = dialog_text_cb,
> >>> +    .new_line         = dialog_new_line_cb,
> >>> +    .drawing_mode     = dialog_drawing_mode_cb,
> >>> +    .animate          = dialog_animate_cb,
> >>> +    .move             = dialog_move_cb,
> >>> +};
> >>> +
> >>> +static int query_formats(AVFilterContext *ctx)
> >>> +{
> >>> +    AVFilterFormats *formats;
> >>> +    AVFilterLink *inlink = ctx->inputs[0];
> >>> +    AVFilterLink *outlink = ctx->outputs[0];
> >>> +    static const enum AVSubtitleType subtitle_fmts[] = {
> >> AV_SUBTITLE_FMT_ASS, AV_SUBTITLE_FMT_NONE };
> >>> +    int ret;
> >>> +
> >>> +    /* set input subtitle format */
> >>> +    formats = ff_make_format_list(subtitle_fmts);
> >>> +    if ((ret = ff_formats_ref(formats, &inlink->outcfg.formats)) < 0)
> >>> +        return ret;
> >>> +
> >>> +    /* set output video format */
> >>> +    if ((ret = ff_formats_ref(formats, &outlink->incfg.formats)) < 0)
> >>> +        return ret;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static char *ass_get_line(int readorder, int layer, const char *style,
> >>> +                        const char *speaker, const char *effect, const
> >> char *text)
> >>> +{
> >>> +    return av_asprintf("%d,%d,%s,%s,0,0,0,%s,%s",
> >>> +                       readorder, layer, style ? style : "Default",
> >>> +                       speaker ? speaker : "", effect, text);
> >>> +}
> >>> +
> >>> +static char *process_dialog(StripStylesContext *s, const char *ass_line)
> >>> +{
> >>> +    ASSDialog *dialog = ff_ass_split_dialog(NULL, ass_line);
> >>> +    char *result = NULL, *text;
> >>> +    DialogContext dlg_ctx = { 0 };
> >>> +
> >>> +    if (!dialog)
> >>> +        return NULL;
> >>> +
> >>> +    dlg_ctx.ss_ctx = s;
> >>> +
> >>> +    av_bprint_init(&dlg_ctx.buffer, 0, AV_BPRINT_SIZE_UNLIMITED);
> >>> +
> >>> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
> >>> text);
> >>
> >> This function is in libavcodec and is not public; it is not available
> >> when using shared linking. The same goes for all the other ff_ass_*
> >> functions. Generally, the ff-prefix denotes something that is library
> >> internal, but is not static, i.e. not confined to one translation unit.
> >
> > I thought this could be ensured by having something like this in
> > configure:
> >
> > overlay_textsubs_filter_deps="avcodec libass"
> >
> 
> This won't work (as it won't make libavcodec export these functions).
> 
> >
> > If not, what would you suggest. Duplicate the ass code in avfilter,
> > move it to avutil and make it public, or any other idea?
> >
> 
> Making it public? You know that your 10/13 would not be possible if this
> were public? (And it would also not work if it were avpriv; at least not
> without some versioning uglyness.)
> I don't like the idea of duplicating the code. The only vague suggestion
> that I have is that this might be more suitable for a bitstream filter.

The ass subtitle format is used as the internal text subtitle format, 
which means that those ass manipulation function are required to be 
accessible by many other text subtitle filter in some way.
I'm using them in more filters already..

Kind regards,
sw

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 03:58
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>> stripstyles filter
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>>>> Rheinhardt
>>>> Sent: Wednesday, 22 September 2021 03:06
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>>>> stripstyles filter
>>>>
>>>> Soft Works:
>>>>> - stripstyles {S -> S)
>>>>>   Remove all inline styles from subtitle events
>>>>>
>>>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>>>> ---
>>>>>  libavfilter/Makefile         |   1 +
>>>>>  libavfilter/allfilters.c     |   1 +
>>>>>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 213 insertions(+)
>>>>>  create mode 100644 libavfilter/sf_stripstyles.c
>>>>>
>>>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>>>>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
>>>>>
>>>>>  # multimedia filters
>>>>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
>>>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>>>>> index fc72858d18..e2e4deea22 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_stripstyles;
>>>>>  extern const AVFilter ff_sf_textmod;
>>>>>  extern const AVFilter ff_svf_graphicsub2video;
>>>>>  extern const AVFilter ff_svf_textsub2video;
>>>>> diff --git a/libavfilter/sf_stripstyles.c b/libavfilter/sf_stripstyles.c
>>>>> new file mode 100644
>>>>> index 0000000000..15c00e73b1
>>>>> --- /dev/null
>>>>> +++ b/libavfilter/sf_stripstyles.c
>>>>> @@ -0,0 +1,211 @@
>>>>> +/*
>>>>> + * 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
>>>>> + * text subtitle filter which removes inline-styles from subtitles
>>>>> + */
>>>>> +
>>>>> +#include <libavcodec/ass.h>
>>>>
>>>> This header and the API in it is not public.
>>>
>>> I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
>>> are accessing non-public symbols from each other?
>>>
>>
>> Yes, there are via avpriv functions. Yet this is strongly discouraged,
>> so the bar for new ones is pretty high.
>>
>>>
>>>>> +
>>>>> +#include "libavutil/avassert.h"
>>>>> +#include "libavutil/opt.h"
>>>>> +#include "avfilter.h"
>>>>> +#include "internal.h"
>>>>> +#include "libavcodec/avcodec.h"
>>>>> +#include "libavcodec/ass_split.h"
>>>>> +
>>>>> +typedef struct StripStylesContext {
>>>>> +    const AVClass *class;
>>>>> +    enum AVSubtitleType format;
>>>>> +    int remove_animated;
>>>>> +} StripStylesContext;
>>>>> +
>>>>> +typedef struct DialogContext {
>>>>> +    const AVClass *class;
>>>>
>>>> An AVClass is for options and logging (and would actually need an
>>>> AVClass, which you didn't define); you are not using it for that, so
>>>> just remove this.
>>>
>>> No I can't, because this is being passed to the
>>> ff_ass_split_override_codes which expects an AVClass at the first
>>> position of the struct.
>>>
>>>
>>
>> Why? The docs don't say so. And the opaque priv pointer is never used
>> for logging. (Or did I miss something?)
> 
> It crashed at some point, I don't remember where exactly, but the first
> member of the struct got overwritten and invalidated. Then I realized that
> it was assumed to have an AVClass.
> 

This surprises me. Are you sure that you did not forget the AVClass from
your private filter context (in this case, StripStylesContext)?

>>>>> +
>>>>> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
>>>>> text);
>>>>
>>>> This function is in libavcodec and is not public; it is not available
>>>> when using shared linking. The same goes for all the other ff_ass_*
>>>> functions. Generally, the ff-prefix denotes something that is library
>>>> internal, but is not static, i.e. not confined to one translation unit.
>>>
>>> I thought this could be ensured by having something like this in
>>> configure:
>>>
>>> overlay_textsubs_filter_deps="avcodec libass"
>>>
>>
>> This won't work (as it won't make libavcodec export these functions).
>>
>>>
>>> If not, what would you suggest. Duplicate the ass code in avfilter,
>>> move it to avutil and make it public, or any other idea?
>>>
>>
>> Making it public? You know that your 10/13 would not be possible if this
>> were public? (And it would also not work if it were avpriv; at least not
>> without some versioning uglyness.)
>> I don't like the idea of duplicating the code. The only vague suggestion
>> that I have is that this might be more suitable for a bitstream filter.
> 
> The ass subtitle format is used as the internal text subtitle format, 
> which means that those ass manipulation function are required to be 
> accessible by many other text subtitle filter in some way.
> I'm using them in more filters already..
> 

Then we have a problem. And I don't have a solution.

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 04:48
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> stripstyles filter
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 03:58
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >> stripstyles filter
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas
> >>>> Rheinhardt
> >>>> Sent: Wednesday, 22 September 2021 03:06
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >>>> stripstyles filter
> >>>>
> >>>> Soft Works:
> >>>>> - stripstyles {S -> S)
> >>>>>   Remove all inline styles from subtitle events
> >>>>>
> >>>>> Signed-off-by: softworkz <softworkz@hotmail.com>
> >>>>> ---
> >>>>>  libavfilter/Makefile         |   1 +
> >>>>>  libavfilter/allfilters.c     |   1 +
> >>>>>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
> >>>>>  3 files changed, 213 insertions(+)
> >>>>>  create mode 100644 libavfilter/sf_stripstyles.c
> >>>>>
> >>>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> >>>>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
> >>>>>
> >>>>>  # multimedia filters
> >>>>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
> >>>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> >>>>> index fc72858d18..e2e4deea22 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_stripstyles;
> >>>>>  extern const AVFilter ff_sf_textmod;
> >>>>>  extern const AVFilter ff_svf_graphicsub2video;
> >>>>>  extern const AVFilter ff_svf_textsub2video;
> >>>>> diff --git a/libavfilter/sf_stripstyles.c
> b/libavfilter/sf_stripstyles.c
> >>>>> new file mode 100644
> >>>>> index 0000000000..15c00e73b1
> >>>>> --- /dev/null
> >>>>> +++ b/libavfilter/sf_stripstyles.c
> >>>>> @@ -0,0 +1,211 @@
> >>>>> +/*
> >>>>> + * 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
> >>>>> + * text subtitle filter which removes inline-styles from subtitles
> >>>>> + */
> >>>>> +
> >>>>> +#include <libavcodec/ass.h>
> >>>>
> >>>> This header and the API in it is not public.
> >>>
> >>> I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
> >>> are accessing non-public symbols from each other?
> >>>
> >>
> >> Yes, there are via avpriv functions. Yet this is strongly discouraged,
> >> so the bar for new ones is pretty high.
> >>
> >>>
> >>>>> +
> >>>>> +#include "libavutil/avassert.h"
> >>>>> +#include "libavutil/opt.h"
> >>>>> +#include "avfilter.h"
> >>>>> +#include "internal.h"
> >>>>> +#include "libavcodec/avcodec.h"
> >>>>> +#include "libavcodec/ass_split.h"
> >>>>> +
> >>>>> +typedef struct StripStylesContext {
> >>>>> +    const AVClass *class;
> >>>>> +    enum AVSubtitleType format;
> >>>>> +    int remove_animated;
> >>>>> +} StripStylesContext;
> >>>>> +
> >>>>> +typedef struct DialogContext {
> >>>>> +    const AVClass *class;
> >>>>
> >>>> An AVClass is for options and logging (and would actually need an
> >>>> AVClass, which you didn't define); you are not using it for that, so
> >>>> just remove this.
> >>>
> >>> No I can't, because this is being passed to the
> >>> ff_ass_split_override_codes which expects an AVClass at the first
> >>> position of the struct.
> >>>
> >>>
> >>
> >> Why? The docs don't say so. And the opaque priv pointer is never used
> >> for logging. (Or did I miss something?)
> >
> > It crashed at some point, I don't remember where exactly, but the first
> > member of the struct got overwritten and invalidated. Then I realized that
> > it was assumed to have an AVClass.
> >
> 
> This surprises me. Are you sure that you did not forget the AVClass from
> your private filter context (in this case, StripStylesContext)?

That's something that happened as well, but at an earlier time and
with a different filter..

I think I had some logging in the callback functions (ASSCodesCallbacks)
before, probably that's how it happened to crash.

> 
> >>>>> +
> >>>>> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
> >>>>> text);
> >>>>
> >>>> This function is in libavcodec and is not public; it is not available
> >>>> when using shared linking. The same goes for all the other ff_ass_*
> >>>> functions. Generally, the ff-prefix denotes something that is library
> >>>> internal, but is not static, i.e. not confined to one translation unit.
> >>>
> >>> I thought this could be ensured by having something like this in
> >>> configure:
> >>>
> >>> overlay_textsubs_filter_deps="avcodec libass"
> >>>
> >>
> >> This won't work (as it won't make libavcodec export these functions).
> >>
> >>>
> >>> If not, what would you suggest. Duplicate the ass code in avfilter,
> >>> move it to avutil and make it public, or any other idea?
> >>>
> >>
> >> Making it public? You know that your 10/13 would not be possible if this
> >> were public? (And it would also not work if it were avpriv; at least not
> >> without some versioning uglyness.)
> >> I don't like the idea of duplicating the code. The only vague suggestion
> >> that I have is that this might be more suitable for a bitstream filter.
> >
> > The ass subtitle format is used as the internal text subtitle format,
> > which means that those ass manipulation function are required to be
> > accessible by many other text subtitle filter in some way.
> > I'm using them in more filters already..
> >
> 
> Then we have a problem. And I don't have a solution.

Function prefixes per preprocessor defs to compile the same two source files
into both libs?

Moving into avutil would appear to make more sense to me. Anyway, it's exactly 
that: utility functions. (it's not the ass encoder or decoder)

softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Wednesday, 22 September 2021 04:48
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>> stripstyles filter
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
>>>> Rheinhardt
>>>> Sent: Wednesday, 22 September 2021 03:58
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>>>> stripstyles filter
>>>>
>>>> Soft Works:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas
>>>>>> Rheinhardt
>>>>>> Sent: Wednesday, 22 September 2021 03:06
>>>>>> To: ffmpeg-devel@ffmpeg.org
>>>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
>>>>>> stripstyles filter
>>>>>>
>>>>>> Soft Works:
>>>>>>> - stripstyles {S -> S)
>>>>>>>   Remove all inline styles from subtitle events
>>>>>>>
>>>>>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>>>>>> ---
>>>>>>>  libavfilter/Makefile         |   1 +
>>>>>>>  libavfilter/allfilters.c     |   1 +
>>>>>>>  libavfilter/sf_stripstyles.c | 211 +++++++++++++++++++++++++++++++++++
>>>>>>>  3 files changed, 213 insertions(+)
>>>>>>>  create mode 100644 libavfilter/sf_stripstyles.c
>>>>>>>
>>>>>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>>>>>>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
>>>>>>>
>>>>>>>  # multimedia filters
>>>>>>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
>>>>>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>>>>>>> index fc72858d18..e2e4deea22 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_stripstyles;
>>>>>>>  extern const AVFilter ff_sf_textmod;
>>>>>>>  extern const AVFilter ff_svf_graphicsub2video;
>>>>>>>  extern const AVFilter ff_svf_textsub2video;
>>>>>>> diff --git a/libavfilter/sf_stripstyles.c
>> b/libavfilter/sf_stripstyles.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..15c00e73b1
>>>>>>> --- /dev/null
>>>>>>> +++ b/libavfilter/sf_stripstyles.c
>>>>>>> @@ -0,0 +1,211 @@
>>>>>>> +/*
>>>>>>> + * 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
>>>>>>> + * text subtitle filter which removes inline-styles from subtitles
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <libavcodec/ass.h>
>>>>>>
>>>>>> This header and the API in it is not public.
>>>>>
>>>>> I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
>>>>> are accessing non-public symbols from each other?
>>>>>
>>>>
>>>> Yes, there are via avpriv functions. Yet this is strongly discouraged,
>>>> so the bar for new ones is pretty high.
>>>>
>>>>>
>>>>>>> +
>>>>>>> +#include "libavutil/avassert.h"
>>>>>>> +#include "libavutil/opt.h"
>>>>>>> +#include "avfilter.h"
>>>>>>> +#include "internal.h"
>>>>>>> +#include "libavcodec/avcodec.h"
>>>>>>> +#include "libavcodec/ass_split.h"
>>>>>>> +
>>>>>>> +typedef struct StripStylesContext {
>>>>>>> +    const AVClass *class;
>>>>>>> +    enum AVSubtitleType format;
>>>>>>> +    int remove_animated;
>>>>>>> +} StripStylesContext;
>>>>>>> +
>>>>>>> +typedef struct DialogContext {
>>>>>>> +    const AVClass *class;
>>>>>>
>>>>>> An AVClass is for options and logging (and would actually need an
>>>>>> AVClass, which you didn't define); you are not using it for that, so
>>>>>> just remove this.
>>>>>
>>>>> No I can't, because this is being passed to the
>>>>> ff_ass_split_override_codes which expects an AVClass at the first
>>>>> position of the struct.
>>>>>
>>>>>
>>>>
>>>> Why? The docs don't say so. And the opaque priv pointer is never used
>>>> for logging. (Or did I miss something?)
>>>
>>> It crashed at some point, I don't remember where exactly, but the first
>>> member of the struct got overwritten and invalidated. Then I realized that
>>> it was assumed to have an AVClass.
>>>
>>
>> This surprises me. Are you sure that you did not forget the AVClass from
>> your private filter context (in this case, StripStylesContext)?
> 
> That's something that happened as well, but at an earlier time and
> with a different filter..
> 
> I think I had some logging in the callback functions (ASSCodesCallbacks)
> before, probably that's how it happened to crash.
> 

That would make sense. If you want to have logging in these callbacks,
you should probably move DialogContext into StripStylesContext and use
the latter as opaque priv pointer.

>>
>>>>>>> +
>>>>>>> +    ff_ass_split_override_codes(&dialog_callbacks, &dlg_ctx, dialog-
>>>>>>> text);
>>>>>>
>>>>>> This function is in libavcodec and is not public; it is not available
>>>>>> when using shared linking. The same goes for all the other ff_ass_*
>>>>>> functions. Generally, the ff-prefix denotes something that is library
>>>>>> internal, but is not static, i.e. not confined to one translation unit.
>>>>>
>>>>> I thought this could be ensured by having something like this in
>>>>> configure:
>>>>>
>>>>> overlay_textsubs_filter_deps="avcodec libass"
>>>>>
>>>>
>>>> This won't work (as it won't make libavcodec export these functions).
>>>>
>>>>>
>>>>> If not, what would you suggest. Duplicate the ass code in avfilter,
>>>>> move it to avutil and make it public, or any other idea?
>>>>>
>>>>
>>>> Making it public? You know that your 10/13 would not be possible if this
>>>> were public? (And it would also not work if it were avpriv; at least not
>>>> without some versioning uglyness.)
>>>> I don't like the idea of duplicating the code. The only vague suggestion
>>>> that I have is that this might be more suitable for a bitstream filter.
>>>
>>> The ass subtitle format is used as the internal text subtitle format,
>>> which means that those ass manipulation function are required to be
>>> accessible by many other text subtitle filter in some way.
>>> I'm using them in more filters already..
>>>
>>
>> Then we have a problem. And I don't have a solution.
> 
> Function prefixes per preprocessor defs to compile the same two source files
> into both libs?
> 

If we opt for duplicating, then it could be made in a way that it is
only duplicated for shared builds; see
https://github.com/mkver/FFmpeg/commits/avpriv (I finally need to get
this patchset updated and sent before the next release while the ABI is
still open.)

> Moving into avutil would appear to make more sense to me. Anyway, it's exactly 
> that: utility functions. (it's not the ass encoder or decoder)
> 

That has the compatibility problem I mentioned. Which is the reason why
avpriv symbols are disliked.
(Some devs (like Nicholas George) want to merge all the libraries or at
least disallow using libraries from different commits together (I like
the latter approach). This would automatically solve the compatibility
issue.)

- Andreas

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 05:28
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> stripstyles filter
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 04:48
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >> stripstyles filter
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas
> >>>> Rheinhardt
> >>>> Sent: Wednesday, 22 September 2021 03:58
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >>>> stripstyles filter
> >>>>
> >>>> Soft Works:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas
> >>>>>> Rheinhardt
> >>>>>> Sent: Wednesday, 22 September 2021 03:06
> >>>>>> To: ffmpeg-devel@ffmpeg.org
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> >>>>>> stripstyles filter
> >>>>>>
> >>>>>> Soft Works:
> >>>>>>> - stripstyles {S -> S)
> >>>>>>>   Remove all inline styles from subtitle events
> >>>>>>>
> >>>>>>> Signed-off-by: softworkz <softworkz@hotmail.com>
> >>>>>>> ---
> >>>>>>>  libavfilter/Makefile         |   1 +
> >>>>>>>  libavfilter/allfilters.c     |   1 +
> >>>>>>>  libavfilter/sf_stripstyles.c | 211
> +++++++++++++++++++++++++++++++++++
> >>>>>>>  3 files changed, 213 insertions(+)
> >>>>>>>  create mode 100644 libavfilter/sf_stripstyles.c
> >>>>>>>
> >>>>>>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> >>>>>>> index e6fef97c08..309c404bf7 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_STRIPSTYLES_FILTER)            += sf_stripstyles.o
> >>>>>>>
> >>>>>>>  # multimedia filters
> >>>>>>>  OBJS-$(CONFIG_ABITSCOPE_FILTER)              += avf_abitscope.o
> >>>>>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> >>>>>>> index fc72858d18..e2e4deea22 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_stripstyles;
> >>>>>>>  extern const AVFilter ff_sf_textmod;
> >>>>>>>  extern const AVFilter ff_svf_graphicsub2video;
> >>>>>>>  extern const AVFilter ff_svf_textsub2video;
> >>>>>>> diff --git a/libavfilter/sf_stripstyles.c
> >> b/libavfilter/sf_stripstyles.c
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000000..15c00e73b1
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/libavfilter/sf_stripstyles.c
> >>>>>>> @@ -0,0 +1,211 @@
> >>>>>>> +/*
> >>>>>>> + * 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
> >>>>>>> + * text subtitle filter which removes inline-styles from subtitles
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +#include <libavcodec/ass.h>
> >>>>>>
> >>>>>> This header and the API in it is not public.
> >>>>>
> >>>>> I'm not sure IIRC, but aren't there other cases where the ffmpeg libs
> >>>>> are accessing non-public symbols from each other?
> >>>>>
> >>>>
> >>>> Yes, there are via avpriv functions. Yet this is strongly discouraged,
> >>>> so the bar for new ones is pretty high.
> >>>>
> >>>>>
> >>>>>>> +
> >>>>>>> +#include "libavutil/avassert.h"
> >>>>>>> +#include "libavutil/opt.h"
> >>>>>>> +#include "avfilter.h"
> >>>>>>> +#include "internal.h"
> >>>>>>> +#include "libavcodec/avcodec.h"
> >>>>>>> +#include "libavcodec/ass_split.h"
> >>>>>>> +
> >>>>>>> +typedef struct StripStylesContext {
> >>>>>>> +    const AVClass *class;
> >>>>>>> +    enum AVSubtitleType format;
> >>>>>>> +    int remove_animated;
> >>>>>>> +} StripStylesContext;
> >>>>>>> +
> >>>>>>> +typedef struct DialogContext {
> >>>>>>> +    const AVClass *class;
> >>>>>>
> >>>>>> An AVClass is for options and logging (and would actually need an
> >>>>>> AVClass, which you didn't define); you are not using it for that, so
> >>>>>> just remove this.
> >>>>>
> >>>>> No I can't, because this is being passed to the
> >>>>> ff_ass_split_override_codes which expects an AVClass at the first
> >>>>> position of the struct.
> >>>>>
> >>>>>
> >>>>
> >>>> Why? The docs don't say so. And the opaque priv pointer is never used
> >>>> for logging. (Or did I miss something?)
> >>>
> >>> It crashed at some point, I don't remember where exactly, but the first
> >>> member of the struct got overwritten and invalidated. Then I realized
> that
> >>> it was assumed to have an AVClass.
> >>>
> >>
> >> This surprises me. Are you sure that you did not forget the AVClass from
> >> your private filter context (in this case, StripStylesContext)?
> >
> > That's something that happened as well, but at an earlier time and
> > with a different filter..
> >
> > I think I had some logging in the callback functions (ASSCodesCallbacks)
> > before, probably that's how it happened to crash.
> >
> 
> That would make sense. If you want to have logging in these callbacks,
> you should probably move DialogContext into StripStylesContext and use
> the latter as opaque priv pointer.

StripStylesContext is the context of the filter and DialogContext has 
a lifetime of a single execution of process_dialog()..

Sounds weird to me, probably because it's somewhat difficult for me 
to switch off object-oriented thinking ;-)


> >>>>> If not, what would you suggest. Duplicate the ass code in avfilter,
> >>>>> move it to avutil and make it public, or any other idea?
> >>>>>
> >>>>
> >>>> Making it public? You know that your 10/13 would not be possible if this
> >>>> were public? (And it would also not work if it were avpriv; at least not
> >>>> without some versioning uglyness.)
> >>>> I don't like the idea of duplicating the code. The only vague suggestion
> >>>> that I have is that this might be more suitable for a bitstream filter.
> >>>
> >>> The ass subtitle format is used as the internal text subtitle format,
> >>> which means that those ass manipulation function are required to be
> >>> accessible by many other text subtitle filter in some way.
> >>> I'm using them in more filters already..
> >>>
> >>
> >> Then we have a problem. And I don't have a solution.
> >
> > Function prefixes per preprocessor defs to compile the same two source
> files
> > into both libs?
> >
> 
> If we opt for duplicating, then it could be made in a way that it is
> only duplicated for shared builds; see
> https://github.com/mkver/FFmpeg/commits/avpriv (I finally need to get
> this patchset updated and sent before the next release while the ABI is
> still open.)
> 
> > Moving into avutil would appear to make more sense to me. Anyway, it's
> exactly
> > that: utility functions. (it's not the ass encoder or decoder)
> >
> 
> That has the compatibility problem I mentioned. Which is the reason why
> avpriv symbols are disliked.
> (Some devs (like Nicholas George) want to merge all the libraries or at
> least disallow using libraries from different commits together (I like
> the latter approach). This would automatically solve the compatibility
> issue.)

That's just one of the many things I don't understand - why there's a need
to allow different versions to interoperate, given that all of them are
having the same origin. Having one version for all libs that needs to match
seems to me just natural - even mandatory, but that's a discussion I 
don't want to get involved. 

Ok, while these things are pending, I suppose as a temporary solution
I'd just duplicate that code until a final solution can be found?

softworkz

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 05:28
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add
> stripstyles filter

> >>> It crashed at some point, I don't remember where exactly, but the first
> >>> member of the struct got overwritten and invalidated. Then I realized
> that
> >>> it was assumed to have an AVClass.
> >>>
> >>
> >> This surprises me. Are you sure that you did not forget the AVClass from
> >> your private filter context (in this case, StripStylesContext)?
> >
> > That's something that happened as well, but at an earlier time and
> > with a different filter..
> >
> > I think I had some logging in the callback functions (ASSCodesCallbacks)
> > before, probably that's how it happened to crash.
> >
> 
> That would make sense. If you want to have logging in these callbacks,
> you should probably move DialogContext into StripStylesContext and use
> the latter as opaque priv pointer.

I did it the other way round now. As StripStylesContext is already a member
of DialogContext, I'm just passing this one as the context for logging

typedef struct DialogContext {
    StripStylesContext* ss_ctx;
    AVBPrint buffer;
    int drawing_scale;
    int is_animated;
} DialogContext;

static void dialog_text_cb(void *priv, const char *text, int len)
{
    DialogContext *s = priv;

    av_log(s->ss_ctx, AV_LOG_DEBUG, "dialog_text_cb: %s\n", text);

----

The AVClass is removed and all should be fine that way.


> >>>>> If not, what would you suggest. Duplicate the ass code in avfilter,
> >>>>> move it to avutil and make it public, or any other idea?
> >>>>>
> >>>>
> >>>> Making it public? You know that your 10/13 would not be possible if this
> >>>> were public? (And it would also not work if it were avpriv; at least not
> >>>> without some versioning uglyness.)
> >>>> I don't like the idea of duplicating the code. The only vague suggestion
> >>>> that I have is that this might be more suitable for a bitstream filter.
> >>>
> >>> The ass subtitle format is used as the internal text subtitle format,
> >>> which means that those ass manipulation function are required to be
> >>> accessible by many other text subtitle filter in some way.
> >>> I'm using them in more filters already..
> >>>
> >>
> >> Then we have a problem. And I don't have a solution.
> >
> > Function prefixes per preprocessor defs to compile the same two source
> files
> > into both libs?
> >
> 
> If we opt for duplicating, then it could be made in a way that it is
> only duplicated for shared builds; see
> https://github.com/mkver/FFmpeg/commits/avpriv (I finally need to get
> this patchset updated and sent before the next release while the ABI is
> still open.)
> 
> > Moving into avutil would appear to make more sense to me. Anyway, it's
> exactly
> > that: utility functions. (it's not the ass encoder or decoder)
> >
> 
> That has the compatibility problem I mentioned.

Yes, you said: 

> >>>> Making it public? You know that your 10/13 would not be possible if this
> >>>> were public? 

That is clearly true, but this is not something that will happen again,
soon. I have looked at the history of the exports of ass.h and ass_split.h
and there haven't been changes for years to the definitions and just one 
function added.

After re-thinking and looking at all available options, my conclusion is
that the best option would be to put it into avutil and make it public. 
(an ABI break is pending anyway).

avutil contains a number of utility functions for audio and video, so
it seems to be the right place for having a few utility functions for 
subtitles that are commonly used from multiple libraries.

If there aren't any objections (please let me know), I would incorporate 
that change into the subtitle patchset.

Thanks,
softworkz