[FFmpeg-devel] [PATCH v16 09/16] avfilter/overlaytextsubs: Add overlaytextsubs and textsubs2video filters

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Friday, November 26, 2021 2:16 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v16 09/16] avfilter/overlaytextsubs: Add
> overlaytextsubs and textsubs2video filters
> 
> Changes to the framework and modifying/adding a filter should not be
> done in the same commit.

OK, I've split this part.

> > +
> > +static av_cold void uninit(AVFilterContext *ctx)
> > +{
> > +    TextSubsContext *s = ctx->priv;
> > +
> > +    if (s->track)
> > +        ass_free_track(s->track);
> > +    if (s->renderer)
> > +        ass_renderer_done(s->renderer);
> > +    if (s->library)
> > +        ass_library_done(s->library);
> > +
> > +    s->track = NULL;
> > +    s->renderer = NULL;
> > +    s->library = NULL;
> > +
> > +    ff_mutex_destroy(&s->mutex);
> 
> You are destroying this mutex even if it has never been successfully
> initialized.
> 

I had looked at all other uses of ff_mutex_* and none of them did an initialization check
nor any check before calling destroy. As far as I've understood the docs, ff_mutex_destroy()
would simply return an error code when the supplied mutex is not valid. Shouldn't that 
be ok, then?

> > +
> > +    ff_mutex_init(&s->mutex, NULL);
> 
> Unchecked initialization.

Added check.


> > +    for (unsigned i = 0; i < sub->num_subtitle_areas; i++) {
> > +        char *ass_line = sub->subtitle_areas[i]->ass;
> > +        if (!ass_line)
> > +            break;
> > +        ff_mutex_lock(&s->mutex);
> > +        ass_process_chunk(s->track, ass_line, strlen(ass_line),
> start_time, duration);
> > +        ff_mutex_unlock(&s->mutex);
> 
> Using this mutex for this filter seems unnecessary: There is no other
> input whose filter_frame function could run concurrently.

Right. Removed.


Thanks again,
softworkz