[FFmpeg-devel] [PATCH v5 10/12] avfilter/textmod: Add textmod filter
Soft Works
softworkz at hotmail.com
Mon Sep 13 01:34:08 EEST 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Sunday, 12 September 2021 23:56
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v5 10/12] avfilter/textmod: Add
> textmod filter
>
> Soft Works:
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> > doc/filters.texi | 64 +++++++
> > libavfilter/Makefile | 3 +
> > libavfilter/allfilters.c | 1 +
> > libavfilter/sf_textmod.c | 381
> +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 449 insertions(+)
> > create mode 100644 libavfilter/sf_textmod.c
> >
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 1d76461ada..9fd2876d63 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -25024,6 +25024,70 @@ existing filters using @code{--disable-
> filters}.
[...]
> > +static void uninit(AVFilterContext *ctx)
> > +{
> > + TextModContext *s = ctx->priv;
> > + int i;
> > +
> > + for (i = 0; i < s->nb_find_list; i++) {
> > + av_free(&s->find_list[i]);
>
> This is completely wrong and will crash: You either want to do
> av_freep(&s->find_list[i]) or av_free(s->find_list[i]) if these
> strings
> were independently allocated; but looking at split_string() shows
> that
> they are not, they are substrings of s->find. Similar for the loop
> below.
You are right of course, thanks.
> > + }
> > + s->nb_find_list = 0;
> > + av_freep(&s->find_list);
> > +
> > + for (i = 0; i < s->nb_replace_list; i++) {
> > + av_free(&s->replace_list[i]);
> > + }
> > + s->nb_replace_list = 0;
> > + av_freep(&s->replace_list);
> > +}
> > +
> > +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 *process_text(TextModContext *s, char *text)
> > +{
> > + const char *char_src = s->find;
> > + const char *char_dst = s->replace;
> > + char *result = NULL;
> > + int escape_level = 0, k = 0;
> > +
> > + switch (s->operation) {
> > + case OP_LEET:
> > + case OP_REPLACE_CHARS:
> > +
> > + if (s->operation == OP_LEET) {
> > + char_src = leet_src;
> > + char_dst = leet_dst;
> > + }
> > +
> > + result = av_strdup(text);
> > + if (!result)
> > + return NULL;
> > +
> > + for (size_t n = 0; n < strlen(result); n++) {
> > + if (result[n] == '{')
> > + escape_level++;
> > +
> > + if (!escape_level) {
> > + for (size_t t = 0; t < FF_ARRAY_ELEMS(char_src);
> t++) {
> > + if (result[n] == char_src[t]) {
> > + result[n] = char_dst[t];
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (result[n] == '}')
> > + escape_level--;
> > + }
> > +
> > + break;
> > + case OP_TO_UPPER:
> > + case OP_TO_LOWER:
> > +
> > + result = av_strdup(text);
> > + if (!result)
> > + return NULL;
> > +
> > + for (size_t n = 0; n < strlen(result); n++) {
> > + if (result[n] == '{')
> > + escape_level++;
> > + if (!escape_level)
> > + result[n] = s->operation == OP_TO_LOWER ?
> av_tolower(result[n]) : av_toupper(result[n]);
> > + if (result[n] == '}')
> > + escape_level--;
> > + }
> > +
> > + break;
> > + case OP_REMOVE_CHARS:
> > +
> > + result = av_strdup(text);
> > + if (!result)
> > + return NULL;
> > +
> > + for (size_t n = 0; n < strlen(result); n++) {
> > + int skip_char = 0;
> > +
> > + if (result[n] == '{')
> > + escape_level++;
> > +
> > + if (!escape_level) {
> > + for (size_t t = 0; t < FF_ARRAY_ELEMS(char_src);
> t++) {
> > + if (result[n] == char_src[t]) {
> > + skip_char = 1;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (!skip_char)
> > + result[k++] = result[n];
> > +
> > + if (result[n] == '}')
> > + escape_level--;
> > + }
> > +
> > + result[k] = 0;
> > +
> > + break;
> > + case OP_REPLACE_WORDS:
> > + case OP_REMOVE_WORDS:
> > +
> > + result = av_strdup(text);
> > + if (!result)
> > + return NULL;
> > +
> > + for (int n = 0; n < s->nb_find_list; n++) {
> > + char *tmp = result;
> > + const char *replace = (s->operation ==
> OP_REPLACE_WORDS) ? s->replace_list[n] : "";
> > +
> > + result = av_strireplace(result, s->find_list[n],
> replace);
> > + if (!result)
> > + return NULL;
> > +
> > + av_free(tmp);
> > + }
> > +
> > + break;
> > + }
> > +
> > + return result;
> > +}
> > +
> > +static char *process_dialog(TextModContext *s, char *ass_line)
> > +{
> > + ASSDialog *dialog = ff_ass_split_dialog(NULL, ass_line);
> > + char *result, *text;
> > +
> > + if (!dialog)
> > + return NULL;
> > +
> > + text = process_text(s, dialog->text);
> > + if (!text)
> > + return NULL;
> > +
> > + result = ff_ass_get_dialog(dialog->readorder, dialog->layer,
> dialog->style, dialog->name, text);
> > +
> > + av_free(text);
> > + ff_ass_free_dialog(&dialog);
> > + return result;
> > +}
> > +
> > +static int filter_frame(AVFilterLink *inlink, AVFrame *src_frame)
> > +{
> > + TextModContext *s = inlink->dst->priv;
> > + AVFilterLink *outlink = inlink->dst->outputs[0];
> > + int ret;
> > + AVFrame *out;
> > +
> > + outlink->format = inlink->format;
> > +
> > + out = av_frame_clone(src_frame);
>
> Why clone? You can just reuse src_frame as is.
[..]
>
> You may not be the sole owner of this AVSubtitleRect; after all,
> they are shared. Ergo you must not modify it. Is it possible that you
> believed that av_frame_clone() would make the frame writable? It does
> not. For non-subtitle frames, av_frame_make_writable() makes them
> writable; but it does not for subtitles, because you made
> av_frame_get_buffer2() a no-op for subtitles and so
> av_frame_make_writable() will temporarily increment the refcount and
> then decrement it again.
One unsolved problem I have about dealing with AVSubtitleRect
as being part of AVFrame is that it's not possible to make a copy,
in a reliable way because the allocated sizes of the data[4] pointers
are not reliably known.
Usually, data[0] is the image and data[1] is the palette, but will
it always be like this? Then better not have a data array but
named pointer variables instead.
I was not sure what will be the general route: Merging AVSubtitle
into AVFrame or attaching AVSubtitle as a property to AVFrame.
BTW - what is your opinion about that question?
Also, thank you very much for your detailed reviews of my patches!
Kind regards,
softworkz
More information about the ffmpeg-devel
mailing list