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

Steve Lhomme robux4 at ycbcr.xyz
Fri Jul 31 06:47:29 CEST 2020


On 2020-07-30 16:13, Alexandre Janniaux wrote:
> 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.

My use of builder patterns come from Java and there you add only the 
parts you want before actually creating the object. This is not what we 
have here. It's a lot of params forced to be set to NULL when you don't 
need them. At least splitting the gc and planes in 
picture_NewFromResource would be a step in a cleaner builder (patch 08/19).

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

Right now you don't get to pick the order in which you do the 
allocations (with the most likely failure first) because you can't pass 
a NULL picture_resource_t. After this patchset you do it the way you want.

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

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

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

One step at a time. We've been saying for a while that between p_sys and 
context there's one too many. With the gc more evidently put aside now, 
it seems clear they pretty much to the same thing. So the logical step 
after this would be to merge them.

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

It's a structure exposed by the core, because that's how it allocates 
memory (on Linux). It seems logical that it's visible to all.

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