From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 6E3F94429D for ; Sun, 4 Sep 2022 21:29:17 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id CB4AC68B8C8; Mon, 5 Sep 2022 00:29:13 +0300 (EEST) Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3736168B72B for ; Mon, 5 Sep 2022 00:29:07 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1662326946; bh=GnPeq4WtC2SBlWaqwd1GJHsMhWCTpGtQCwrj6a5l23A=; h=X-UI-Sender-Class:From:To:Subject:Date:In-Reply-To:References; b=ZSEyFP7chfgVkLCD3pVx8gV5YIaWT4RaLk3RKnamTRFxqAqdEPf7RinBU37J4LGYX s/fSSPvT1lYZCuu06dLJUIxqqUOzEHqAChlt7VYcaMDNdwLrc6quwPZcdiq9vGbLtp Y20EwbRCpmxl94X5ctMI+CePmok1DFf8YgrFbf78= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from [94.134.107.138] ([94.134.107.138]) by web-mail.gmx.net (3c-app-gmx-bs72.server.lan [172.19.170.208]) (via HTTP); Sun, 4 Sep 2022 23:29:06 +0200 MIME-Version: 1.0 Message-ID: From: Lukas Fellechner To: ffmpeg-devel@ffmpeg.org Date: Sun, 4 Sep 2022 23:29:06 +0200 Importance: normal Sensitivity: Normal In-Reply-To: References: <20220823190326.249-1-lukas.fellechner@gmx.net> <20220823190326.249-3-lukas.fellechner@gmx.net> X-UI-Message-Type: mail X-Priority: 3 X-Provags-ID: V03:K1:eFtGuv49gjzRXlBbgO/D3T9jnDsuPVsq4TSmpSXYy43lAfQTMMlqRdI8o7+RpjtYNB/UU oEz0ovoDw7s6bFs8DuuI0h/qXsvkojfrh3mZxSMKXQ+RVmQ6mKMD1OarNS3Uih6QeVpLnx2cUOe/ cqyk3x1lbTTTkiB1gpnuyGSfekfe1bz5MBVadEIh4sdsye/RJ5SlRaBwFmoWblVOu2P2JZIohwgL 9G/yLqfUM7TyXf5QXwoF5YC9Kq6G5zsnjwJXhmhs2q+xQrtVb2DyrwYta07KJlMVsOfPH7bPMNlh R8= X-Spam-Flag: NO X-UI-Out-Filterresults: notjunk:1;V03:K0:UtGp5TCVz9s=:vepvJMTlNru0vGbKlz6Qb0 r8x1wfejhE0fzkzO+Vc8j/7/GP8MSmry6dHV8OUzj8AvQdXxuL31bBGJCytknevJ6tzkAU03o F5zzcoxLIDxIVpZEnPdheTgHhRaB6P6jYwya+UEknFIRKsto363zJ4/KglFx0zizZAigcHuy/ mByCMZTTUAIgwNRdBR0vlTNS1ze5xaotpre/yb5n/iQ0AJIw+zgZId5BazoPTbD/uB72KIv2x qpQmiPYIiFFM/nicP4xVajm7rcK9t/TwVgVgJxF73qlyHu5OiNX67PVjLJmqKGdlk3vOwXIjY VC3nWFtVF1pTXaYBSCf3PUQUli9t+qYZkvSbsYOr/3ZD57/RGUkfWba+jkApoaGt8N3LZRMTg hXUd5GQhMhLm2VnZyyaibzLfam7Q5V/nL/pAJiwfcXTgfOFuzSZnY6Sufo3iSjwLSRzUhT4qO ldDDafMbF6tIUR8eTF5kRi1Il+1FJZTrW+iOZbyoww5Q5rlTCy2UlTSIyD71Wq/6s2snE482v hxFXzGmI3+EMx9nJHkHp++XBgfXJKnZwjHX1dGcZXpXJV9KVSwZOgmGI1H4WK0enbKU+STZY8 w4g9h14fCDdUsjtZ1p4rDwK7jPaogrE3QzTFBQExOBdceqdJCZqqTeYeFZ6czL8Tf0NxCppLt OZ0S6ENQ6PWxfExSGyGg6TKTd2O65M3rXvTW9ReoUmMJ+xd3e/4qQ6eqyhK2s525EzD3JeYOG 4rMaj9H8O+R3lWu2hrgJ+I/5r3W3wMJytpPRky9oi4Kthhf1dOPJPGeoQ3fVx7hEG2XDHCV1z 2rAkba6 Subject: Re: [FFmpeg-devel] [PATCH v3 2/3] lavf/dashdec: Multithreaded DASH initialization X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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-request@ffmpeg.org with subject "unsubscribe".