[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 05:28
> To: ffmpeg-devel at 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