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 65CF54947B for ; Tue, 12 Mar 2024 20:20:30 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4E4B568CABD; Tue, 12 Mar 2024 22:20:28 +0200 (EET) Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B60D268C0A2 for ; Tue, 12 Mar 2024 22:20:22 +0200 (EET) Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-55a5e7fa471so5661082a12.1 for ; Tue, 12 Mar 2024 13:20:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1710274820; x=1710879620; darn=ffmpeg.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=2d4Ts/8g0zNdJmaR/CK+edjH434Z7iQ0c3lL7EWg5jU=; b=bRhRfrQ91mHlXgNnU0EszFLW/d3LX9BF4I3yxw3wQKzCSssXwFPMEJeXbhLbahEz65 cOECB906izeHdiGHIXLTX1P/SSUOCcl5q3YVV74U1LPNB+1Hb0+SyQ5TcdJ7e60a91BE //mV8Xtp+N3nqGCyugw7EZOr/Ph2EmLq9k6hgT3cv7dpBul3STUmpvMqa79XGtR9fVVn MhNf63IikhyVyin7AxZFBMq64NZixe7lqcOqOikIOLXNWAJY4u43+NgTl9bqoqo9QyTg kUlqS6VHKxILNqAkQj4T0K2Bop7Q3JEGvEU+r30UbdqDpV7wi8PszsH1yFgzlAFCKCa6 eDdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710274820; x=1710879620; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=2d4Ts/8g0zNdJmaR/CK+edjH434Z7iQ0c3lL7EWg5jU=; b=mkBfbhh5ImbHr8UBsFCql5ZE3AGc4i2x+ep47YgnzuekLxP2SWPGk9hc3iA1hyJKda cni7LHPIx/CvASavykjSUw34ryDW3/bR+u3vXSrbDWH1vmC9iPl4VDTM5+OX0eSeUttf VylPVMwkDs+n8J97/oOC/c+sjfrseUZ6tJysUPSf6AWEUzFHmSMpYxet3K9bVaUcGD6/ V8M3ytMt7PNlrx3CSUo+9B29W4RK7az7uyjqlf6UcJHp3hYI7yUT8d9X9fHScM0thSW8 dVsJ1tvT2ouKVOevsRkl8RLbUTsndzalx89tCOJ58aPChlfHSi75oMRanAIzw/upZJ5R 5YXQ== X-Gm-Message-State: AOJu0Yzq6LiEcg7aWjXmv9uDndzsN3sHceWoi9crJOz1gqjFppwgEzKZ rM2f9lBpH9kNQ1qrTYV5oE9Vq3vHM80F5r5MLQegeT0Hvm4Z1z6VdGdLv/Dl X-Google-Smtp-Source: AGHT+IHP7400nBlAZnt72g8x8RFaXPkpAESFfa9XhqADDxNIQjyOgSbOjYhb//ykGyRhE1mT3+xyeA== X-Received: by 2002:a50:f681:0:b0:566:c388:ceb4 with SMTP id d1-20020a50f681000000b00566c388ceb4mr7841199edn.28.1710274820191; Tue, 12 Mar 2024 13:20:20 -0700 (PDT) Received: from mariano ([188.210.239.79]) by smtp.gmail.com with ESMTPSA id d22-20020aa7d696000000b00566d9c8e6cesm4276558edr.21.2024.03.12.13.20.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Mar 2024 13:20:19 -0700 (PDT) Received: by mariano (Postfix, from userid 1000) id 3AC25BFCDC; Tue, 12 Mar 2024 21:20:17 +0100 (CET) Date: Tue, 12 Mar 2024 21:20:17 +0100 From: Stefano Sabatini To: FFmpeg development discussions and patches Message-ID: Mail-Followup-To: FFmpeg development discussions and patches , Andreas Rheinhardt References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.1.4 (2021-12-11) Subject: Re: [FFmpeg-devel] [PATCH] avformat/fifo_test: Move into tests/fifo_muxer.c 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 Cc: Andreas Rheinhardt 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: On date Tuesday 2024-03-12 19:43:29 +0100, Andreas Rheinhardt wrote: > This muxer solely exists to test the fifo muxer via a dedicated > test tool in libavformat/tests/fifo_muxer.c. It fulfills no > other role and it is only designed with this role in mind. > > The latter can be seen in two facts: The muxer uses printf > for logging and it simply presumes the packets' data to contain > a FailingMuxerPacketData (a struct duplicated in fifo_test.c > and tests/fifo_muxer.c.); in particular, it presumes packets > to have data at all, but this need not be true with side-data > only packets and a segfault can easily be triggered by e.g. > encoding flac (our native encoder sends a side-data only packet > with updated extradata at the end of encoding). > > This patch fixes this by moving the test muxer into the fifo > test tool, making it inaccessible via the API (and actually > removing it from libavformat.so and libavformat.a). > While this muxer was accessible via e.g. av_guess_format(), > it was not really usable for an API user as FailingMuxerPacketData > was not public. Therefore this is not considered a breaking change. > > In order to continue to use the test muxer in the test tool, > the ordinary fifo muxer had to be overridden: fifo_muxer.c > includes lavf/fifo.c but with FIFO_TEST defined which makes > it support the fifo_test muxer. This is possible because > test tools are always linked statically to their respective > library. > > Signed-off-by: Andreas Rheinhardt > --- > Will I have to bump minor when applying this? Given that this is a fix, probably a micro bump is enough. > > libavformat/Makefile | 1 - > libavformat/allformats.c | 1 - > libavformat/fifo.c | 7 ++ > libavformat/fifo_test.c | 157 --------------------------------- > libavformat/tests/fifo_muxer.c | 126 +++++++++++++++++++++++++- > 5 files changed, 132 insertions(+), 160 deletions(-) > delete mode 100644 libavformat/fifo_test.c > > diff --git a/libavformat/Makefile b/libavformat/Makefile > index 785349c036..94a949f555 100644 > --- a/libavformat/Makefile > +++ b/libavformat/Makefile > @@ -205,7 +205,6 @@ OBJS-$(CONFIG_EPAF_DEMUXER) += epafdec.o pcm.o > OBJS-$(CONFIG_FFMETADATA_DEMUXER) += ffmetadec.o > OBJS-$(CONFIG_FFMETADATA_MUXER) += ffmetaenc.o > OBJS-$(CONFIG_FIFO_MUXER) += fifo.o > -OBJS-$(CONFIG_FIFO_TEST_MUXER) += fifo_test.o > OBJS-$(CONFIG_FILMSTRIP_DEMUXER) += filmstripdec.o > OBJS-$(CONFIG_FILMSTRIP_MUXER) += filmstripenc.o rawenc.o > OBJS-$(CONFIG_FITS_DEMUXER) += fitsdec.o > diff --git a/libavformat/allformats.c b/libavformat/allformats.c > index 5639715104..e15d0fa6d7 100644 > --- a/libavformat/allformats.c > +++ b/libavformat/allformats.c > @@ -165,7 +165,6 @@ extern const FFOutputFormat ff_f4v_muxer; > extern const FFInputFormat ff_ffmetadata_demuxer; > extern const FFOutputFormat ff_ffmetadata_muxer; > extern const FFOutputFormat ff_fifo_muxer; > -extern const FFOutputFormat ff_fifo_test_muxer; > extern const FFInputFormat ff_filmstrip_demuxer; > extern const FFOutputFormat ff_filmstrip_muxer; > extern const FFInputFormat ff_fits_demuxer; > diff --git a/libavformat/fifo.c b/libavformat/fifo.c > index a3d41ba0d3..2a2673f4d8 100644 > --- a/libavformat/fifo.c > +++ b/libavformat/fifo.c > @@ -528,6 +528,13 @@ static int fifo_init(AVFormatContext *avf) > atomic_init(&fifo->queue_duration, 0); > fifo->last_sent_dts = AV_NOPTS_VALUE; > > +#ifdef FIFO_TEST > + /* This exists for the fifo_muxer test tool. */ > + if (fifo->format && !strcmp(fifo->format, "fifo_test")) { > + extern const FFOutputFormat ff_fifo_test_muxer; > + oformat = &ff_fifo_test_muxer.p; > + } else > +#endif I see, but as unrelated note, it seems odd we don't have a register API to add a custom muxer/demuxer, and having to go through this dance is not really viable at the application level. That's why I wonder why we don't have such API (I remember it was available at some point). Anyway this hack is still better than keeping the fifo_test publicly available. > oformat = av_guess_format(fifo->format, avf->url, NULL); > if (!oformat) { > ret = AVERROR_MUXER_NOT_FOUND; > diff --git a/libavformat/fifo_test.c b/libavformat/fifo_test.c > deleted file mode 100644 > index 3861c4aee4..0000000000 > --- a/libavformat/fifo_test.c > +++ /dev/null > @@ -1,157 +0,0 @@ > -/* > - * FIFO test pseudo-muxer > - * Copyright (c) 2016 Jan Sebechlebsky > - * > - * This file is part of FFmpeg. > - * > - * FFmpeg is free software; you can redistribute it and/or > - * modify it under the terms of the GNU Lesser General Public License > - * as published by the Free Software Foundation; either > - * version 2.1 of the License, or (at your option) any later version. > - * > - * FFmpeg is distributed in the hope that it will be useful, > - * but WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > - * GNU Lesser General Public License for more details. > - * > - * You should have received a copy of the GNU Lesser General Public License > - * along with FFmpeg; if not, write to the Free Software * Foundation, Inc., > - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > - */ > - > -#include > - > -#include "libavutil/opt.h" > -#include "libavutil/time.h" > - > -#include "avformat.h" > -#include "mux.h" > -#include "url.h" > - > -/* Implementation of mock muxer to simulate real muxer failures */ > - > -#define MAX_TST_PACKETS 128 > -#define SLEEPTIME_50_MS 50000 > -#define SLEEPTIME_10_MS 10000 > - > -/* Implementation of mock muxer to simulate real muxer failures */ > - > -/* This is structure of data sent in packets to > - * failing muxer */ > -typedef struct FailingMuxerPacketData { > - int ret; /* return value of write_packet call*/ > - int recover_after; /* set ret to zero after this number of recovery attempts */ > - unsigned sleep_time; /* sleep for this long in write_packet to simulate long I/O operation */ > -} FailingMuxerPacketData; > - > - > -typedef struct FailingMuxerContext { > - AVClass *class; > - int write_header_ret; > - int write_trailer_ret; > - /* If non-zero, summary of processed packets will be printed in deinit */ > - int print_deinit_summary; > - > - int flush_count; > - int pts_written[MAX_TST_PACKETS]; > - int pts_written_nr; > -} FailingMuxerContext; > - > -static int failing_write_header(AVFormatContext *avf) > -{ > - FailingMuxerContext *ctx = avf->priv_data; > - return ctx->write_header_ret; > -} > - > -static int failing_write_packet(AVFormatContext *avf, AVPacket *pkt) > -{ > - FailingMuxerContext *ctx = avf->priv_data; > - int ret = 0; > - if (!pkt) { > - ctx->flush_count++; > - } else { > - FailingMuxerPacketData *data = (FailingMuxerPacketData*) pkt->data; > - > - if (!data->recover_after) { > - data->ret = 0; > - } else { > - data->recover_after--; > - } > - > - ret = data->ret; > - > - if (data->sleep_time) { > - int64_t slept = 0; > - while (slept < data->sleep_time) { > - if (ff_check_interrupt(&avf->interrupt_callback)) > - return AVERROR_EXIT; > - av_usleep(SLEEPTIME_10_MS); > - slept += SLEEPTIME_10_MS; > - } > - } > - > - if (!ret) { > - ctx->pts_written[ctx->pts_written_nr++] = pkt->pts; > - av_packet_unref(pkt); > - } > - } > - return ret; > -} > - > -static int failing_write_trailer(AVFormatContext *avf) > -{ > - FailingMuxerContext *ctx = avf->priv_data; > - return ctx->write_trailer_ret; > -} > - > -static void failing_deinit(AVFormatContext *avf) > -{ > - int i; > - FailingMuxerContext *ctx = avf->priv_data; > - > - if (!ctx->print_deinit_summary) > - return; > - > - printf("flush count: %d\n", ctx->flush_count); > - printf("pts seen nr: %d\n", ctx->pts_written_nr); > - printf("pts seen: "); > - for (i = 0; i < ctx->pts_written_nr; ++i ) { > - printf(i ? ",%d" : "%d", ctx->pts_written[i]); > - } > - printf("\n"); > -} > -#define OFFSET(x) offsetof(FailingMuxerContext, x) > -static const AVOption options[] = { > - {"write_header_ret", "write_header() return value", OFFSET(write_header_ret), > - AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, > - {"write_trailer_ret", "write_trailer() return value", OFFSET(write_trailer_ret), > - AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, > - {"print_deinit_summary", "print summary when deinitializing muxer", OFFSET(print_deinit_summary), > - AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM}, > - {NULL} > - }; > - > -static const AVClass failing_muxer_class = { > - .class_name = "Fifo test muxer", > - .item_name = av_default_item_name, > - .option = options, > - .version = LIBAVUTIL_VERSION_INT, > -}; > - > -const FFOutputFormat ff_fifo_test_muxer = { > - .p.name = "fifo_test", > - .p.long_name = NULL_IF_CONFIG_SMALL("Fifo test muxer"), > - .priv_data_size = sizeof(FailingMuxerContext), > - .write_header = failing_write_header, > - .write_packet = failing_write_packet, > - .write_trailer = failing_write_trailer, > - .deinit = failing_deinit, > - .p.priv_class = &failing_muxer_class, > -#if FF_API_ALLOW_FLUSH > - .p.flags = AVFMT_NOFILE | AVFMT_ALLOW_FLUSH, > -#else > - .p.flags = AVFMT_NOFILE, > -#endif > - .flags_internal = FF_FMT_ALLOW_FLUSH, > -}; > - > diff --git a/libavformat/tests/fifo_muxer.c b/libavformat/tests/fifo_muxer.c > index 11a557c1a0..34aaa55326 100644 > --- a/libavformat/tests/fifo_muxer.c > +++ b/libavformat/tests/fifo_muxer.c > @@ -23,8 +23,20 @@ > #include "libavutil/opt.h" > #include "libavutil/time.h" > #include "libavformat/avformat.h" > +#include "libavformat/mux.h" > #include "libavformat/url.h" > -#include "libavformat/network.h" > + > +/* > + * Include fifo.c directly to override libavformat/fifo.c and > + * thereby prevent libavformat/fifo.o from being pulled in when linking. > + * This relies on libavformat always being linked statically to its > + * test tools (like this one). > + * Due to FIFO_TEST, our fifo muxer will include special handling > + * for tests, i.e. it allows to select the fifo_test muxer below > + * even though it is not accessible via the API. > + */ > +#define FIFO_TEST > +#include "libavformat/fifo.c" > > #define MAX_TST_PACKETS 128 > #define SLEEPTIME_50_MS 50000 > @@ -38,6 +50,118 @@ typedef struct FailingMuxerPacketData { > unsigned sleep_time; /* sleep for this long in write_packet to simulate long I/O operation */ > } FailingMuxerPacketData; nit: FifoTestMuxerPacketData > > +typedef struct FifoTestMuxerContext { > + AVClass *class; > + int write_header_ret; > + int write_trailer_ret; > + /* If non-zero, summary of processed packets will be printed in deinit */ > + int print_deinit_summary; > + > + int flush_count; > + int pts_written[MAX_TST_PACKETS]; > + int pts_written_nr; > +} FifoTestMuxerContext; > + > +static int failing_write_header(AVFormatContext *avf) nit: while at it let's use a more sensible prefix, failing => fifo_test > +{ > + FifoTestMuxerContext *ctx = avf->priv_data; > + return ctx->write_header_ret; > +} > + > +static int failing_write_packet(AVFormatContext *avf, AVPacket *pkt) > +{ > + FifoTestMuxerContext *ctx = avf->priv_data; > + int ret = 0; > + if (!pkt) { > + ctx->flush_count++; > + } else { > + FailingMuxerPacketData *data = (FailingMuxerPacketData*) pkt->data; > + > + if (!data->recover_after) { > + data->ret = 0; > + } else { > + data->recover_after--; > + } > + > + ret = data->ret; > + > + if (data->sleep_time) { > + int64_t slept = 0; > + while (slept < data->sleep_time) { > + if (ff_check_interrupt(&avf->interrupt_callback)) > + return AVERROR_EXIT; > + av_usleep(SLEEPTIME_10_MS); > + slept += SLEEPTIME_10_MS; > + } > + } > + > + if (!ret) { > + ctx->pts_written[ctx->pts_written_nr++] = pkt->pts; > + av_packet_unref(pkt); > + } > + } > + return ret; > +} > + > +static int failing_write_trailer(AVFormatContext *avf) > +{ > + FifoTestMuxerContext *ctx = avf->priv_data; > + return ctx->write_trailer_ret; > +} > + > +static void failing_deinit(AVFormatContext *avf) > +{ > + int i; > + FifoTestMuxerContext *ctx = avf->priv_data; > + > + if (!ctx->print_deinit_summary) > + return; > + > + printf("flush count: %d\n", ctx->flush_count); > + printf("pts seen nr: %d\n", ctx->pts_written_nr); > + printf("pts seen: "); > + for (i = 0; i < ctx->pts_written_nr; ++i ) { > + printf(i ? ",%d" : "%d", ctx->pts_written[i]); > + } > + printf("\n"); > +} > + > +#define OFF(x) offsetof(FifoTestMuxerContext, x) > +static const AVOption fifo_test_options[] = { > + {"write_header_ret", "write_header() return value", OFF(write_header_ret), > + AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, > + {"write_trailer_ret", "write_trailer() return value", OFF(write_trailer_ret), > + AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM}, > + {"print_deinit_summary", "print summary when deinitializing muxer", OFF(print_deinit_summary), > + AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM}, > + {NULL} > + }; > + > +static const AVClass failing_muxer_class = { > + .class_name = "Fifo test muxer", > + .item_name = av_default_item_name, > + .option = fifo_test_options, > + .version = LIBAVUTIL_VERSION_INT, > +}; > + > +const FFOutputFormat ff_fifo_test_muxer = { > + .p.name = "fifo_test", > + .p.long_name = NULL_IF_CONFIG_SMALL("Fifo test muxer"), > + .priv_data_size = sizeof(FifoTestMuxerContext), > + .write_header = failing_write_header, > + .write_packet = failing_write_packet, > + .write_trailer = failing_write_trailer, > + .deinit = failing_deinit, > + .p.priv_class = &failing_muxer_class, > +#if FF_API_ALLOW_FLUSH > + .p.flags = AVFMT_NOFILE | AVFMT_ALLOW_FLUSH, > +#else > + .p.flags = AVFMT_NOFILE, > +#endif > + .flags_internal = FF_FMT_ALLOW_FLUSH, > +}; > + > + nit++++: drop double empty line > static int prepare_packet(AVPacket *pkt, const FailingMuxerPacketData *pkt_data, int64_t pts) > { > int ret = av_new_packet(pkt, sizeof(*pkt_data)); > -- > 2.40.1 LGTM, thanks. _______________________________________________ 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".