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

[Andreas Rheinhardt]

Lukas Fellechner:
> Andreas Rheinhardt andreas.rheinhardt at outlook.com
> Wed Aug 31 05:54:12 EEST 2022
>>
>>> +#if HAVE_THREADS
>>> +
>>> +struct work_pool_data
>>> +{
>>> +    AVFormatContext *ctx;
>>> +    struct representation *pls;
>>> +    struct representation *common_pls;
>>> +    pthread_mutex_t *common_mutex;
>>> +    pthread_cond_t *common_condition;
>>> +    int is_common;
>>> +    int is_started;
>>> +    int result;
>>> +};
>>> +
>>> +struct thread_data
>>
>> This is against our naming conventions: CamelCase for struct tags and
>> typedefs, lowercase names with underscore for variable names.
> 
> In the code files I looked at, CamelCase is only used for typedef structs.
> All structs without typedef are lower case with underscores, so I aligned
> with that, originally.
> 
> I will make this a typedef struct and use CamelCase for next patch.
> 
>>> +static int init_streams_multithreaded(AVFormatContext *s, int nstreams, int threads)
>>> +{
>>> +    DASHContext *c = s->priv_data;
>>> +    int ret = 0;
>>> +    int stream_index = 0;
>>> +    int i;
>>
>> We allow "for (int i = 0;"
> 
> Oh, I did not know that, and I did not see it being used here anywhere.
> Will use in next patch, it's my preferred style, actually.
> 
>>> +
>>> +    // alloc data
>>> +    struct work_pool_data *init_data = (struct work_pool_data*)av_mallocz(sizeof(struct work_pool_data) * nstreams);
>>> +    if (!init_data)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    struct thread_data *thread_data = (struct thread_data*)av_mallocz(sizeof(struct thread_data) * threads);
>>> +    if (!thread_data)
>>> +        return AVERROR(ENOMEM);
>>
>> 1. init_data leaks here on error.
>> 2. In fact, it seems to me that both init_data and thread_data are
>> nowhere freed.
> 
> True, I must have lost the av_free call at some point.
> 
>>> +    // init work pool data
>>> +    struct work_pool_data* current_data = init_data;
>>> +
>>> +    for (i = 0; i < c->n_videos; i++) {
>>> +        create_work_pool_data(s, stream_index, c->videos[i],
>>> +            c->is_init_section_common_video ? c->videos[0] : NULL,
>>> +            current_data, &common_video_mutex, &common_video_cond);
>>> +
>>> +        stream_index++;
>>> +        current_data++;
>>> +    }
>>> +
>>> +    for (i = 0; i < c->n_audios; i++) {
>>> +        create_work_pool_data(s, stream_index, c->audios[i],
>>> +            c->is_init_section_common_audio ? c->audios[0] : NULL,
>>> +            current_data, &common_audio_mutex, &common_audio_cond);
>>> +
>>> +        stream_index++;
>>> +        current_data++;
>>> +    }
>>> +
>>> +    for (i = 0; i < c->n_subtitles; i++) {
>>> +        create_work_pool_data(s, stream_index, c->subtitles[i],
>>> +            c->is_init_section_common_subtitle ? c->subtitles[0] : NULL,
>>> +            current_data, &common_subtitle_mutex, &common_subtitle_cond);
>>> +
>>> +        stream_index++;
>>> +        current_data++;
>>> +    }
>>
>> This is very repetitive.
> 
> Will improve for next patch.
> 
>> 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 also noticed one cross-thread issue that I will solve in the next patch.