Soft Works:
>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Friday, November 26, 2021 2:16 PM
>> To: ffmpeg-devel@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
