[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