[vlc-devel] [PATCH 02/26] vout: create/release the decoder device when the window is enabled/disabled

[Steve Lhomme]

On 2019-09-20 21:02, Rémi Denis-Courmont wrote:
> Le perjantaina 20. syyskuuta 2019, 17.28.32 EEST Steve Lhomme a écrit :
>> The decoder device ("dec-dev") should be tied to the lifecyle of the
>> active/enabled window.
> 
> Not really. It is bound by that the lifetime of a window enablement, *but*
> nothing says it has to be as long as the window enablement as such. Some
> hardware APIs are horribly slow. If you create a decoder device even when it's
> not needed, there'll likely be performance problems.

It's like the display module, it's treated as a resource that is created 
on demand by the decoder. In the past we need a display module to create 
the decoder/VA (depending on the display pictures). Now the VA needs a 
"decoder device" (the hint) before it's created.

It's true that most decoders (ie not lavc or nvdec) will not need a 
decoder device at all. And I agree we should not create it for those. 
This can be handled in the "hold_device" callback (patch 08/26). If the 
vlc_decoder_device ** passed in NULL then no decoder device should be 
created. It can be done in the generic decoder_UpdateVideoFormat() 
function. lavc and nvdec call the hold_device explicitly (14/26 and 
22/26 respectively).

In an ideal world hold_device would not be called at all in those cases. 
But since the init order of many things in the display init rely on 
things that need to be done in ModuleThread_HoldDecoderDevice(), in 
particular the vout thread. So for now I keep it like that. It can be 
cleaned later.

In the end decoders that don't need a decoder device don't see it and 
won't create one, one I change 08/26 not to request one at all.

> Besides, this won't work if there are more than one candidate decoder device
> plugin for the active window, and the right one depends on the decoder plugin.
> As much as I wish we could ignore that case, I doubt that it's realistic
> especially now that NVDEC is added.

This is a case we already had between D3D11/D3D9 to pick the right one 
automatically. At some point we make a choice what we think is best for 
which platform and hardcode that. For NVDEC it will probably have a 
lower priority than the D3D11/D3D9 on Windows because the API requires 
an extra copy. It may be different on other OSes.

For now the decoder is picked before the "decoder device" so there can 
be a mismatch there too. Maybe the decoder should always give a hint of 
what kind of "decoder device" it wants. For example NVDEC could tell 
that it prefers a nvdec "decoder device". If it doesn't get one it means 
NVDEC should probably not be used and the next decoder should be used 
(ie NVDEC returns an error on Open()).

>> It won't change during the lifetime of the window enabled state.
> 
> That does not sound like a safe assumption at all to me.

I meant the other way around. The window enabled state won't change 
during the lifetime of the decoder device. When the window is disabled, 
the cached "decoder device" is released. If a decoder needs one, a new 
one will be created.

>> This decoder device is stored in the vout private structure.
> 
> I don't see what the *decoder* device has to do there, if not for the fact it
> is created from the window, and the window is still allocated by the vout
> instead of the player.

This will use the same system as caching the vout thread in the decoder.

> AFAIU, the decoder should pass the video context to the vout when enabling it.

That's why I sent this patchset. To do this, the decoders have to create 
a video context. Once we reach this point, then the video context can be 
used downstream.