[vlc-devel] [PATCH 00/19] picture cleaning

[Steve Lhomme]

On 2020-07-30 14:54, Alexandre Janniaux wrote:
> Hi,
> 
> I'm not at ease with this patchset, it feels like removing the
> picture_resource_t layer and doing it by hand. I'm not sure I

This is already entirely done by hand. Instead of putting the fields in 
picture_resource_t you put them in the picture directly. That seems like 
an improvement to me.

> understand the cases you're mentionning, can you detail it?
> The picture allocation functions matched my needs, be it when
> writing decoders, encoders, or filters.

The magic done in picture_NewFormFormat is unchanged (externally). 
picture_NewFromResource has nothing convenient right now and doesn't 
pretend to after this patch.

Before you have to set your planes in picture_resource_t before creating 
the picture, and deal with the case where the picture allocation failed. 
Now you allocate the picture and set the planes after. It's even 
possible to create those planes after the picture has set its preferred 
alignment values, which is impossible right now.

> I'll provide more remarks inline:
> 
> On Thu, Jul 30, 2020 at 02:16:41PM +0200, Steve Lhomme wrote:
>> There is currently 2 ways to create pictures: picture_NewFromFormat and
>> picture_NewFromResource. But in many cases what these functions do is
>> not what the caller wants. For example it may want a picture with no allocation
>> but with the planes setup correctly (fake pictures with a hardware buffer in
>> some display modules, d3d9/d3d11 CPU<>GPU conversion).
> 
> The example you're giving is exactly the kind of case requiring
> picture_NewFromResource, isn't it?

They use picture_NewFromResource.

>> This forces call with some dummy values that are copied for nothing and never
>> used. It also requires knowing what will picture_NewFromResource() do
>> internally to do the proper call.
> 
> What dummy values?

NULL destroy callbacks, NULL p_sys, empty planes. Sometimes all that at 
once. That's unneeded boilerplate code.

>> These set of patches reshuffle the picture allocation API to avoid those hacks
>> and avoid some unecessary casts. Now the planes that were passed to
>> picture_NewFromResource() are set outside of this function, after the picture
>> allocation worked and the planes are setup. The caller can set an optional
>> "garbage collection" callback to release the resources when the picture is
>> no longer used. This is optional, although it should be mandatory for pictures
>> that are pushed (so they don't rely on their creator to still be alive to
>> release the resources).
> 
> In the model before this patchset, the caller usually rely on
> picture_context and video_context to provide exactly this. It
> didn't work with picture_pool_t but I sent a patch to have it
> working in this case too.

No it doesn't "usually" work like that. See all the non core files 
modified in the patchset.

>> The destructor callback doesn't rely on picture->p_sys anymore so in many
>> cases that value doesn't need to be set anymore.
> 
> This value previously held vout data in the pull model, if
> I am not wrong. I still use it in scenario where the vout
> binds an object (eg. wl_buffer) to an underlying object in
> the picture (eg. a GBM buffer).

This is unchanged. Instead of putting the value in picture_resource_t 
you put it in the picture_t. That seems more logical to me.

> But I'm not sure why the allocation with NewFromFormat is
> going through this sys instead of accessing it through the
> picture private data through a container_of.

Because the p_sys is visible to some (Linux) modules that can access the 
picture_buffer_t directly. By the was it seems the base/size fields 
could be moved outside of this structure as they don't seem to be used 
by any module.

>> After this cleanup only picture.c knows what is inside a picture_priv_t. The
>> picture_pool doesn't need to know about it anymore.
> 
> I agree this goal could be nice. Can't it just be done by
> allowing to define picture_resource_t::gc instead ?
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
>>
>> Steve Lhomme (19):
>>    vout: kms: don't use a custom picture destroy
>>    picture_pool: factorize the code to clone a picture for Wait/Get
>>    picture: pass the clone destructor via a picture_gc_t structure
>>    picture: move the resource init outside of picture_InitPrivate
>>    picture: do not use picture_NewFromResource for picture_InternalClone
>>    picture: use picture_DestroyDummy for picture_NewFromFormat() with no
>>      planes
>>    picture: allow NULL gc callback
>>    picture: pass a picture_gc_t directly to picture_NewFromResource()
>>    picture: fix indentation
>>    vout: opengl: do not setup the picture planes a second time
>>    vout: opengl: keep the number of bound planes in picture_sys_t
>>    chroma: copy: set the picture test buffers after
>>      picture_NewFromResource()
>>    picture: pass the opaque pointer to the destroy callback rather than
>>      the picture
>>    picture: make picture_priv_t private to picture.c
>>    vt_utils: set the picture planes once the picture is created
>>    vout: fb: set the picture planes once the picture is created
>>    vout: kms: set the picture planes once the picture is created
>>    vout: vmem: set the picture planes once the picture is created
>>    picture: remove unused resource parameter from picture_NewFromResource
>>
>>   include/vlc_picture.h                    |  24 ++---
>>   include/vlc_picture_pool.h               |   2 +-
>>   modules/codec/vt_utils.c                 |  33 ++++---
>>   modules/hw/d3d11/d3d11_surface.c         |   4 +-
>>   modules/hw/d3d9/dxa9.c                   |   4 +-
>>   modules/hw/nvdec/nvdec.c                 |   6 +-
>>   modules/hw/vaapi/vlc_vaapi.c             |  13 +--
>>   modules/hw/vdpau/picture.c               |  11 +--
>>   modules/video_chroma/copy.c              |  50 ++++++++--
>>   modules/video_output/android/display.c   |  12 +--
>>   modules/video_output/fb.c                |  17 +---
>>   modules/video_output/kms.c               |  37 ++------
>>   modules/video_output/opengl/interop_sw.c |  34 +++----
>>   modules/video_output/vmem.c              |  15 ++-
>>   modules/video_output/win32/direct3d11.c  |  11 +--
>>   src/misc/picture.c                       | 113 +++++++++++------------
>>   src/misc/picture.h                       |  12 +--
>>   src/misc/picture_pool.c                  |  28 +++---
>>   18 files changed, 185 insertions(+), 241 deletions(-)
>>
>> --
>> 2.26.2
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>