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 9328843041 for ; Thu, 13 Jan 2022 10:50:59 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0444968B7D5; Thu, 13 Jan 2022 12:50:57 +0200 (EET) Received: from EUR02-HE1-obe.outbound.protection.outlook.com (mail-oln040092068018.outbound.protection.outlook.com [40.92.68.18]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id DA33E68B77F for ; Thu, 13 Jan 2022 12:50:50 +0200 (EET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=f/fIaDUGsOnGVL2zbv5nGJjG0HQP3EFX1GYQ8ZtcmtbWQ3hrfwHBz+TG/YZgjLs/QLNcjGU4q1VxRdVNZGJowr3gQoIGw3muellKo1V7kIlqf5+D+RUqTXmsBUjWy9ar0p8oMWRn4l7ip/1Wdoz5+wCDQmQTVEk+WiNQ3znhXzkn+TN5vxUITZ1nDIpNVTm+6+fT3GT3SeP0mQfEYiAeTOMwAcjuWzhUYI9UGcCfv2SSfsUpnvbkw0EPJ2HZ4tAvL2YwKwmYWxGt0fEILAqjb8ixr7KRV2GWz5juXZusMWimx7SmLYoII7EOVes1CIaHF3jHVzEyXzvHy1aPHZmZGQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=3TqH2ZcoDmI6yRgbKwyLHIDpvDTGwD5wIoT89GG44po=; b=XIfmm7qr/X/9u3EtwoQTod720PCpznaONhtJR2LsA54ChsEg9Pf/8wgHYarHbFbzBhE3uCanTW3wUbadVwxX9brm+OfQR1bwT6lyAGnqJuJq0xacLeLUf/Kk2ciZFsRTtee0lw+lv1dt4hkJOSdsJmeaq6w5Tmy8OgRefuWlRwwrP5WQ2IkEh0Rz7CCKyQgtZrLOyySYO+5kF510iUzZs3zYB4ezr7KD2dYo5sBaMIKeda5mh74lI3cIhCTA7OJ4DZwcjwOLXBlNQR+2P/IdRlbf+G/2rjiyHI5drxix9n7RpsmCT5TPGsUYLcrlwur7gm3fpkGZFqC8hRBodOWYrA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3TqH2ZcoDmI6yRgbKwyLHIDpvDTGwD5wIoT89GG44po=; b=rf7+oiq+RRIw81dCZwOk7L13Lnvx/taCY7u3L3M3YiIdwwC2NMM9P3+grIPBQjb4xKdhIkKQfZL6m8qOO/eZaWfrOiTPJCs5Ki4c9AAjBekOc9vvlIXWGfg0j9RnPVOdEVddyGbVjiV3JOdzB1ggBJ113Wd3DCzKFpDJfoFqxcChYDD+uxr5ix5//q73VQ9l2xkIZoRzrxKlqRUxP10SoYSzgB0kr1OApViFlogFDvW00GV2QHg3zmao7vMefHRsZ6WzdDB5zQ5C3k8J/2Kmjgl25B6uxKmvk4/5cJeBJek41UhvBxvg6OYDxDJb4t077HTNDxYdjJtgC0Rf+/0QMg== Received: from AM7PR03MB6660.eurprd03.prod.outlook.com (2603:10a6:20b:1c1::22) by AM5PR03MB3154.eurprd03.prod.outlook.com (2603:10a6:206:17::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4888.11; Thu, 13 Jan 2022 10:50:49 +0000 Received: from AM7PR03MB6660.eurprd03.prod.outlook.com ([fe80::19fc:be9f:2c9c:53f5]) by AM7PR03MB6660.eurprd03.prod.outlook.com ([fe80::19fc:be9f:2c9c:53f5%9]) with mapi id 15.20.4888.010; Thu, 13 Jan 2022 10:50:49 +0000 Message-ID: Date: Thu, 13 Jan 2022 11:50:48 +0100 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220111095830.31542-1-anton@khirnov.net> <20220111095830.31542-17-anton@khirnov.net> From: Andreas Rheinhardt In-Reply-To: <20220111095830.31542-17-anton@khirnov.net> X-TMN: [El1wIyxyhrUT8OygrtMpD8AkxTnD751S] X-ClientProxiedBy: AM4P190CA0012.EURP190.PROD.OUTLOOK.COM (2603:10a6:200:56::22) To AM7PR03MB6660.eurprd03.prod.outlook.com (2603:10a6:20b:1c1::22) X-Microsoft-Original-Message-ID: <3e19ff8e-ff9a-7468-a40e-9386380e5b31@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: a5d444b3-6806-40df-cffe-08d9d6828f5e X-MS-TrafficTypeDiagnostic: AM5PR03MB3154:EE_ X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 4Wd+n9hAHO6bKXcFo5yfx/GdMQyIgnut4mMa0u0s1s8V02sokxt3FoyvhZgOGjLh/bMo26uB6hFMNuptO8HGkq5nMLSkohfi4+PvdAKQePnBheMIJt8hQO6YcOCN5DWSjupb/r7jjZ79dkdHBui3rPfOUXemlN2aGhejLchuNlWs6LC+x7UZdfOeur226KUxRixAAjMngY/tFTIzzfvFB+4OXoHBaxWlNqzzjJkH3hUHxnAmXyFHoDMaQq3HmQgopKQCO10NCIOrduu+xDpmE5hWXZhirrRP8wyU0wSxdU617EIU08vzQOo5wq+/KIoQPvn4PapwdKpl75DdI53Fz4TlDtNH8BD4SSmvj+HPFvhwAukT1S829vJCdyQbkTGpDnhhReV03XdZxJKg+G5SaAyGSI1Q88zjZnYmuQNq8DEVdSy0uLffT3aXQK7dcE/fkpC9IMenwW+AS5U6W7su4EDGOT2CPgfD07NgiM9ndWPA9u+k6Z+S8w3jlgfItDC6ZAb+pYhTA+dSlkWhGbykmrDZ22gYW6M0G/2HwZxLFD4MUBpE67b/q3Wsy9SoDLNNHDPwv1IueJ4LvU1IleDf+g== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RzlvdFdZMmhudGRvN2h6OVU4TTc3Ym1JRHpSS3llekxHYzRCUWMzcWo3Q1Aw?= =?utf-8?B?U1k3ZUVhb2JlWVB0ME85eEJFRnR4d2JSMittSDdJYTdzYXRHRDUxZ0lUamRE?= =?utf-8?B?NGkyc0JDVkNDb2tJTjgxOU1JMldsTitHYWtaY0JycVR6aXB1ZHRSYWtSdVhZ?= =?utf-8?B?YWVZQVhDSWFHeXpjT2tWb1h3ckpibWpzWDBBbzVJczVwSEFrR09sRzhJMDky?= =?utf-8?B?U2tlRDU4WklaR0NvV1hUMXdHVDU0b0tjREk3VGdidFdTSnRCaytyS1RpMkQ0?= =?utf-8?B?ZmdIRzJlMTVrMHRlWVRBZXNYVldVNEp1K0FURVlwQTBTTjVBNmtCbDJxV2hv?= =?utf-8?B?TVdWVENFY0FyR0o5QXQzWDZWc1gvamhUeTAyd0FmVUpQRFpnMnFkQk5rQ24z?= =?utf-8?B?ejJmMG40R3JkTktsSVp3MFdKU3lYYlJzeFVyeVJxNitFQWFQSzZ2dmpnU3lO?= =?utf-8?B?bDM4bWtzLzg5YzNWb0NEZ3BwLzRzVGUvUWhBZDJXekEvQ1ZYYzBmRGVjV01N?= =?utf-8?B?TGZTeEpGVXJtK2RacWMvekJEemVGRzZxcEtGU2lrUlFVVDYxUUJycmpaeTFR?= =?utf-8?B?SmtnWFY3Nnc4bi9vTjlwdEs3S25XWFNxRGE1TVJzL3NXQ1BIU0l5aVBRQTN0?= =?utf-8?B?cm4wOWlTYnU3QXJob1pGQXo2eGMzNTJxWjhwdk1ld2dKRC8rdEtQS2U1TEw3?= =?utf-8?B?S0dzMzQ2UmgvTlV5ZlFPakphMjFSUmpudThMOExYU1JwbTcxd3hrQTl1dUVY?= =?utf-8?B?bUI0a25PcE8xMG1NNGRMNkM0SEx2ZHJUaDJxc2ZwdWtQKzV4MnpJdVBTZ2Fq?= =?utf-8?B?RkNhZE5MRWx5djZlOTBTYzdxbktjS2o4eVlqWlZrNDN2aU9UT29TYWs5UEdn?= =?utf-8?B?aVdGeHdFUVgxZ0Z4dElDZkc0WHpUUVBCL0F1eHRDL2R2L3RjS1BMbWxJVlZx?= =?utf-8?B?UG0wNXg2cFlSaHowQ0UwMnhTR0JIdklkNTRodEdXTzNpaEt5Tlp1cTV1dzUw?= =?utf-8?B?Y3Zia3JKTTRuOG1lbjcxeEdTSDZ5OG5INFBrQWxUNXh1cmFjd3NaZGZ3ZVUz?= =?utf-8?B?QktPa3pJblROeWd5QWlQdE8xT09vSDdOMkdzS2RqVUo4Q0tmc25VZXdvdVBM?= =?utf-8?B?N0xVNmo0MEZXeE9BbVBsbGVJTllFM0ZZL2VDRnYrNTZaaGV4UUFYWnNBNGhH?= =?utf-8?B?VDQrNE9DU1d0RTlWZ2Z2NlJJZ09lOGs5WWhxQitDN1pjQTBUS0lrVDFHNWkz?= =?utf-8?B?ZjE1YUt0UkE2VnpyRnd2ZFZjYTVsNklSZUpCa0JpL1U1MENnUT09?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: a5d444b3-6806-40df-cffe-08d9d6828f5e X-MS-Exchange-CrossTenant-AuthSource: AM7PR03MB6660.eurprd03.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jan 2022 10:50:49.2667 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR03MB3154 Subject: Re: [FFmpeg-devel] [PATCH 17/28] ffmpeg: move the mux queue into muxer private data 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: Anton Khirnov: > The muxing queue currently lives in OutputStream, which is a very large > struct storing the state for both encoding and muxing. The muxing queue > is only used by the code in ffmpeg_mux, so it makes sense to restrict it > to that file. > > This makes the first step towards reducing the scope of OutputStream. > --- > fftools/ffmpeg.c | 9 ----- > fftools/ffmpeg.h | 9 ----- > fftools/ffmpeg_mux.c | 91 ++++++++++++++++++++++++++++++++++++-------- > fftools/ffmpeg_opt.c | 6 --- > 4 files changed, 76 insertions(+), 39 deletions(-) > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index 6c774e9615..c1bb3926c4 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -595,15 +595,6 @@ static void ffmpeg_cleanup(int ret) > avcodec_free_context(&ost->enc_ctx); > avcodec_parameters_free(&ost->ref_par); > > - if (ost->muxing_queue) { > - while (av_fifo_size(ost->muxing_queue)) { > - AVPacket *pkt; > - av_fifo_generic_read(ost->muxing_queue, &pkt, sizeof(pkt), NULL); > - av_packet_free(&pkt); > - } > - av_fifo_freep(&ost->muxing_queue); > - } > - > av_freep(&output_streams[i]); > } > #if HAVE_THREADS > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h > index e828f71dc0..28df1b179f 100644 > --- a/fftools/ffmpeg.h > +++ b/fftools/ffmpeg.h > @@ -555,15 +555,6 @@ typedef struct OutputStream { > > int max_muxing_queue_size; > > - /* the packets are buffered here until the muxer is ready to be initialized */ > - AVFifoBuffer *muxing_queue; > - > - /* > - * The size of the AVPackets' buffers in queue. > - * Updated when a packet is either pushed or pulled from the queue. > - */ > - size_t muxing_queue_data_size; > - > /* Threshold after which max_muxing_queue_size will be in effect */ > size_t muxing_queue_data_threshold; > > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c > index f4d76e1533..f03202bbb7 100644 > --- a/fftools/ffmpeg_mux.c > +++ b/fftools/ffmpeg_mux.c > @@ -32,7 +32,20 @@ > #include "libavformat/avformat.h" > #include "libavformat/avio.h" > > +typedef struct MuxStream { > + /* the packets are buffered here until the muxer is ready to be initialized */ > + AVFifoBuffer *muxing_queue; > + > + /* > + * The size of the AVPackets' buffers in queue. > + * Updated when a packet is either pushed or pulled from the queue. > + */ > + size_t muxing_queue_data_size; > +} MuxStream; > + > struct Muxer { > + MuxStream *streams; > + > /* filesize limit expressed in bytes */ > int64_t limit_filesize; > int64_t final_filesize; > @@ -55,6 +68,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, > { > AVFormatContext *s = of->ctx; > AVStream *st = ost->st; > + MuxStream *ms = &of->mux->streams[st->index]; > int ret; > > /* > @@ -76,10 +90,10 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, > if (!of->mux->header_written) { > AVPacket *tmp_pkt; > /* the muxer is not initialized yet, buffer the packet */ > - if (!av_fifo_space(ost->muxing_queue)) { > - size_t cur_size = av_fifo_size(ost->muxing_queue); > + if (!av_fifo_space(ms->muxing_queue)) { > + size_t cur_size = av_fifo_size(ms->muxing_queue); > unsigned int are_we_over_size = > - (ost->muxing_queue_data_size + pkt->size) > ost->muxing_queue_data_threshold; > + (ms->muxing_queue_data_size + pkt->size) > ost->muxing_queue_data_threshold; > size_t limit = are_we_over_size ? ost->max_muxing_queue_size : INT_MAX; > size_t new_size = FFMIN(2 * cur_size, limit); > > @@ -89,7 +103,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, > ost->file_index, ost->st->index); > exit_program(1); > } > - ret = av_fifo_realloc2(ost->muxing_queue, new_size); > + ret = av_fifo_realloc2(ms->muxing_queue, new_size); > if (ret < 0) > exit_program(1); > } > @@ -100,8 +114,8 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, > if (!tmp_pkt) > exit_program(1); > av_packet_move_ref(tmp_pkt, pkt); > - ost->muxing_queue_data_size += tmp_pkt->size; > - av_fifo_generic_write(ost->muxing_queue, &tmp_pkt, sizeof(tmp_pkt), NULL); > + ms->muxing_queue_data_size += tmp_pkt->size; > + av_fifo_generic_write(ms->muxing_queue, &tmp_pkt, sizeof(tmp_pkt), NULL); > return; > } > > @@ -283,16 +297,17 @@ int of_check_init(OutputFile *of) > > /* flush the muxing queues */ > for (i = 0; i < of->ctx->nb_streams; i++) { > + MuxStream *ms = &of->mux->streams[i]; > OutputStream *ost = output_streams[of->ost_index + i]; > > /* try to improve muxing time_base (only possible if nothing has been written yet) */ > - if (!av_fifo_size(ost->muxing_queue)) > + if (!av_fifo_size(ms->muxing_queue)) > ost->mux_timebase = ost->st->time_base; > > - while (av_fifo_size(ost->muxing_queue)) { > + while (av_fifo_size(ms->muxing_queue)) { > AVPacket *pkt; > - av_fifo_generic_read(ost->muxing_queue, &pkt, sizeof(pkt), NULL); > - ost->muxing_queue_data_size -= pkt->size; > + av_fifo_generic_read(ms->muxing_queue, &pkt, sizeof(pkt), NULL); > + ms->muxing_queue_data_size -= pkt->size; > of_write_packet(of, pkt, ost, 1); > av_packet_free(&pkt); > } > @@ -333,6 +348,31 @@ int of_write_trailer(OutputFile *of) > return 0; > } > > +static void mux_free(Muxer **pmux, int nb_streams) > +{ > + Muxer *mux = *pmux; > + > + if (!mux) > + return; > + > + for (int i = 0; i < nb_streams; i++) { > + MuxStream *ms = &mux->streams[i]; > + > + if (!ms->muxing_queue) > + continue; > + > + while (av_fifo_size(ms->muxing_queue)) { > + AVPacket *pkt; > + av_fifo_generic_read(ms->muxing_queue, &pkt, sizeof(pkt), NULL); > + av_packet_free(&pkt); > + } > + av_fifo_freep(&ms->muxing_queue); > + } > + av_freep(&mux->streams); > + > + av_freep(pmux); > +} > + > void of_close(OutputFile **pof) > { > OutputFile *of = *pof; > @@ -342,25 +382,42 @@ void of_close(OutputFile **pof) > return; > > s = of->ctx; > + > + mux_free(&of->mux, s ? s->nb_streams : 0); > + > if (s && s->oformat && !(s->oformat->flags & AVFMT_NOFILE)) > avio_closep(&s->pb); > avformat_free_context(s); > av_dict_free(&of->opts); > > - av_freep(&of->mux); > - > av_freep(pof); > } > > int of_muxer_init(OutputFile *of, int64_t limit_filesize) > { > Muxer *mux = av_mallocz(sizeof(*mux)); > + int ret = 0; > > if (!mux) > return AVERROR(ENOMEM); > > + mux->streams = av_calloc(of->ctx->nb_streams, sizeof(*mux->streams)); > + if (!mux->streams) { > + av_freep(&mux); > + return AVERROR(ENOMEM); > + } > + > of->mux = mux; > > + for (int i = 0; i < of->ctx->nb_streams; i++) { > + MuxStream *ms = &mux->streams[i]; > + ms->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket)); > + if (!ms->muxing_queue) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > + } > + > mux->limit_filesize = limit_filesize; > > if (strcmp(of->format->name, "rtp")) > @@ -368,12 +425,16 @@ int of_muxer_init(OutputFile *of, int64_t limit_filesize) > > /* write the header for files with no streams */ > if (of->format->flags & AVFMT_NOSTREAMS && of->ctx->nb_streams == 0) { > - int ret = of_check_init(of); > + ret = of_check_init(of); > if (ret < 0) > - return ret; > + goto fail; > } > > - return 0; > +fail: > + if (ret < 0) > + mux_free(&of->mux, of->ctx->nb_streams); > + > + return ret; > } > > int of_finished(OutputFile *of) > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index ed3fd818d0..c7d1d21a37 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -1613,8 +1613,6 @@ static OutputStream *new_output_stream(OptionsContext *o, AVFormatContext *oc, e > ost->max_muxing_queue_size = FFMIN(ost->max_muxing_queue_size, INT_MAX / sizeof(ost->pkt)); > ost->max_muxing_queue_size *= sizeof(ost->pkt); > > - ost->muxing_queue_data_size = 0; > - > ost->muxing_queue_data_threshold = 50*1024*1024; > MATCH_PER_STREAM_OPT(muxing_queue_data_threshold, i, ost->muxing_queue_data_threshold, oc, st); > > @@ -1638,10 +1636,6 @@ static OutputStream *new_output_stream(OptionsContext *o, AVFormatContext *oc, e > } > ost->last_mux_dts = AV_NOPTS_VALUE; > > - ost->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket)); > - if (!ost->muxing_queue) > - exit_program(1); > - > return ost; > } > > My objections to adding a separately allocated muxing context and to this MuxStream have not changed. Both incur unnecessary allocations and indirections and (in case of the latter) loops; the latter is also very unnatural. The patch here actually shows it: You only use the muxer context to get the MuxStream context corresponding to the OutputStream you are interested in: > for (i = 0; i < of->ctx->nb_streams; i++) { > + MuxStream *ms = &of->mux->streams[i]; > OutputStream *ost = output_streams[of->ost_index + i]; > AVStream *st = ost->st; > + MuxStream *ms = &of->mux->streams[st->index]; > int ret; Your aim of making sure what code can use/modify what parts can also be fulfilled by comments. - Andreas _______________________________________________ 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".