[vlc-devel] [PATCH 00/19] picture cleaning
Alexandre Janniaux
ajanni at videolabs.io
Thu Jul 30 16:13:59 CEST 2020
On Thu, Jul 30, 2020 at 03:25:52PM +0200, Steve Lhomme wrote:
> 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.
picture_resource_t is a builder pattern, it allows to always
produce correct object in the end and allows some wiggle room
in the internals for the initialization stage.
> > 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.
Picture allocation failure with custom resources is an
unlikely event, and you still have to handle both the
allocation failure and the resource allocation failure
in both cases anyway.
> > 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.
Then the following assertion doesn't look correct:
> > > But in many cases what these functions do is
> > > not what the caller wants.
Or I don't see from where it comes.
> > > 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.
It doesn't seem to me that it comes from the NewFromResource
code but instead from the fact picture data is directly
accessible from the picture object itself. A more balanced
situation between addressable memory buffers and HW buffers
would probably have p_planes/p_pixels redirected to a picture
context object instead but this is not what this patchset is
changing. Otherwise, sys should probably somehow disappear
and destroy callback being NULL is certainly not an issue
in my opinion since it exactly matches the case: "don't
handle the resources for me because I'll do it locally".
> > > 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.
To be honest, I wouldn't call vmem, fb and kms as being the
best example of what is the current design, and none of them
are currently handling hardware buffers.
> > > 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.
Visibility is just about including the correct header to use
the correct size here, just like it is for picture_context.
There is no need to expose a field only for some posix
modules in an object present everywhere in the output pipeline
if it's just about knowing the common allocation type for this
kind of buffer.
Regards,
--
Alexandre Janniaux
Videolabs
> > > 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
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list