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

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

[Mark Thompson]

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.
   *
-- 
2.43.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Mark Thompson]

For hardware cases where we are forced to have a fixed pool of frames
allocated up-front (such as array textures on decoder output), suggest
a possible workaround to the user if an allocation fails because the
pool is exhausted.
---
Perhaps helpful; any thoughts?

The warning comes out before any errors, looking like:

[mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame from a fixed pool of hardware frames.
[mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a larger value (currently set to 8, giving a pool size of 14).
[mpeg2video @ 0x560ff51b4600] get_buffer() failed
[vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @ 0x560ff51b3b40] Error submitting packet to decoder: Operation not permitted

  libavcodec/get_buffer.c | 16 ++++++++++++++++
  libavcodec/internal.h   |  6 ++++++
  2 files changed, 22 insertions(+)

diff --git a/libavcodec/get_buffer.c b/libavcodec/get_buffer.c
index 46c20781af..9b35fde7c6 100644
--- a/libavcodec/get_buffer.c
+++ b/libavcodec/get_buffer.c
@@ -257,6 +257,22 @@ int avcodec_default_get_buffer2(AVCodecContext *avctx, AVFrame *frame, int flags

      if (avctx->hw_frames_ctx) {
          ret = av_hwframe_get_buffer(avctx->hw_frames_ctx, frame, 0);
+        if (ret == AVERROR(ENOMEM)) {
+            AVHWFramesContext *frames_ctx =
+                (AVHWFramesContext*)avctx->hw_frames_ctx->data;
+            if (frames_ctx->initial_pool_size > 0 &&
+                !avctx->internal->warned_on_failed_allocation_from_fixed_pool) {
+                av_log(avctx, AV_LOG_WARNING, "Failed to allocate a %s/%s "
+                       "frame from a fixed pool of hardware frames.\n",
+                       av_get_pix_fmt_name(frames_ctx->format),
+                       av_get_pix_fmt_name(frames_ctx->sw_format));
+                av_log(avctx, AV_LOG_WARNING, "Consider setting "
+                       "extra_hw_frames to a larger value "
+                       "(currently set to %d, giving a pool size of %d).\n",
+                       avctx->extra_hw_frames, frames_ctx->initial_pool_size);
+                avctx->internal->warned_on_failed_allocation_from_fixed_pool = 1;
+            }
+        }
          frame->width  = avctx->coded_width;
          frame->height = avctx->coded_height;
          return ret;
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index 04f7cebdcb..64fe0122c8 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -144,6 +144,12 @@ typedef struct AVCodecInternal {
  #if CONFIG_LCMS2
      FFIccContext icc; /* used to read and write embedded ICC profiles */
  #endif
+
+    /**
+     * Set when the user has been warned about a failed allocation from
+     * a fixed frame pool.
+     */
+    int warned_on_failed_allocation_from_fixed_pool;
  } AVCodecInternal;

  /**
-- 
2.43.0
_______________________________________________
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".

Re: [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

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Xiang, Haihao]

On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
> For hardware cases where we are forced to have a fixed pool of frames
> allocated up-front (such as array textures on decoder output), suggest
> a possible workaround to the user if an allocation fails because the
> pool is exhausted.
> ---
> Perhaps helpful; any thoughts?
> 
> The warning comes out before any errors, looking like:
> 
> [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame from a
> fixed pool of hardware frames.
> [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a larger
> value (currently set to 8, giving a pool size of 14).
> [mpeg2video @ 0x560ff51b4600] get_buffer() failed
> [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @ 0x560ff51b3b40] Error
> submitting packet to decoder: Operation not permitted

I'm OK to print such warning so user may know how to work around it. But now
many cases are impacted by this error (e.g. https://trac.ffmpeg.org/ticket/10856
), I think it is a regression to user. I still prefer to use a dynamic buffer
pool instead fixed frame pool to avoid such error when the dynamic buffer pool
can work.

Thanks
Haihao

> 
>   libavcodec/get_buffer.c | 16 ++++++++++++++++
>   libavcodec/internal.h   |  6 ++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/libavcodec/get_buffer.c b/libavcodec/get_buffer.c
> index 46c20781af..9b35fde7c6 100644
> --- a/libavcodec/get_buffer.c
> +++ b/libavcodec/get_buffer.c
> @@ -257,6 +257,22 @@ int avcodec_default_get_buffer2(AVCodecContext *avctx,
> AVFrame *frame, int flags
> 
>       if (avctx->hw_frames_ctx) {
>           ret = av_hwframe_get_buffer(avctx->hw_frames_ctx, frame, 0);
> +        if (ret == AVERROR(ENOMEM)) {
> +            AVHWFramesContext *frames_ctx =
> +                (AVHWFramesContext*)avctx->hw_frames_ctx->data;
> +            if (frames_ctx->initial_pool_size > 0 &&
> +                !avctx->internal-
> >warned_on_failed_allocation_from_fixed_pool) {
> +                av_log(avctx, AV_LOG_WARNING, "Failed to allocate a %s/%s "
> +                       "frame from a fixed pool of hardware frames.\n",
> +                       av_get_pix_fmt_name(frames_ctx->format),
> +                       av_get_pix_fmt_name(frames_ctx->sw_format));
> +                av_log(avctx, AV_LOG_WARNING, "Consider setting "
> +                       "extra_hw_frames to a larger value "
> +                       "(currently set to %d, giving a pool size of %d).\n",
> +                       avctx->extra_hw_frames, frames_ctx-
> >initial_pool_size);
> +                avctx->internal->warned_on_failed_allocation_from_fixed_pool
> = 1;
> +            }
> +        }
>           frame->width  = avctx->coded_width;
>           frame->height = avctx->coded_height;
>           return ret;
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 04f7cebdcb..64fe0122c8 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -144,6 +144,12 @@ typedef struct AVCodecInternal {
>   #if CONFIG_LCMS2
>       FFIccContext icc; /* used to read and write embedded ICC profiles */
>   #endif
> +
> +    /**
> +     * Set when the user has been warned about a failed allocation from
> +     * a fixed frame pool.
> +     */
> +    int warned_on_failed_allocation_from_fixed_pool;
>   } AVCodecInternal;
> 
>   /**

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Mark Thompson]

On 18/03/2024 05:53, Xiang, Haihao wrote:
> On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
>> For hardware cases where we are forced to have a fixed pool of frames
>> allocated up-front (such as array textures on decoder output), suggest
>> a possible workaround to the user if an allocation fails because the
>> pool is exhausted.
>> ---
>> Perhaps helpful; any thoughts?
>>
>> The warning comes out before any errors, looking like:
>>
>> [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame from a
>> fixed pool of hardware frames.
>> [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a larger
>> value (currently set to 8, giving a pool size of 14).
>> [mpeg2video @ 0x560ff51b4600] get_buffer() failed
>> [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @ 0x560ff51b3b40] Error
>> submitting packet to decoder: Operation not permitted
> 
> I'm OK to print such warning so user may know how to work around it. But now
> many cases are impacted by this error (e.g. https://trac.ffmpeg.org/ticket/10856
> ), I think it is a regression to user. I still prefer to use a dynamic buffer
> pool instead fixed frame pool to avoid such error when the dynamic buffer pool
> can work.

How would we implement this on D3D11 or D3D12?

A way of doing the second in particular would be very useful, because the current decoder only works on a subset of drivers which don't require array textures.

Thanks,

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Xiang, Haihao]

On Ma, 2024-03-18 at 21:33 +0000, Mark Thompson wrote:
> On 18/03/2024 05:53, Xiang, Haihao wrote:
> > On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
> > > For hardware cases where we are forced to have a fixed pool of frames
> > > allocated up-front (such as array textures on decoder output), suggest
> > > a possible workaround to the user if an allocation fails because the
> > > pool is exhausted.
> > > ---
> > > Perhaps helpful; any thoughts?
> > > 
> > > The warning comes out before any errors, looking like:
> > > 
> > > [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame from a
> > > fixed pool of hardware frames.
> > > [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a larger
> > > value (currently set to 8, giving a pool size of 14).
> > > [mpeg2video @ 0x560ff51b4600] get_buffer() failed
> > > [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @ 0x560ff51b3b40]
> > > Error
> > > submitting packet to decoder: Operation not permitted
> > 
> > I'm OK to print such warning so user may know how to work around it. But now
> > many cases are impacted by this error
> > (e.g. https://trac.ffmpeg.org/ticket/10856
> > ), I think it is a regression to user. I still prefer to use a dynamic
> > buffer
> > pool instead fixed frame pool to avoid such error when the dynamic buffer
> > pool
> > can work.
> 
> How would we implement this on D3D11 or D3D12?

I understand not all can support dynamic frame pool, your patch is useful for
decoders using fixed pool. But for driver which doesn't require array textures,
I think we'd be better to use dynamic frame pool instead so user needn't worry
about frame allocation. For example, we may use dynamic frame pool for vaapi
with iHD driver, what do you think about adding a quirk to enable dynamic frame
pool for special drivers ? 

Thanks
Haihao

> 
> A way of doing the second in particular would be very useful, because the
> current decoder only works on a subset of drivers which don't require array
> textures.
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> 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".

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Mark Thompson]

On 19/03/2024 04:16, Xiang, Haihao wrote:
> On Ma, 2024-03-18 at 21:33 +0000, Mark Thompson wrote:
>> On 18/03/2024 05:53, Xiang, Haihao wrote:
>>> On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
>>>> For hardware cases where we are forced to have a fixed pool of frames
>>>> allocated up-front (such as array textures on decoder output), suggest
>>>> a possible workaround to the user if an allocation fails because the
>>>> pool is exhausted.
>>>> ---
>>>> Perhaps helpful; any thoughts?
>>>>
>>>> The warning comes out before any errors, looking like:
>>>>
>>>> [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame from a
>>>> fixed pool of hardware frames.
>>>> [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a larger
>>>> value (currently set to 8, giving a pool size of 14).
>>>> [mpeg2video @ 0x560ff51b4600] get_buffer() failed
>>>> [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @ 0x560ff51b3b40]
>>>> Error
>>>> submitting packet to decoder: Operation not permitted
>>>
>>> I'm OK to print such warning so user may know how to work around it. But now
>>> many cases are impacted by this error
>>> (e.g. https://trac.ffmpeg.org/ticket/10856
>>> ), I think it is a regression to user. I still prefer to use a dynamic
>>> buffer
>>> pool instead fixed frame pool to avoid such error when the dynamic buffer
>>> pool
>>> can work.
>>
>> How would we implement this on D3D11 or D3D12?
> 
> I understand not all can support dynamic frame pool, your patch is useful for
> decoders using fixed pool. But for driver which doesn't require array textures,
> I think we'd be better to use dynamic frame pool instead so user needn't worry
> about frame allocation. For example, we may use dynamic frame pool for vaapi
> with iHD driver, what do you think about adding a quirk to enable dynamic frame
> pool for special drivers ?

I think we should come to a conclusion on what the generic code for this case should be and then consider whether any special cases are needed.

When compared to the state right now, I agree with you that just switching VAAPI to always be dynamic would probably be better just to avoid the nasty failures, but given that we need to improve the situation for cases (like D3D11) where we don't have an ad-hoc workaround we should be comparing to whatever that concludes rather than the broken state right now.

Thanks,

- Mark
_______________________________________________
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".

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

[Mark Thompson]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Xiang, Haihao]

On Di, 2024-03-19 at 22:52 +0000, Mark Thompson wrote:
> On 19/03/2024 04:16, Xiang, Haihao wrote:
> > On Ma, 2024-03-18 at 21:33 +0000, Mark Thompson wrote:
> > > On 18/03/2024 05:53, Xiang, Haihao wrote:
> > > > On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
> > > > > For hardware cases where we are forced to have a fixed pool of frames
> > > > > allocated up-front (such as array textures on decoder output), suggest
> > > > > a possible workaround to the user if an allocation fails because the
> > > > > pool is exhausted.
> > > > > ---
> > > > > Perhaps helpful; any thoughts?
> > > > > 
> > > > > The warning comes out before any errors, looking like:
> > > > > 
> > > > > [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame
> > > > > from a
> > > > > fixed pool of hardware frames.
> > > > > [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a
> > > > > larger
> > > > > value (currently set to 8, giving a pool size of 14).
> > > > > [mpeg2video @ 0x560ff51b4600] get_buffer() failed
> > > > > [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @
> > > > > 0x560ff51b3b40]
> > > > > Error
> > > > > submitting packet to decoder: Operation not permitted
> > > > 
> > > > I'm OK to print such warning so user may know how to work around it. But
> > > > now
> > > > many cases are impacted by this error
> > > > (e.g. https://trac.ffmpeg.org/ticket/10856
> > > > ), I think it is a regression to user. I still prefer to use a dynamic
> > > > buffer
> > > > pool instead fixed frame pool to avoid such error when the dynamic
> > > > buffer
> > > > pool
> > > > can work.
> > > 
> > > How would we implement this on D3D11 or D3D12?
> > 
> > I understand not all can support dynamic frame pool, your patch is useful
> > for
> > decoders using fixed pool. But for driver which doesn't require array
> > textures,
> > I think we'd be better to use dynamic frame pool instead so user needn't
> > worry
> > about frame allocation. For example, we may use dynamic frame pool for vaapi
> > with iHD driver, what do you think about adding a quirk to enable dynamic
> > frame
> > pool for special drivers ?
> 
> I think we should come to a conclusion on what the generic code for this case
> should be and then consider whether any special cases are needed.
> 
> When compared to the state right now, I agree with you that just switching
> VAAPI to always be dynamic would probably be better just to avoid the nasty
> failures, but given that we need to improve the situation for cases (like
> D3D11) where we don't have an ad-hoc workaround we should be comparing to
> whatever that concludes rather than the broken state right now.

Hi Mark,

I agree with you. Will you push this patch ? We may switch to dynamic frame pool
for vaapi cases later after pushing your patch. 

Thanks
Haihao


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v3 2/2] lavc/get_buffer: Add a warning on failed allocation from a fixed pool

[Mark Thompson]

On 25/03/2024 07:35, Xiang, Haihao wrote:
> On Di, 2024-03-19 at 22:52 +0000, Mark Thompson wrote:
>> On 19/03/2024 04:16, Xiang, Haihao wrote:
>>> On Ma, 2024-03-18 at 21:33 +0000, Mark Thompson wrote:
>>>> On 18/03/2024 05:53, Xiang, Haihao wrote:
>>>>> On So, 2024-03-17 at 20:51 +0000, Mark Thompson wrote:
>>>>>> For hardware cases where we are forced to have a fixed pool of frames
>>>>>> allocated up-front (such as array textures on decoder output), suggest
>>>>>> a possible workaround to the user if an allocation fails because the
>>>>>> pool is exhausted.
>>>>>> ---
>>>>>> Perhaps helpful; any thoughts?
>>>>>>
>>>>>> The warning comes out before any errors, looking like:
>>>>>>
>>>>>> [mpeg2video @ 0x560ff51b4600] Failed to allocate a vaapi/nv12 frame
>>>>>> from a
>>>>>> fixed pool of hardware frames.
>>>>>> [mpeg2video @ 0x560ff51b4600] Consider setting extra_hw_frames to a
>>>>>> larger
>>>>>> value (currently set to 8, giving a pool size of 14).
>>>>>> [mpeg2video @ 0x560ff51b4600] get_buffer() failed
>>>>>> [vist#0:0/mpeg2video @ 0x560ff5199840] [dec:mpeg2video @
>>>>>> 0x560ff51b3b40]
>>>>>> Error
>>>>>> submitting packet to decoder: Operation not permitted
>>>>>
>>>>> I'm OK to print such warning so user may know how to work around it. But
>>>>> now
>>>>> many cases are impacted by this error
>>>>> (e.g. https://trac.ffmpeg.org/ticket/10856
>>>>> ), I think it is a regression to user. I still prefer to use a dynamic
>>>>> buffer
>>>>> pool instead fixed frame pool to avoid such error when the dynamic
>>>>> buffer
>>>>> pool
>>>>> can work.
>>>>
>>>> How would we implement this on D3D11 or D3D12?
>>>
>>> I understand not all can support dynamic frame pool, your patch is useful
>>> for
>>> decoders using fixed pool. But for driver which doesn't require array
>>> textures,
>>> I think we'd be better to use dynamic frame pool instead so user needn't
>>> worry
>>> about frame allocation. For example, we may use dynamic frame pool for vaapi
>>> with iHD driver, what do you think about adding a quirk to enable dynamic
>>> frame
>>> pool for special drivers ?
>>
>> I think we should come to a conclusion on what the generic code for this case
>> should be and then consider whether any special cases are needed.
>>
>> When compared to the state right now, I agree with you that just switching
>> VAAPI to always be dynamic would probably be better just to avoid the nasty
>> failures, but given that we need to improve the situation for cases (like
>> D3D11) where we don't have an ad-hoc workaround we should be comparing to
>> whatever that concludes rather than the broken state right now.
> 
> Hi Mark,
> 
> I agree with you. Will you push this patch ? We may switch to dynamic frame pool
> for vaapi cases later after pushing your patch.

Ok, pushed.

Thoughts:

* Passthrough filter chains can have the two queues worth of frames stored (when e.g. only editing metadata).
   * Possibly fixable by just doubling the extra frames added?  That seems very clumsy.

* Filters have no idea how big to make a pool when it is fixed.
   * lavfi would need negotiation backwards through the whole graph to make this work.

* Encoders have no way to signal that they want to hold on to more frames.
   * New API needed?
   * Without negotiation in lavfi it would be hard to work out in ffmpeg /which/ pool to increase the size of in the presence of nontrivial filter graphs, though.

* V4L2 M2M decode does not use hwcontext, but suffers from the same problems with a fixed pool.
   * Not updated since e0da916, probably broken by it in some cases.  (Does warn with the ad-hoc workaround option "num_capture_buffers".)
   * Does anyone ever use this in the ffmpeg utility?

Thanks,

- Mark
_______________________________________________
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".