[{"id":19486,"web_url":"https://patchwork.libcamera.org/comment/19486/","msgid":"<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>","date":"2021-09-06T23:54:15","subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 06/09/2021 23:56, Laurent Pinchart wrote:\n> When dequeueing a buffer from a V4L2VideoDevice, the number of planes in\n> the FrameBuffer may not match the number of V4L2 buffer planes if the\n> PixelFormat is multi-planar (has multiple colour planes) and the V4L2\n> format is single-planar (has a single buffer plane). In this case, we\n> need to split the single V4L2 buffer plane into FrameBuffer planes. Do\n> so, and add checks to reject invalid V4L2 buffers in case of a driver\n> issue.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n> \n> - Reduce indentation\n> ---\n> src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++---\n> 1 file changed, 48 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 6c1ca9bb0a5c..2e458fcc22e0 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \tbuffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> \t\t\t\t + buf.timestamp.tv_usec * 1000ULL;\n> \n> -\tbuffer->metadata_.planes.clear();\n> -\tif (multiPlanar) {\n> -\t\tfor (unsigned int nplane = 0; nplane < buf.length; nplane++)\n> -\t\t\tbuffer->metadata_.planes.push_back({ planes[nplane].bytesused });\n> +\tif (V4L2_TYPE_IS_OUTPUT(buf.type))\n> +\t\treturn buffer;\n> +\n> +\tunsigned int numV4l2Planes = multiPlanar ? buf.length : 1;\n> +\tFrameMetadata &metadata = buffer->metadata_;\n> +\tmetadata.planes.clear();\n> +\n> +\tif (numV4l2Planes != buffer->planes().size()) {\n> +\t\t/*\n> +\t\t * If we have a multi-planar buffer with a V4L2\n> +\t\t * single-planar format, split the V4L2 buffer across\n> +\t\t * the buffer planes. Only the last plane may have less\n> +\t\t * bytes used than its length.\n> +\t\t */\n> +\t\tif (numV4l2Planes != 1) {\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Invalid number of planes (\" << numV4l2Planes\n> +\t\t\t\t<< \" != \" << buffer->planes().size() << \")\";\n\nI'm weary that this error print is more about the first conditional\nstatement, but I think it still fits here anyway.\n\n\n> +\n> +\t\t\tmetadata.status = FrameMetadata::FrameError;\n> +\t\t\treturn buffer;\n> +\t\t}\n> +\n> +\t\tunsigned int bytesused = multiPlanar ? planes[0].bytesused\n> +\t\t\t\t : buf.bytesused;\n> +\n> +\t\tfor (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> +\t\t\tif (!bytesused) {\n> +\t\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t\t<< \"Dequeued buffer is too small\";\n> +\n> +\t\t\t\tmetadata.status = FrameMetadata::FrameError;\n> +\t\t\t\treturn buffer;\n> +\t\t\t}\n> +\n> +\t\t\tmetadata.planes.push_back({ std::min(plane.length, bytesused) });\n> +\t\t\tbytesused -= metadata.planes.back().bytesused;\n> +\t\t}\n> +\t} else if (multiPlanar) {\n> +\t\t/*\n> +\t\t * If we use the multi-planar API, fill in the planes.\n> +\t\t * The number of planes in the frame buffer and in the\n> +\t\t * V4L2 buffer is guaranteed to be equal at this point.\n> +\t\t */\n> +\t\tfor (unsigned int i = 0; i < numV4l2Planes; ++i)\n> +\t\t\tmetadata.planes.push_back({ planes[i].bytesused });\n> \t} else {\n> -\t\tbuffer->metadata_.planes.push_back({ buf.bytesused });\n> +\t\tmetadata.planes.push_back({ buf.bytesused });\n\nI was going to ask about the metadata plan allocations, but now I see\nthat's handled in 16/30\n\nReviewed-by: Kieran Bingham \n\n> \t}\n> \n> \treturn buffer;\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 6CE9EBE175\n\tfor ;\n\tMon, 6 Sep 2021 23:54:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B53069167;\n\tTue, 7 Sep 2021 01:54:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B99C560253\n\tfor ;\n\tTue, 7 Sep 2021 01:54:18 +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 43475891;\n\tTue, 7 Sep 2021 01:54:18 +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=\"bfwkKmKb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630972458;\n\tbh=FvoVmMvPxU1XtaaFmTTawYryjMKwSfCulg4AoJI1dxQ=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=bfwkKmKbj/4K9fL6r7lw9LQQjZqs3Gt/jl9dQXd+Hn9AcKDndggS+AqulPBc7lVUb\n\tnuruuzLNcbBno6w6zziOpyGlSXT7pBuIYcqUgo96K9KJnEUdmo6rK86bILMWm3qaHt\n\tsVHWOFztmoV3DXpmTBzMa02jpn1u/nmv2deapZqg=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210906225420.13275-1-laurent.pinchart@ideasonboard.com>\n\t<20210906225636.14683-14-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>","Date":"Tue, 7 Sep 2021 00:54:15 +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":"<20210906225636.14683-14-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19491,"web_url":"https://patchwork.libcamera.org/comment/19491/","msgid":"","date":"2021-09-07T00:05:53","subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote:\n> On 06/09/2021 23:56, Laurent Pinchart wrote:\n> > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in\n> > the FrameBuffer may not match the number of V4L2 buffer planes if the\n> > PixelFormat is multi-planar (has multiple colour planes) and the V4L2\n> > format is single-planar (has a single buffer plane). In this case, we\n> > need to split the single V4L2 buffer plane into FrameBuffer planes. Do\n> > so, and add checks to reject invalid V4L2 buffers in case of a driver\n> > issue.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> > \n> > - Reduce indentation\n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++---\n> > 1 file changed, 48 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 6c1ca9bb0a5c..2e458fcc22e0 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -7,6 +7,7 @@\n> > \n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > \tbuffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > \t\t\t\t + buf.timestamp.tv_usec * 1000ULL;\n> > \n> > -\tbuffer->metadata_.planes.clear();\n> > -\tif (multiPlanar) {\n> > -\t\tfor (unsigned int nplane = 0; nplane < buf.length; nplane++)\n> > -\t\t\tbuffer->metadata_.planes.push_back({ planes[nplane].bytesused });\n> > +\tif (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > +\t\treturn buffer;\n> > +\n> > +\tunsigned int numV4l2Planes = multiPlanar ? buf.length : 1;\n> > +\tFrameMetadata &metadata = buffer->metadata_;\n> > +\tmetadata.planes.clear();\n> > +\n> > +\tif (numV4l2Planes != buffer->planes().size()) {\n\n*1\n\n> > +\t\t/*\n> > +\t\t * If we have a multi-planar buffer with a V4L2\n> > +\t\t * single-planar format, split the V4L2 buffer across\n> > +\t\t * the buffer planes. Only the last plane may have less\n> > +\t\t * bytes used than its length.\n> > +\t\t */\n> > +\t\tif (numV4l2Planes != 1) {\n> > +\t\t\tLOG(V4L2, Error)\n> > +\t\t\t\t<< \"Invalid number of planes (\" << numV4l2Planes\n> > +\t\t\t\t<< \" != \" << buffer->planes().size() << \")\";\n> \n> I'm weary that this error print is more about the first conditional\n> statement, but I think it still fits here anyway.\n\nDid you mean *1 ? I'm not sure to follow you.\n\n> > +\n> > +\t\t\tmetadata.status = FrameMetadata::FrameError;\n> > +\t\t\treturn buffer;\n> > +\t\t}\n> > +\n> > +\t\tunsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > +\t\t\t\t : buf.bytesused;\n> > +\n> > +\t\tfor (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > +\t\t\tif (!bytesused) {\n> > +\t\t\t\tLOG(V4L2, Error)\n> > +\t\t\t\t\t<< \"Dequeued buffer is too small\";\n> > +\n> > +\t\t\t\tmetadata.status = FrameMetadata::FrameError;\n> > +\t\t\t\treturn buffer;\n> > +\t\t\t}\n> > +\n> > +\t\t\tmetadata.planes.push_back({ std::min(plane.length, bytesused) });\n> > +\t\t\tbytesused -= metadata.planes.back().bytesused;\n> > +\t\t}\n> > +\t} else if (multiPlanar) {\n> > +\t\t/*\n> > +\t\t * If we use the multi-planar API, fill in the planes.\n> > +\t\t * The number of planes in the frame buffer and in the\n> > +\t\t * V4L2 buffer is guaranteed to be equal at this point.\n> > +\t\t */\n> > +\t\tfor (unsigned int i = 0; i < numV4l2Planes; ++i)\n> > +\t\t\tmetadata.planes.push_back({ planes[i].bytesused });\n> > \t} else {\n> > -\t\tbuffer->metadata_.planes.push_back({ buf.bytesused });\n> > +\t\tmetadata.planes.push_back({ buf.bytesused });\n> \n> I was going to ask about the metadata plan allocations, but now I see\n> that's handled in 16/30\n> \n> Reviewed-by: Kieran Bingham \n> \n> > \t}\n> > \n> > \treturn buffer;","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 6F54DBE175\n\tfor ;\n\tTue, 7 Sep 2021 00:06:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43B486916D;\n\tTue, 7 Sep 2021 02:06:15 +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 0E9C560253\n\tfor ;\n\tTue, 7 Sep 2021 02:06:13 +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 99E72891;\n\tTue, 7 Sep 2021 02:06:12 +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=\"A2zcgurx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630973172;\n\tbh=LT05KOETBNi1najXLfsS6yY/Moh404YBJqv+x9hcfrM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A2zcgurxjjFxPmp5LZLQbASxY9NqYleH2y64/4WvKmjAqv/kzn2LLkI5qjixnjBo9\n\tKfy7lc0wqT/VpHV2F0jgt6reTrwLx9+0n05L7ZpQit4QUd4TBOqIHRwAZm7UbBdAy2\n\tmbdqQpn17uwSvtGUks1RLmbb9oK2ot0TEjVOBbYU=","Date":"Tue, 7 Sep 2021 03:05:53 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210906225420.13275-1-laurent.pinchart@ideasonboard.com>\n\t<20210906225636.14683-14-laurent.pinchart@ideasonboard.com>\n\t<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","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":19495,"web_url":"https://patchwork.libcamera.org/comment/19495/","msgid":"","date":"2021-09-07T08:07:04","subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Sep 7, 2021 at 9:06 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote:\n> > On 06/09/2021 23:56, Laurent Pinchart wrote:\n> > > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in\n> > > the FrameBuffer may not match the number of V4L2 buffer planes if the\n> > > PixelFormat is multi-planar (has multiple colour planes) and the V4L2\n> > > format is single-planar (has a single buffer plane). In this case, we\n> > > need to split the single V4L2 buffer plane into FrameBuffer planes. Do\n> > > so, and add checks to reject invalid V4L2 buffers in case of a driver\n> > > issue.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n\nReviewed-by: Hirokazu Honda \n\n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Reduce indentation\n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++---\n> > > 1 file changed, 48 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 6c1ca9bb0a5c..2e458fcc22e0 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -7,6 +7,7 @@\n> > >\n> > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > > + buf.timestamp.tv_usec * 1000ULL;\n> > >\n> > > - buffer->metadata_.planes.clear();\n> > > - if (multiPlanar) {\n> > > - for (unsigned int nplane = 0; nplane < buf.length; nplane++)\n> > > - buffer->metadata_.planes.push_back({ planes[nplane].bytesused });\n> > > + if (V4L2_TYPE_IS_OUTPUT(buf.type))\n> > > + return buffer;\n> > > +\n> > > + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;\n> > > + FrameMetadata &metadata = buffer->metadata_;\n> > > + metadata.planes.clear();\n> > > +\n> > > + if (numV4l2Planes != buffer->planes().size()) {\n>\n> *1\n>\n> > > + /*\n> > > + * If we have a multi-planar buffer with a V4L2\n> > > + * single-planar format, split the V4L2 buffer across\n> > > + * the buffer planes. Only the last plane may have less\n> > > + * bytes used than its length.\n> > > + */\n> > > + if (numV4l2Planes != 1) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Invalid number of planes (\" << numV4l2Planes\n> > > + << \" != \" << buffer->planes().size() << \")\";\n> >\n> > I'm weary that this error print is more about the first conditional\n> > statement, but I think it still fits here anyway.\n>\n> Did you mean *1 ? I'm not sure to follow you.\n>\n> > > +\n> > > + metadata.status = FrameMetadata::FrameError;\n> > > + return buffer;\n> > > + }\n> > > +\n> > > + unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > > + : buf.bytesused;\n> > > +\n> > > + for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > > + if (!bytesused) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Dequeued buffer is too small\";\n> > > +\n> > > + metadata.status = FrameMetadata::FrameError;\n> > > + return buffer;\n> > > + }\n> > > +\n> > > + metadata.planes.push_back({ std::min(plane.length, bytesused) });\n> > > + bytesused -= metadata.planes.back().bytesused;\n> > > + }\n> > > + } else if (multiPlanar) {\n> > > + /*\n> > > + * If we use the multi-planar API, fill in the planes.\n> > > + * The number of planes in the frame buffer and in the\n> > > + * V4L2 buffer is guaranteed to be equal at this point.\n> > > + */\n> > > + for (unsigned int i = 0; i < numV4l2Planes; ++i)\n> > > + metadata.planes.push_back({ planes[i].bytesused });\n> > > } else {\n> > > - buffer->metadata_.planes.push_back({ buf.bytesused });\n> > > + metadata.planes.push_back({ buf.bytesused });\n> >\n> > I was going to ask about the metadata plan allocations, but now I see\n> > that's handled in 16/30\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > }\n> > >\n> > > return buffer;\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 6D413BE175\n\tfor ;\n\tTue, 7 Sep 2021 08:07:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E818F6916E;\n\tTue, 7 Sep 2021 10:07:17 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFE726916A\n\tfor ;\n\tTue, 7 Sep 2021 10:07:15 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id i21so18053178ejd.2\n\tfor ;\n\tTue, 07 Sep 2021 01:07:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BCQuje+b\"; 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=0CIhUZyjNQ80EMTgJT6968IeAv6eLJMP0MmQiW8mG/8=;\n\tb=BCQuje+bR49rv6hp6H8nrGERtqsdw7APevNYrfiUD5piVQ/dRhCzIkZfIMvmHdKZyg\n\tq9jN4qBpv8LI8Aky9o2/4hedHMgGDSc0YHvVp9ztarlLRGmbZhF7qNOvPnhaddTG+SUt\n\taXQNQQWQEAPzgpgsoj03EYwpFJ7egd+2DPbcA=","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=0CIhUZyjNQ80EMTgJT6968IeAv6eLJMP0MmQiW8mG/8=;\n\tb=R0vn8gb3gHT/d+qoxZIh8095qz9ObOSVhdHCj95uwYGnnVaPYY5WhjV+/O5AFnpjZg\n\tZciGUFwVR8kqgaHdrtIbqsh5BQ4obSCZ3bHS36PcBG4QL35ER87NNMbyMTOhKF25k9Cw\n\tnweynII5NmM+7O+QRn8DQJD+/VlTa+2clY4jzyfehzKirIMSzVJcIsqcu7UNBSYAUySt\n\t3+BylJc5FWyNlTVeBWb9Ec1eq3Tkrr6srux3LW8QjctgjYIxinEvK8MmnNgMzewHvLM4\n\tenzJ4eT9awUcu4mGr9YWWbpcxLpaBBr5iVFdmVS3jQZDxKxs2yMdE8OsZ6y89tlDBvRj\n\tRltQ==","X-Gm-Message-State":"AOAM533o6xju/kO89LyMXm7p8rzsDRffffAebZV0iYqhuOj7zyRq4bxH\n\tav6xnBCCvn407Jk4rUACVSSIhDwRLjNV3x6EitZOQQ==","X-Google-Smtp-Source":"ABdhPJw/Kc0j7XwJGsNw7T5safsgJ+1SNY8Ka7ax8FblJPuR7sUAy6Aa9Wv2J5bUbu/7L1L9BmXqD4O3duAHmmR+zKY=","X-Received":"by 2002:a17:906:3adb:: with SMTP id\n\tz27mr17506577ejd.291.1631002035281; \n\tTue, 07 Sep 2021 01:07:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210906225420.13275-1-laurent.pinchart@ideasonboard.com>\n\t<20210906225636.14683-14-laurent.pinchart@ideasonboard.com>\n\t<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 7 Sep 2021 17:07:04 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","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":19500,"web_url":"https://patchwork.libcamera.org/comment/19500/","msgid":"","date":"2021-09-07T09:40:22","subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 07/09/2021 01:05, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote:\n>> On 06/09/2021 23:56, Laurent Pinchart wrote:\n>>> When dequeueing a buffer from a V4L2VideoDevice, the number of planes in\n>>> the FrameBuffer may not match the number of V4L2 buffer planes if the\n>>> PixelFormat is multi-planar (has multiple colour planes) and the V4L2\n>>> format is single-planar (has a single buffer plane). In this case, we\n>>> need to split the single V4L2 buffer plane into FrameBuffer planes. Do\n>>> so, and add checks to reject invalid V4L2 buffers in case of a driver\n>>> issue.\n>>>\n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> Changes since v1:\n>>>\n>>> - Reduce indentation\n>>> ---\n>>> src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++---\n>>> 1 file changed, 48 insertions(+), 5 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>> index 6c1ca9bb0a5c..2e458fcc22e0 100644\n>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>> @@ -7,6 +7,7 @@\n>>> \n>>> #include \"libcamera/internal/v4l2_videodevice.h\"\n>>> \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n>>> \tbuffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n>>> \t\t\t\t + buf.timestamp.tv_usec * 1000ULL;\n>>> \n>>> -\tbuffer->metadata_.planes.clear();\n>>> -\tif (multiPlanar) {\n>>> -\t\tfor (unsigned int nplane = 0; nplane < buf.length; nplane++)\n>>> -\t\t\tbuffer->metadata_.planes.push_back({ planes[nplane].bytesused });\n>>> +\tif (V4L2_TYPE_IS_OUTPUT(buf.type))\n>>> +\t\treturn buffer;\n>>> +\n>>> +\tunsigned int numV4l2Planes = multiPlanar ? buf.length : 1;\n>>> +\tFrameMetadata &metadata = buffer->metadata_;\n>>> +\tmetadata.planes.clear();\n>>> +\n>>> +\tif (numV4l2Planes != buffer->planes().size()) {\n> \n> *1\n> \n>>> +\t\t/*\n>>> +\t\t * If we have a multi-planar buffer with a V4L2\n>>> +\t\t * single-planar format, split the V4L2 buffer across\n>>> +\t\t * the buffer planes. Only the last plane may have less\n>>> +\t\t * bytes used than its length.\n>>> +\t\t */\n>>> +\t\tif (numV4l2Planes != 1) {\n>>> +\t\t\tLOG(V4L2, Error)\n>>> +\t\t\t\t<< \"Invalid number of planes (\" << numV4l2Planes\n>>> +\t\t\t\t<< \" != \" << buffer->planes().size() << \")\";\n>>\n>> I'm weary that this error print is more about the first conditional\n>> statement, but I think it still fits here anyway.\n> \n> Did you mean *1 ? I'm not sure to follow you.\n\nYes, the error print matches the conditional check of *1, but the actual\nerror is that\n (numV4l2Planes != buffer->planes().size)\n && numV4l2Planes != 1.\n\nIt will be evident enough what has happened to any reader digging into\nthat error message, so it's fine.\n\n\n\n\n> \n>>> +\n>>> +\t\t\tmetadata.status = FrameMetadata::FrameError;\n>>> +\t\t\treturn buffer;\n>>> +\t\t}\n>>> +\n>>> +\t\tunsigned int bytesused = multiPlanar ? planes[0].bytesused\n>>> +\t\t\t\t : buf.bytesused;\n>>> +\n>>> +\t\tfor (auto [i, plane] : utils::enumerate(buffer->planes())) {\n>>> +\t\t\tif (!bytesused) {\n>>> +\t\t\t\tLOG(V4L2, Error)\n>>> +\t\t\t\t\t<< \"Dequeued buffer is too small\";\n>>> +\n>>> +\t\t\t\tmetadata.status = FrameMetadata::FrameError;\n>>> +\t\t\t\treturn buffer;\n>>> +\t\t\t}\n>>> +\n>>> +\t\t\tmetadata.planes.push_back({ std::min(plane.length, bytesused) });\n>>> +\t\t\tbytesused -= metadata.planes.back().bytesused;\n>>> +\t\t}\n>>> +\t} else if (multiPlanar) {\n>>> +\t\t/*\n>>> +\t\t * If we use the multi-planar API, fill in the planes.\n>>> +\t\t * The number of planes in the frame buffer and in the\n>>> +\t\t * V4L2 buffer is guaranteed to be equal at this point.\n>>> +\t\t */\n>>> +\t\tfor (unsigned int i = 0; i < numV4l2Planes; ++i)\n>>> +\t\t\tmetadata.planes.push_back({ planes[i].bytesused });\n>>> \t} else {\n>>> -\t\tbuffer->metadata_.planes.push_back({ buf.bytesused });\n>>> +\t\tmetadata.planes.push_back({ buf.bytesused });\n>>\n>> I was going to ask about the metadata plan allocations, but now I see\n>> that's handled in 16/30\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> \t}\n>>> \n>>> \treturn buffer;\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 7CA22BE175\n\tfor ;\n\tTue, 7 Sep 2021 09:40:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DACC96916C;\n\tTue, 7 Sep 2021 11:40:26 +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 2144869167\n\tfor ;\n\tTue, 7 Sep 2021 11:40:26 +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 974E98F;\n\tTue, 7 Sep 2021 11:40:25 +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=\"lupNQUEi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631007625;\n\tbh=8izlEmkfZuA0oUGA4IPlcRt9twBiUphB2I5xBPveuM8=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=lupNQUEiZfKKKJ8unvsnlZCrWP1wnEvyKkVP4DHCpBTbID3zes9zqlWt7XZood+/0\n\tPKVHhvGcio+XpW+g4vN3URuJkQbL2RxfcnvyFOo2y6LkkfcsacujQfZtC4uJCWGyz4\n\tS5iQjiZXHGi1tKeaFnU991dw5f56un5fTRThg6rk=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210906225420.13275-1-laurent.pinchart@ideasonboard.com>\n\t<20210906225636.14683-14-laurent.pinchart@ideasonboard.com>\n\t<915f6bde-b020-1140-cf8e-d7e7976628ca@ideasonboard.com>\n\t","Message-ID":"","Date":"Tue, 7 Sep 2021 10:40:22 +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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 14/30] libcamera: v4l2_videodevice:\n\tSplit planes when dequeuing buffer","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\" "}}]