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

[Steve Lhomme]

On 2020-11-25 19:47, Rémi Denis-Courmont wrote:
> 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.

Or a broader format_update callback which the display could decide to 
accept or reject. Just like they are allowed to reject a 
VOUT_DISPLAY_CHANGE_xxx control.

In the end there's a balance between controls and a callback. With a 
control you call tell exactly the one thing that changed. If many 
parameters change you need to make many control changes. Further 
delaying the handling of source changes.

With a callback you can handle all changes at once but the receiver must 
find out what changed, in most cases it's just applying the new format 
without even wondering what changed.

>>>> 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

Yes, the converter is a special kind of filter, it's a "video converter" 
rather than a "video filter". That differentiation will still be needed 
no matter what. Not all display will be able to handle all pixel 
formats. This patchset doesn't change that. The update_format callback 
updated the converter according to the new preferred input of the display.

> convention is informed by years of experience and several rewrites of the
> video output - especially the previous one by Laurent and myself.

You're talking about a time when there was no "push" and hardly any GPU 
handling. We're in unknown territory (in the context of VLC).

> 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

It's not a hard *must*. They are free not to support those format 
changes and request a format reset, which also resets the 
osys->converters to match the new display format. That's pretty much the 
update_format callback process but in 2 steps (or more, see above).

> 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.

I 100% agree. For example in D3D11/9 I'd like the GPU upload to be done 
outside of the display and only accept D3D11/9 textures.

> 3) I fear that too wild and drastic change of pixel formats will just break
> the conversion chain and expose bugs in displays.

IMO we already have all the tools to handle this.

>> 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
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>