| Message ID | 1970349688.12787.1688447320689@localhost |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/libvpxenc: prevent fifo from filling up | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
On Wed, Jul 5, 2023 at 12:15 PM James Zern <jzern@google.com> wrote: > > Hi, > +ffmpeg-dev. I took the wrong email off the reply. > On Mon, Jul 3, 2023 at 10:08 PM David Lemler <david@lemler.family> wrote: > > > > Prevent the fifo used in encoding VPx videos from filling up and stopping > > encode when it reaches 21845 items, which happens when the video has more > > than > > that number of frames. > > > > What failure do you see, a memory allocation error? 21845 sounds > somewhat arbitrary, maybe specific to your machine, so I'd leave it > off the comment and commit message. > > > This problem occurs when performing the first pass of a 2-pass encode, as > > otherwise, the fifo is properly drained, preventing it from overflowing. > > > > Problem is fixed by manually draining the fifo when performing the first > > pass > > of a 2-pass encode. > > > > Fixes the regression originally introduced in > > 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb > > > > Anton, any comments here? > > > Signed-off-by: David Lemler <david@lemler.family> > > --- > > libavcodec/libvpxenc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > > index 8833df2d68..e4ab13e6ab 100644 > > --- a/libavcodec/libvpxenc.c > > +++ b/libavcodec/libvpxenc.c > > @@ -1452,6 +1452,12 @@ static int queue_frames(AVCodecContext *avctx, struct > > vpx_codec_ctx *encoder, > > memcpy((uint8_t*)stats->buf + stats->sz, > > pkt->data.twopass_stats.buf, > > pkt->data.twopass_stats.sz); > > stats->sz += pkt->data.twopass_stats.sz; > > + /* Drain the fifo if it has any items in it to prevent it from > > + filling up when performing the first pass of a 2-pass encode > > if > > + the video has more than 21845 frames. */ > > + if (av_fifo_can_read(ctx->fifo) > 0) { > > Another option would be to avoid calling frame_data_submit() in the first pass. > > > + av_fifo_drain2(ctx->fifo, 1); > > + } > > break; > > } > > case VPX_CODEC_PSNR_PKT: > > -- > > 2.41.0.windows.1 > >
> On 07/05/2023 2:16 PM CDT James Zern <jzern-at-google.com@ffmpeg.org> wrote: > > > On Wed, Jul 5, 2023 at 12:15 PM James Zern <jzern@google.com> wrote: > > > > Hi, > > > > +ffmpeg-dev. I took the wrong email off the reply. > > > On Mon, Jul 3, 2023 at 10:08 PM David Lemler <david@lemler.family> wrote: > > > > > > Prevent the fifo used in encoding VPx videos from filling up and stopping > > > encode when it reaches 21845 items, which happens when the video has more > > > than > > > that number of frames. > > > > > > > What failure do you see, a memory allocation error? 21845 sounds > > somewhat arbitrary, maybe specific to your machine, so I'd leave it > > off the comment and commit message. > > When running a command like the following to perform the first pass of a 2-pass VP9 encode: ffmpeg -i input.mp4 -c:v libvpx-vp9 -b:v 0 -crf 31 -pass 1 -an \ -deadline best -threads 8 -row-mt 1 -f null /dev/null ffmpeg exited with the error: Error submitting video frame to the encoder Conversion failed! and the ffmpeg2pass-0.log file existed but was empty. I've tested and reproduced the issue on Intel x86-64 on Windows (10 22H2, gyan.dev build) and Linux (Debian 12, own compiled build), and on AMD EPYC x86-64 on Linux (Debian 12, own compiled build). I believe the number of frames is (1024 * 1024) / sizeof(FrameData); see lavc/libvpxenc.c the call to av_fifo_alloc2 around line 999 and lavutil/fifo.c the assignment of f->auto_grow_limit around line 72. I'm not sure if that calculation would change on different systems/architectures. During testing, I added a debug log below the can_grow assignment ternary in fifo_check_space in lavutil/fifo.c logging out the values of need_grow, can_grow, f->auto_grow_limit, and f->nb_elems and noticed that as the encode was happening, the nb_elems value kept increasing and can_grow kept decreasing until nb_elems got to f->auto_grow_limit and can_grow to 0, at which point the encode failed with the error mentioned previously. This is the first time I've contributed to FFmpeg (or a project that uses a mailing list), so I'm still becoming familiar with standard practices for commit formatting and development. If that value is dependent on variables like compiler or architecture or you otherwise think I should remove it, let me know and I'll re-submit a modified patch. > > > This problem occurs when performing the first pass of a 2-pass encode, as > > > otherwise, the fifo is properly drained, preventing it from overflowing. > > > > > > Problem is fixed by manually draining the fifo when performing the first > > > pass > > > of a 2-pass encode. > > > > > > Fixes the regression originally introduced in > > > 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb > > > > > > > Anton, any comments here? > > > > > Signed-off-by: David Lemler <david@lemler.family> > > > --- > > > libavcodec/libvpxenc.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > > > index 8833df2d68..e4ab13e6ab 100644 > > > --- a/libavcodec/libvpxenc.c > > > +++ b/libavcodec/libvpxenc.c > > > @@ -1452,6 +1452,12 @@ static int queue_frames(AVCodecContext *avctx, struct > > > vpx_codec_ctx *encoder, > > > memcpy((uint8_t*)stats->buf + stats->sz, > > > pkt->data.twopass_stats.buf, > > > pkt->data.twopass_stats.sz); > > > stats->sz += pkt->data.twopass_stats.sz; > > > + /* Drain the fifo if it has any items in it to prevent it from > > > + filling up when performing the first pass of a 2-pass encode > > > if > > > + the video has more than 21845 frames. */ > > > + if (av_fifo_can_read(ctx->fifo) > 0) { > > > > Another option would be to avoid calling frame_data_submit() in the first pass. > > > > > + av_fifo_drain2(ctx->fifo, 1); > > > + } > > > break; > > > } > > > case VPX_CODEC_PSNR_PKT: > > > -- > > > 2.41.0.windows.1 > > >
Quoting James Zern (2023-07-05 21:16:37) > On Wed, Jul 5, 2023 at 12:15 PM James Zern <jzern@google.com> wrote: > > > > Hi, > > > > +ffmpeg-dev. I took the wrong email off the reply. > > > On Mon, Jul 3, 2023 at 10:08 PM David Lemler <david@lemler.family> wrote: > > > > > > Prevent the fifo used in encoding VPx videos from filling up and stopping > > > encode when it reaches 21845 items, which happens when the video has more > > > than > > > that number of frames. > > > > > > > What failure do you see, a memory allocation error? 21845 sounds > > somewhat arbitrary, maybe specific to your machine, so I'd leave it > > off the comment and commit message. > > > > > This problem occurs when performing the first pass of a 2-pass encode, as > > > otherwise, the fifo is properly drained, preventing it from overflowing. > > > > > > Problem is fixed by manually draining the fifo when performing the first > > > pass > > > of a 2-pass encode. > > > > > > Fixes the regression originally introduced in > > > 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb > > > > > > > Anton, any comments here? Is it guaranteed by the libvpx API that no encoded packets are returned for the first pass of 2pass encoding? If so, then it probably makes most sense not to send anything to the fifo in the first place.
On Thu, Jul 6, 2023 at 12:45 AM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting James Zern (2023-07-05 21:16:37) > > On Wed, Jul 5, 2023 at 12:15 PM James Zern <jzern@google.com> wrote: > > > > > > Hi, > > > > > > > +ffmpeg-dev. I took the wrong email off the reply. > > > > > On Mon, Jul 3, 2023 at 10:08 PM David Lemler <david@lemler.family> wrote: > > > > > > > > Prevent the fifo used in encoding VPx videos from filling up and stopping > > > > encode when it reaches 21845 items, which happens when the video has more > > > > than > > > > that number of frames. > > > > > > > > > > What failure do you see, a memory allocation error? 21845 sounds > > > somewhat arbitrary, maybe specific to your machine, so I'd leave it > > > off the comment and commit message. > > > > > > > This problem occurs when performing the first pass of a 2-pass encode, as > > > > otherwise, the fifo is properly drained, preventing it from overflowing. > > > > > > > > Problem is fixed by manually draining the fifo when performing the first > > > > pass > > > > of a 2-pass encode. > > > > > > > > Fixes the regression originally introduced in > > > > 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb > > > > > > > > > > Anton, any comments here? > > Is it guaranteed by the libvpx API that no encoded packets are returned > for the first pass of 2pass encoding? If so, then it probably makes most > sense not to send anything to the fifo in the first place. > You'll only get stats packets in the first pass, no frames. David, you can check `!(avctx->flags & AV_CODEC_FLAG_PASS1)` before calling `frame_data_submit()`.
On Wed, Jul 5, 2023 at 7:22 PM David Lemler <david@lemler.family> wrote: > > > On 07/05/2023 2:16 PM CDT James Zern <jzern-at-google.com@ffmpeg.org> wrote: > > > > > > On Wed, Jul 5, 2023 at 12:15 PM James Zern <jzern@google.com> wrote: > > > > > > Hi, > > > > > > > +ffmpeg-dev. I took the wrong email off the reply. > > > > > On Mon, Jul 3, 2023 at 10:08 PM David Lemler <david@lemler.family> wrote: > > > > > > > > Prevent the fifo used in encoding VPx videos from filling up and stopping > > > > encode when it reaches 21845 items, which happens when the video has more > > > > than > > > > that number of frames. > > > > > > > > > > What failure do you see, a memory allocation error? 21845 sounds > > > somewhat arbitrary, maybe specific to your machine, so I'd leave it > > > off the comment and commit message. > > > > > When running a command like the following to perform the first pass of a > 2-pass VP9 encode: > > ffmpeg -i input.mp4 -c:v libvpx-vp9 -b:v 0 -crf 31 -pass 1 -an \ > -deadline best -threads 8 -row-mt 1 -f null /dev/null > > ffmpeg exited with the error: > Error submitting video frame to the encoder > Conversion failed! > > and the ffmpeg2pass-0.log file existed but was empty. > > I've tested and reproduced the issue on Intel x86-64 on Windows > (10 22H2, gyan.dev build) and Linux (Debian 12, own compiled build), and on > AMD EPYC x86-64 on Linux (Debian 12, own compiled build). I believe the > number of frames is (1024 * 1024) / sizeof(FrameData); see lavc/libvpxenc.c > the call to av_fifo_alloc2 around line 999 and lavutil/fifo.c the assignment > of f->auto_grow_limit around line 72. I'm not sure if that calculation > would change on different systems/architectures. > Thanks for the explanation, I didn't look closely enough before making my comment.
diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c index 8833df2d68..e4ab13e6ab 100644 --- a/libavcodec/libvpxenc.c +++ b/libavcodec/libvpxenc.c @@ -1452,6 +1452,12 @@ static int queue_frames(AVCodecContext *avctx, struct vpx_codec_ctx *encoder, memcpy((uint8_t*)stats->buf + stats->sz, pkt->data.twopass_stats.buf, pkt->data.twopass_stats.sz); stats->sz += pkt->data.twopass_stats.sz; + /* Drain the fifo if it has any items in it to prevent it from + filling up when performing the first pass of a 2-pass encode if + the video has more than 21845 frames. */ + if (av_fifo_can_read(ctx->fifo) > 0) { + av_fifo_drain2(ctx->fifo, 1); + }
Prevent the fifo used in encoding VPx videos from filling up and stopping encode when it reaches 21845 items, which happens when the video has more than that number of frames. This problem occurs when performing the first pass of a 2-pass encode, as otherwise, the fifo is properly drained, preventing it from overflowing. Problem is fixed by manually draining the fifo when performing the first pass of a 2-pass encode. Fixes the regression originally introduced in 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb Signed-off-by: David Lemler <david@lemler.family> --- libavcodec/libvpxenc.c | 6 ++++++ 1 file changed, 6 insertions(+) break; } case VPX_CODEC_PSNR_PKT: