[FFmpeg-devel] [PATCH v8 11/13] avfilter/stripstyles: Add stripstyles filter
Soft Works
softworkz at hotmail.com
Wed Sep 22 04:43:17 EEST 2021
> -----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?
> > +
> > +#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
More information about the ffmpeg-devel
mailing list