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

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

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

On 18/03/2024 05:35, Xiang, Haihao wrote:
> 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

Thanks.  Pushed because this is a strict improvement on what we have at the moment, but I still think we should be able to do better for this generic code.

The two obvious outstanding problems that I see are:

* Components can consume more than one frame and ffmpeg doesn't have any way to notice this; this is particularly a problem with asynchronous encoders.

* A filter graph which uses the same pool as output (say it only changes metadata or drops frames) ends up needing double this because there are two frame queues.

Ideas on how to deal with either of these in a generic way are welcome.

- Mark