[FFmpeg-devel] [PATCH] lavc/libvpxenc: prevent fifo from filling up

[FFmpeg-devel] [PATCH] lavc/libvpxenc: prevent fifo from filling up

[David Lemler]

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(+)

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);
+            }
             break;
         }
         case VPX_CODEC_PSNR_PKT:
-- 
2.41.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] lavc/libvpxenc: prevent fifo from filling up

[James Zern]

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
> >
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[David Lemler]

> 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
> > >
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[James Zern]

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()`.
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[James Zern]

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.
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[David Lemler]

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.

Incorporated suggestion from James Zern to prevent calling
frame_data_submit() at all when performing the first pass of a 2-pass
encode so the fifo is not filled at all; replaces original patch which
drained the fifo after filling to prevent it from becoming full.

Fixes the regression originally introduced in
5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb

Co-authored-by: James Zern <jzern@google.com>
Signed-off-by: David Lemler <david@lemler.family>
---
 libavcodec/libvpxenc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index 8833df2d68..549ac55aaa 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -1780,9 +1780,11 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket
*pkt,
             }
         }
 
-        res = frame_data_submit(avctx, ctx->fifo, frame);
-        if (res < 0)
-            return res;
+        if (!(avctx->flags & AV_CODEC_FLAG_PASS1)) {
+            res = frame_data_submit(avctx, ctx->fifo, frame);
+            if (res < 0)
+                return res;
+        }
     }
 
     // this is for encoding with preset temporal layering patterns defined
in
-- 
2.41.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] lavc/libvpxenc: prevent fifo from filling up

[James Zern]

On Fri, Jul 7, 2023 at 2:31 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.
>
> Incorporated suggestion from James Zern to prevent calling
> frame_data_submit() at all when performing the first pass of a 2-pass
> encode so the fifo is not filled at all; replaces original patch which
> drained the fifo after filling to prevent it from becoming full.
>
> Fixes the regression originally introduced in
> 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb
>
> Co-authored-by: James Zern <jzern@google.com>
> Signed-off-by: David Lemler <david@lemler.family>
> ---
>  libavcodec/libvpxenc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>

lgtm. I'll submit this soon if there aren't any further comments.
Note the patch is corrupt, but easily fixed. In the future try to use
git send-email or attach the output of git format-patch.
_______________________________________________
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] lavc/libvpxenc: prevent fifo from filling up

[James Zern]

On Thu, Jul 13, 2023 at 12:38 PM James Zern <jzern@google.com> wrote:
>
> On Fri, Jul 7, 2023 at 2:31 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.
> >
> > Incorporated suggestion from James Zern to prevent calling
> > frame_data_submit() at all when performing the first pass of a 2-pass
> > encode so the fifo is not filled at all; replaces original patch which
> > drained the fifo after filling to prevent it from becoming full.
> >
> > Fixes the regression originally introduced in
> > 5bda4ec6c3cb6f286bb40dee4457c3c26e0f78cb
> >
> > Co-authored-by: James Zern <jzern@google.com>
> > Signed-off-by: David Lemler <david@lemler.family>
> > ---
> >  libavcodec/libvpxenc.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
>
> lgtm. I'll submit this soon if there aren't any further comments.
> Note the patch is corrupt, but easily fixed. In the future try to use
> git send-email or attach the output of git format-patch.

Applied. Thanks for the patch.
_______________________________________________
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".