[vlc-devel] [PATCH 00/19] picture cleaning
ajanni at videolabs.io
Thu Jul 30 14:54:29 CEST 2020
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
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.
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?
> 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?
> 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.
> 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).
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.
> 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 ?
> 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
> 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: 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(-)
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
More information about the vlc-devel