[vlc-devel] [PATCH 17/17] video_output: restart display after filter change

[Rémi Denis-Courmont]

Le mercredi 25 novembre 2020, 09:05:01 EET Steve Lhomme a écrit :
> On 2020-11-24 16:37, Rémi Denis-Courmont wrote:
> > Le lundi 23 novembre 2020, 14:21:29 EET Steve Lhomme a écrit :
> >> On 2020-11-22 10:22, Rémi Denis-Courmont wrote:
> >>> Le perjantaina 20. marraskuuta 2020, 16.45.07 EET Steve Lhomme a écrit :
> >>>> This is done in the vout thread loop, immediately after the filter
> >>>> output
> >>>> change is detected, before processing pictures coming out of the
> >>>> filters.
> >>>> The filters are untouched (not flushed).
> >>>> 
> >>>> The new display module is created with the format coming out of the
> >>>> last
> >>>> filter.
> >>> 
> >>> As you have pointed out yourself, this causes glitches if the windowing
> >>> system is not capable of retaining the window content while restarting
> >>> the display. AFAICT; without this patch, it works just fine: the
> >>> conversion filter chain is adjusted and the display has nothing to worry
> >>> about.
> >> 
> >> It doesn't work *just fine*, an extra filter is added to match the
> >> original display format.
> > 
> > Itss removing the old conversion and adding a new conversion. That's
> > working fine and as intended.
> 
> No. The osys->converter is untouched.

Fair enough. But IMO, it should not. As long as the chroma is the same, the 
change should be handled with source-change controls, and if the chroma 
changes, then we should first try to adjust osys->converter and only if that 
fails, restart the display.

> >> You said yourself that the display should be restarted when the source
> >> changes and now you're against it because it causes glitches ?
> > 
> > I wrote that, if it is somehow necessary to change the video format of a
> > display in a way that is not currently permitted by the change controls,
> > the display should be restarted, e.g. the "chroma" or pixel format.
> 
> This definition of change is completely arbitrary.

No. It's a convention, and all conventions have a level of arbitrary. But that 
convention is informed by years of experience and several rewrites of the 
video output - especially the previous one by Laurent and myself.

We have to have the same rules for all display plugins, and we also have to 
worry complexity of the core and plusin and overall performance.

So far the rules are that changing the source crop or A/R must be supported by 
the display, and nothing else. We could probably consider adding more change 
controls for select properties. Orientation and colorimetry come to mind. We 
should also clarify how the push model affects i_width and i_height.

> Even a chroma/pixel format will be handled just fine by OpenGL/D3D11 with
> small changes in the pixel shaders.

There are several problems with changing the pixel format:
1) Currently, it affects the probing. We'd have to audit all displays not to 
succeed/fail based on the input pixel format first.
2) For the sake of GPU filters, we have some, and will need more, CPU->GPU 
pixel format converters. Duplicating the conversion code in display plugins is 
undesirable.
3) I fear that too wild and drastic change of pixel formats will just break 
the conversion chain and expose bugs in displays.

> Changing from rectangular to 360 video
> on the other hand is just one flag changed but may not be handled properly
> (dunno if Vulkan handles it yet for example).

The current handling of projection is half-assed. I don't know if it needs a 
source change control, a display restart or what, but it needs to be cleaned 
up regardless of this argument. A conversion filter is also necessary for those 
displays that will never support non-rectangular projections.

-- 
Rémi Denis-Courmont