[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
>
> > +#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.

I also noticed one cross-thread issue that I will solve in the next patch.