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. _______________________________________________ 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".