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

[Steve Lhomme]

On 2020-08-17 16:15, Alexandre Janniaux wrote:
> Hi,
> 
> On Mon, Aug 17, 2020 at 03:47:47PM +0200, Steve Lhomme wrote:
>> 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.
> 
> I agree, but it doesn't mean there should not be discussion.
> In particular, I highlighted that my problem was not with the
> patchset itself, but the fact it's a new direction in the middle
> of a work in progress.

It's not really a new direction. What was in picture_resource_t is now 
either in picture_gc_t (and your opaque is still set it p_sys) or you 
handle the resource management yourself.

With push it's better to use pictures with a GC. Only display modules 
don't really need one because they push anything and release the 
resource(s) when they want. They usually use picture_t for the plane 
copying API.