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

[Alexandre Janniaux]

On Mon, Aug 17, 2020 at 04:20:00PM +0200, Steve Lhomme wrote:
> 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.

My issue is not with sys or gc, or whatever directly.

My issue is that there are multiple concepts for the same
feature (meaning managing resource life).

First:
 - I used sys, but I can and would remove it
 - I don't complain about flaws in either picture_resource_t
   or picture_context_t or whatever

I complain about making change to this without a clear guideline,
as it would lead to private / sent branch being rebased multiple
time with different code for nothing gained.

I do GPU push buffer with picture_context_t, like it was done
before and is done in some HW decoders.

I don't care if it's the right way to handle the resource or if
yours is better. Even though I'm a big client of picture_context
I don't have the years of experience to decide whether a method
will work for everyone and in the long term or not.

I just ask to discuss on which method we choose before having two
ways (or even a third way with sys), forgetting about it, and have
a bigger mess in later time.

Regards,
--
Alexandre Janniaux
Videolabs

Regards,
--
Alexandre Janniaux
Videolabs