[FFmpeg-devel] libavcodec/nvdec: Do not exceed 32 surfaces when initializing hw_frames_ctx

From: ameerj <52414509+ameerj@users.noreply.github.com>

nvdec is likely to fail when the initial pool size exceeds 32. This change ensures we don't exceed the limit when initializing a new hw_frames_ctx

Signed-off-by: ameerj <52414509+ameerj@users.noreply.github.com>
---
 libavcodec/nvdec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On 10.08.2021 04:37, Ameer J wrote:
> From: ameerj <52414509+ameerj@users.noreply.github.com>
> 
> nvdec is likely to fail when the initial pool size exceeds 32. This change ensures we don't exceed the limit when initializing a new hw_frames_ctx
> 
> Signed-off-by: ameerj <52414509+ameerj@users.noreply.github.com>
> ---
>   libavcodec/nvdec.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c
> index 251be039a8..bef33dbae9 100644
> --- a/libavcodec/nvdec.c
> +++ b/libavcodec/nvdec.c
> @@ -303,8 +303,10 @@ static int nvdec_init_hwframes(AVCodecContext *avctx, AVBufferRef **out_frames_r
>       frames_ctx = (AVHWFramesContext*)(*out_frames_ref)->data;
>   
>       if (dummy) {
> -        // Copied from ff_decode_get_hw_frames_ctx for compatibility
> -        frames_ctx->initial_pool_size += 3;
> +        // The function above guarantees 1 work surface, We must guarantee 4 work surfaces.
> +        // (the absolute minimum), so add the missing count without exceeding the maximum
> +        // recommended for nvdec.
> +        frames_ctx->initial_pool_size = min(frames_ctx->initial_pool_size + 3, 32);

Where does this min() function come from?

>           frames_ctx->free = nvdec_free_dummy;
>           frames_ctx->pool = av_buffer_pool_init(0, nvdec_alloc_dummy);
> 

So far, the 32 surface limit has only ever been enforced as a soft 
limit, and a warning printed when exceeded. Since it being a limit is 
not strictly documented and it might be risen in the future.

Also, if the pool is limited in size, something is likely to fail due to 
that further down the chain.
So I'm not sure if this is a better solution.

On Tue, 10 Aug 2021 at 15:04, Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 10.08.2021 04:37, Ameer J wrote:
> > From: ameerj <52414509+ameerj@users.noreply.github.com>
> >
> > nvdec is likely to fail when the initial pool size exceeds 32. This
> change ensures we don't exceed the limit when initializing a new
> hw_frames_ctx
> >
> > Signed-off-by: ameerj <52414509+ameerj@users.noreply.github.com>
> > ---
> >   libavcodec/nvdec.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c
> > index 251be039a8..bef33dbae9 100644
> > --- a/libavcodec/nvdec.c
> > +++ b/libavcodec/nvdec.c
> > @@ -303,8 +303,10 @@ static int nvdec_init_hwframes(AVCodecContext
> *avctx, AVBufferRef **out_frames_r
> >       frames_ctx = (AVHWFramesContext*)(*out_frames_ref)->data;
> >
> >       if (dummy) {
> > -        // Copied from ff_decode_get_hw_frames_ctx for compatibility
> > -        frames_ctx->initial_pool_size += 3;
> > +        // The function above guarantees 1 work surface, We must
> guarantee 4 work surfaces.
> > +        // (the absolute minimum), so add the missing count without
> exceeding the maximum
> > +        // recommended for nvdec.
> > +        frames_ctx->initial_pool_size =
> min(frames_ctx->initial_pool_size + 3, 32);
>
> Where does this min() function come from?
>
> >           frames_ctx->free = nvdec_free_dummy;
> >           frames_ctx->pool = av_buffer_pool_init(0, nvdec_alloc_dummy);
> >
>
> So far, the 32 surface limit has only ever been enforced as a soft
> limit, and a warning printed when exceeded. Since it being a limit is
> not strictly documented and it might be risen in the future.
>
> Also, if the pool is limited in size, something is likely to fail due to
> that further down the chain.
> So I'm not sure if this is a better solution.



Related,

Can the pool surface setting be made configurable?
The older CUVID based wrappers expose it as an option (via -surfaces) as
does the NVENC encoder wrapper implementation. This would be useful in
cases where lower VRAM foot print is required (for multiple transcodes on
the same GPU) and at the same time, tunable enough to prevent downstream
failures if surfaces are insufficient.

>

Turns out the issue I was facing which prompted this patch request stemmed
from improper usage of the APIs, and this patch was a workaround that was
hiding the proper fix.
Please feel free to reject this patch request, especially if it is likely
to cause unforeseen issues down the line.

Thanks
-Ameer

On Tue, Aug 10, 2021 at 8:14 AM Dennis Mungai <dmngaie@gmail.com> wrote:

> On Tue, 10 Aug 2021 at 15:04, Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
>
> > On 10.08.2021 04:37, Ameer J wrote:
> > > From: ameerj <52414509+ameerj@users.noreply.github.com>
> > >
> > > nvdec is likely to fail when the initial pool size exceeds 32. This
> > change ensures we don't exceed the limit when initializing a new
> > hw_frames_ctx
> > >
> > > Signed-off-by: ameerj <52414509+ameerj@users.noreply.github.com>
> > > ---
> > >   libavcodec/nvdec.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c
> > > index 251be039a8..bef33dbae9 100644
> > > --- a/libavcodec/nvdec.c
> > > +++ b/libavcodec/nvdec.c
> > > @@ -303,8 +303,10 @@ static int nvdec_init_hwframes(AVCodecContext
> > *avctx, AVBufferRef **out_frames_r
> > >       frames_ctx = (AVHWFramesContext*)(*out_frames_ref)->data;
> > >
> > >       if (dummy) {
> > > -        // Copied from ff_decode_get_hw_frames_ctx for compatibility
> > > -        frames_ctx->initial_pool_size += 3;
> > > +        // The function above guarantees 1 work surface, We must
> > guarantee 4 work surfaces.
> > > +        // (the absolute minimum), so add the missing count without
> > exceeding the maximum
> > > +        // recommended for nvdec.
> > > +        frames_ctx->initial_pool_size =
> > min(frames_ctx->initial_pool_size + 3, 32);
> >
> > Where does this min() function come from?
> >
> > >           frames_ctx->free = nvdec_free_dummy;
> > >           frames_ctx->pool = av_buffer_pool_init(0, nvdec_alloc_dummy);
> > >
> >
> > So far, the 32 surface limit has only ever been enforced as a soft
> > limit, and a warning printed when exceeded. Since it being a limit is
> > not strictly documented and it might be risen in the future.
> >
> > Also, if the pool is limited in size, something is likely to fail due to
> > that further down the chain.
> > So I'm not sure if this is a better solution.
>
>
>
> Related,
>
> Can the pool surface setting be made configurable?
> The older CUVID based wrappers expose it as an option (via -surfaces) as
> does the NVENC encoder wrapper implementation. This would be useful in
> cases where lower VRAM foot print is required (for multiple transcodes on
> the same GPU) and at the same time, tunable enough to prevent downstream
> failures if surfaces are insufficient.
>
> >
> _______________________________________________
> 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".
>