[vlc-devel] Rant about VLC video outputs

[Laurent Aimar]

Hi,
On Mon, Jan 26, 2009, Rémi Denis-Courmont wrote:
> This is just a list of thoughs on the current state of the video ouput 
> subsystem. These came mostly while writing the (still very incomplete) XCB 
> video output. I do not expect, nor do I want, a rework of the video outputs 
> one week before the feeature freeze. But I'd rather brain dump on the mailing 
> list that forget about these.
 Me too, I will not do anymore vout changes for 1.0

> * The developper documentation is way out of date, and mostly helpless. This 
> is probably not specific to video output ;)
> 
> * There is some confusing overlap between picture heaps and video format 
> fields. Laurent hinted he would clean that up later.
 Yep.

> * pf_control is used from callbacks and third party threads. This requires the 
> video output plugin to handle mutual exclusion internally. This is 
> inconsistent with all other object types that I know. Also, this has 
> accounted for deadlocks and infinite callback loops (I won't regret the 
> suxxor thread).
> 
> * A bunch of parameters are set somehow (hopefully by the video output core 
> thread), and then signaled via a change bits mask to the Manage function. I 
> wondered why this is not done directly via pf_control. Then I realized 
> pf_control was for asynchronous requests (see previous comment).
> 
> * Generally speaking there seem to be way too many public parameter inside the 
> vout_thread_t. It is not very clear who sets and gets what and under what 
> locking rules.
 It seems to me that vout_thread_t contains way too much. It does the vout module
management AND the vout thread management. I thing there should be 2 objects for
that:
 - a vout_thread_t that does the generic vout works (wait for the right time,
 filters managment, video heaps, subtitles(at least some part), mouse, etc).
 - a vout_display_t (whatever the name) that would be implemented by a module
and that does ONLY the specific video output part (x11 calls, or gl calls ...)

 That changes would simplify every vout module AND will hide more internal values
with a clean/simple API.

 The current state looks like as if we had input_thread_t and demux_t merged
for example.

> * Snapshots are really poorly designed. For instance, if more than one thread 
> requests a snapshot simultaneously, boom. Olivier partially fixed this today. 
> On the other hand, it added to the vout_thread_t mess (see previous comment).
> 
> * X11 screen access seems not to use shared memory (MIT-SHM). This may 
> *partially* account for its horrible performance.
 That's specific to screen, a ticket could be added.

> * The use of pf_init and pf_end in addition to pf_open and pf_close is 
> confusing. pf_open is supposed to probe, and pf_init is supposed to 
> initialize the buffers. That does not work too well, since some errors are 
> only detected in pf_init (verified with the X11 output). When this happens, 
> there is no video at all.
 It is there for when a heavy change in the configuration is needed, ie the
one that changes the video buffer but not the widget (like resize for an X11
only module, without xv support).
 Now, a simple pf_control that would only control the vout_display_t would
be enought I think.

> I assume that was meant to handle recycling inspite of different input 
> formats, but I am not sure if that makes so much sense. Calling pf_end then 
> pf_init will cause flickering anyway.
 As the widget is not destroyed (at least for X11), you do not have flickering.
I don't think it is used for recycling (it could have been but not).
 It is used when you have a widget resize and that resize changes the vout
buffers size.

> * vout_thread_t.pf_manage seems to suffer from the same problem as 
> demux_t.pf_demux, but worse. It can -obviously- not sleep, but I suspect it 
> does not get called if there is no new picture. If that is true, then we 
> could not implement say XDamage, which would precisely make sense when there 
> is no new picture for an extended period of time. Then again, mouse seems to 
> work on a static DVD menu, so I guess I'm wrong.
 The vout thread workaround this problem by repeating the last picture regularly
when too much time have elapsed. It is probably needed for displaying subtitles
anyway.

> * Screen-saver preemption should perhaps be done directly by the video 
> outputs... ? In most cases, the preemption is dependent on the platform 
> consistent with the video output. Also, it makes no sense to prevent 
> screensaving when using say the vmem plugin.
 No idea on this one.

> * I wonder if we distinguish HWND providers from XID providers. At least, in 
> theory, one could have an X11 output on Windows. To split hairs further, one 
> could in principle use the D3D output against libwine. In these case, using 
> the same vout_RequestWindow() API does not work.
 ?

> * Static image DVD menus seem to be broken again.
 It might be, but it is probably an input issue.
 Anyway, if it had worked, it was by pure luck anyway ;)

> * I am suspicious about the new invmem plugin being a dummy codec rather than 
> a dummy demux. rawvideo is a demux.
 I don't know what it does. If it is to inject data, doing it as a demux
would be cleaner BUT a bit slower (you would have a copy from the demux output
to a picture_t). But I am not sure it would be critical enough to avoid doing it
as a demux if cleaner/simpler.

For more remarks:
 The current way vout pictures are handled forbids a clean video filter chain.
The result of a filtered picture and the picture comming from the decoder simply
share the same pools, loosing the source.
 This prevent filtering only one time a picture and worst, prevent a filter from
creating multiple pictures from one. For example, because of that, deinterlacer
that doubles the frame rate (bob mode), has to be implemented as vout filter, which
is plain ugly, slower, and cause a lot of problems.

 It also prevents correct dynamic pools extension when more frames are needed (like
with h264 where up to 16 frames have to be kept as references) making 
direct rendering prone to deadlock for such cases.

 The pf_control calls are ugly, they should be splitted in two (to follow the 2 parts
I proposed).

 There are probably other things I will find later ;)

-- 
fenrir