[vlc-devel] [PATCH v2 14/14] picture: always copy context when cloning

[Steve Lhomme]

On 2020-08-17 11:56, Alexandre Janniaux wrote:
> Hi,
> 
> I didn't have the time to extend the previous discussion
> on Steve patchset, which is merged now, so I'll post remarks
> here about last patchset and this patch as the subject is
> quite alike.
> 
> On Thu, Jul 30, 2020 at 07:10:09PM +0300, Rémi Denis-Courmont wrote:
>> Le torstaina 30. heinäkuuta 2020, 15.54.29 EEST Alexandre Janniaux a écrit :
>>> Hi,
>>>
>>> I'm not at ease with this patchset, it feels like removing the
>>> picture_resource_t layer and doing it by hand.
>>
>> Leaving aside my minor detail comments, I think the patchset makes sense.
> 
> I agree that it makes sense, I just fear that it is doing
> more than what we really need, while breaking existing
> unmerged code. Push model is already unfinished, decoder

You can't block patches just because it will force you to do a rebase 
and edit your code. Even less if your branch was not even sent for 
review. I do rebases like this all the time, it's part of getting your 
code ready for merge.

> devices face strong misunderstanding and imho has both
> abstraction and specification issues that are still unsolved,
> we got vdpau and vaapi working back correctly a few days ago
> only so I'm in particular not at ease with half-changing yet
> another layer that is in particular used in very different
> context wihout having the final target in mind, meaning
> at least re-defining the allocation for both opaque and
> memory addressable picture (which are not so different),
> so as to settle this patchset into something understandable
> instead of just dropping it in the core and have everyone
> after wonder why it became like that.
> 
>> The whole point of push-buffers was to uniformise picture buffer allocation.
>> Everything should be allocated as shared memory that can be manipulated
>> locally, passed onto compositor with zero copy IPC, and eventually exchanged
>> with sandboxed decoders orf ilters. picture_NewFromFormat() takes care of
>> that. Pictures resources should not be needed anymore.
> 
> I agree, though radical push buffers (in production site and
> configuration site) does not uniformise with all kind of
> resources provision, since encoders working with opaque
> usually require some usage flag set on the buffers, and was

Display modules too. That's why all the D3D11 pictures I create also 
have the ID3D11ShaderResourceView setup even if they are not displayed. 
In this case that's more a speed optimization than a design drawback.

> sometimes encapsulated into an object provided by the encoder
> itself (think mediacodec input surface for example) unless we
> have stronger requirements and remove compatibility or increase
> scarce resource usage.
> 
> I've been working around those questions for some time now, and
> found different solutions with different level of intrusiveness
> into the code base.
> 
> For example, it's still possible to achieve this in the push
> + configure mode we use by adding converters and having the
> converters provide the pool to the previous video context
> depending on the usage after, but it has some issues regarding
> automatic configuration in the filter pipeline, in particular
> since the filter allocation is not recursive, thus the filter
> chain isn't creating the optimal chain.
> 
> Your patch on module sorting separated from module probe could
> help design a cleaner solution though.
> 
> IMHO the filter chain should adapt to the decoder device and
> fallback to a standard resource kind ("CPU" buffer) if no
> filters are compatible with it, which is likely the original
> intent in general since that's what we do for the decoder.
> But there's still issues regarding how the decoder device is
> defined and it conflicts with the fact we use the decoder
> device as a decoder switch.
> 
> To get back to this patch in particular, I'll also quote the
> last part of your previous message:
> 
>> Regardless we still need a mean to wrap opaque picture buffers, which cannot
>> really use picture_NewFromFormat().
>>
>> However...
>>
>>> 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).
>>
>> If we need to handle non-opaque pictures coming from an input peripheral (as
>> opposed to going to an output peripheral), then we need something like similar
>> to picture_NewFromResource(). I guess that is your implication?
> 
> Not directly, but this case need to be handled, like you answered
> right after. I'm not sure we want a new picture_New* specifically
> designed to handled the file descriptor, mapping the offset, etc
> though.
> 
> I was mentioning cases where an opaque picture goes through the
> output peripheral with the need to be adapted.
> 
> In my case in particular, we need to use objects embodying dma
> sharing requests that can be serialized into what the windowing
> system expects, so as to bind a picture to the underlying display
> system.
> 
> I'm still not completely convinced on whether the dma request
> should be recreated each time or not, especially with regards to
> a sandbox, because I still don't know much about related issues
> but I found platforms on which multiple DMA requests lead to
> issues, .
> 
> Anyway, in this situation, sys was used to achieve the binding

