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

[FFmpeg-devel] [PATCH v3 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.

Fix regressing since ea511196a6c85eb433e10cdbecb0b2c722faf20d

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

diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index cc47a43288..5d755e3bdd 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -84,6 +84,7 @@ typedef struct MpegMuxContext {
     int64_t vcd_padding_bytes_written;
 
     int preload;
+    uint32_t 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;
@@ -1151,7 +1153,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
     StreamInfo *stream = st->priv_data;
     int64_t pts, dts;
     PacketDesc *pkt_desc;
-    int preload, ret;
+    int preload;
     const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
                           (pkt->flags & AV_PKT_FLAG_KEY);
 
@@ -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 &&
@@ -1277,6 +1275,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 }, 0, UINT_MAX, E},
     { NULL },
 };
 
-- 
2.34.0

_______________________________________________
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 v3 2/2] lavf/mpegenc: fix termination following a fifo overrun

[Nicolas Gaullier]

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

A fatal error mentions the availability of fifo_size_limit option.

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

diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index 5d755e3bdd..eff4531037 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -85,6 +85,7 @@ typedef struct MpegMuxContext {
 
     int preload;
     uint32_t fifo_size_limit;
+    int fifo_size_exceeded;
 } MpegMuxContext;
 
 extern const AVOutputFormat ff_mpeg1vcd_muxer;
@@ -1153,7 +1154,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
     StreamInfo *stream = st->priv_data;
     int64_t pts, dts;
     PacketDesc *pkt_desc;
-    int preload;
+    int preload, ret;
     const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
                           (pkt->flags & AV_PKT_FLAG_KEY);
 
@@ -1220,10 +1221,17 @@ 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)) {
+        s->fifo_size_exceeded = 1;
+        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;
     }
@@ -1231,9 +1239,13 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
 
 static int mpeg_mux_end(AVFormatContext *ctx)
 {
+    MpegMuxContext *s = ctx->priv_data;
     StreamInfo *stream;
     int i;
 
+    if (s->fifo_size_exceeded)
+        return 0;
+
     for (;;) {
         int ret = output_packet(ctx, 1);
         if (ret < 0)
-- 
2.34.0

_______________________________________________
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 v3 1/2] lavf/mpegenc: fix ever-growing fifo size since the new API

[Andreas Rheinhardt]

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.
> 
> Fix regressing since ea511196a6c85eb433e10cdbecb0b2c722faf20d
> 
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> ---
>  libavformat/mpegenc.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
> index cc47a43288..5d755e3bdd 100644
> --- a/libavformat/mpegenc.c
> +++ b/libavformat/mpegenc.c
> @@ -84,6 +84,7 @@ typedef struct MpegMuxContext {
>      int64_t vcd_padding_bytes_written;
>  
>      int preload;
> +    uint32_t 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;
> @@ -1151,7 +1153,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>      StreamInfo *stream = st->priv_data;
>      int64_t pts, dts;
>      PacketDesc *pkt_desc;
> -    int preload, ret;
> +    int preload;
>      const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
>                            (pkt->flags & AV_PKT_FLAG_KEY);
>  
> @@ -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 &&
> @@ -1277,6 +1275,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 }, 0, UINT_MAX, E},
>      { NULL },
>  };
>  

1. Options of type AV_OPT_TYPE_INT need to have a target of type int.
2. Setting UINT_MAX as maximum for such an option is nonsense; INT_MAX
is the maximum for it. (FFMIN(INT_MAX, SIZE_MAX) would be even better.)
3. Allowing zero for fifo_size_limit makes no sense, as the above code
allocates 16 when allocating the FIFO.
4. After removing av_fifo_grow2() it is no longer assured that
av_fifo_write() succeeds; it therefore needs to be checked.

- 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 v3 2/2] lavf/mpegenc: fix termination following a fifo overrun

[Andreas Rheinhardt]

Nicolas Gaullier:
> Avoid an infinite 'retry' loop in output_packet when flushing.
> 
> A fatal error mentions the availability of fifo_size_limit option.
> 
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> ---
>  libavformat/mpegenc.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
> index 5d755e3bdd..eff4531037 100644
> --- a/libavformat/mpegenc.c
> +++ b/libavformat/mpegenc.c
> @@ -85,6 +85,7 @@ typedef struct MpegMuxContext {
>  
>      int preload;
>      uint32_t fifo_size_limit;
> +    int fifo_size_exceeded;
>  } MpegMuxContext;
>  
>  extern const AVOutputFormat ff_mpeg1vcd_muxer;
> @@ -1153,7 +1154,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>      StreamInfo *stream = st->priv_data;
>      int64_t pts, dts;
>      PacketDesc *pkt_desc;
> -    int preload;
> +    int preload, ret;
>      const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
>                            (pkt->flags & AV_PKT_FLAG_KEY);
>  
> @@ -1220,10 +1221,17 @@ 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)) {
> +        s->fifo_size_exceeded = 1;
> +        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;
>      }
> @@ -1231,9 +1239,13 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>  
>  static int mpeg_mux_end(AVFormatContext *ctx)
>  {
> +    MpegMuxContext *s = ctx->priv_data;
>      StreamInfo *stream;
>      int i;
>  
> +    if (s->fifo_size_exceeded)
> +        return 0;
> +
>      for (;;) {
>          int ret = output_packet(ctx, 1);
>          if (ret < 0)

Could this infinite loop also happen before switching to the new API?
Does it happen because avail_data is zero for all streams?

- 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 v3 1/2] lavf/mpegenc: fix ever-growing fifo size since the new API

[Nicolas Gaullier]

>1. Options of type AV_OPT_TYPE_INT need to have a target of type int.
>2. Setting UINT_MAX as maximum for such an option is nonsense; INT_MAX is the maximum for it. (FFMIN(INT_MAX, SIZE_MAX) would be even better.) 3. Allowing zero for fifo_size_limit makes no sense, as the above code allocates 16 >when allocating the FIFO.
>4. After removing av_fifo_grow2() it is no longer assured that
>av_fifo_write() succeeds; it therefore needs to be checked.
>
>- Andreas

Fixed in v4. Thanks
(I have revised the spliting between the 2 patches).

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

Re: [FFmpeg-devel] [PATCH v3 2/2] lavf/mpegenc: fix termination following a fifo overrun

[Nicolas Gaullier]

>Could this infinite loop also happen before switching to the new API?
>Does it happen because avail_data is zero for all streams?
>
>- Andreas

I will take time to double-check this very carefully, but in my experience, if I remember correctly, it is nothing easy and was already buggy before.
(but with the older API, it was almost impossible to reach such conditions).

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

Re: [FFmpeg-devel] [PATCH v3 2/2] lavf/mpegenc: fix termination following a fifo overrun

[Nicolas Gaullier]

>>Could this infinite loop also happen before switching to the new API?
>>Does it happen because avail_data is zero for all streams?
>>
>>- Andreas
>
>I will take time to double-check this very carefully, but in my experience, if I remember correctly, it is nothing easy and was already buggy before.
>(but with the older API, it was almost impossible to reach such conditions).

Well, I really don't know how I missed that but indeed, no avail_data...
So, I am doing some testing and post right afterwards an updated patch with an 'has_avail_data' check.
The problem happen before the new API, for a long time ago (I checked about one year ago).
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".