[{"id":18057,"web_url":"https://patchwork.libcamera.org/comment/18057/","msgid":"<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>","date":"2021-07-09T14:13:18","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 09/07/2021 12:47, Umang Jain wrote:\n> Populating of the cache in V4L2BufferCache's constructor is incorrect.\n> The cache_ in V4L2BufferCache is a vector of struct Entry, whose\n> constructor takes the form:\n> \n> \tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> \n> Passing a FrameBuffer::Plane vector via buffer->planes() is\n> incorrect. Rectify by passing in a Framebuffer reference.\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  src/libcamera/v4l2_videodevice.cpp | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3d2d99b4..da2af6a1 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n>  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n>  \t\tcache_.emplace_back(true,\n>  \t\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> -\t\t\t\t    buffer->planes());\n> +\t\t\t\t    *buffer.get());\n\nThis is curious indeed.\n\nAfter all, it's compiling so it is working - so I assume here we are\nconstructing a new FrameBuffer from the buffer->planes() as there is a\nconstructor from FrameBuffer() to do just that.\n\nI think that means that we will be making a copy of the Planes and\npossibly duplicating everything just so that we can make a temporary\ncopy/reference to pass to the entry constructor.\n\nAt which point, the entry makes another copy.\n\nSo I think this is a correct change, and removes some redundant\nconstructions, but I think the copying of the fd's themselves is handled\nwith a shared pointer - so I don't think there's any particular\nperformance penalty otherwise, but this still seems to be a good change.\n\nI think that's a\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nfrom me, but I feel a little uncertain - so I'll be interested to see\nwhat another pair of eyes makes of this.\n\n\n\n--\nKieran\n\n\n\n>  }\n>  \n>  V4L2BufferCache::~V4L2BufferCache()\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 11AC8C3224\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  9 Jul 2021 14:13:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 577766851B;\n\tFri,  9 Jul 2021 16:13:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A7C1605AC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Jul 2021 16:13:21 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B49C1E7;\n\tFri,  9 Jul 2021 16:13:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MYc2YMmJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625840000;\n\tbh=frHpL+hClgjgvxwFlbsEwPRgWV2l502lbBVGwCT6kAg=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=MYc2YMmJQGczyU69GpYP3Oi4wxEFaTQ94q/j8q4Ufn0hKWDbpDkYhvHn8zZE0WbyY\n\tMk/w1pesIZZ6uzy2rrrXzj4A0jp3GSijhCnhnGVVnt/1dHCIYnQIdt0tuMgvZCpmck\n\t9Q7bmayfTdzjylIJ9yWVCKYbKFIUCJAgX0Qw7Tds=","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210709114736.875760-1-umang.jain@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>","Date":"Fri, 9 Jul 2021 15:13:18 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210709114736.875760-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18062,"web_url":"https://patchwork.libcamera.org/comment/18062/","msgid":"<20210709160255.kbawppxyztbmi47l@uno.localdomain>","date":"2021-07-09T16:02:55","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 09/07/2021 12:47, Umang Jain wrote:\n> > Populating of the cache in V4L2BufferCache's constructor is incorrect.\n> > The cache_ in V4L2BufferCache is a vector of struct Entry, whose\n> > constructor takes the form:\n> >\n> > \tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> >\n> > Passing a FrameBuffer::Plane vector via buffer->planes() is\n> > incorrect. Rectify by passing in a Framebuffer reference.\n\nWhy is it incorrect ? We have a constructor for that :)\n\n> >\n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > ---\n> >  src/libcamera/v4l2_videodevice.cpp | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 3d2d99b4..da2af6a1 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n> >  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n> >  \t\tcache_.emplace_back(true,\n> >  \t\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > -\t\t\t\t    buffer->planes());\n> > +\t\t\t\t    *buffer.get());\n>\n> This is curious indeed.\n>\n> After all, it's compiling so it is working - so I assume here we are\n> constructing a new FrameBuffer from the buffer->planes() as there is a\n> constructor from FrameBuffer() to do just that.\n>\n> I think that means that we will be making a copy of the Planes and\n> possibly duplicating everything just so that we can make a temporary\n> copy/reference to pass to the entry constructor.\n\ndoesn't buffer->planes() returns a reference ?\n>\n> At which point, the entry makes another copy.\n\nThe\n        Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0)\n\nconstructor indeed copies the planes vector in it's planes_ but I\nthink calling the copy constructor as this patch is doing shouldn't\nmake any difference, right ?\n\nUmang, did you see any issue related to this code path ?\n\nThanks\n  j\n\n\n>\n> So I think this is a correct change, and removes some redundant\n> constructions, but I think the copying of the fd's themselves is handled\n> with a shared pointer - so I don't think there's any particular\n> performance penalty otherwise, but this still seems to be a good change.\n>\n> I think that's a\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> from me, but I feel a little uncertain - so I'll be interested to see\n> what another pair of eyes makes of this.\n>\n>\n>\n> --\n> Kieran\n>\n>\n>\n> >  }\n> >\n> >  V4L2BufferCache::~V4L2BufferCache()\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F0D48C3224\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  9 Jul 2021 16:02:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5EE246851B;\n\tFri,  9 Jul 2021 18:02:08 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F4EF605AC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 Jul 2021 18:02:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id A6348FF804;\n\tFri,  9 Jul 2021 16:02:06 +0000 (UTC)"],"Date":"Fri, 9 Jul 2021 18:02:55 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20210709160255.kbawppxyztbmi47l@uno.localdomain>","References":"<20210709114736.875760-1-umang.jain@ideasonboard.com>\n\t<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18082,"web_url":"https://patchwork.libcamera.org/comment/18082/","msgid":"<YOtTYOaKPQyS2Ved@pendragon.ideasonboard.com>","date":"2021-07-11T20:24:00","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jul 09, 2021 at 06:02:55PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote:\n> > On 09/07/2021 12:47, Umang Jain wrote:\n> > > Populating of the cache in V4L2BufferCache's constructor is incorrect.\n> > > The cache_ in V4L2BufferCache is a vector of struct Entry, whose\n> > > constructor takes the form:\n> > >\n> > > \tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> > >\n> > > Passing a FrameBuffer::Plane vector via buffer->planes() is\n> > > incorrect. Rectify by passing in a Framebuffer reference.\n> \n> Why is it incorrect ? We have a constructor for that :)\n\nThat's what I was going to say too. It's not wrong per se, but may not\nbe the optimal implementation.\n\n> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > > ---\n> > >  src/libcamera/v4l2_videodevice.cpp | 2 +-\n> > >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 3d2d99b4..da2af6a1 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n> > >  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n> > >  \t\tcache_.emplace_back(true,\n> > >  \t\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > > -\t\t\t\t    buffer->planes());\n> > > +\t\t\t\t    *buffer.get());\n> >\n> > This is curious indeed.\n> >\n> > After all, it's compiling so it is working - so I assume here we are\n> > constructing a new FrameBuffer from the buffer->planes() as there is a\n> > constructor from FrameBuffer() to do just that.\n> >\n> > I think that means that we will be making a copy of the Planes and\n> > possibly duplicating everything just so that we can make a temporary\n> > copy/reference to pass to the entry constructor.\n> \n> doesn't buffer->planes() returns a reference ?\n>\n> > At which point, the entry makes another copy.\n> \n> The\n>         Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0)\n> \n> constructor indeed copies the planes vector in it's planes_ but I\n> think calling the copy constructor as this patch is doing shouldn't\n> make any difference, right ?\n> \n> Umang, did you see any issue related to this code path ?\n\nIt shoudn't make any functional difference indeed. If it does, there's\nsomething that I don't get, and I'd like to know about it.\n\nIf our understanding that this makes no functional difference, the\ncommit message should be updated, and then\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > So I think this is a correct change, and removes some redundant\n> > constructions, but I think the copying of the fd's themselves is handled\n> > with a shared pointer - so I don't think there's any particular\n> > performance penalty otherwise, but this still seems to be a good change.\n> >\n> > I think that's a\n> >\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > from me, but I feel a little uncertain - so I'll be interested to see\n> > what another pair of eyes makes of this.\n> >\n> > >  }\n> > >\n> > >  V4L2BufferCache::~V4L2BufferCache()","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C316EC3224\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 11 Jul 2021 20:24:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CB966851B;\n\tSun, 11 Jul 2021 22:24:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C691A68519\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 11 Jul 2021 22:24:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40306CC;\n\tSun, 11 Jul 2021 22:24:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tpB2ahoO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626035086;\n\tbh=0pQ8GTSUh3ii39Y9g/ivPaKwINbQFtbU4wFNtHLkbrE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tpB2ahoOh2DEyYhnb7ZcAYt7ymoUN3N1jOpk7RvQZrrRnJhqs04GNkeahuyg9I86B\n\tp8g1Nhw+LJlMqr5JhsOgXyIafssC/iLZpNNZWE7N3voz33TnkJ66Ns5L/qSMVVxf3g\n\tOCNoAL1KAyE/XPIfmSUL/+daRCY7KkKGWMqL8AkQ=","Date":"Sun, 11 Jul 2021 23:24:00 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YOtTYOaKPQyS2Ved@pendragon.ideasonboard.com>","References":"<20210709114736.875760-1-umang.jain@ideasonboard.com>\n\t<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>\n\t<20210709160255.kbawppxyztbmi47l@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210709160255.kbawppxyztbmi47l@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18222,"web_url":"https://patchwork.libcamera.org/comment/18222/","msgid":"<b1721a1f-fcad-ecd5-bd7b-879672c206ab@ideasonboard.com>","date":"2021-07-19T04:07:36","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi all,\n\nOn 7/12/21 1:54 AM, Laurent Pinchart wrote:\n> Hello,\n>\n> On Fri, Jul 09, 2021 at 06:02:55PM +0200, Jacopo Mondi wrote:\n>> On Fri, Jul 09, 2021 at 03:13:18PM +0100, Kieran Bingham wrote:\n>>> On 09/07/2021 12:47, Umang Jain wrote:\n>>>> Populating of the cache in V4L2BufferCache's constructor is incorrect.\n>>>> The cache_ in V4L2BufferCache is a vector of struct Entry, whose\n>>>> constructor takes the form:\n>>>>\n>>>> \tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n>>>>\n>>>> Passing a FrameBuffer::Plane vector via buffer->planes() is\n>>>> incorrect. Rectify by passing in a Framebuffer reference.\n>> Why is it incorrect ? We have a constructor for that :)\n> That's what I was going to say too. It's not wrong per se, but may not\n> be the optimal implementation.\n>\n>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>>> ---\n>>>>   src/libcamera/v4l2_videodevice.cpp | 2 +-\n>>>>   1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>> index 3d2d99b4..da2af6a1 100644\n>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>> @@ -183,7 +183,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n>>>>   \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n>>>>   \t\tcache_.emplace_back(true,\n>>>>   \t\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n>>>> -\t\t\t\t    buffer->planes());\n>>>> +\t\t\t\t    *buffer.get());\n>>> This is curious indeed.\n>>>\n>>> After all, it's compiling so it is working - so I assume here we are\n>>> constructing a new FrameBuffer from the buffer->planes() as there is a\n>>> constructor from FrameBuffer() to do just that.\n>>>\n>>> I think that means that we will be making a copy of the Planes and\n>>> possibly duplicating everything just so that we can make a temporary\n>>> copy/reference to pass to the entry constructor.\n>> doesn't buffer->planes() returns a reference ?\n>>\n>>> At which point, the entry makes another copy.\n>> The\n>>          Framebuffer::Framebuffer(const std::vector<Plane> &planes, unsigned int cookie = 0)\n>>\n>> constructor indeed copies the planes vector in it's planes_ but I\n>> think calling the copy constructor as this patch is doing shouldn't\n>> make any difference, right ?\nRight, my understanding was mistaken due ignorance of this constructor. \nThanks for pointing this out.\n>>\n>> Umang, did you see any issue related to this code path ?\nno, I didn't see any issue.\n> It shoudn't make any functional difference indeed. If it does, there's\n> something that I don't get, and I'd like to know about it.\n>\n> If our understanding that this makes no functional difference, the\n> commit message should be updated, and then\nYes, no functional difference.\n>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n>>> So I think this is a correct change, and removes some redundant\n>>> constructions, but I think the copying of the fd's themselves is handled\n>>> with a shared pointer - so I don't think there's any particular\n>>> performance penalty otherwise, but this still seems to be a good change.\nOk, I am revamped the commit message and posted a new v2 under the \nsubject \"libcamera: v4l2_videodevice: Avoid extra construction of \nFramebuffer\"\n>>>\n>>> I think that's a\n>>>\n>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>\n>>> from me, but I feel a little uncertain - so I'll be interested to see\n>>> what another pair of eyes makes of this.\n>>>\n>>>>   }\n>>>>\n>>>>   V4L2BufferCache::~V4L2BufferCache()","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DB2AFC0109\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 19 Jul 2021 04:07:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 314286853A;\n\tMon, 19 Jul 2021 06:07:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFDEF60278\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 19 Jul 2021 06:07:42 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.64])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3407465;\n\tMon, 19 Jul 2021 06:07:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Vhx4i9cb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626667662;\n\tbh=0z7IkxnoWcIjN9Wxa4seaKH0YmgFynCuke4zZOKkhTk=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Vhx4i9cbvZbDhDF/qkULMwu73MK2Vv90SBavs/SICr9OBZFHLzrdIvkcGQGVtF39s\n\teznmGIjpoyDeoRASWLSAbXqOICvhFao5CdGYu1XhpeF3p1/3OqI6P08pSelx2zUf2o\n\t/2DgtzpSxPbH8Dj2IXKgJDjLQAWIMge9wuublkK8=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tJacopo Mondi <jacopo@jmondi.org>","References":"<20210709114736.875760-1-umang.jain@ideasonboard.com>\n\t<6331d92b-631b-3a55-dfdb-c6d1f6ded5b2@ideasonboard.com>\n\t<20210709160255.kbawppxyztbmi47l@uno.localdomain>\n\t<YOtTYOaKPQyS2Ved@pendragon.ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<b1721a1f-fcad-ecd5-bd7b-879672c206ab@ideasonboard.com>","Date":"Mon, 19 Jul 2021 09:37:36 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<YOtTYOaKPQyS2Ved@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice:\n\tCorrectly populate V4L2BufferCache cache","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]