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

[Alexandre Janniaux]

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

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