[FFmpeg-devel] [PATCH v5 0/2] lavf/mpegenc: fixes

[FFmpeg-devel] [PATCH v5 0/2] lavf/mpegenc: fixes

[Nicolas Gaullier]

The first patch is left unchanged since v4.
The second has been totally reworked after some investigations advised by Andreas.

Nicolas Gaullier (2):
  lavf/mpegenc: fix ever-growing fifo size since the new API
  lavf/mpegenc: fix termination on error conditions

 libavformat/mpegenc.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

-- 
2.34.0.windows.1

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

[FFmpeg-devel] [PATCH v5 1/2] lavf/mpegenc: fix ever-growing fifo size since the new API

[Nicolas Gaullier]

The older av_fifo_realloc2 implemented an auto grow that
should be ported as such in the new API.

This patch introduces a limitation in the fifo buffer size.
The default is set to 128MB and may be overriden by a new user option.
The amount of memory allocated depends on multiple factors, including
the number of audio streams.
A worst case scenario is where an out-of-spec high video bitrate is
combined with numerous low bitrate audios.

A fatal error mentions the availability of fifo_size_limit option.

Fix regressing since ea511196a6c85eb433e10cdbecb0b2c722faf20d

Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
---
 libavformat/mpegenc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index cc47a43288..e0955a7d33 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -84,6 +84,7 @@ typedef struct MpegMuxContext {
     int64_t vcd_padding_bytes_written;
 
     int preload;
+    int fifo_size_limit;
 } MpegMuxContext;
 
 extern const AVOutputFormat ff_mpeg1vcd_muxer;
@@ -461,9 +462,10 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx)
                    av_get_media_type_string(st->codecpar->codec_type), i);
             return AVERROR(EINVAL);
         }
-        stream->fifo = av_fifo_alloc2(16, 1, 0);
+        stream->fifo = av_fifo_alloc2(16, 1, AV_FIFO_FLAG_AUTO_GROW);
         if (!stream->fifo)
             return AVERROR(ENOMEM);
+        av_fifo_auto_grow_limit(stream->fifo, s->fifo_size_limit);
     }
     bitrate       = 0;
     audio_bitrate = 0;
@@ -1207,10 +1209,6 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
     pkt_desc->unwritten_size =
     pkt_desc->size           = size;
 
-    ret = av_fifo_grow2(stream->fifo, size);
-    if (ret < 0)
-        return ret;
-
     if (s->is_dvd) {
         // min VOBU length 0.4 seconds (mpucoder)
         if (is_iframe &&
@@ -1222,10 +1220,15 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
         }
     }
 
-    av_fifo_write(stream->fifo, buf, size);
+    ret = av_fifo_write(stream->fifo, buf, size);
+    if (ret == AVERROR(ENOSPC))
+        av_log(s, AV_LOG_FATAL, "Input stream buffer overrun. "
+                "To avoid, increase fifo_size_limit option.\n");
+    if (ret < 0)
+        return ret;
 
     for (;;) {
-        int ret = output_packet(ctx, 0);
+        ret = output_packet(ctx, 0);
         if (ret <= 0)
             return ret;
     }
@@ -1277,6 +1280,7 @@ static void mpeg_mux_deinit(AVFormatContext *ctx)
 static const AVOption options[] = {
     { "muxrate", NULL,                                          OFFSET(user_mux_rate), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, ((1<<22) - 1) * (8 * 50), E },
     { "preload", "Initial demux-decode delay in microseconds.", OFFSET(preload),  AV_OPT_TYPE_INT, { .i64 = 500000 }, 0, INT_MAX, E },
+    { "fifo_size_limit", "Maximum allowed memory for buffering an input stream in bytes", OFFSET(fifo_size_limit), AV_OPT_TYPE_INT, {.i64 = 128 * 1024 * 1024 }, 16, FFMIN(INT_MAX, SIZE_MAX), E},
     { NULL },
 };
 
-- 
2.34.0.windows.1

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

