[{"id":19179,"web_url":"https://patchwork.libcamera.org/comment/19179/","msgid":"","date":"2021-08-30T15:57:43","subject":"Re: [libcamera-devel] [PATCH v4 7/9] libcamera: v4l2_videodevice:\n\tCreate color-format planes 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 Fri, Aug 27, 2021 at 03:44:39PM +0900, Hirokazu Honda wrote:\n> V4L2VideDevice::createBuffer() creates the same number of\n> FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2\n> format single is single-planar format, the created number of\n> FrameBuffer::Planes is 1.\n> It should rather create the same number of FrameBuffer::Planes as the\n> color format planes.\n> \n> Signed-off-by: Hirokazu Honda \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> \n> Let me send this patch only in v4.\n> \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 1 +\n> src/libcamera/v4l2_videodevice.cpp | 69 ++++++++++++++++---\n> 2 files changed, 61 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index e767ec84..4a5d2cad 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -242,6 +242,7 @@ private:\n> \tFrameBuffer *dequeueBuffer();\n> \n> \tV4L2Capability caps_;\n> +\tV4L2DeviceFormat format_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> \tenum v4l2_memory memoryType_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 2ff25af2..9d35618d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -25,6 +25,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/media_object.h\"\n> \n> @@ -579,6 +580,12 @@ int V4L2VideoDevice::open()\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -668,6 +675,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -761,12 +774,20 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> */\n> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> {\n> +\tint ret = 0;\n> \tif (caps_.isMeta())\n> -\t\treturn trySetFormatMeta(format, true);\n> +\t\tret = trySetFormatMeta(format, true);\n> \telse if (caps_.isMultiplanar())\n> -\t\treturn trySetFormatMultiplane(format, true);\n> +\t\tret = trySetFormatMultiplane(format, true);\n> \telse\n> -\t\treturn trySetFormatSingleplane(format, true);\n> +\t\tret = trySetFormatSingleplane(format, true);\n> +\n> +\t/* Cache the set format on success. */\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tformat_ = *format;\n> +\treturn 0;\n> }\n> \n> int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> @@ -1152,8 +1173,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> * successful return the driver's internal buffer management is initialized in\n> * MMAP mode, and the video device is ready to accept queueBuffer() calls.\n> *\n> - * The number of planes and the plane sizes for the allocation are determined\n> - * by the currently active format on the device as set by setFormat().\n> + * The number of planes and their offsets and sizes are determined by the\n> + * currently active format on the device as set by setFormat(). They do not map\n> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> + * instances will have two planes, for the luma and chroma components,\n> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> + * V4L2_PIX_FMT_NV12M.\n> *\n> * Buffers allocated with this function shall later be free with\n> * releaseBuffers(). If buffers have already been allocated with\n> @@ -1190,8 +1216,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> * usable with any V4L2 video device in DMABUF mode, or with other dmabuf\n> * importers.\n> *\n> - * The number of planes and the plane sizes for the allocation are determined\n> - * by the currently active format on the device as set by setFormat().\n> + * The number of planes and their offsets and sizes are determined by the\n> + * currently active format on the device as set by setFormat(). They do not map\n> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> + * instances will have two planes, for the luma and chroma components,\n> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> + * V4L2_PIX_FMT_NV12M.\n> *\n> * Multiple independent sets of buffers can be allocated with multiple calls to\n> * this function. Device-specific limitations may apply regarding the minimum\n> @@ -1289,12 +1320,32 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> \t\t * \\todo Set the right offset once V4L2 API provides a way.\n> \t\t */\n> \t\tplane.offset = 0;\n> -\t\tplane.length = multiPlanar ?\n> -\t\t\tbuf.m.planes[nplane].length : buf.length;\n> +\t\tplane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> \n> \t\tplanes.push_back(std::move(plane));\n> \t}\n> \n> +\tconst auto &info = PixelFormatInfo::info(format_.fourcc);\n> +\tif (info.isValid() && info.numPlanes() != numPlanes) {\n> +\t\tASSERT(numPlanes == 1u);\n> +\t\tconst size_t numColorPlanes = info.numPlanes();\n> +\t\tplanes.resize(numColorPlanes);\n> +\t\tconst FileDescriptor &fd = planes[0].fd;\n> +\t\tsize_t offset = 0;\n> +\t\tfor (size_t i = 0; i < numColorPlanes; ++i) {\n> +\t\t\tplanes[i].fd = fd;\n> +\t\t\tplanes[i].offset = offset;\n> +\n> +\t\t\t/* \\todo Take the V4L2 stride into account */\n> +\t\t\tconst unsigned int vertSubSample =\n> +\t\t\t\tinfo.planes[i].verticalSubSampling;\n> +\t\t\tplanes[i].length =\n> +\t\t\t\tinfo.stride(format_.size.width, i, 1u) *\n> +\t\t\t\t((format_.size.height + vertSubSample - 1) / vertSubSample);\n> +\t\t\toffset += planes[i].length;\n> +\t\t}\n> +\t}\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 CCED0BD87D\n\tfor ;\n\tMon, 30 Aug 2021 15:57:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3770F69169;\n\tMon, 30 Aug 2021 17:57:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9238460258\n\tfor ;\n\tMon, 30 Aug 2021 17:57: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 132A65A7;\n\tMon, 30 Aug 2021 17:57:57 +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=\"pmVAKyNy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630339077;\n\tbh=IGT17hxejsA1YCNTZBLEIYLWObz61xAlWe3ISUuMJn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pmVAKyNyQzKGvp8B2eIUBZhBXcqwC2wwjadBzKAukCt4m8B8BpO+fus+MOBKc8H4H\n\tkXzrtLz04+gimC72YAQPBCbA5ic3g2P87LgMqkpP5siHmBNPWE2pFrMDnG1OwDsW+i\n\thFp6y31cD/3a85fCoEYLTPjyipJurnKi/r4IaUTY=","Date":"Mon, 30 Aug 2021 18:57:43 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210827064439.879308-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210827064439.879308-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 7/9] libcamera: v4l2_videodevice:\n\tCreate color-format planes 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\" "}}]