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

Andreas Rheinhardt andreas.rheinhardt at outlook.com
Wed Sep 22 04:58:09 EEST 2021


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


More information about the ffmpeg-devel mailing list