[FFmpeg-devel] [PATCH v5 2/2] lavf/mpegenc: fix termination on error conditions

[Nicolas Gaullier]

Avoid an infinite 'retry' loop in output_packet when flushing.

Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
---
 libavformat/mpegenc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index e0955a7d33..e113a42867 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -1002,7 +1002,7 @@ static int output_packet(AVFormatContext *ctx, int flush)
     MpegMuxContext *s = ctx->priv_data;
     AVStream *st;
     StreamInfo *stream;
-    int i, avail_space = 0, es_size, trailer_size;
+    int i, has_avail_data = 0, avail_space = 0, es_size, trailer_size;
     int best_i = -1;
     int best_score = INT_MIN;
     int ignore_constraints = 0;
@@ -1028,6 +1028,7 @@ retry:
         if (avail_data == 0)
             continue;
         av_assert0(avail_data > 0);
+        has_avail_data = 1;
 
         if (space < s->packet_size && !ignore_constraints)
             continue;
@@ -1048,6 +1049,8 @@ retry:
         int64_t best_dts = INT64_MAX;
         int has_premux = 0;
 
+        if (!has_avail_data)
+            return 0;
         for (i = 0; i < ctx->nb_streams; i++) {
             AVStream *st = ctx->streams[i];
             StreamInfo *stream = st->priv_data;
-- 
2.34.0.windows.1

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

Re: [FFmpeg-devel] [PATCH v5 2/2] lavf/mpegenc: fix termination on error conditions

[Andreas Rheinhardt]

Nicolas Gaullier:
> Avoid an infinite 'retry' loop in output_packet when flushing.
> 
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> ---
>  libavformat/mpegenc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
> index e0955a7d33..e113a42867 100644
> --- a/libavformat/mpegenc.c
> +++ b/libavformat/mpegenc.c
> @@ -1002,7 +1002,7 @@ static int output_packet(AVFormatContext *ctx, int flush)
>      MpegMuxContext *s = ctx->priv_data;
>      AVStream *st;
>      StreamInfo *stream;
> -    int i, avail_space = 0, es_size, trailer_size;
> +    int i, has_avail_data = 0, avail_space = 0, es_size, trailer_size;
>      int best_i = -1;
>      int best_score = INT_MIN;
>      int ignore_constraints = 0;
> @@ -1028,6 +1028,7 @@ retry:
>          if (avail_data == 0)
>              continue;
>          av_assert0(avail_data > 0);
> +        has_avail_data = 1;
>  
>          if (space < s->packet_size && !ignore_constraints)
>              continue;
> @@ -1048,6 +1049,8 @@ retry:
>          int64_t best_dts = INT64_MAX;
>          int has_premux = 0;
>  
> +        if (!has_avail_data)
> +            return 0;
>          for (i = 0; i < ctx->nb_streams; i++) {
>              AVStream *st = ctx->streams[i];
>              StreamInfo *stream = st->priv_data;

in case of errors, the context is left in an inconsistent state: The
PacketDesc linked-list claims that there is data in the FIFO although
this is wrong. I always prefer avoiding such scenarios over fixing them
lateron. In this case, fixing them would mean growing the FIFO before
allocating the new PacketDesc (if the FIFO needs growing at all). Shall
I do this or do you want to?
(In any case, thanks for reporting this issue.)

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

Re: [FFmpeg-devel] [PATCH v5 2/2] lavf/mpegenc: fix termination on error conditions

[Nicolas Gaullier]

>in case of errors, the context is left in an inconsistent state: The PacketDesc linked-list claims that there is data in the FIFO although this is wrong. I always prefer avoiding such scenarios over fixing them lateron. In this case, fixing them would mean growing the FIFO before >allocating the new PacketDesc (if the FIFO needs growing at all). Shall I do this or do you want to?
>(In any case, thanks for reporting this issue.)
>
>- Andreas

I fully understand.
I think the most time-effective is to let you do that if you are ok, you also know this code better than me.
Thanks to you

Nicolas
_______________________________________________
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".