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

[Andreas Rheinhardt]

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