[vlc-devel] [PATCH 12/14] filter_chain: add a default implementation for the video_allocator

[Steve Lhomme]

On 2019-07-30 19:12, Rémi Denis-Courmont wrote:
> Le tiistaina 30. heinäkuuta 2019, 10.14.17 EEST Steve Lhomme a écrit :
>> On 2019-07-29 21:18, Rémi Denis-Courmont wrote:
>>> Le maanantaina 29. heinäkuuta 2019, 22.08.58 EEST Alexandre Janniaux a
> écrit :
>>>> While I agree that we probably don't need flow control and that image
>>>> number is probably dominated, the starvation issue seems a real issue to
>>>> tackle. What are the solution against starvation on Windows ? Should we
>>>> just fail if we got starved ?
>>>
>>> In general, it is the halting problemm which is to say that the general
>>> case cannot be solved and assumptions have to be made.
>>>
>>> If we need to make assumptions, we might just as well assume that filters
>>> are well-behaved, as indeed they have so far been. Ideally, a filter that
>>> needs to allocate surfaces (rather than modify input surface in-place)
>>> should allocate a suitably large pool for itself. And that turns out
>>> incompatible with filter_NewPicture().
>>
>> I don't see how. This particular patch adds the possibility for filters
>> to provide their own (output) allocator. A filter could very well create
>> a pool and have its allocator pick pictures in that pool. That still
>> goes through filter_NewPicture, because the filter doesn't know if it
>> has to use pictures from the outside or it can use its own.
> 
> That makes no sense. The "outside" cannot know if the filter allocates or not.
> Already now, both cases have to work - it has more or less always been that
> way with filters.

It works through the use of picture_pool_OwnsPic() and picture_Copy(). 
It should be the job of the last filter (a converter) to handle this if 
necessary.

> So far only converters could be assumed to allocate pictures

Most filters need to allocate pictures. You don't want to modify 
pictures still used as reference frames by the decoder.

> - and in the push
> model, that would be from the video context, not from the downstream/
> filter_NewPicture().

So you want to attach an allocator in each video context ?

>>
>>> Since the filter chain is (partially) dynamic, we cannot even rely on
>>> filters telling how many "extra" surfaces they need - the total value
>>> could change in the middle of the stream. Instead, the filter ought to
>>> allocate its surfaces during initialization, and potentially refuse to
>>> start if it cannot succeed - but not break the whole pipeline.
>>
>> I'm not sure we can estimate this amount easily. For example what would
>> be the amount an adjust filter has to allocate ?
> 
> The adjust filter needs 0 surfaces since it operates in place.

Definitely not :
http://git.videolan.org/?p=vlc.git;a=blob;f=modules/video_filter/adjust.c;h=e184a9fc70b4dca7d9229be70e3446448ab14b63;hb=HEAD#l243
http://git.videolan.org/?p=vlc.git;a=blob;f=modules/video_filter/adjust.c;h=e184a9fc70b4dca7d9229be70e3446448ab14b63;hb=HEAD#l461

> Point being anyway that only the filter can know that, if anything. Since the
> filter chain is dynamic, it cannot be taken into account when the decoder and
> display are set up. 

That's a further step from where I am. At some point adding/removing a 
filter (not a converter) might recreate the display module. But that's 
not handled yet. It involves tweaking in the dual filter chain that we 
currently use.

> It may be that even the filter does not know how many
> pictures it needs, but then there is nothing to fix; it will just maybe or
> maybe not work by allocating on the fly.

That I agree with. At some point there will always be resource limits 
(imagine filtering 8K sources) we still have to handle this gracefully 
(not crash and currently not display anything).