[{"id":20208,"web_url":"https://patchwork.libcamera.org/comment/20208/","msgid":"","date":"2021-10-14T20:12:18","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Oct 14, 2021 at 09:54:05AM +0100, Kieran Bingham wrote:\n> The ternary operation used to get the total bytesused of a V4L2 single\n> planar format which is stored in a multiplanar buffer can easily be\n> mis-read to think it's a bug, and appears to be reading the value of the\n> first of N planes as the total.\n> \n> Directly explain the reasoning for why it looks like the condition is\n> inverted, as it is correct that the total bytes used is stored in only\n> the first plane of the multiplanar buffer.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ba5f88cd41ed..f4752f8bb23f 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1711,6 +1711,7 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\t\treturn buffer;\n> \t\t}\n> \n> +\t\t/* Single planar format stored in a multiplanar buffer */\n\nNow I find this a bit confusing :-) How about the following ?\n\n\t\t/*\n\t\t * With a V4L2 single-planar format, all the data is stored in a\n\t\t * single memory plane. The number of bytes used is conveyed\n\t\t * through that plane when using the V4L2 multi-planar API, or\n\t\t * set directly in the buffer when using the V4L2 single-planar\n\t\t * API.\n\t\t */\n\nIt's a bit verbose perhaps ?\n\n> \t\tunsigned int bytesused = multiPlanar ? planes[0].bytesused\n> \t\t\t\t : buf.bytesused;\n>","headers":{"Return-Path":"","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 9CF64C323E\n\tfor ;\n\tThu, 14 Oct 2021 20:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAA9568F4F;\n\tThu, 14 Oct 2021 22:12:36 +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 A600E68541\n\tfor ;\n\tThu, 14 Oct 2021 22:12:35 +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 0AD7E2F3;\n\tThu, 14 Oct 2021 22:12:34 +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=\"VlGzQKta\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634242355;\n\tbh=ParmaYsB2MZ/Thvbk+ZzHuHPD3WNZmCLrph9iaeGyek=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VlGzQKtaVtUuYMX23QBSSW2MllQ4YzNM1AKhfkWJkDc9gQl1MycMYVQA57u83s4RY\n\tAuFk8yhlZvwzX3hoPhxONoxFnAT/zhkcGTiLLnGz5Q3WAvaLWILjlKLOzfulz693k8\n\t+m1xae85e+UriBmZSz6uu/BhdyGxADD3tutqEPz0=","Date":"Thu, 14 Oct 2021 23:12:18 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211014085405.3795243-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211014085405.3795243-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20240,"web_url":"https://patchwork.libcamera.org/comment/20240/","msgid":"<163429543421.4164224.8407001761599506075@Monstersaurus>","date":"2021-10-15T10:57:14","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-14 21:12:18)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Oct 14, 2021 at 09:54:05AM +0100, Kieran Bingham wrote:\n> > The ternary operation used to get the total bytesused of a V4L2 single\n> > planar format which is stored in a multiplanar buffer can easily be\n> > mis-read to think it's a bug, and appears to be reading the value of the\n> > first of N planes as the total.\n> > \n> > Directly explain the reasoning for why it looks like the condition is\n> > inverted, as it is correct that the total bytes used is stored in only\n> > the first plane of the multiplanar buffer.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 1 +\n> > 1 file changed, 1 insertion(+)\n> > \n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index ba5f88cd41ed..f4752f8bb23f 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1711,6 +1711,7 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > return buffer;\n> > }\n> > \n> > + /* Single planar format stored in a multiplanar buffer */\n> \n> Now I find this a bit confusing :-) How about the following ?\n> \n> /*\n> * With a V4L2 single-planar format, all the data is stored in a\n> * single memory plane. The number of bytes used is conveyed\n> * through that plane when using the V4L2 multi-planar API, or\n> * set directly in the buffer when using the V4L2 single-planar\n> * API.\n> */\n> \n> It's a bit verbose perhaps ?\n\nPerhaps, but I don't mind verbose there.\nThe line on it's own looks like there is a bug because of the inversion.\nSo it warrants an explanation. And if you've written one - we can add\nit.\n\n--\nKieran\n\n> \n> > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > : buf.bytesused;\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","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 AE981C324C\n\tfor ;\n\tFri, 15 Oct 2021 10:57:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1349668F4F;\n\tFri, 15 Oct 2021 12:57:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C705868546\n\tfor ;\n\tFri, 15 Oct 2021 12:57:16 +0200 (CEST)","from pendragon.ideasonboard.com\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 641432E3;\n\tFri, 15 Oct 2021 12:57:16 +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=\"drWT0x4J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634295436;\n\tbh=zGuFiBLz60of2EoqEXRquz9NKSPoMQw50q7/9JjdhJo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=drWT0x4JY5I/hoOxs/GO4Z8S7loY/fHtFRhw/OmI2CUsJcSrsdkFlQ1Yl0Eev6BHT\n\tY4uIt95Yw6o6LD6/4T/iueRHiN5MnUAvgrbTpebkFUk0T+QbbKC3xNbFj2+2iWoaDm\n\tOnYRKDg0jLpc3UnrA6GGVOIe3kLjxEvp/d7+n1N0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211014085405.3795243-1-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ","Date":"Fri, 15 Oct 2021 11:57:14 +0100","Message-ID":"<163429543421.4164224.8407001761599506075@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20250,"web_url":"https://patchwork.libcamera.org/comment/20250/","msgid":"","date":"2021-10-15T22:55:18","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Oct 15, 2021 at 11:57:14AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-10-14 21:12:18)\n> > On Thu, Oct 14, 2021 at 09:54:05AM +0100, Kieran Bingham wrote:\n> > > The ternary operation used to get the total bytesused of a V4L2 single\n> > > planar format which is stored in a multiplanar buffer can easily be\n> > > mis-read to think it's a bug, and appears to be reading the value of the\n> > > first of N planes as the total.\n> > > \n> > > Directly explain the reasoning for why it looks like the condition is\n> > > inverted, as it is correct that the total bytes used is stored in only\n> > > the first plane of the multiplanar buffer.\n> > > \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 1 +\n> > > 1 file changed, 1 insertion(+)\n> > > \n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index ba5f88cd41ed..f4752f8bb23f 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1711,6 +1711,7 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > return buffer;\n> > > }\n> > > \n> > > + /* Single planar format stored in a multiplanar buffer */\n> > \n> > Now I find this a bit confusing :-) How about the following ?\n> > \n> > /*\n> > * With a V4L2 single-planar format, all the data is stored in a\n> > * single memory plane. The number of bytes used is conveyed\n> > * through that plane when using the V4L2 multi-planar API, or\n> > * set directly in the buffer when using the V4L2 single-planar\n> > * API.\n> > */\n> > \n> > It's a bit verbose perhaps ?\n> \n> Perhaps, but I don't mind verbose there.\n> The line on it's own looks like there is a bug because of the inversion.\n> So it warrants an explanation. And if you've written one - we can add\n> it.\n\nIf you're fine with that text,\n\nReviewed-by: Laurent Pinchart \n\n> > > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > > : buf.bytesused;\n> > >","headers":{"Return-Path":"","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 0799DC323E\n\tfor ;\n\tFri, 15 Oct 2021 22:55:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9EB268F4F;\n\tSat, 16 Oct 2021 00:55:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A43E68541\n\tfor ;\n\tSat, 16 Oct 2021 00:55:34 +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 BE45129B;\n\tSat, 16 Oct 2021 00:55:33 +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=\"Pp4tOBnx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634338534;\n\tbh=0SICFQD5JsrH/h5O/8h7JUi0kxxEg6Nl0Wq65qIAZ9s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pp4tOBnxgIQYqjFqrYj5nS9eEec6O4AsdaTs607r/3JdwVt4vcY2K8sN1nuuNlF3Y\n\t4EK29Za3Ih+Zp7E5jfVhEe094LvxUYBsP3aVcvubfrE4Et2h76TpaBfqTrXDQohS6M\n\tfiP+UL30p+Rr7OHDH+2Jv0Ml5vBI0fzbGcmDahJU=","Date":"Sat, 16 Oct 2021 01:55:18 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211014085405.3795243-1-kieran.bingham@ideasonboard.com>\n\t\n\t<163429543421.4164224.8407001761599506075@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163429543421.4164224.8407001761599506075@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Explain\n\tmultiplanar bytesused reasoning","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]