[{"id":1623,"web_url":"https://patchwork.libcamera.org/comment/1623/","msgid":"<20190518182047.GF4995@pendragon.ideasonboard.com>","date":"2019-05-18T18:20:47","subject":"Re: [libcamera-devel] [PATCH/RFC 10/12] libcamera: v4l2_device: Add\n\tmethod to enumerate all discrete frame sizes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sat, May 18, 2019 at 02:06:19AM +0300, Laurent Pinchart wrote:\n> From: Niklas Söderlund \n> \n> Allow the video device to be interrogated about which discrete frame\n> sizes it supports.\n\nThe V4L2Subdevice class has a formats() method with a similar purpose. I\nthink we need to address them both, if not with a single API, at least\nwith APIs that work well together, and are based on the same design\nconcepts.\n\nWe should also keep in mind that the stream formats exposed to\napplications (which is missing from this series), and the enumerated\nformats used internally don't need to be a single class. They serve two\nsimilar but different purposes, so I don't think they should necessarily\nbe identical.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/v4l2_device.h | 4 +++\n> src/libcamera/v4l2_device.cpp | 51 +++++++++++++++++++++++++++++\n> 2 files changed, 55 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 2e7bd1e7f4cc..481d63d9cc4c 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -8,6 +8,8 @@\n> #define __LIBCAMERA_V4L2_DEVICE_H__\n> \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> \n> @@ -126,6 +128,8 @@ public:\n> \n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> +\tstd::map>\n> +\tenumerateFrameSizes(std::set pixelformats);\n> \n> \tint exportBuffers(BufferPool *pool);\n> \tint importBuffers(BufferPool *pool);\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 8366ffc4db55..d26a89f4a27d 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -564,6 +564,57 @@ int V4L2Device::setFormatMultiplane(V4L2DeviceFormat *format)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Enumerate all discrete frame sizes for a set of pixel formats\n> + * \\param[in] pixelformats A set of pixel formats to enumerate sizes for\n> + *\n> + * Enumerate all discrete frame sizes reported by the video device.\n> + *\n> + * \\return A map of pixel format to frame sizes\n> + */\n> +std::map>\n> +V4L2Device::enumerateFrameSizes(std::set pixelformats)\n\nWhy do you pass a set of pixel formats instead of enumerating everything\n?\n\n> +{\n> +\tstd::map> sizes;\n> +\tint ret;\n> +\n> +\tfor (unsigned int pixelformat : pixelformats) {\n> +\t\tstruct v4l2_frmsizeenum frameSize = {};\n> +\t\tunsigned int index = 0;\n> +\n> +\t\tframeSize.pixel_format = pixelformat;\n> +\n> +\t\twhile (true) {\n\nHow about making this a for loop ?\n\n\t\tfor (unsigned int index = 0; ; index++) {\n\n> +\t\t\tframeSize.index = index;\n\nYou should declare the frameSize variable within this loop, as it should\nbe fully initialised for each ioctl call.\n\n\t\t\tstruct v4l2_frmsizeenum frameSize = {\n\t\t\t\t.pixel_format = pixelformat,\n\t\t\t\t.index = index,\n\t\t\t};\n\n> +\n> +\t\t\tret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);\n> +\t\t\tif (ret) {\n> +\t\t\t\tret = -errno;\n> +\n> +\t\t\t\tif (ret == -EINVAL)\n> +\t\t\t\t\tbreak;\n> +\n> +\t\t\t\tif (ret != -ENOTTY)\n> +\t\t\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t\t\t<< \"Unable to enumerate frame size\"\n> +\t\t\t\t\t\t<< strerror(-ret);\n\nWhy do you skip the error message when the error code is ENOTTY ?\n\n> +\n> +\t\t\t\treturn {};\n> +\t\t\t}\n> +\n> +\t\t\tif (frameSize.type != V4L2_FRMSIZE_TYPE_DISCRETE)\n> +\t\t\t\treturn {};\n\nDon't we need to support ranges ?\n\n> +\n> +\t\t\tsizes[pixelformat].push_back(Size(frameSize.discrete.width,\n> +\t\t\t\t\t\t\t frameSize.discrete.height));\n> +\n> +\t\t\tindex++;\n> +\t\t}\n> +\t}\n> +\n> +\treturn sizes;\n> +}\n> +\n> int V4L2Device::requestBuffers(unsigned int count)\n> {\n> \tstruct v4l2_requestbuffers rb = {};","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22AB860C1D\n\tfor ;\n\tSat, 18 May 2019 20:21:04 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89656D5\n\tfor ;\n\tSat, 18 May 2019 20:21:03 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558203663;\n\tbh=ND1nk8mDjOjpS60HDV5E7T8T5MGWEqtYEO/JWWm01C8=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=UnOwc6K/Y/BhNpbFuyYXBYbrA31khXTtyNbWx0Z2awzZt74rAKDOCVubK8SqzlxBn\n\tbfjiyv4zfbdvILuw9IbyYijUBJFtmp+6AV/HjSA4t0g6r5rPhezC+olnwGArKS+w2b\n\t9mrVACXZh40D3Ikv96vbWzWd+IjRMCboHfmIuDu8=","Date":"Sat, 18 May 2019 21:20:47 +0300","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190518182047.GF4995@pendragon.ideasonboard.com>","References":"<20190517230621.24668-1-laurent.pinchart@ideasonboard.com>\n\t<20190517230621.24668-11-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190517230621.24668-11-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH/RFC 10/12] libcamera: v4l2_device: Add\n\tmethod to enumerate all discrete frame sizes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 18 May 2019 18:21:04 -0000"}}]