[{"id":10875,"web_url":"https://patchwork.libcamera.org/comment/10875/","msgid":"<20200626014908.GW5865@pendragon.ideasonboard.com>","date":"2020-06-26T01:49:08","subject":"Re: [libcamera-devel] [PATCH v2 6/6] libcamera: v4l2_videodevice:\n\tUse ImageFormats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Jun 09, 2020 at 01:28:44AM +0200, Jacopo Mondi wrote:\n> ImageFormats was meant to be used not only for V4L2Subdevice but\n> for video devices as well. Since the introduction of of V4L2PixelFormat\n\ns/of of/of/\n\n> the V4L2VideoDevice class and its users have been using a raw map to\n> enumerate and inspect the V4L2VideoDevice formats.\n> \n> Provide a V4L2VideoDevice::Formats type definition as for V4L2Subdevice\n> and use ImageFormats wherever possible in pipeline handlers.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 4 +++-\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++--------\n> src/libcamera/pipeline/simple/simple.cpp | 3 +--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +--\n> src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> 5 files changed, 21 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 4d21f5a01ec8..06c65e863a99 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -168,6 +168,8 @@ public:\n> class V4L2VideoDevice : public V4L2Device\n> {\n> public:\n> +\tusing Formats = ImageFormats;\n> +\n> \texplicit V4L2VideoDevice(const std::string &deviceNode);\n> \texplicit V4L2VideoDevice(const MediaEntity *entity);\n> \tV4L2VideoDevice(const V4L2VideoDevice &) = delete;\n> @@ -187,7 +189,7 @@ public:\n> \n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> -\tstd::map> formats(uint32_t code = 0);\n> +\tFormats formats(uint32_t code = 0);\n> \n> \tint setSelection(unsigned int target, Rectangle *rect);\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index b9b88506c646..861d0864f9f7 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -37,8 +37,6 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RPI)\n> \n> -using V4L2PixFmtMap = std::map>;\n> -\n> namespace {\n> \n> bool isRaw(PixelFormat &pixFmt)\n> @@ -67,7 +65,7 @@ double scoreFormat(double desired, double actual)\n> \treturn score;\n> }\n> \n> -V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)\n> +V4L2DeviceFormat findBestMode(V4L2VideoDevice::Formats &formatsMap, const Size &req)\n> {\n> \tdouble bestScore = 9e9, score;\n> \tV4L2DeviceFormat bestMode = {};\n> @@ -427,7 +425,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\t\t * Calculate the best sensor mode we can use based on\n> \t\t\t * the user request.\n> \t\t\t */\n> -\t\t\tV4L2PixFmtMap fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> +\t\t\tV4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();\n> \t\t\tV4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);\n> \t\t\tPixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();\n> \t\t\tif (cfg.size != sensorFormat.size ||\n> @@ -481,14 +479,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\t *\n> \t\t */\n> \t\tPixelFormat &cfgPixFmt = config_.at(outSize[i].first).pixelFormat;\n> -\t\tV4L2PixFmtMap fmts;\n> +\t\tV4L2VideoDevice::Formats fmts;\n> \n> \t\tif (i == maxIndex)\n> \t\t\tfmts = data_->isp_[Isp::Output0].dev()->formats();\n> \t\telse\n> \t\t\tfmts = data_->isp_[Isp::Output1].dev()->formats();\n> \n> -\t\tif (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {\n> +\t\tif (!fmts.contains(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false))) {\n> \t\t\t/* If we cannot find a native format, use a default one. */\n> \t\t\tcfgPixFmt = PixelFormat(DRM_FORMAT_NV12);\n> \t\t\tstatus = Adjusted;\n> @@ -518,7 +516,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \tRPiCameraData *data = cameraData(camera);\n> \tCameraConfiguration *config = new RPiCameraConfiguration(data);\n> \tV4L2DeviceFormat sensorFormat;\n> -\tV4L2PixFmtMap fmts;\n> +\tV4L2VideoDevice::Formats fmts;\n> \n> \tif (roles.empty())\n> \t\treturn config;\n> @@ -605,7 +603,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t}\n> \n> \t/* First calculate the best sensor mode we can use based on the user request. */\n> -\tV4L2PixFmtMap fmts = data->unicam_[Unicam::Image].dev()->formats();\n> +\tV4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();\n> \tV4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);\n> \n> \t/*\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 1ec8d0f7de03..59c4a6da1ee9 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -275,8 +275,7 @@ int SimpleCameraData::init()\n> \t\t\treturn ret;\n> \t\t}\n> \n> -\t\tstd::map> videoFormats =\n> -\t\t\tvideo_->formats(format.mbus_code);\n> +\t\tV4L2VideoDevice::Formats videoFormats = video_->formats(format.mbus_code);\n> \n> \t\tLOG(SimplePipeline, Debug)\n> \t\t\t<< \"Adding configuration for \" << format.size.toString()\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index a074909499f1..9e41cbd50c1a 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -159,8 +159,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> \tif (roles.empty())\n> \t\treturn config;\n> \n> -\tstd::map> v4l2Formats =\n> -\t\tdata->video_->formats();\n> +\tV4L2VideoDevice::Formats v4l2Formats = data->video_->formats();\n> \tstd::map> deviceFormats;\n> \tstd::transform(v4l2Formats.begin(), v4l2Formats.end(),\n> \t\t std::inserter(deviceFormats, deviceFormats.begin()),\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3614b2ed1cbc..077e150ceda6 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -461,6 +461,12 @@ const std::string V4L2DeviceFormat::toString() const\n> * \\context This class is \\threadbound.\n> */\n> \n> +/**\n> + * \\typedef V4L2VideoDevice::Formats\n> + * \\brief Enumeration of V4L2PixelFormat instances associated to image\n> + * resolutions\n\nSame comment as for the V4L2Subdevice variant.\n\n> + */\n> +\n> /**\n> * \\brief Construct a V4L2VideoDevice\n> * \\param[in] deviceNode The file-system path to the video device node\n> @@ -925,23 +931,23 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> *\n> * \\return A list of the supported video device formats\n> */\n> -std::map> V4L2VideoDevice::formats(uint32_t code)\n> +V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)\n> {\n> -\tstd::map> formats;\n> +\tFormats formats;\n> \n> \tfor (V4L2PixelFormat pixelFormat : enumPixelformats(code)) {\n> \t\tstd::vector sizes = enumSizes(pixelFormat);\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n> \n> -\t\tif (formats.find(pixelFormat) != formats.end()) {\n> +\t\tif (!formats.contains(pixelFormat)) {\n\nThis should for\n\n\t\tif (formats.contains(pixelFormat)) {\n\nThe patch otherwise looks good to me, but I'll wait until we finish\ndiscussing patch 3/6 before acking it. In the meantime, could you test\nthe series on RPi, as I believe the bug in the implementation of\ncontains() should have affected it ?\n\n> \t\t\tLOG(V4L2, Error)\n> \t\t\t\t<< \"Could not add sizes for pixel format \"\n> \t\t\t\t<< pixelFormat;\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tformats.emplace(pixelFormat, sizes);\n> +\t\tformats.addFormat(pixelFormat, sizes);\n> \t}\n> \n> \treturn formats;","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 037E9C2E65\n\tfor ;\n\tFri, 26 Jun 2020 01:49:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69FB1609C5;\n\tFri, 26 Jun 2020 03:49:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA721603BA\n\tfor ;\n\tFri, 26 Jun 2020 03:49:10 +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 BFFD172E;\n\tFri, 26 Jun 2020 03:49:09 +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=\"v20FPvOj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593136149;\n\tbh=kJoLLI6aobJ97zEEBIIRzuGgfOShhUvLcCyU628x97k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v20FPvOjFNF7QLP+SQRs1lWwwzpewH/kC4J/kvyT7SyYskEyuXDalpk+vKcz1/DdG\n\tERgqKcuf+JgAGl0LIm1XRCXS44uaAV7SEwEDT4eARtxVFVg+NDKpKXIOU1sN1RxcDp\n\tGfB08PoE27zv0NStYB9N7a+++aiL2Wo2izUYT4YI=","Date":"Fri, 26 Jun 2020 04:49:08 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200626014908.GW5865@pendragon.ideasonboard.com>","References":"<20200608232844.10150-1-jacopo@jmondi.org>\n\t<20200608232844.10150-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200608232844.10150-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] libcamera: v4l2_videodevice:\n\tUse ImageFormats","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]