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

[Kieran Bingham]

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

At which point, the entry makes another copy.

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()
>