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

[Jacopo Mondi]

Hello,

On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote:
> Hi Umang,
>
> 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 :)

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

Thanks
  j


>
> 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.
>
>
>
> --
> Kieran
>
>
>
> >  }
> >
> >  V4L2BufferCache::~V4L2BufferCache()
> >