[FFmpeg-devel] [PATCH v3 2/3] lavf/dashdec: Multithreaded DASH initialization

[Andreas Rheinhardt]

Lukas Fellechner:
>> Gesendet: Montag, 05. September 2022 um 00:50 Uhr
>> Von: "Andreas Rheinhardt" <andreas.rheinhardt at outlook.com>
>> An: ffmpeg-devel at ffmpeg.org
>> Betreff: Re: [FFmpeg-devel] [PATCH v3 2/3] lavf/dashdec: Multithreaded DASH initialization
>> Lukas Fellechner:
>>> Andreas Rheinhardt andreas.rheinhardt at outlook.com
>>> Wed Aug 31 05:54:12 EEST 2022
>>>>
>>>
>>>> 1. We actually have an API to process multiple tasks by different
>>>> threads: Look at libavutil/slicethread.h. Why can't you reuse that?
>>>> 2. In case initialization of one of the conditions/mutexes fails, you
>>>> are nevertheless destroying them; you are even destroying completely
>>>> uninitialized mutexes. This is undefined behaviour. Checking the result
>>>> of it does not fix this.
>>>>
>>>> - Andreas
>>>
>>> 1. The slicethread implementation is pretty hard to understand.
>>> I was not sure if it can be used for normal parallelization, because
>>> it looked more like some kind of thread pool for continuous data
>>> processing. But after taking a second look, I think I can use it here.
>>> I will try and see if it works well.
>>>
>>> 2. I was not aware that this is undefined behavior. Will switch to
>>> PTHREAD_MUTEX_INITIALIZER and PTHREAD_COND_INITIALIZER macros,
>>> which return a safely initialized mutex/cond.
>>>
>>
>> "The behavior is undefined if the value specified by the mutex argument
>> to pthread_mutex_destroy() does not refer to an initialized mutex."
>> (From
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_destroy.html)
>>
>> Furthermore: "In cases where default mutex attributes are appropriate,
>> the macro PTHREAD_MUTEX_INITIALIZER can be used to initialize mutexes.
>> The effect shall be equivalent to dynamic initialization by a call to
>> pthread_mutex_init() with parameter attr specified as NULL, except that
>> no error checks are performed." The last sentence sounds as if one would
>> then have to check mutex locking.
>>
>> Moreover, older pthread standards did not allow to use
>> PTHREAD_MUTEX_INITIALIZER with non-static mutexes, so I don't know
>> whether we can use that. Also our pthreads-wrapper on top of
>> OS/2-threads does not provide PTHREAD_COND_INITIALIZER (which is used
>> nowhere in the codebase).
> 
> I missed that detail about the initializer macro. Thank you for clearing
> that up.
> 
> After looking more into threads implementation in ffmpeg, I wonder if I
> really need to check any results of init/destroy or other functions.
> In slicethreads.c, there is zero checking on any of the lock functions.
> The pthreads-based implementation does internally check the results of all
> function calls and calls abort() in case of errors ("strict_" wrappers).
> The Win32 implementation uses SRW locks which cannot even return errors. 
> And the OS2 implementation returns 0 on all calls as well.
> 
> So right now, I think that I should continue with normal _init() calls
> (no macros) and drop all error checking, just like slicethread does.
> Are you fine with that approach?

Zero checking is our old approach; the new approach checks for errors
and ensures that only mutexes/condition variables that have been
properly initialized are destroyed. See ff_pthread_init/free in
libavcodec/pthread.c (you can't use this in libavformat, because these
functions are local to libavcodec).

- Andreas