Message ID | 3deacbbf-0087-477c-83b9-910bdba15b7d@jkqxz.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] ffmpeg: set extra_hw_frames to account for frames held in queues | expand |
Context | Check | Description |
---|---|---|
andriy/configure_x86 | warning | Failed to apply patch |
yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
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. 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. > + */ > +#define DEFAULT_THREAD_QUEUE_SIZE 8 Regards, Marton
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. > 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. Thanks, - Mark
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
diff --git a/fftools/ffmpeg_dec.c b/fftools/ffmpeg_dec.c index 8c92b27cc1..0dab989cdd 100644 --- a/fftools/ffmpeg_dec.c +++ b/fftools/ffmpeg_dec.c @@ -1146,6 +1146,19 @@ int dec_open(Decoder **pdec, Scheduler *sch, goto fail; } + 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_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) goto fail; diff --git a/fftools/ffmpeg_sched.c b/fftools/ffmpeg_sched.c index 1144fce958..9ff9431270 100644 --- a/fftools/ffmpeg_sched.c +++ b/fftools/ffmpeg_sched.c @@ -361,7 +361,16 @@ static int queue_alloc(ThreadQueue **ptq, unsigned nb_streams, unsigned queue_si ThreadQueue *tq; ObjPool *op; - queue_size = queue_size > 0 ? queue_size : 8; + queue_size = queue_size > 0 ? queue_size : DEFAULT_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_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 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(). + */ +#define DEFAULT_THREAD_QUEUE_SIZE 8 + /** * Add a muxed stream for a previously added muxer. *