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

[Umang Jain]

Hi all,

On 7/12/21 1:54 AM, Laurent Pinchart wrote:
> 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 ?
Right, my understanding was mistaken due ignorance of this constructor. 
Thanks for pointing this out.
>>
>> Umang, did you see any issue related to this code path ?
no, I didn't see any issue.
> 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
Yes, no functional difference.
>
> 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.
Ok, I am revamped the commit message and posted a new v2 under the 
subject "libcamera: v4l2_videodevice: Avoid extra construction of 
Framebuffer"
>>>
>>> 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()