Stefano Sabatini:
> 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 <andreas.rheinha...@outlook.com>
>> ---
>
>> 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).
>
The reason is that this would complicate changing the internals of
(de)muxers.
> 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 <stdlib.h>
>> -
>> -#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
>
This would necessitate changes to the part of the test tool that I did
not change and would therefore need to be a commit of its own. I can do
this, of course.
>>
>> +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
>
Ok.
>> +{
>> + 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
This is actually intentional to separate the test muxer from the actual
test tool.
>
>> 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-requ...@ffmpeg.org with subject "unsubscribe".
