Message ID | 20220322163911.64772-1-nicolas.gaullier@cji.paris |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v3,1/2] lavf/mpegenc: fix ever-growing fifo size since the new API | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
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
>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
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 }, };
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(-)