[{"id":18718,"web_url":"https://patchwork.libcamera.org/comment/18718/","msgid":"","date":"2021-08-11T23:22:03","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:\n> FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same\n> number of planes as v4l2 buffer planes. However, if the format is\n> a single planar format, the number of planes is one. FrameBuffer\n> should have the same number of planes as color format planes.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-\n> 1 file changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ce60dff6..e70076f3 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1269,7 +1269,6 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> \n> \tconst bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> \tconst unsigned int numPlanes = multiPlanar ? buf.length : 1;\n> -\n\nUnrelated change ?\n\n> \tif (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n> \t\tLOG(V4L2, Error) << \"Invalid number of planes\";\n> \t\treturn nullptr;\n> @@ -1289,6 +1288,20 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> \t\tplanes.push_back(std::move(plane));\n> \t}\n> \n> +\n\nExtra blank line.\n\n> +\tV4L2DeviceFormat format{};\n\nV4L2DeviceFormat initializes all members to 0 when constructed, you can\ndrop {}.\n\n> +\tret = getFormat(&format);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get buffer format\";\n> +\t\treturn nullptr;\n> +\t}\n\nI'd move the extra blank line from above here :-)\n\nThis being said, maybe we could cache the fourcc in V4L2VideoDevice\ninstead of querying it back from the kernel ?\n\n> +\tif (format.fourcc == V4L2_PIX_FMT_NV12) {\n> +\t\tplanes.resize(2);\n> +\t\tplanes[0].length = format.size.width * format.size.height;\n> +\t\tplanes[1].fd = planes[0].fd;\n> +\t\tplanes[1].length = format.size.width * ((format.size.height + 1) / 2);\n> +\t}\n\nI don't think this is right. Well, it is, but at the same time, it isn't\n:-)\n\nAt the moment, the number of planes in the FrameBuffer class matches the\nnumber of \"buffer planes\" used by V4L2 (1 for NV12, as opposed to the\nnumber of \"format planes\", which would be 2). I agree that we want to\nmove to \"format planes\" in the long term here, to avoid exposing the\nV4L2 historical design mistake to applications (\"long term\" meaning \"as\nsoon as possible\" here). However, this requires adding an offset field\nto the FrameBuffer::Plane structure, as otherwise we miss the\ninformation to implement the calculation properly (or at least we miss\nexposing the information explictly and rely on undocumented\nimplementation details).\n\nThere's no need, I think, to implement this as part of this series. We\ncan keep the calculation internal to MappedFrameBuffer as a first step,\nand then address the issue in the FrameBuffer class. It may also be\npossible to do it the other way around, and move to \"format planes\" in\nFrameBuffer at the bottom of the series. I have a feeling that it may be\ndifficult to keep patches reasonable in size and avoid bisection\nbreakages in that case, but I may be wrong.\n\n> +\n> \treturn std::make_unique(std::move(planes));\n> }\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 9CCB5C3240\n\tfor ;\n\tWed, 11 Aug 2021 23:22:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 033DD6884F;\n\tThu, 12 Aug 2021 01:22:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC269687F0\n\tfor ;\n\tThu, 12 Aug 2021 01:22:06 +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 2E1BFEE;\n\tThu, 12 Aug 2021 01:22:06 +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=\"u0yAr8lD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628724126;\n\tbh=N0Zva7sUSFIY5xiiBNssHgmy4u2mJmqdlBgtB3oLh8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u0yAr8lDg/USqbGhp182lDfGo03OB7UhTlC9bn0ZUWOxo3ajUyUu+PAhMW7DUteBQ\n\tLHsiLrQAvCCD2TO6acBGArc7kxOZWoXTEiq6XWjveftTIED7i8+CVupny5Smz3gPHp\n\tL4qS9iCUwad0ChRJFL3cNBlOWt2tRZQoEIJlGRCo=","Date":"Thu, 12 Aug 2021 02:22:03 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210811124015.2116188-1-hiroh@chromium.org>\n\t<20210811124015.2116188-5-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210811124015.2116188-5-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18719,"web_url":"https://patchwork.libcamera.org/comment/18719/","msgid":"","date":"2021-08-12T05:18:11","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for reviewing.\n\nOn Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:\n> > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same\n> > number of planes as v4l2 buffer planes. However, if the format is\n> > a single planar format, the number of planes is one. FrameBuffer\n> > should have the same number of planes as color format planes.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-\n> > 1 file changed, 14 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index ce60dff6..e70076f3 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1269,7 +1269,6 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> >\n> > const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > const unsigned int numPlanes = multiPlanar ? buf.length : 1;\n> > -\n>\n> Unrelated change ?\n>\n> > if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n> > LOG(V4L2, Error) << \"Invalid number of planes\";\n> > return nullptr;\n> > @@ -1289,6 +1288,20 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > planes.push_back(std::move(plane));\n> > }\n> >\n> > +\n>\n> Extra blank line.\n>\n> > + V4L2DeviceFormat format{};\n>\n> V4L2DeviceFormat initializes all members to 0 when constructed, you can\n> drop {}.\n>\n> > + ret = getFormat(&format);\n> > + if (ret < 0) {\n> > + LOG(V4L2, Error) << \"Failed to get buffer format\";\n> > + return nullptr;\n> > + }\n>\n> I'd move the extra blank line from above here :-)\n>\n> This being said, maybe we could cache the fourcc in V4L2VideoDevice\n> instead of querying it back from the kernel ?\n>\n> > + if (format.fourcc == V4L2_PIX_FMT_NV12) {\n> > + planes.resize(2);\n> > + planes[0].length = format.size.width * format.size.height;\n> > + planes[1].fd = planes[0].fd;\n> > + planes[1].length = format.size.width * ((format.size.height + 1) / 2);\n> > + }\n>\n> I don't think this is right. Well, it is, but at the same time, it isn't\n> :-)\n>\n> At the moment, the number of planes in the FrameBuffer class matches the\n> number of \"buffer planes\" used by V4L2 (1 for NV12, as opposed to the\n> number of \"format planes\", which would be 2). I agree that we want to\n> move to \"format planes\" in the long term here, to avoid exposing the\n> V4L2 historical design mistake to applications (\"long term\" meaning \"as\n> soon as possible\" here). However, this requires adding an offset field\n> to the FrameBuffer::Plane structure, as otherwise we miss the\n> information to implement the calculation properly (or at least we miss\n> exposing the information explictly and rely on undocumented\n> implementation details).\n>\n\nI agree. We need `offset`.\n\n> There's no need, I think, to implement this as part of this series. We\n> can keep the calculation internal to MappedFrameBuffer as a first step,\n> and then address the issue in the FrameBuffer class. It may also be\n> possible to do it the other way around, and move to \"format planes\" in\n> FrameBuffer at the bottom of the series. I have a feeling that it may be\n> difficult to keep patches reasonable in size and avoid bisection\n> breakages in that case, but I may be wrong.\n>\n\nMappedFrameBuffer (or even FrameBuffer) doesn't have info about format.\nIn the single planar NV12 case, the created FrameBuffer::planes has one plane.\nHow can I workaround within MappedFrameBuffer while MappedFrameBuffer\ncouldn't recognize the number of format planes is two?\nI also wonder if FrameBuffer::plane.fds are different in planes thanks\nto exportDmabuf?\n\nBest Regards,\n-Hiro\n> > +\n> > return std::make_unique(std::move(planes));\n> > }\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 A8DE6C3240\n\tfor ;\n\tThu, 12 Aug 2021 05:18:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16F0968864;\n\tThu, 12 Aug 2021 07:18:23 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5F8060260\n\tfor ;\n\tThu, 12 Aug 2021 07:18:21 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id t1so7638028edd.6\n\tfor ;\n\tWed, 11 Aug 2021 22:18:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OjAMD1EY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dTwpxmC3+wXZfDb9qL4zoHFZ1oJGySqiGQj8dx39pXQ=;\n\tb=OjAMD1EYr60QyFWKRKWZh1BtHdz1nBiMVlhGHIuJQq+BmYykPwyjyCVO+AFQtFIV2y\n\tZFwU10FxEZ4KXfF566l9+2jsaV5B1cNfU6xzYExCp0+nZ6JMOU5+n5VI+3LJzsgf6Eq7\n\tMxqBNxJcCcwgGDy1+BRBG+bN1Y1jm9rYPg47g=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dTwpxmC3+wXZfDb9qL4zoHFZ1oJGySqiGQj8dx39pXQ=;\n\tb=DrhPhRwwDptwEho59Ch8bQ/o56vuKUbouYHuNY4y4/ZWy/LQVJ+iH3/1F3tp7c0NkM\n\tUlz92WqXrJQe4JF4JAFxTunIWYbYW5N+UoofRThyuPesBMLdujUjkCllCxNaNYryvV5v\n\ts2SBFYXpdx9CSV9BvDXp4ybvdyqU9NBdZxK8N4XWyzkbhDOP5Dqv0ZIOr9fq2eJBiuLZ\n\tLLo/j4o+1ESQefEaGkyaveF5xhbaFayWs0657C2nTpnpyLQUxHaI0FdijtTeqI0xXMwr\n\tKvQxa2Rp2nAvN1RMKbmNtLuD7wcCqcthN743hJ8sQDp2jcPcASONbC4wsWq/lbyzE3s2\n\t7xSA==","X-Gm-Message-State":"AOAM531Dso02ssRb34NVn8rGtLzLSdumFX+m2MZPi7YhQkWrts6EnImT\n\t8RGVaUd2XZGwTwnCdVBa3hvYU6XYvtM4piVYO+Skug==","X-Google-Smtp-Source":"ABdhPJyDdvbI+B3t1LTvg7CVyNt5q2nH1MIzQqhzed0L/rz0gxPcvwNoYHRlV4cbxSVzNScSeT1f66EUso9JNpeZY/k=","X-Received":"by 2002:a50:ed99:: with SMTP id\n\th25mr3364470edr.327.1628745501435; \n\tWed, 11 Aug 2021 22:18:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210811124015.2116188-1-hiroh@chromium.org>\n\t<20210811124015.2116188-5-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 12 Aug 2021 14:18:11 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","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":18744,"web_url":"https://patchwork.libcamera.org/comment/18744/","msgid":"","date":"2021-08-12T20:25:52","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:\n> On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:\n> > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:\n> > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same\n> > > number of planes as v4l2 buffer planes. However, if the format is\n> > > a single planar format, the number of planes is one. FrameBuffer\n> > > should have the same number of planes as color format planes.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-\n> > > 1 file changed, 14 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index ce60dff6..e70076f3 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1269,7 +1269,6 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > >\n> > > const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > const unsigned int numPlanes = multiPlanar ? buf.length : 1;\n> > > -\n> >\n> > Unrelated change ?\n> >\n> > > if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n> > > LOG(V4L2, Error) << \"Invalid number of planes\";\n> > > return nullptr;\n> > > @@ -1289,6 +1288,20 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > > planes.push_back(std::move(plane));\n> > > }\n> > >\n> > > +\n> >\n> > Extra blank line.\n> >\n> > > + V4L2DeviceFormat format{};\n> >\n> > V4L2DeviceFormat initializes all members to 0 when constructed, you can\n> > drop {}.\n> >\n> > > + ret = getFormat(&format);\n> > > + if (ret < 0) {\n> > > + LOG(V4L2, Error) << \"Failed to get buffer format\";\n> > > + return nullptr;\n> > > + }\n> >\n> > I'd move the extra blank line from above here :-)\n> >\n> > This being said, maybe we could cache the fourcc in V4L2VideoDevice\n> > instead of querying it back from the kernel ?\n> >\n> > > + if (format.fourcc == V4L2_PIX_FMT_NV12) {\n> > > + planes.resize(2);\n> > > + planes[0].length = format.size.width * format.size.height;\n> > > + planes[1].fd = planes[0].fd;\n> > > + planes[1].length = format.size.width * ((format.size.height + 1) / 2);\n> > > + }\n> >\n> > I don't think this is right. Well, it is, but at the same time, it isn't\n> > :-)\n> >\n> > At the moment, the number of planes in the FrameBuffer class matches the\n> > number of \"buffer planes\" used by V4L2 (1 for NV12, as opposed to the\n> > number of \"format planes\", which would be 2). I agree that we want to\n> > move to \"format planes\" in the long term here, to avoid exposing the\n> > V4L2 historical design mistake to applications (\"long term\" meaning \"as\n> > soon as possible\" here). However, this requires adding an offset field\n> > to the FrameBuffer::Plane structure, as otherwise we miss the\n> > information to implement the calculation properly (or at least we miss\n> > exposing the information explictly and rely on undocumented\n> > implementation details).\n> \n> I agree. We need `offset`.\n> \n> > There's no need, I think, to implement this as part of this series. We\n> > can keep the calculation internal to MappedFrameBuffer as a first step,\n> > and then address the issue in the FrameBuffer class. It may also be\n> > possible to do it the other way around, and move to \"format planes\" in\n> > FrameBuffer at the bottom of the series. I have a feeling that it may be\n> > difficult to keep patches reasonable in size and avoid bisection\n> > breakages in that case, but I may be wrong.\n> \n> MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.\n> In the single planar NV12 case, the created FrameBuffer::planes has one plane.\n> How can I workaround within MappedFrameBuffer while MappedFrameBuffer\n> couldn't recognize the number of format planes is two?\n\nMappedFrameBuffer is indeed missing that information. One option could\nbe to then pass the format to the constructor. The other option is to go\nahead with the full change, and add the offset field to\nFrameBuffer::Plane. If you decide to go with the latter (it's a bit more\nwork, but will bring much larger benefits), please let me know if I can\nhelp you.\n\n> I also wonder if FrameBuffer::plane.fds are different in planes thanks\n> to exportDmabuf?\n\nWhen using NV12M, we will get two buffer planes from V4L2, which will be\nexported in two calls to exportDmabufFd(). With NV12, there will be a\nsingle buffer plane, so a single fd. In both case, the V4L2VideoDevice\ncreateBuffer() function will need to initialize two Plane entries in\nFrameBuffer.\n\nThe NV12M case is easy. In the NV12 case it will be a bit more\ndifficult. Brainstorming a little bit, we can compare the number of\nbuffer planes reported by V4L2 and the number of planes reported by\nPixelFormatInfo::numPlanes(). If they differ, we know we have to handle\nthe calculation internally.\n\n> > > +\n> > > return std::make_unique(std::move(planes));\n> > > }\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 A8B0EBD87D\n\tfor ;\n\tThu, 12 Aug 2021 20:25:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0298A68888;\n\tThu, 12 Aug 2021 22:25:59 +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 67EED68823\n\tfor ;\n\tThu, 12 Aug 2021 22:25:57 +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 D5C9DEE;\n\tThu, 12 Aug 2021 22:25:56 +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=\"ZsiWn9Ng\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628799957;\n\tbh=nHVgrwam7ZrYwUOEBqg5FkFoPJo/Uhm9kPaHKvvdSJo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZsiWn9NgxIsKHoQZnw0tWTqBRolZgXMqaoOCagHWdZSBj16apbQzN2RDugae8pNJX\n\tgqD92QZ1dsGRbIYHZRmHxCb5bikXnjx0m4SrrX5mSZVvv2nDfmZRQdBSAuyUoZPp31\n\t7293ReILIWWTsPadrK0wmkYs+RyvNUIPhWDXv284=","Date":"Thu, 12 Aug 2021 23:25:52 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210811124015.2116188-1-hiroh@chromium.org>\n\t<20210811124015.2116188-5-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","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":18749,"web_url":"https://patchwork.libcamera.org/comment/18749/","msgid":"","date":"2021-08-13T03:29:30","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\n\nOn Fri, Aug 13, 2021 at 5:25 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:\n> > On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:\n> > > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:\n> > > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same\n> > > > number of planes as v4l2 buffer planes. However, if the format is\n> > > > a single planar format, the number of planes is one. FrameBuffer\n> > > > should have the same number of planes as color format planes.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-\n> > > > 1 file changed, 14 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index ce60dff6..e70076f3 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -1269,7 +1269,6 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > > >\n> > > > const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > > const unsigned int numPlanes = multiPlanar ? buf.length : 1;\n> > > > -\n> > >\n> > > Unrelated change ?\n> > >\n> > > > if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n> > > > LOG(V4L2, Error) << \"Invalid number of planes\";\n> > > > return nullptr;\n> > > > @@ -1289,6 +1288,20 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > > > planes.push_back(std::move(plane));\n> > > > }\n> > > >\n> > > > +\n> > >\n> > > Extra blank line.\n> > >\n> > > > + V4L2DeviceFormat format{};\n> > >\n> > > V4L2DeviceFormat initializes all members to 0 when constructed, you can\n> > > drop {}.\n> > >\n> > > > + ret = getFormat(&format);\n> > > > + if (ret < 0) {\n> > > > + LOG(V4L2, Error) << \"Failed to get buffer format\";\n> > > > + return nullptr;\n> > > > + }\n> > >\n> > > I'd move the extra blank line from above here :-)\n> > >\n> > > This being said, maybe we could cache the fourcc in V4L2VideoDevice\n> > > instead of querying it back from the kernel ?\n> > >\n> > > > + if (format.fourcc == V4L2_PIX_FMT_NV12) {\n> > > > + planes.resize(2);\n> > > > + planes[0].length = format.size.width * format.size.height;\n> > > > + planes[1].fd = planes[0].fd;\n> > > > + planes[1].length = format.size.width * ((format.size.height + 1) / 2);\n> > > > + }\n> > >\n> > > I don't think this is right. Well, it is, but at the same time, it isn't\n> > > :-)\n> > >\n> > > At the moment, the number of planes in the FrameBuffer class matches the\n> > > number of \"buffer planes\" used by V4L2 (1 for NV12, as opposed to the\n> > > number of \"format planes\", which would be 2). I agree that we want to\n> > > move to \"format planes\" in the long term here, to avoid exposing the\n> > > V4L2 historical design mistake to applications (\"long term\" meaning \"as\n> > > soon as possible\" here). However, this requires adding an offset field\n> > > to the FrameBuffer::Plane structure, as otherwise we miss the\n> > > information to implement the calculation properly (or at least we miss\n> > > exposing the information explictly and rely on undocumented\n> > > implementation details).\n> >\n> > I agree. We need `offset`.\n> >\n> > > There's no need, I think, to implement this as part of this series. We\n> > > can keep the calculation internal to MappedFrameBuffer as a first step,\n> > > and then address the issue in the FrameBuffer class. It may also be\n> > > possible to do it the other way around, and move to \"format planes\" in\n> > > FrameBuffer at the bottom of the series. I have a feeling that it may be\n> > > difficult to keep patches reasonable in size and avoid bisection\n> > > breakages in that case, but I may be wrong.\n> >\n> > MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.\n> > In the single planar NV12 case, the created FrameBuffer::planes has one plane.\n> > How can I workaround within MappedFrameBuffer while MappedFrameBuffer\n> > couldn't recognize the number of format planes is two?\n>\n> MappedFrameBuffer is indeed missing that information. One option could\n> be to then pass the format to the constructor. The other option is to go\n> ahead with the full change, and add the offset field to\n> FrameBuffer::Plane. If you decide to go with the latter (it's a bit more\n> work, but will bring much larger benefits), please let me know if I can\n> help you.\n>\n\nIf we pass format info to MappedFrameBuffer, we need to carry the\nformat info in another way around.\nI am going to introduce offset to FrameBuffer::Plane. It is the correct manner.\nOnce you acknowledge this, I will start working for this.\n\n-Hiro\n> > I also wonder if FrameBuffer::plane.fds are different in planes thanks\n> > to exportDmabuf?\n>\n> When using NV12M, we will get two buffer planes from V4L2, which will be\n> exported in two calls to exportDmabufFd(). With NV12, there will be a\n> single buffer plane, so a single fd. In both case, the V4L2VideoDevice\n> createBuffer() function will need to initialize two Plane entries in\n> FrameBuffer.\n>\n> The NV12M case is easy. In the NV12 case it will be a bit more\n> difficult. Brainstorming a little bit, we can compare the number of\n> buffer planes reported by V4L2 and the number of planes reported by\n> PixelFormatInfo::numPlanes(). If they differ, we know we have to handle\n> the calculation internally.\n>\n> > > > +\n> > > > return std::make_unique(std::move(planes));\n> > > > }\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 97019BD87D\n\tfor ;\n\tFri, 13 Aug 2021 03:29:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B629C6025F;\n\tFri, 13 Aug 2021 05:29:42 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE8F16025F\n\tfor ;\n\tFri, 13 Aug 2021 05:29:40 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id n12so13218579edx.8\n\tfor ;\n\tThu, 12 Aug 2021 20:29:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"TU8UwUHv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=1tSKRSkRHOLrewAnbuip9kw0LqisjHXwOCSLJ6moH3w=;\n\tb=TU8UwUHv/9vkcBtztoDHaVmRvNbSgFCuKif9opSB/QRi61gdfRy0GsGvKHZP6TmXIL\n\tp4wvtvTETHcKszKE861tYkcOZcUui06VblZbnrLRA8Po1Gk/FrkZZV0uL/vZ4vwyUn5f\n\tseCHkOLZbVhok+1/+r9WgcPD0dnKTHu8Nyxn4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=1tSKRSkRHOLrewAnbuip9kw0LqisjHXwOCSLJ6moH3w=;\n\tb=DcRI9VcYrXDiHMZXnKQCW1PdFmjiesVntl8T9qMFcY7IEWYKy/Oah4JScdA+jG4DK/\n\tObt/MMw1vVMNyCxiMXyOh14OwLkOt5hN9/48E53ax/NjAcWreJDCcQ25VqZ1Nvp3vm+K\n\tHbTeXkwZTV/RlYS5/ho3s+1zlE7NB0K/MWGC+Yr9UWA3h7myNCCZYm156+wqlTwsCw3G\n\tX4QsJEwwYVszx1uh4FmL4Zl/45e4DSejDGMRE9JR2qlpenoiB7LndonKLnhKWfwkGy7o\n\t4GmY/PL7cGbXslnAs8Cx8Qj6CWhklnB3wlGVZiIUbdIHWIU2NRuYtLCQE+1BjZYTai5z\n\tAefw==","X-Gm-Message-State":"AOAM532za4mh1njRKj3cy+sO0HBgKydiS8YjTgIP9lRNOvT9Jpz3mMhX\n\tAaR3kuYk0UHYp+fuWohPoz5vKDZe0kZ7fMFHtKgCnQ==","X-Google-Smtp-Source":"ABdhPJztlArEUg5wDZGh/bZCBFK9pn5UeFMwQvvJTBfamfn1PyqmdlPsUlcvXLXJEiq3QYnRihSuUkXH+J2uVnIeur4=","X-Received":"by 2002:a50:da0e:: with SMTP id z14mr317116edj.73.1628825380257; \n\tThu, 12 Aug 2021 20:29:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210811124015.2116188-1-hiroh@chromium.org>\n\t<20210811124015.2116188-5-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 13 Aug 2021 12:29:30 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","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":18750,"web_url":"https://patchwork.libcamera.org/comment/18750/","msgid":"","date":"2021-08-13T03:43:17","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 13, 2021 at 12:29:30PM +0900, Hirokazu Honda wrote:\n> On Fri, Aug 13, 2021 at 5:25 AM Laurent Pinchart wrote:\n> > On Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:\n> > > On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:\n> > > > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:\n> > > > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same\n> > > > > number of planes as v4l2 buffer planes. However, if the format is\n> > > > > a single planar format, the number of planes is one. FrameBuffer\n> > > > > should have the same number of planes as color format planes.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-\n> > > > > 1 file changed, 14 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index ce60dff6..e70076f3 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -1269,7 +1269,6 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > > > >\n> > > > > const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > > > const unsigned int numPlanes = multiPlanar ? buf.length : 1;\n> > > > > -\n> > > >\n> > > > Unrelated change ?\n> > > >\n> > > > > if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {\n> > > > > LOG(V4L2, Error) << \"Invalid number of planes\";\n> > > > > return nullptr;\n> > > > > @@ -1289,6 +1288,20 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > > > > planes.push_back(std::move(plane));\n> > > > > }\n> > > > >\n> > > > > +\n> > > >\n> > > > Extra blank line.\n> > > >\n> > > > > + V4L2DeviceFormat format{};\n> > > >\n> > > > V4L2DeviceFormat initializes all members to 0 when constructed, you can\n> > > > drop {}.\n> > > >\n> > > > > + ret = getFormat(&format);\n> > > > > + if (ret < 0) {\n> > > > > + LOG(V4L2, Error) << \"Failed to get buffer format\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > >\n> > > > I'd move the extra blank line from above here :-)\n> > > >\n> > > > This being said, maybe we could cache the fourcc in V4L2VideoDevice\n> > > > instead of querying it back from the kernel ?\n> > > >\n> > > > > + if (format.fourcc == V4L2_PIX_FMT_NV12) {\n> > > > > + planes.resize(2);\n> > > > > + planes[0].length = format.size.width * format.size.height;\n> > > > > + planes[1].fd = planes[0].fd;\n> > > > > + planes[1].length = format.size.width * ((format.size.height + 1) / 2);\n> > > > > + }\n> > > >\n> > > > I don't think this is right. Well, it is, but at the same time, it isn't\n> > > > :-)\n> > > >\n> > > > At the moment, the number of planes in the FrameBuffer class matches the\n> > > > number of \"buffer planes\" used by V4L2 (1 for NV12, as opposed to the\n> > > > number of \"format planes\", which would be 2). I agree that we want to\n> > > > move to \"format planes\" in the long term here, to avoid exposing the\n> > > > V4L2 historical design mistake to applications (\"long term\" meaning \"as\n> > > > soon as possible\" here). However, this requires adding an offset field\n> > > > to the FrameBuffer::Plane structure, as otherwise we miss the\n> > > > information to implement the calculation properly (or at least we miss\n> > > > exposing the information explictly and rely on undocumented\n> > > > implementation details).\n> > >\n> > > I agree. We need `offset`.\n> > >\n> > > > There's no need, I think, to implement this as part of this series. We\n> > > > can keep the calculation internal to MappedFrameBuffer as a first step,\n> > > > and then address the issue in the FrameBuffer class. It may also be\n> > > > possible to do it the other way around, and move to \"format planes\" in\n> > > > FrameBuffer at the bottom of the series. I have a feeling that it may be\n> > > > difficult to keep patches reasonable in size and avoid bisection\n> > > > breakages in that case, but I may be wrong.\n> > >\n> > > MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.\n> > > In the single planar NV12 case, the created FrameBuffer::planes has one plane.\n> > > How can I workaround within MappedFrameBuffer while MappedFrameBuffer\n> > > couldn't recognize the number of format planes is two?\n> >\n> > MappedFrameBuffer is indeed missing that information. One option could\n> > be to then pass the format to the constructor. The other option is to go\n> > ahead with the full change, and add the offset field to\n> > FrameBuffer::Plane. If you decide to go with the latter (it's a bit more\n> > work, but will bring much larger benefits), please let me know if I can\n> > help you.\n> >\n> \n> If we pass format info to MappedFrameBuffer, we need to carry the\n> format info in another way around.\n> I am going to introduce offset to FrameBuffer::Plane. It is the correct manner.\n> Once you acknowledge this, I will start working for this.\n\nAck :-)\n\nOverall I think we have all the pieces we need to introduce this, but if\nyou run into any issue along the way, please feel free to report it and\nwe can discuss solutions.\n\n> > > I also wonder if FrameBuffer::plane.fds are different in planes thanks\n> > > to exportDmabuf?\n> >\n> > When using NV12M, we will get two buffer planes from V4L2, which will be\n> > exported in two calls to exportDmabufFd(). With NV12, there will be a\n> > single buffer plane, so a single fd. In both case, the V4L2VideoDevice\n> > createBuffer() function will need to initialize two Plane entries in\n> > FrameBuffer.\n> >\n> > The NV12M case is easy. In the NV12 case it will be a bit more\n> > difficult. Brainstorming a little bit, we can compare the number of\n> > buffer planes reported by V4L2 and the number of planes reported by\n> > PixelFormatInfo::numPlanes(). If they differ, we know we have to handle\n> > the calculation internally.\n> >\n> > > > > +\n> > > > > return std::make_unique(std::move(planes));\n> > > > > }\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 C66E6C3240\n\tfor ;\n\tFri, 13 Aug 2021 03:43:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 360496888F;\n\tFri, 13 Aug 2021 05:43:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A9CF6025F\n\tfor ;\n\tFri, 13 Aug 2021 05:43:22 +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 10CD3EE;\n\tFri, 13 Aug 2021 05:43:21 +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=\"nf9xs+nt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628826202;\n\tbh=BVX34qMSVdQRL5wdFLRRl/w6DYyDVJUoRMQfoCl259c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nf9xs+nt0t0L6vJY7l0HoaDoL5+C40TQaiec7q1LWP1fZGxRWVcv21y11xf9rsRpV\n\teurCKfb7Ptbm+Ej2bdyFcudLz9BJ4HM8T+FxiEHgNAFcTTNktBvFbuZiQox8ITkvii\n\tcsTASrOkZD1she6n4sCXqZZSnRrNo8F738Aa1kaA=","Date":"Fri, 13 Aug 2021 06:43:17 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210811124015.2116188-1-hiroh@chromium.org>\n\t<20210811124015.2116188-5-hiroh@chromium.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] WIP: libcamera:\n\tV4L2VideoDevice: Fix a bug in CreateBuffer()","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\" "}}]