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

[Andreas Rheinhardt]

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?
> 

There is documentation for ff_mutex_destroy? Anyway, we typically use
the pthread naming throughout the codebase. And we have started checking
initialization of mutexes. And destroying a mutex that has not been
successfully initialized results in undefined behaviour. See
https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/284673.html for
an example.
(Actually, we should remove the asserts for mutex initialization in
libavutil/thread.h when all of them are checked.)

>>> +
>>> +    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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>