[vlc-devel] [PATCH v1 06/33] filters: allow filters to output multiple pictures without chaining them

[Steve Lhomme]

On 2020-09-26 8:21, Rémi Denis-Courmont wrote:
> Le lauantaina 26. syyskuuta 2020, 8.58.15 EEST Steve Lhomme a écrit :
>> The owner is one special kind of sink which is attached (to the decoder)
>> for good.
> 
> The owner is just a name for the structure that carries callbacks for the
> module to use.

But it doesn't carry the same concept. You probably don't want to change 
the owner once your filter is created. Whereas you can redirect the 
output with every call.

This can be done via a fixed owner too. But that's the kind of 
"centralization" I mentioned earlier which IMO adds too much complexity 
in the end. For example doing it in filter_chain would be a PITA.

>> Filters on the other hand can tell for sure where they have to
>> send their output.
> 
> Neither decoders nor filters know what happens to their output. That's the
> owner's job to manage. There's no differences there.
> 
>> It can be dynamically changed in a filter chain if
>> you append another filter after this filter.
> 
> That's the filter chain's problem to handle, in other words the owner.
> 
>> I think the sink concept gives more flexibility and less complexity on
>> both sides.
> 
> The sink concept hardly gives more flexibility on the owner. The owner could
> just as well use a sink behind the owner callback to the exact same effect
> (just with a little extra boiler plate), if it needed to.
> 
> But on the filter side, this drastically limits flexibility on the filter. It
> makes asynchronous filtering outright impossible, and threaded filtering very
> inefficient and potentially flaky (as was threaded decoding before asynchronous
> decoding became supported).

The sink doesn't prevent asynchronous use of filters. In fact it makes 
it more asynchronous. If you look at filter_FilterSingle() it's exactly 
like turning an aysnchronous call (sink) into a synchronous one 
(returning synchronously a picture).

Sinks are what people use when you want a proper asynchronous system 
using plenty of threads (I looked for the "owner" design pattern but 
never found anything, maybe it's known by another name). For now the 
sink is tightly linked to the owner, but it may change in the future.

As for the multithreaded, I thought about it when the executor API was 
discussed. Being able to send picture+filter chain to process in an 
executor would be great. Or even picture+single filter. The main problem 
is the order of the output. There is no guarantee that one thread will 
not finish after another. That could be solved by chaining filtering via 
executor, when a job finishes the next one is sent until the chain is 
finished. That's why being able to chain jobs/tasks is important IMO.

Now there would still be a problem if 2 consecutive filter chains are 
sending jobs. The second one might finish before the first one.

>>> Queueing multiple pictures at a time seems like a useless intricacy for
>>> every implementation of the interface.
>>
>> But that already exists in the code. deinterlacers and fps filter do
>> output multiple pictures in one call.
> 
> Yes but:
> 
> 1) On output side, this is currently necessary due to passing by return value.
> It is no longer needed if passing by callback.
> 
> 2) On input side, this was never allowed, and it should not be because it just
> adds complexity to every filter.

Correct, although it was not enforced nor enforceable. That's why I'm 
being doing all the tooling about picture_t::p_next. First to see who 
does what (I found 2 issues) and then to clean it. At the end of my 
branch I changed it to a vlc_list to check everything is clean now. I 
may not submit it but there is no more unclean usage and it can be enforced.

>> And the receiver has to be ready to handle that.
> 
> AFAIR, the filter chain already has a queue on pending pictures. Though that's
> only really needed because it doesn't support asynchronous filtering yet.

Yes and no. It's still tricky if the user filter pushes more than one 
picture. In the current video output it's handled through the "pending" 
pictures that the chain keeps for the next call with a NULL. In push we 
shouldn't do that.