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

[Marton Balint]

On Sun, 25 Feb 2024, Mark Thompson wrote:

> On 25/02/2024 15:01, Marton Balint wrote:
>>  On Sun, 25 Feb 2024, 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.
>>
>>  [...]
>>
>>>  diff --git a/fftools/ffmpeg_sched.h b/fftools/ffmpeg_sched.h
>>>  index 95f9c1d4db..315053ae42 100644
>>>  --- a/fftools/ffmpeg_sched.h
>>>  +++ b/fftools/ffmpeg_sched.h
>>>  @@ -233,6 +233,13 @@ 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 thread queue, used if thread_queue_size is not set
>>>  on a
>>>  + * call to sch_add_mux().
>>
>>  Not precisely, as this thread queue size is used for both frame queues and
>>  packet queues.
>
> Yes, it applies to both - hence the description I added not mentioning frames 
> or packets.

For some reason I assumed the description implies it is only used in 
sched_add_mux().

>
>>  Historically the thread_queue_size option was introduced for packet queues
>>  for demuxed packets, and recently on the output for muxing packets.
>>
>>  If we want to make the frame queue size adjustable as well, I think it
>>  should be a separate option and maybe a separate constant should be added
>>  for its default value.
>
> This part is not changing anything about the queue sizes, it is just moving 
> the existing magic number hidden in queue_alloc() to a named constant.
>
> I don't have any motivation to make the frame queue size adjustable; I added 
> the assert so that if someone wants to do that in future they know that they 
> need to take additional action to avoid breaking some decoders again.
>
>>>  + */
>>>  +#define DEFAULT_THREAD_QUEUE_SIZE 8
>
> Would you prefer that I make distinct DEFAULT_FRAME_THREAD_QUEUE_SIZE and 
> DEFAULT_PACKET_THREAD_QUEUE_SIZE (both 8?) and replace the magic number in 
> queue_alloc() with a selection between them based on the type?  I have no 
> strong opinion on that, so I don't mind doing it if you would prefer it.

I think its worth doing.

Thanks,
Marton