[vlc-devel] [PATCH] core: add a callback to init/release data for picture pool of opaque formats

[Steve Lhomme]

On Mon, Apr 20, 2015 at 4:14 PM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> Le 2015-04-20 17:01, Steve Lhomme a écrit :
>>
>> On Mon, Apr 20, 2015 at 3:43 PM, Rémi Denis-Courmont <remi at remlab.net>
>> wrote:
>>>
>>> Le 2015-04-20 16:25, Steve Lhomme a écrit :
>>>>
>>>>
>>>> a gargabe collector callback is introduced for the global release
>>>> after all pictures are released from the pool
>>>
>>>
>>>
>>> I still think this is completely useless. And on top of that, it's very
>>> invasive.
>>
>>
>>
>> I'm open to other working solutions. I think it makes sense that a
>> pixel format comes with the instructions on how to handle it pixels.
>
>
> And I think it makes no sense for reasons already outlined on this list.
>
>> The tricky part in the case of D3D9 compared to the others is that the
>> surface passed from the decoder to the vout need to be created from
>> the same D3D9 device handle. So this handle needs to be shared one way
>> or another between the decoder and the vout.
>
>
> And I already stated that the video output, not the decoder, should allocate
> the device in that case.

This is not possible in the case of avcodec's dxva decoder.
When vlc_va_Setup() is called, we need to fill the dxva_context with
the surfaces it can use internally. That's way before a vout is
created. So I designed my solution around this constraint. The other
tricky one being the D3D9 device having to be shared between the
picture pools.

Also if you use a D3D11 vout, you can't expect it to allocate D3D9 surfaces.

>> First I went with each individual picture_t being passed some blob to
>> handle extra init. But in the end we allocate picture_t in a pool so a
>> "pool_setup_t" made more sense.
>
>
> All the possible customization for allocating and freeing picture from pools
> is already there.

No, all you get to initialize a picture is a chroma and some sizes.
There's no object instance you can use in video_format_t so
picture_NewFromFormat() is not an option. And that's what is used in
vout_InitWrapper() to create the decoder pool (in this case the
decoder already has its pool), the display pool (which needs to be
initialized with the same handles as the decoder pool).

> Pool-wide resources can already be garbage collected with a reference
> counter in the picture release callback. Adding a convenience callback is
> possible. But your patch is much much more invasive than necessary for that
> purpose.
>
>> I think in general some deeper changes could be made to picture_t and
>> picture_pool_t so they don't assume that their data are in planes
>> allocated in CPU memory.
>
>
> The only assumption was that planes are mapped in CPU address space, not
> necessarily in main memory. And that assumption was already removed when
> support for chromas with zero- planes was added.

There remains the assumption that pixels/surfaces can be allocated out
of thin air and copied between picture_t. This is not the case with
D3D9 surfaces. They need a D3D9 device (where does it come from) and
if 2 surfaces use a different D3D9 device, they can't be copied
between each other.