[libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Correctly populate V4L2BufferCache cache

[Laurent Pinchart]

Hello,

On Fri, Jul 09, 2021 at 06:02:55PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote:
> > On 09/07/2021 12:47, Umang Jain wrote:
> > > Populating of the cache in V4L2BufferCache's constructor is incorrect.
> > > The cache_ in V4L2BufferCache is a vector of struct Entry, whose
> > > constructor takes the form:
> > >
> > > 	Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> > >
> > > Passing a FrameBuffer::Plane vector via buffer->planes() is
> > > incorrect. Rectify by passing in a Framebuffer reference.
> 
> Why is it incorrect ? We have a constructor for that :)

That's what I was going to say too. It's not wrong per se, but may not
be the optimal implementation.

> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 3d2d99b4..da2af6a1 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
> > >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> > >  		cache_.emplace_back(true,
> > >  				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > > -				    buffer->planes());
> > > +				    *buffer.get());
> >
> > This is curious indeed.
> >
> > After all, it's compiling so it is working - so I assume here we are
> > constructing a new FrameBuffer from the buffer->planes() as there is a
> > constructor from FrameBuffer() to do just that.
> >
> > I think that means that we will be making a copy of the Planes and
> > possibly duplicating everything just so that we can make a temporary
> > copy/reference to pass to the entry constructor.
> 
> doesn't buffer->planes() returns a reference ?
>
> > At which point, the entry makes another copy.
> 
> The
>         Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0)
> 
> constructor indeed copies the planes vector in it's planes_ but I
> think calling the copy constructor as this patch is doing shouldn't
> make any difference, right ?
> 
> Umang, did you see any issue related to this code path ?

It shoudn't make any functional difference indeed. If it does, there's
something that I don't get, and I'd like to know about it.

If our understanding that this makes no functional difference, the
commit message should be updated, and then

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > So I think this is a correct change, and removes some redundant
> > constructions, but I think the copying of the fd's themselves is handled
> > with a shared pointer - so I don't think there's any particular
> > performance penalty otherwise, but this still seems to be a good change.
> >
> > I think that's a
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > from me, but I feel a little uncertain - so I'll be interested to see
> > what another pair of eyes makes of this.
> >
> > >  }
> > >
> > >  V4L2BufferCache::~V4L2BufferCache()

-- 
Regards,

Laurent Pinchart