Re: [FFmpeg-devel] [PATCH] avformat/fifo_test: Move into tests/fifo_muxer.c

[Stefano Sabatini <stefasab@gmail.com>]

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.rheinhardt@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).

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

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