Do you mean the p_sys of the picture or the display module sys ?

> in the picture and tie the DMA request with the GBM object, but
> my point in the previous mail was that it could be removed since
> the buffer could be discriminated against the value of the fd.
> 
> The point I'd like to clarify is specifically what should be
> used by developers from now on at the allocation time and how
> it should be used by following modules.

My patchset doesn't claim that it's the final state of how picture_t 
should be used. It's just a lot of cleanup, moving code around, avoiding 
casts. In fact it doesn't change the picture_context_t vs p_sys usage. 
They are still used as they are now.

>> Not sure this will work. Originally, picture context was specifically meant to
>> work-around the pool and not do that.
>>
>> What is the practical use case, and why can't it use picture_t.sys?
>>
> 
> Originally, sys was used to store vout data too so I'm not sure
> it's relevant here. However there's many place where the modules
> are creating picture_context and then allocating picture to wrap
> them.
> 
> In my case, I have a predefined number of buffers, so instead of
> creating a pool of picture context, handle their release and reuse
> and allocate picture each time, I directly create the wrapping
> picture_t and bind them, and then create a picture_pool with those
> pictures so as to handle the release/reuse cycle.
> 
> Thus, the handling of hardware buffers is not really different
> from software buffers, and actually we could also make software
> buffers behave the same.
> 
> Using sys is a distinct solution, which lacks the destroy/copy
> callbacks but could use gc's features for that. However, it
> different from what we currently had and Steve patchset overlap
> with the picture_context concept, hence my reluctance to have
> his patchset merged before further discussion and choice.
> 
> This patch fixed a difference in behaviour between normal clone
> and clones from a picture_pool, so it makes sense to normalize it
> anyway instead of getting picture with null context and sending them.

In particular right now picture_context_t is not set on pictures coming 
out of picture pool. Which means you can't "prepare" the 
picture_context_t ahead and just clone it when using the picture. That's 
a design drawback that forces a lot of dirty boilerplate code.

> I don't know what's the best way to discuss this though. A third
> workshop to finish the design details of the push regarding filters
> and encoders, with this question in the middle because it mainly
> relates to picture producer in general, would probably be a great
> thing but given the current situation it might be too hard and
> unsafe to setup in the short term.
> 
> I wrote content on the whole OpenGL work we did at Videolabs, I'll
> try to highlight limits we reached in the current model to make
> all of this clearer.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Fri, Aug 14, 2020 at 06:48:13PM +0300, Rémi Denis-Courmont wrote:
>> Le perjantaina 14. elokuuta 2020, 15.01.49 EEST Steve Lhomme a écrit :
>>> From: Alexandre Janniaux <ajanni at videolabs.io>
>>>
>>
>> p...
>>
>>> icture_Clone was copying the original picture context into the new
>>> clone while picture_pool_Clone wasn't. As it is possible to create a
>>> pool from existing pictures, it's quite handy to be able to associate
>>> the graphical resources needed for each picture to their context so
>>> that it can be retrieved after a call to picture_pool_Wait/Get.
>>>
>>> The rationale is that because resources like pixels are shared between
>>> the original picture and the clone, we should probably do the same with
>>> picture_context and have a unified way for picture producer to
>>> reference such resources.
>>
>> Not sure this will work. Originally, picture context was specifically meant to
>> work-around the pool and not do that.
>>
>> What is the practical use case, and why can't it use picture_t.sys?
>>
>> --
>> レミ・デニ-クールモン
>> http://www.remlab.net/
>>
>>
>>
>> _______________________________________________
>> 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
>