[FFmpeg-devel] [PATCH v3 1/2] ffmpeg: set extra_hw_frames to account for frames held in queues

[Xiang, Haihao]

On So, 2024-03-17 at 20:49 +0000, Mark Thompson wrote:
> Since e0da916b8f5b079a4865eef7f64863f50785463d the ffmpeg utility has
> held multiple frames output by the decoder in internal queues without
> telling the decoder that it is going to do so.  When the decoder has a
> fixed-size pool of frames (common in some hardware APIs where the output
> frames must be stored as an array texture) this could lead to the pool
> being exhausted and the decoder getting stuck.  Fix this by telling the
> decoder to allocate additional frames according to the queue size.
> ---
> Rebased but otherwise unchanged since previous version.
> 
>   fftools/ffmpeg_dec.c   | 13 +++++++++++++
>   fftools/ffmpeg_sched.c | 16 +++++++++++++++-
>   fftools/ffmpeg_sched.h | 12 ++++++++++++
>   3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_dec.c b/fftools/ffmpeg_dec.c
> index c41c5748e5..ed411b6bf8 100644
> --- a/fftools/ffmpeg_dec.c
> +++ b/fftools/ffmpeg_dec.c
> @@ -1207,6 +1207,19 @@ static int dec_open(DecoderPriv *dp, AVDictionary
> **dec_opts,
>           return ret;
>       }
> 
> +    if (dp->dec_ctx->hw_device_ctx) {
> +        // Update decoder extra_hw_frames option to account for the
> +        // frames held in queues inside the ffmpeg utility.  This is
> +        // called after avcodec_open2() because the user-set value of
> +        // extra_hw_frames becomes valid in there, and we need to add
> +        // this on top of it.
> +        int extra_frames = DEFAULT_FRAME_THREAD_QUEUE_SIZE;
> +        if (dp->dec_ctx->extra_hw_frames >= 0)
> +            dp->dec_ctx->extra_hw_frames += extra_frames;
> +        else
> +            dp->dec_ctx->extra_hw_frames = extra_frames;
> +    }
> +
>       ret = check_avoptions(*dec_opts);
>       if (ret < 0)
>           return ret;
> diff --git a/fftools/ffmpeg_sched.c b/fftools/ffmpeg_sched.c
> index f739066921..ec88017e21 100644
> --- a/fftools/ffmpeg_sched.c
> +++ b/fftools/ffmpeg_sched.c
> @@ -365,7 +365,21 @@ static int queue_alloc(ThreadQueue **ptq, unsigned
> nb_streams, unsigned queue_si
>       ThreadQueue *tq;
>       ObjPool *op;
> 
> -    queue_size = queue_size > 0 ? queue_size : 8;
> +    if (queue_size <= 0) {
> +        if (type == QUEUE_FRAMES)
> +            queue_size = DEFAULT_FRAME_THREAD_QUEUE_SIZE;
> +        else
> +            queue_size = DEFAULT_PACKET_THREAD_QUEUE_SIZE;
> +    }
> +
> +    if (type == QUEUE_FRAMES) {
> +        // This queue length is used in the decoder code to ensure that
> +        // there are enough entries in fixed-size frame pools to account
> +        // for frames held in queues inside the ffmpeg utility.  If this
> +        // can ever dynamically change then the corresponding decode
> +        // code needs to be updated as well.
> +        av_assert0(queue_size == DEFAULT_FRAME_THREAD_QUEUE_SIZE);
> +    }
> 
>       op = (type == QUEUE_PACKETS) ? objpool_alloc_packets() :
>                                      objpool_alloc_frames();
> diff --git a/fftools/ffmpeg_sched.h b/fftools/ffmpeg_sched.h
> index a9190bd3d1..e51c26cec9 100644
> --- a/fftools/ffmpeg_sched.h
> +++ b/fftools/ffmpeg_sched.h
> @@ -233,6 +233,18 @@ int sch_add_filtergraph(Scheduler *sch, unsigned
> nb_inputs, unsigned nb_outputs,
>    */
>   int sch_add_mux(Scheduler *sch, SchThreadFunc func, int (*init)(void *),
>                   void *ctx, int sdp_auto, unsigned thread_queue_size);
> +
> +/**
> + * Default size of a packet thread queue.  For muxing this can be overridden
> by
> + * the thread_queue_size option as passed to a call to sch_add_mux().
> + */
> +#define DEFAULT_PACKET_THREAD_QUEUE_SIZE 8
> +
> +/**
> + * Default size of a frame thread queue.
> + */
> +#define DEFAULT_FRAME_THREAD_QUEUE_SIZE 8
> +
>   /**
>    * Add a muxed stream for a previously added muxer.

LGTM

- Haihao