[{"id":1368,"web_url":"https://patchwork.libcamera.org/comment/1368/","msgid":"<20190415213929.GZ1980@bigcity.dyn.berto.se>","date":"2019-04-15T21:39:29","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-04-15 19:56:58 +0300, Laurent Pinchart wrote:\n> The CameraSensor class abstracts camera sensors and provides helper\n> functions to ease interactions with them. It is currently limited to\n> sensors that expose a single subdev, and offer the same frame sizes for\n> all media bus codes, but will be extended to support more complex\n> sensors as the needs arise.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/camera_sensor.cpp | 245 ++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 56 ++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 303 insertions(+)\n> create mode 100644 src/libcamera/camera_sensor.cpp\n> create mode 100644 src/libcamera/include/camera_sensor.h\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> new file mode 100644\n> index 000000000000..aca9e77fd986\n> --- /dev/null\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -0,0 +1,245 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_sensor.cpp - A camera sensor\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"camera_sensor.h\"\n> +#include \"formats.h\"\n> +#include \"v4l2_subdevice.h\"\n> +\n> +/**\n> + * \\file camera_sensor.h\n> + * \\brief A camera sensor\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensor);\n> +\n> +/**\n> + * \\class CameraSensor\n> + * \\brief A camera sensor\n> + *\n> + * The CameraSensor class eases handling of sensors for pipeline handlers by\n> + * hiding the details of the V4L2 subdevice kernel API and caching sensor\n> + * information.\n> + *\n> + * The implementation is currently limited to sensors that expose a single V4L2\n> + * subdevice with a single pad, and support the same frame sizes for all\n> + * supported media bus codes. It will be extended to support more complex\n> + * devices as the needs arise.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensor\n> + * \\param[in] entity The media entity for the camera sensor\n> + *\n> + * Once constructed the instance must be initialized with init().\n> + */\n> +CameraSensor::CameraSensor(const MediaEntity *entity)\n> +\t: entity_(entity)\n> +{\n> +\tsubdev_ = new V4L2Subdevice(entity);\n> +}\n> +\n> +/**\n> + * \\brief Destroy a CameraSensor\n> + */\n> +CameraSensor::~CameraSensor()\n> +{\n> +\tdelete subdev_;\n> +}\n> +\n> +/**\n> + * \\brief Initialize the camera sensor instance\n> + *\n> + * This methods perform the initialisation steps of the CameraSensor that may\n> + * fail. It shall be called once and only once after constructing the instance.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::init()\n> +{\n> +\tint ret;\n> +\n> +\tif (entity_->pads().size() != 1) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Sensors with more than one pad are not supported\"\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Enumerate and cache media bus codes and sizes. */\n> +\tconst FormatEnum formats = subdev_->formats(0);\n> +\tif (formats.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"No image format found\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::transform(formats.begin(), formats.end(),\n> +\t\t std::back_inserter(mbusCodes_),\n> +\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> +\n> +\tconst std::vector &sizes = formats.begin()->second;\n\nI might add a comment somewhere around here mentioning the limitation \nand why only sizes from formats.begin() are considered. I had to reread \nthe commit message to figure it out. Not a big issue as it's documented \nelsewhere so it won't be forgotten. Maybe me pointing it out here for \nother reviewers benefit is enough.\n\n> +\tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> +\t\t [](const SizeRange &range) { return range.max; });\n> +\n> +\t/*\n> +\t * Verify the assumption that all available media bus codes support the\n> +\t * same frame sizes.\n> +\t */\n> +\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> +\t\tif (it->second != sizes) {\n> +\t\t\tLOG(CameraSensor, Error)\n> +\t\t\t\t<< \"Frame sizes differ between media bus codes\"\n> +\t\t\t\t<< std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\t/* Sort the sizes. */\n> +\tstd::sort(sizes_.begin(), sizes_.end());\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensor::entity()\n> + * \\brief Retrieve the sensor media entity\n> + * \\return The sensor media entity\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::mbusCodes()\n> + * \\brief Retrieve the media bus codes supported by the camera sensor\n> + * \\return The supported media bus codes\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::sizes()\n> + * \\brief Retrieve the frame sizes supported by the camera sensor\n> + * \\return The supported frame sizes\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::resolution()\n> + * \\brief Retrieve the camera sensor resolution\n> + * \\return The camera sensor resolution in pixels\n> + */\n\nMaybe move the function implementation here adding a comment that the \nsizes_ vector is sorted so that the resolution will always be the \nlargest one supported.\n\nWith or without the above nit-pick comments addressed,\n\nReviewed-by: Niklas Söderlund \n\n> +\n> +/**\n> + * \\brief Retrieve the best sensor format for a desired output\n> + * \\param[in] mbusCodes The list of acceptable media bus codes\n> + * \\param[in] size The desired size\n> + *\n> + * Media bus codes are selected from \\a mbusCodes, which lists all acceptable\n> + * codes in decreasing order of preference. This method selects the first code\n> + * from the list that is supported by the sensor. If none of the desired codes\n> + * is supported, it returns an error.\n> + *\n> + * \\a size indicates the desired size at the output of the sensor. This method\n> + * selects the best size supported by the sensor according to the following\n> + * criteria.\n> + *\n> + * - The desired \\a size shall fit in the sensor output size to avoid the need\n> + * to up-scale.\n> + * - The sensor output size shall match the desired aspect ratio to avoid the\n> + * need to crop the field of view.\n> + * - The sensor output size shall be as small as possible to lower the required\n> + * bandwidth.\n> + *\n> + * The use of this method is optional, as the above criteria may not match the\n> + * needs of all pipeline handlers. Pipeline handlers may implement custom\n> + * sensor format selection when needed.\n> + *\n> + * The returned sensor output format is guaranteed to be acceptable by the\n> + * setFormat() method without any modification.\n> + *\n> + * \\return The best sensor output format matching the desired media bus codes\n> + * and size on success, or an empty format otherwise.\n> + */\n> +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbusCodes,\n> +\t\t\t\t\t const Size &size) const\n> +{\n> +\tV4L2SubdeviceFormat format{};\n> +\n> +\tfor (unsigned int code : mbusCodes_) {\n> +\t\tif (std::any_of(mbusCodes.begin(), mbusCodes.end(),\n> +\t\t\t\t[code](unsigned int c) { return c == code; })) {\n> +\t\t\tformat.mbus_code = code;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!format.mbus_code) {\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"No supported format found\" << std::endl;\n> +\t\treturn format;\n> +\t}\n> +\n> +\tunsigned int desiredArea = size.width * size.height;\n> +\tunsigned int bestArea = UINT_MAX;\n> +\tfloat desiredRatio = static_cast(size.width) / size.height;\n> +\tfloat bestRatio = FLT_MAX;\n> +\tconst Size *bestSize = nullptr;\n> +\n> +\tfor (const Size &sz : sizes_) {\n> +\t\tif (sz.width < size.width || sz.height < size.height)\n> +\t\t\tcontinue;\n> +\n> +\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> +\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> +\t\tunsigned int area = sz.width * sz.height;\n> +\t\tunsigned int areaDiff = area - desiredArea;\n> +\n> +\t\tif (ratioDiff > bestRatio)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> +\t\t\tbestRatio = ratioDiff;\n> +\t\t\tbestArea = areaDiff;\n> +\t\t\tbestSize = &sz;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!bestSize) {\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"No supported size found\" << std::endl;\n> +\t\treturn format;\n> +\t}\n> +\n> +\tformat.width = bestSize->width;\n> +\tformat.height = bestSize->height;\n> +\n> +\treturn format;\n> +}\n> +\n> +/**\n> + * \\brief Set the sensor output format\n> + * \\param[in] format The desired sensor output format\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> +{\n> +\treturn subdev_->setFormat(0, format);\n> +}\n> +\n> +std::string CameraSensor::logPrefix() const\n> +{\n> +\treturn \"'\" + subdev_->entity()->name() + \"'\";\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> new file mode 100644\n> index 000000000000..7d92af05248c\n> --- /dev/null\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -0,0 +1,56 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_sensor.h - A camera sensor\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> +#define __LIBCAMERA_CAMERA_SENSOR_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_subdevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +class MediaEntity;\n> +class V4L2SubdeviceFormat;\n> +\n> +class CameraSensor : public Loggable\n> +{\n> +public:\n> +\texplicit CameraSensor(const MediaEntity *entity);\n> +\t~CameraSensor();\n> +\n> +\tCameraSensor(const CameraSensor &) = delete;\n> +\tCameraSensor &operator=(const CameraSensor &) = delete;\n> +\n> +\tint init();\n> +\n> +\tconst MediaEntity *entity() const { return entity_; }\n> +\tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> +\tconst std::vector &sizes() const { return sizes_; }\n> +\tconst Size &resolution() const { return sizes_.back(); }\n> +\n> +\tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> +\t\t\t\t const Size &size) const;\n> +\tint setFormat(V4L2SubdeviceFormat *format);\n> +\n> +protected:\n> +\tstd::string logPrefix() const;\n> +\n> +private:\n> +\tconst MediaEntity *entity_;\n> +\tV4L2Subdevice *subdev_;\n> +\n> +\tstd::vector mbusCodes_;\n> +\tstd::vector sizes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index cd36ac307518..cf4edec05755 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_sources = files([\n> 'buffer.cpp',\n> 'camera.cpp',\n> 'camera_manager.cpp',\n> + 'camera_sensor.cpp',\n> 'device_enumerator.cpp',\n> 'event_dispatcher.cpp',\n> 'event_dispatcher_poll.cpp',\n> @@ -23,6 +24,7 @@ libcamera_sources = files([\n> ])\n> \n> libcamera_headers = files([\n> + 'include/camera_sensor.h',\n> 'include/device_enumerator.h',\n> 'include/event_dispatcher_poll.h',\n> 'include/formats.h',\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C42E60B2E\n\tfor ;\n\tMon, 15 Apr 2019 23:39:31 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id j89so17119858ljb.1\n\tfor ;\n\tMon, 15 Apr 2019 14:39:31 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te14sm11046499ljk.45.2019.04.15.14.39.29\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 14:39:29 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=lwUY3kpkwveT+86JqMU+N5SOxHxwC09hKusiwYohD7Q=;\n\tb=y2IYYV4HhNYnMt5CMwwgcqkYPSFOOpoqtifRzMr8JuONVv4a+qVUF8S7rqKQTmi5Yh\n\taGfR6OKPb4VzvmY3vAyY0t/1zsZMD/Lp/j94TgZGDTFlLBBmlHw5YqoYblqBZTYMckN1\n\tWHfy004AJJuDsZQ0ppsldIZEEj6/7veQsbBxFYS86qGT6TpLKolwwY4a3fUDvsZKQDs2\n\tQn6itjnTsVqYaBzrCDV8ao5ub7ySD7lgbeQZzXHXBq8omkpU5TC7eU+5Wj/2sfCvri3D\n\t4HFUS1VlZiPnQxpJhrYjCgdtLZ2uYA5Ja+XyLKIsipxDtyEXqf7d2s0wdkPEInvQE3Wv\n\tQRkA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=lwUY3kpkwveT+86JqMU+N5SOxHxwC09hKusiwYohD7Q=;\n\tb=iihwromXLjdjRQ94J50EOkQHXWGQw6qNa7Off/LbyA4xabBY8Ant78kNYymaxq3v0y\n\t/SDGZ/J3DWJ4JIDTUw/6IODP3FEqtxTU/zLiK8+NFBBTgy9V6Uk67qMpGd5kpGbjgv7R\n\t3OP5KFokcvBtAloZGMSgRrSPSJ3YgeLevL72VIdM6Wi1TcQcqiyjiBMFxGd7hwMsRyhK\n\t2PfXKD18MHcZ/cxYtmxBtBYJnll4o1yf+XTjaxNt48vBzSCIiFgY9kdmtQ04CnWuu7fO\n\t3lRlijSiVAUMEJcpnjJHJqON7N63+fyQUSgR6wLWXckhcAIw9SSxZWqCrIHPFUhmLXw6\n\tGFCQ==","X-Gm-Message-State":"APjAAAVohO2AeVB+PR7zA9t7I3YDsvm7tZrMJ1SAe0hljsSY7b1KcQCB\n\t2Yf53A/od6QccwRakBqz4mUJ1PpyN3I=","X-Google-Smtp-Source":"APXvYqyPZHfwsQl8iYSSZw9gT8U+HsU+qmPt/xa3ufsFlBipTIqxH2sID2QCDYASqF/pcdFP3qpYFg==","X-Received":"by 2002:a2e:8888:: with SMTP id k8mr41238238lji.43.1555364370624;\n\tMon, 15 Apr 2019 14:39:30 -0700 (PDT)","Date":"Mon, 15 Apr 2019 23:39:29 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415213929.GZ1980@bigcity.dyn.berto.se>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","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":"Mon, 15 Apr 2019 21:39:31 -0000"}},{"id":1371,"web_url":"https://patchwork.libcamera.org/comment/1371/","msgid":"<20190415224942.GM17083@pendragon.ideasonboard.com>","date":"2019-04-15T22:49:42","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Apr 15, 2019 at 11:39:29PM +0200, Niklas Söderlund wrote:\n> On 2019-04-15 19:56:58 +0300, Laurent Pinchart wrote:\n> > The CameraSensor class abstracts camera sensors and provides helper\n> > functions to ease interactions with them. It is currently limited to\n> > sensors that expose a single subdev, and offer the same frame sizes for\n> > all media bus codes, but will be extended to support more complex\n> > sensors as the needs arise.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/camera_sensor.cpp | 245 ++++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 56 ++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 303 insertions(+)\n> > create mode 100644 src/libcamera/camera_sensor.cpp\n> > create mode 100644 src/libcamera/include/camera_sensor.h\n> > \n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > new file mode 100644\n> > index 000000000000..aca9e77fd986\n> > --- /dev/null\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -0,0 +1,245 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_sensor.cpp - A camera sensor\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_sensor.h\"\n> > +#include \"formats.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +\n> > +/**\n> > + * \\file camera_sensor.h\n> > + * \\brief A camera sensor\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(CameraSensor);\n> > +\n> > +/**\n> > + * \\class CameraSensor\n> > + * \\brief A camera sensor\n> > + *\n> > + * The CameraSensor class eases handling of sensors for pipeline handlers by\n> > + * hiding the details of the V4L2 subdevice kernel API and caching sensor\n> > + * information.\n> > + *\n> > + * The implementation is currently limited to sensors that expose a single V4L2\n> > + * subdevice with a single pad, and support the same frame sizes for all\n> > + * supported media bus codes. It will be extended to support more complex\n> > + * devices as the needs arise.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a CameraSensor\n> > + * \\param[in] entity The media entity for the camera sensor\n> > + *\n> > + * Once constructed the instance must be initialized with init().\n> > + */\n> > +CameraSensor::CameraSensor(const MediaEntity *entity)\n> > +\t: entity_(entity)\n> > +{\n> > +\tsubdev_ = new V4L2Subdevice(entity);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Destroy a CameraSensor\n> > + */\n> > +CameraSensor::~CameraSensor()\n> > +{\n> > +\tdelete subdev_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Initialize the camera sensor instance\n> > + *\n> > + * This methods perform the initialisation steps of the CameraSensor that may\n> > + * fail. It shall be called once and only once after constructing the instance.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::init()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tif (entity_->pads().size() != 1) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Sensors with more than one pad are not supported\"\n> > +\t\t\t<< std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->open();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Enumerate and cache media bus codes and sizes. */\n> > +\tconst FormatEnum formats = subdev_->formats(0);\n> > +\tif (formats.empty()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"No image format found\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tstd::transform(formats.begin(), formats.end(),\n> > +\t\t std::back_inserter(mbusCodes_),\n> > +\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> > +\n> > +\tconst std::vector &sizes = formats.begin()->second;\n> \n> I might add a comment somewhere around here mentioning the limitation \n> and why only sizes from formats.begin() are considered. I had to reread \n> the commit message to figure it out. Not a big issue as it's documented \n> elsewhere so it won't be forgotten. Maybe me pointing it out here for \n> other reviewers benefit is enough.\n\nI'll add the following comment:\n\n /*\n * Extract the supported sizes from the first format as we only support\n * sensors that offer the same frame sizes for all media bus codes.\n * Verify this assumption and reject the sensor if it isn't true.\n */\n\n> > +\tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> > +\t\t [](const SizeRange &range) { return range.max; });\n> > +\n> > +\t/*\n> > +\t * Verify the assumption that all available media bus codes support the\n> > +\t * same frame sizes.\n> > +\t */\n\nAnd will remove this one.\n\n> > +\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> > +\t\tif (it->second != sizes) {\n> > +\t\t\tLOG(CameraSensor, Error)\n> > +\t\t\t\t<< \"Frame sizes differ between media bus codes\"\n> > +\t\t\t\t<< std::endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Sort the sizes. */\n> > +\tstd::sort(sizes_.begin(), sizes_.end());\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraSensor::entity()\n> > + * \\brief Retrieve the sensor media entity\n> > + * \\return The sensor media entity\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::mbusCodes()\n> > + * \\brief Retrieve the media bus codes supported by the camera sensor\n> > + * \\return The supported media bus codes\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::sizes()\n> > + * \\brief Retrieve the frame sizes supported by the camera sensor\n> > + * \\return The supported frame sizes\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::resolution()\n> > + * \\brief Retrieve the camera sensor resolution\n> > + * \\return The camera sensor resolution in pixels\n> > + */\n> \n> Maybe move the function implementation here adding a comment that the \n> sizes_ vector is sorted so that the resolution will always be the \n> largest one supported.\n\nDone.\n\n> With or without the above nit-pick comments addressed,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > +\n> > +/**\n> > + * \\brief Retrieve the best sensor format for a desired output\n> > + * \\param[in] mbusCodes The list of acceptable media bus codes\n> > + * \\param[in] size The desired size\n> > + *\n> > + * Media bus codes are selected from \\a mbusCodes, which lists all acceptable\n> > + * codes in decreasing order of preference. This method selects the first code\n> > + * from the list that is supported by the sensor. If none of the desired codes\n> > + * is supported, it returns an error.\n> > + *\n> > + * \\a size indicates the desired size at the output of the sensor. This method\n> > + * selects the best size supported by the sensor according to the following\n> > + * criteria.\n> > + *\n> > + * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > + * to up-scale.\n> > + * - The sensor output size shall match the desired aspect ratio to avoid the\n> > + * need to crop the field of view.\n> > + * - The sensor output size shall be as small as possible to lower the required\n> > + * bandwidth.\n> > + *\n> > + * The use of this method is optional, as the above criteria may not match the\n> > + * needs of all pipeline handlers. Pipeline handlers may implement custom\n> > + * sensor format selection when needed.\n> > + *\n> > + * The returned sensor output format is guaranteed to be acceptable by the\n> > + * setFormat() method without any modification.\n> > + *\n> > + * \\return The best sensor output format matching the desired media bus codes\n> > + * and size on success, or an empty format otherwise.\n> > + */\n> > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbusCodes,\n> > +\t\t\t\t\t const Size &size) const\n> > +{\n> > +\tV4L2SubdeviceFormat format{};\n> > +\n> > +\tfor (unsigned int code : mbusCodes_) {\n> > +\t\tif (std::any_of(mbusCodes.begin(), mbusCodes.end(),\n> > +\t\t\t\t[code](unsigned int c) { return c == code; })) {\n> > +\t\t\tformat.mbus_code = code;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!format.mbus_code) {\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"No supported format found\" << std::endl;\n> > +\t\treturn format;\n> > +\t}\n> > +\n> > +\tunsigned int desiredArea = size.width * size.height;\n> > +\tunsigned int bestArea = UINT_MAX;\n> > +\tfloat desiredRatio = static_cast(size.width) / size.height;\n> > +\tfloat bestRatio = FLT_MAX;\n> > +\tconst Size *bestSize = nullptr;\n> > +\n> > +\tfor (const Size &sz : sizes_) {\n> > +\t\tif (sz.width < size.width || sz.height < size.height)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> > +\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> > +\t\tunsigned int area = sz.width * sz.height;\n> > +\t\tunsigned int areaDiff = area - desiredArea;\n> > +\n> > +\t\tif (ratioDiff > bestRatio)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> > +\t\t\tbestRatio = ratioDiff;\n> > +\t\t\tbestArea = areaDiff;\n> > +\t\t\tbestSize = &sz;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!bestSize) {\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"No supported size found\" << std::endl;\n> > +\t\treturn format;\n> > +\t}\n> > +\n> > +\tformat.width = bestSize->width;\n> > +\tformat.height = bestSize->height;\n> > +\n> > +\treturn format;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set the sensor output format\n> > + * \\param[in] format The desired sensor output format\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > +{\n> > +\treturn subdev_->setFormat(0, format);\n> > +}\n> > +\n> > +std::string CameraSensor::logPrefix() const\n> > +{\n> > +\treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > new file mode 100644\n> > index 000000000000..7d92af05248c\n> > --- /dev/null\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -0,0 +1,56 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_sensor.h - A camera sensor\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> > +#define __LIBCAMERA_CAMERA_SENSOR_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class MediaEntity;\n> > +class V4L2SubdeviceFormat;\n> > +\n> > +class CameraSensor : public Loggable\n> > +{\n> > +public:\n> > +\texplicit CameraSensor(const MediaEntity *entity);\n> > +\t~CameraSensor();\n> > +\n> > +\tCameraSensor(const CameraSensor &) = delete;\n> > +\tCameraSensor &operator=(const CameraSensor &) = delete;\n> > +\n> > +\tint init();\n> > +\n> > +\tconst MediaEntity *entity() const { return entity_; }\n> > +\tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > +\tconst std::vector &sizes() const { return sizes_; }\n> > +\tconst Size &resolution() const { return sizes_.back(); }\n> > +\n> > +\tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > +\t\t\t\t const Size &size) const;\n> > +\tint setFormat(V4L2SubdeviceFormat *format);\n> > +\n> > +protected:\n> > +\tstd::string logPrefix() const;\n> > +\n> > +private:\n> > +\tconst MediaEntity *entity_;\n> > +\tV4L2Subdevice *subdev_;\n> > +\n> > +\tstd::vector mbusCodes_;\n> > +\tstd::vector sizes_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index cd36ac307518..cf4edec05755 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_sources = files([\n> > 'buffer.cpp',\n> > 'camera.cpp',\n> > 'camera_manager.cpp',\n> > + 'camera_sensor.cpp',\n> > 'device_enumerator.cpp',\n> > 'event_dispatcher.cpp',\n> > 'event_dispatcher_poll.cpp',\n> > @@ -23,6 +24,7 @@ libcamera_sources = files([\n> > ])\n> > \n> > libcamera_headers = files([\n> > + 'include/camera_sensor.h',\n> > 'include/device_enumerator.h',\n> > 'include/event_dispatcher_poll.h',\n> > 'include/formats.h',","headers":{"Return-Path":"","Received":["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 9813260B2E\n\tfor ;\n\tTue, 16 Apr 2019 00:49:52 +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 67785333;\n\tTue, 16 Apr 2019 00:49:51 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555368591;\n\tbh=PIhtuDe8qxv2tWtHhb4kCPwZH2bZXRAAkIyQNUc/5HA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cP1jYe1Hmjws504j6r/oFozhv4HJYiJytRkvBU9iIKAL5NukXbifI8LoOslwkyiZn\n\tlmYfWVdU3/nAIkDqRsMhhbagMeyfQfSEmIOgAf9/wvDqXfqx7gH6JHNA9uY+HwT9+x\n\tFAyFTjm9YWflRtMcsN7w3Yn1AeS8Iy4JAzOo1JMM=","Date":"Tue, 16 Apr 2019 01:49:42 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415224942.GM17083@pendragon.ideasonboard.com>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>\n\t<20190415213929.GZ1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415213929.GZ1980@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","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":"Mon, 15 Apr 2019 22:49:52 -0000"}},{"id":1395,"web_url":"https://patchwork.libcamera.org/comment/1395/","msgid":"<20190416162548.rufnwl5uwxc2yiyz@uno.localdomain>","date":"2019-04-16T16:25:48","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:\n> The CameraSensor class abstracts camera sensors and provides helper\n> functions to ease interactions with them. It is currently limited to\n> sensors that expose a single subdev, and offer the same frame sizes for\n> all media bus codes, but will be extended to support more complex\n> sensors as the needs arise.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/camera_sensor.cpp | 245 ++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 56 ++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 303 insertions(+)\n> create mode 100644 src/libcamera/camera_sensor.cpp\n> create mode 100644 src/libcamera/include/camera_sensor.h\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> new file mode 100644\n> index 000000000000..aca9e77fd986\n> --- /dev/null\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -0,0 +1,245 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_sensor.cpp - A camera sensor\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"camera_sensor.h\"\n> +#include \"formats.h\"\n> +#include \"v4l2_subdevice.h\"\n> +\n> +/**\n> + * \\file camera_sensor.h\n> + * \\brief A camera sensor\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensor);\n> +\n> +/**\n> + * \\class CameraSensor\n> + * \\brief A camera sensor\n\na very brief \\brief this one\n\n> + *\n> + * The CameraSensor class eases handling of sensors for pipeline handlers by\n> + * hiding the details of the V4L2 subdevice kernel API and caching sensor\n> + * information.\n\n\"information such as sizes and supported image formats\" ?\n\n> + *\n> + * The implementation is currently limited to sensors that expose a single V4L2\n> + * subdevice with a single pad, and support the same frame sizes for all\n> + * supported media bus codes. It will be extended to support more complex\n> + * devices as the needs arise.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensor\n> + * \\param[in] entity The media entity for the camera sensor\n\ns/for/backing ?\n\n> + *\n> + * Once constructed the instance must be initialized with init().\n> + */\n> +CameraSensor::CameraSensor(const MediaEntity *entity)\n> +\t: entity_(entity)\n> +{\n> +\tsubdev_ = new V4L2Subdevice(entity);\n> +}\n> +\n> +/**\n> + * \\brief Destroy a CameraSensor\n> + */\n> +CameraSensor::~CameraSensor()\n> +{\n> +\tdelete subdev_;\n> +}\n> +\n> +/**\n> + * \\brief Initialize the camera sensor instance\n> + *\n> + * This methods perform the initialisation steps of the CameraSensor that may\n\ns/methods/method\ns/perform/performs\n\nOr have I missed why the plural here?\n\n> + * fail. It shall be called once and only once after constructing the instance.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::init()\n> +{\n> +\tint ret;\n> +\n> +\tif (entity_->pads().size() != 1) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Sensors with more than one pad are not supported\"\n> +\t\t\t<< std::endl;\n\nHere and in the rest of the series, LOG() does not need an std::endl\nunless you want an additional empty line after the message.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->open();\n\nDoes this need to be closed before deleting it?\n\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Enumerate and cache media bus codes and sizes. */\n> +\tconst FormatEnum formats = subdev_->formats(0);\n> +\tif (formats.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"No image format found\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::transform(formats.begin(), formats.end(),\n> +\t\t std::back_inserter(mbusCodes_),\n> +\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> +\n> +\tconst std::vector &sizes = formats.begin()->second;\n> +\tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> +\t\t [](const SizeRange &range) { return range.max; });\n\nneat! I hope we could keep something similar when supporting\nper-format sizes...\n\n> +\n> +\t/*\n> +\t * Verify the assumption that all available media bus codes support the\n> +\t * same frame sizes.\n> +\t */\n> +\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> +\t\tif (it->second != sizes) {\n> +\t\t\tLOG(CameraSensor, Error)\n> +\t\t\t\t<< \"Frame sizes differ between media bus codes\"\n> +\t\t\t\t<< std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\t/* Sort the sizes. */\n> +\tstd::sort(sizes_.begin(), sizes_.end());\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensor::entity()\n> + * \\brief Retrieve the sensor media entity\n\nI was sure we enforced a blank line here, but some classes do and some\nother does not :(\n\n> + * \\return The sensor media entity\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::mbusCodes()\n> + * \\brief Retrieve the media bus codes supported by the camera sensor\n> + * \\return The supported media bus codes\n> + */\n\nmbusCode is V4L2-specific, isn't it? I can't suggest a better name\nthough, \"formats\" is confusing, just \"codes\" is not nice... Up to\nyou...\n\n> +\n> +/**\n> + * \\fn CameraSensor::sizes()\n> + * \\brief Retrieve the frame sizes supported by the camera sensor\n> + * \\return The supported frame sizes\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::resolution()\n> + * \\brief Retrieve the camera sensor resolution\n> + * \\return The camera sensor resolution in pixels\n> + */\n\nThis is the maximum resolution the sensor supports, right?\n\n> +\n> +/**\n> + * \\brief Retrieve the best sensor format for a desired output\n> + * \\param[in] mbusCodes The list of acceptable media bus codes\n> + * \\param[in] size The desired size\n> + *\n> + * Media bus codes are selected from \\a mbusCodes, which lists all acceptable\n> + * codes in decreasing order of preference. This method selects the first code\n> + * from the list that is supported by the sensor. If none of the desired codes\n> + * is supported, it returns an error.\n> + *\n> + * \\a size indicates the desired size at the output of the sensor. This method\n> + * selects the best size supported by the sensor according to the following\n> + * criteria.\n> + *\n> + * - The desired \\a size shall fit in the sensor output size to avoid the need\n> + * to up-scale.\n> + * - The sensor output size shall match the desired aspect ratio to avoid the\n> + * need to crop the field of view.\n> + * - The sensor output size shall be as small as possible to lower the required\n> + * bandwidth.\n> + *\n> + * The use of this method is optional, as the above criteria may not match the\n> + * needs of all pipeline handlers. Pipeline handlers may implement custom\n> + * sensor format selection when needed.\n> + *\n> + * The returned sensor output format is guaranteed to be acceptable by the\n> + * setFormat() method without any modification.\n> + *\n> + * \\return The best sensor output format matching the desired media bus codes\n> + * and size on success, or an empty format otherwise.\n> + */\n> +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbusCodes,\n> +\t\t\t\t\t const Size &size) const\n> +{\n> +\tV4L2SubdeviceFormat format{};\n> +\n> +\tfor (unsigned int code : mbusCodes_) {\n> +\t\tif (std::any_of(mbusCodes.begin(), mbusCodes.end(),\n> +\t\t\t\t[code](unsigned int c) { return c == code; })) {\n> +\t\t\tformat.mbus_code = code;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!format.mbus_code) {\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"No supported format found\" << std::endl;\n> +\t\treturn format;\n> +\t}\n> +\n> +\tunsigned int desiredArea = size.width * size.height;\n\nwill we need a Size::area() ?\n\n> +\tunsigned int bestArea = UINT_MAX;\n> +\tfloat desiredRatio = static_cast(size.width) / size.height;\n> +\tfloat bestRatio = FLT_MAX;\n> +\tconst Size *bestSize = nullptr;\n> +\n> +\tfor (const Size &sz : sizes_) {\n> +\t\tif (sz.width < size.width || sz.height < size.height)\n\nCouldn't you just compare \"sz < size\" ?\n\n * - A size with smaller width and smaller height is smaller.\n * - A size with smaller area is smaller.\n * - A size with smaller width is smaller.\n\ndoesn't it apply here?\n\n> +\t\t\tcontinue;\n> +\n> +\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> +\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> +\t\tunsigned int area = sz.width * sz.height;\n> +\t\tunsigned int areaDiff = area - desiredArea;\n> +\n> +\t\tif (ratioDiff > bestRatio)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n\nThis is puzzling, but most probably what you want. If ratio is <= than\nthe best one, then if ratio or area is < than the current best one\nthen select the format. So there is no priority between ratio and\narea differences, right?\n\n> +\t\t\tbestRatio = ratioDiff;\n> +\t\t\tbestArea = areaDiff;\n> +\t\t\tbestSize = &sz;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!bestSize) {\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"No supported size found\" << std::endl;\n> +\t\treturn format;\n> +\t}\n> +\n> +\tformat.width = bestSize->width;\n> +\tformat.height = bestSize->height;\n> +\n> +\treturn format;\n> +}\n> +\n> +/**\n> + * \\brief Set the sensor output format\n> + * \\param[in] format The desired sensor output format\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> +{\n> +\treturn subdev_->setFormat(0, format);\n> +}\n> +\n> +std::string CameraSensor::logPrefix() const\n> +{\n> +\treturn \"'\" + subdev_->entity()->name() + \"'\";\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> new file mode 100644\n> index 000000000000..7d92af05248c\n> --- /dev/null\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -0,0 +1,56 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_sensor.h - A camera sensor\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> +#define __LIBCAMERA_CAMERA_SENSOR_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_subdevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +class MediaEntity;\n> +class V4L2SubdeviceFormat;\n\nAs you include \"v4l2_subdevice.h\" you don't need this\n\n> +\n> +class CameraSensor : public Loggable\n\nnit: other classes that inherits Loggable inherits its protected\nfields only. Is this desirable?\n\nThanks\n j\n\n> +{\n> +public:\n> +\texplicit CameraSensor(const MediaEntity *entity);\n> +\t~CameraSensor();\n> +\n> +\tCameraSensor(const CameraSensor &) = delete;\n> +\tCameraSensor &operator=(const CameraSensor &) = delete;\n> +\n> +\tint init();\n> +\n> +\tconst MediaEntity *entity() const { return entity_; }\n> +\tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> +\tconst std::vector &sizes() const { return sizes_; }\n> +\tconst Size &resolution() const { return sizes_.back(); }\n> +\n> +\tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> +\t\t\t\t const Size &size) const;\n> +\tint setFormat(V4L2SubdeviceFormat *format);\n> +\n> +protected:\n> +\tstd::string logPrefix() const;\n> +\n> +private:\n> +\tconst MediaEntity *entity_;\n> +\tV4L2Subdevice *subdev_;\n> +\n> +\tstd::vector mbusCodes_;\n> +\tstd::vector sizes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index cd36ac307518..cf4edec05755 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_sources = files([\n> 'buffer.cpp',\n> 'camera.cpp',\n> 'camera_manager.cpp',\n> + 'camera_sensor.cpp',\n> 'device_enumerator.cpp',\n> 'event_dispatcher.cpp',\n> 'event_dispatcher_poll.cpp',\n> @@ -23,6 +24,7 @@ libcamera_sources = files([\n> ])\n>\n> libcamera_headers = files([\n> + 'include/camera_sensor.h',\n> 'include/device_enumerator.h',\n> 'include/event_dispatcher_poll.h',\n> 'include/formats.h',\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70CAF60DB4\n\tfor ;\n\tTue, 16 Apr 2019 18:24:58 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id CD92BFF80D;\n\tTue, 16 Apr 2019 16:24:56 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 16 Apr 2019 18:25:48 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416162548.rufnwl5uwxc2yiyz@uno.localdomain>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vbo5skj4guagoypt\"","Content-Disposition":"inline","In-Reply-To":"<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","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":"Tue, 16 Apr 2019 16:24:58 -0000"}},{"id":1406,"web_url":"https://patchwork.libcamera.org/comment/1406/","msgid":"<20190416205242.GD4822@pendragon.ideasonboard.com>","date":"2019-04-16T20:52:42","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 16, 2019 at 06:25:48PM +0200, Jacopo Mondi wrote:\n> On Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:\n> > The CameraSensor class abstracts camera sensors and provides helper\n> > functions to ease interactions with them. It is currently limited to\n> > sensors that expose a single subdev, and offer the same frame sizes for\n> > all media bus codes, but will be extended to support more complex\n> > sensors as the needs arise.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/camera_sensor.cpp | 245 ++++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 56 ++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 303 insertions(+)\n> > create mode 100644 src/libcamera/camera_sensor.cpp\n> > create mode 100644 src/libcamera/include/camera_sensor.h\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > new file mode 100644\n> > index 000000000000..aca9e77fd986\n> > --- /dev/null\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -0,0 +1,245 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_sensor.cpp - A camera sensor\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_sensor.h\"\n> > +#include \"formats.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +\n> > +/**\n> > + * \\file camera_sensor.h\n> > + * \\brief A camera sensor\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(CameraSensor);\n> > +\n> > +/**\n> > + * \\class CameraSensor\n> > + * \\brief A camera sensor\n> \n> a very brief \\brief this one\n\nIsn't it neat ? :-) On a more serious note, is there other information\nyou think should be included in \\brief here ?\n\n> > + *\n> > + * The CameraSensor class eases handling of sensors for pipeline handlers by\n> > + * hiding the details of the V4L2 subdevice kernel API and caching sensor\n> > + * information.\n> \n> \"information such as sizes and supported image formats\" ?\n\nI had this in a previous version :-) Given that the class has limited\nfeatures at the moment and will be extended probably sooner than later I\ndecided to leave specific examples out for now.\n\n> > + *\n> > + * The implementation is currently limited to sensors that expose a single V4L2\n> > + * subdevice with a single pad, and support the same frame sizes for all\n> > + * supported media bus codes. It will be extended to support more complex\n> > + * devices as the needs arise.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a CameraSensor\n> > + * \\param[in] entity The media entity for the camera sensor\n> \n> s/for/backing ?\n\nI think we'll have to update this when the CameraSensor class will be\nextended, but for now I lack a better term, so I'll go with your\nsuggestion.\n\n> > + *\n> > + * Once constructed the instance must be initialized with init().\n> > + */\n> > +CameraSensor::CameraSensor(const MediaEntity *entity)\n> > +\t: entity_(entity)\n> > +{\n> > +\tsubdev_ = new V4L2Subdevice(entity);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Destroy a CameraSensor\n> > + */\n> > +CameraSensor::~CameraSensor()\n> > +{\n> > +\tdelete subdev_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Initialize the camera sensor instance\n> > + *\n> > + * This methods perform the initialisation steps of the CameraSensor that may\n> \n> s/methods/method\n> s/perform/performs\n> \n> Or have I missed why the plural here?\n\nNo, you're right. Fixed.\n\n> > + * fail. It shall be called once and only once after constructing the instance.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::init()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tif (entity_->pads().size() != 1) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Sensors with more than one pad are not supported\"\n> > +\t\t\t<< std::endl;\n> \n> Here and in the rest of the series, LOG() does not need an std::endl\n> unless you want an additional empty line after the message.\n\nOops. Fixed.\n\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->open();\n> \n> Does this need to be closed before deleting it?\n\nI would have sworn we had a destructor for V4L2Subdevice that would\nclose the fd. I'll add an extra patch to fix this.\n\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Enumerate and cache media bus codes and sizes. */\n> > +\tconst FormatEnum formats = subdev_->formats(0);\n> > +\tif (formats.empty()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"No image format found\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tstd::transform(formats.begin(), formats.end(),\n> > +\t\t std::back_inserter(mbusCodes_),\n> > +\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> > +\n> > +\tconst std::vector &sizes = formats.begin()->second;\n> > +\tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> > +\t\t [](const SizeRange &range) { return range.max; });\n> \n> neat! I hope we could keep something similar when supporting\n> per-format sizes...\n\nI pondered for some time on the usage of std::transform with a lambda\nversus an explicit loop, and in the end decided to go for it because it\nreminded me of Python :-)\n\n> > +\n> > +\t/*\n> > +\t * Verify the assumption that all available media bus codes support the\n> > +\t * same frame sizes.\n> > +\t */\n> > +\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> > +\t\tif (it->second != sizes) {\n> > +\t\t\tLOG(CameraSensor, Error)\n> > +\t\t\t\t<< \"Frame sizes differ between media bus codes\"\n> > +\t\t\t\t<< std::endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Sort the sizes. */\n> > +\tstd::sort(sizes_.begin(), sizes_.end());\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraSensor::entity()\n> > + * \\brief Retrieve the sensor media entity\n> \n> I was sure we enforced a blank line here, but some classes do and some\n> other does not :(\n\nI've replied to a similar comment in one of your previous reviews, let's\ndiscuss it there.\n\n> > + * \\return The sensor media entity\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::mbusCodes()\n> > + * \\brief Retrieve the media bus codes supported by the camera sensor\n> > + * \\return The supported media bus codes\n> > + */\n> \n> mbusCode is V4L2-specific, isn't it? I can't suggest a better name\n> though, \"formats\" is confusing, just \"codes\" is not nice... Up to\n> you...\n\nIt is V4L2-specific, true, but I think it's fine. The CameraSensor class\nis a helper to handle sensors exposed through the V4L2 API, and it is\nsupposed to integrate with the rest of a V4L2-based MC pipeline. Usage\nof V4L2-specific terms is fine in my opinion, given that the class is\ninternal to libcamera. I'll add a mention of V4L2 to the class\ndocumentation.\n\n> > +\n> > +/**\n> > + * \\fn CameraSensor::sizes()\n> > + * \\brief Retrieve the frame sizes supported by the camera sensor\n> > + * \\return The supported frame sizes\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::resolution()\n> > + * \\brief Retrieve the camera sensor resolution\n> > + * \\return The camera sensor resolution in pixels\n> > + */\n> \n> This is the maximum resolution the sensor supports, right?\n\nCorrect. Do you think \"sensor resolution\" is too vague and would require\na mention of maximum resolution ?\n\n> > +\n> > +/**\n> > + * \\brief Retrieve the best sensor format for a desired output\n> > + * \\param[in] mbusCodes The list of acceptable media bus codes\n> > + * \\param[in] size The desired size\n> > + *\n> > + * Media bus codes are selected from \\a mbusCodes, which lists all acceptable\n> > + * codes in decreasing order of preference. This method selects the first code\n> > + * from the list that is supported by the sensor. If none of the desired codes\n> > + * is supported, it returns an error.\n> > + *\n> > + * \\a size indicates the desired size at the output of the sensor. This method\n> > + * selects the best size supported by the sensor according to the following\n> > + * criteria.\n> > + *\n> > + * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > + * to up-scale.\n> > + * - The sensor output size shall match the desired aspect ratio to avoid the\n> > + * need to crop the field of view.\n> > + * - The sensor output size shall be as small as possible to lower the required\n> > + * bandwidth.\n> > + *\n> > + * The use of this method is optional, as the above criteria may not match the\n> > + * needs of all pipeline handlers. Pipeline handlers may implement custom\n> > + * sensor format selection when needed.\n> > + *\n> > + * The returned sensor output format is guaranteed to be acceptable by the\n> > + * setFormat() method without any modification.\n> > + *\n> > + * \\return The best sensor output format matching the desired media bus codes\n> > + * and size on success, or an empty format otherwise.\n> > + */\n> > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbusCodes,\n> > +\t\t\t\t\t const Size &size) const\n> > +{\n> > +\tV4L2SubdeviceFormat format{};\n> > +\n> > +\tfor (unsigned int code : mbusCodes_) {\n> > +\t\tif (std::any_of(mbusCodes.begin(), mbusCodes.end(),\n> > +\t\t\t\t[code](unsigned int c) { return c == code; })) {\n> > +\t\t\tformat.mbus_code = code;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!format.mbus_code) {\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"No supported format found\" << std::endl;\n> > +\t\treturn format;\n> > +\t}\n> > +\n> > +\tunsigned int desiredArea = size.width * size.height;\n> \n> will we need a Size::area() ?\n\nPossibly, but in order to be generic, that would need to return a 64-bit\ninteger. I've limited the computation to 32-bit here on purpose as we\nwon't need more than that for the foreseable future (4GPixels sensors\n?).\n\n> > +\tunsigned int bestArea = UINT_MAX;\n> > +\tfloat desiredRatio = static_cast(size.width) / size.height;\n> > +\tfloat bestRatio = FLT_MAX;\n> > +\tconst Size *bestSize = nullptr;\n> > +\n> > +\tfor (const Size &sz : sizes_) {\n> > +\t\tif (sz.width < size.width || sz.height < size.height)\n> \n> Couldn't you just compare \"sz < size\" ?\n> \n> * - A size with smaller width and smaller height is smaller.\n> * - A size with smaller area is smaller.\n> * - A size with smaller width is smaller.\n> \n> doesn't it apply here?\n\nI did to start with, and then realized it wouldn't work. We want to\nselect a size that is larger than the size parameter in both width and\nheight. operator< would accept a size that overlaps with the size\nparameter (has a larger height but smaller width for instance), which we\ndon't want to accept.\n\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> > +\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> > +\t\tunsigned int area = sz.width * sz.height;\n> > +\t\tunsigned int areaDiff = area - desiredArea;\n> > +\n> > +\t\tif (ratioDiff > bestRatio)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> \n> This is puzzling, but most probably what you want. If ratio is <= than\n> the best one, then if ratio or area is < than the current best one\n> then select the format. So there is no priority between ratio and\n> area differences, right?\n\nThere's actually a priority, if the ratio is better but the area isn't,\nthe size is still picked. The area criteria is only taken into account\nwhen the ratios are identical.\n\n> > +\t\t\tbestRatio = ratioDiff;\n> > +\t\t\tbestArea = areaDiff;\n> > +\t\t\tbestSize = &sz;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!bestSize) {\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"No supported size found\" << std::endl;\n> > +\t\treturn format;\n> > +\t}\n> > +\n> > +\tformat.width = bestSize->width;\n> > +\tformat.height = bestSize->height;\n> > +\n> > +\treturn format;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set the sensor output format\n> > + * \\param[in] format The desired sensor output format\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > +{\n> > +\treturn subdev_->setFormat(0, format);\n> > +}\n> > +\n> > +std::string CameraSensor::logPrefix() const\n> > +{\n> > +\treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > new file mode 100644\n> > index 000000000000..7d92af05248c\n> > --- /dev/null\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -0,0 +1,56 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_sensor.h - A camera sensor\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> > +#define __LIBCAMERA_CAMERA_SENSOR_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class MediaEntity;\n> > +class V4L2SubdeviceFormat;\n> \n> As you include \"v4l2_subdevice.h\" you don't need this\n\nI actually don't need to include that header, I'll add a forward\ndeclaration of V4L2Subdevice instead.\n\n> > +\n> > +class CameraSensor : public Loggable\n> \n> nit: other classes that inherits Loggable inherits its protected\n> fields only. Is this desirable?\n\nThis isn't about inheriting the protected members only, but about\nturning all private members of the base class into protected members of\nthe derived class. That is, if Loggable has a public foo() method, and\nCameraSensor derives from Loggable with the protected qualifier, then\nthe foo() method of a CameraSensor instance would be protected, not\npublic.\n\nIt makes no difference in this case as the only public method the\nLoggable function exposes is a virtual destructor, but I'll change it\nfor consistency.\n\n> > +{\n> > +public:\n> > +\texplicit CameraSensor(const MediaEntity *entity);\n> > +\t~CameraSensor();\n> > +\n> > +\tCameraSensor(const CameraSensor &) = delete;\n> > +\tCameraSensor &operator=(const CameraSensor &) = delete;\n> > +\n> > +\tint init();\n> > +\n> > +\tconst MediaEntity *entity() const { return entity_; }\n> > +\tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > +\tconst std::vector &sizes() const { return sizes_; }\n> > +\tconst Size &resolution() const { return sizes_.back(); }\n> > +\n> > +\tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > +\t\t\t\t const Size &size) const;\n> > +\tint setFormat(V4L2SubdeviceFormat *format);\n> > +\n> > +protected:\n> > +\tstd::string logPrefix() const;\n> > +\n> > +private:\n> > +\tconst MediaEntity *entity_;\n> > +\tV4L2Subdevice *subdev_;\n> > +\n> > +\tstd::vector mbusCodes_;\n> > +\tstd::vector sizes_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index cd36ac307518..cf4edec05755 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_sources = files([\n> > 'buffer.cpp',\n> > 'camera.cpp',\n> > 'camera_manager.cpp',\n> > + 'camera_sensor.cpp',\n> > 'device_enumerator.cpp',\n> > 'event_dispatcher.cpp',\n> > 'event_dispatcher_poll.cpp',\n> > @@ -23,6 +24,7 @@ libcamera_sources = files([\n> > ])\n> >\n> > libcamera_headers = files([\n> > + 'include/camera_sensor.h',\n> > 'include/device_enumerator.h',\n> > 'include/event_dispatcher_poll.h',\n> > 'include/formats.h',","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 DD25F60004\n\tfor ;\n\tTue, 16 Apr 2019 22:52:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C6E0E2;\n\tTue, 16 Apr 2019 22:52:59 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555447979;\n\tbh=H15ispL8ZffLjeS/QgyJuoW4svvGl8ZzAOJNb2s2bwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WvAWwf6ue6Lsp2mvASnzw5WN22/u+x+W8c8FRg7WiuIUziR92jIGb2YNZ1O/OkRG3\n\t0iBmKQtSz5VwWuYwCZvOlsu0iuPJJNTjxebsAkLqKPRz4sIypQor6cI0/1jWT6OBYi\n\tp6qM4Kw0E6sUuagt3XTxSGsCjpKMP12KZAE+IJNc=","Date":"Tue, 16 Apr 2019 23:52:42 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416205242.GD4822@pendragon.ideasonboard.com>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>\n\t<20190416162548.rufnwl5uwxc2yiyz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190416162548.rufnwl5uwxc2yiyz@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","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":"Tue, 16 Apr 2019 20:53:00 -0000"}},{"id":1424,"web_url":"https://patchwork.libcamera.org/comment/1424/","msgid":"<20190417074204.76f7dxssmbvozh22@uno.localdomain>","date":"2019-04-17T07:42:04","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 16, 2019 at 11:52:42PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Apr 16, 2019 at 06:25:48PM +0200, Jacopo Mondi wrote:\n> > On Mon, Apr 15, 2019 at 07:56:58PM +0300, Laurent Pinchart wrote:\n> > > The CameraSensor class abstracts camera sensors and provides helper\n> > > functions to ease interactions with them. It is currently limited to\n> > > sensors that expose a single subdev, and offer the same frame sizes for\n> > > all media bus codes, but will be extended to support more complex\n> > > sensors as the needs arise.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 245 ++++++++++++++++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 56 ++++++\n> > > src/libcamera/meson.build | 2 +\n> > > 3 files changed, 303 insertions(+)\n> > > create mode 100644 src/libcamera/camera_sensor.cpp\n> > > create mode 100644 src/libcamera/include/camera_sensor.h\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > new file mode 100644\n> > > index 000000000000..aca9e77fd986\n> > > --- /dev/null\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -0,0 +1,245 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_sensor.cpp - A camera sensor\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"camera_sensor.h\"\n> > > +#include \"formats.h\"\n> > > +#include \"v4l2_subdevice.h\"\n> > > +\n> > > +/**\n> > > + * \\file camera_sensor.h\n> > > + * \\brief A camera sensor\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(CameraSensor);\n> > > +\n> > > +/**\n> > > + * \\class CameraSensor\n> > > + * \\brief A camera sensor\n> >\n> > a very brief \\brief this one\n>\n> Isn't it neat ? :-) On a more serious note, is there other information\n> you think should be included in \\brief here ?\n>\n\nit's fine, there are are short descriptions in other classes\n\n> > > + *\n> > > + * The CameraSensor class eases handling of sensors for pipeline handlers by\n> > > + * hiding the details of the V4L2 subdevice kernel API and caching sensor\n> > > + * information.\n> >\n> > \"information such as sizes and supported image formats\" ?\n>\n> I had this in a previous version :-) Given that the class has limited\n> features at the moment and will be extended probably sooner than later I\n> decided to leave specific examples out for now.\n>\n\nfine with me!\n\n> > > + *\n> > > + * The implementation is currently limited to sensors that expose a single V4L2\n> > > + * subdevice with a single pad, and support the same frame sizes for all\n> > > + * supported media bus codes. It will be extended to support more complex\n> > > + * devices as the needs arise.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a CameraSensor\n> > > + * \\param[in] entity The media entity for the camera sensor\n> >\n> > s/for/backing ?\n>\n> I think we'll have to update this when the CameraSensor class will be\n> extended, but for now I lack a better term, so I'll go with your\n> suggestion.\n>\n> > > + *\n> > > + * Once constructed the instance must be initialized with init().\n> > > + */\n> > > +CameraSensor::CameraSensor(const MediaEntity *entity)\n> > > +\t: entity_(entity)\n> > > +{\n> > > +\tsubdev_ = new V4L2Subdevice(entity);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Destroy a CameraSensor\n> > > + */\n> > > +CameraSensor::~CameraSensor()\n> > > +{\n> > > +\tdelete subdev_;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Initialize the camera sensor instance\n> > > + *\n> > > + * This methods perform the initialisation steps of the CameraSensor that may\n> >\n> > s/methods/method\n> > s/perform/performs\n> >\n> > Or have I missed why the plural here?\n>\n> No, you're right. Fixed.\n>\n> > > + * fail. It shall be called once and only once after constructing the instance.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int CameraSensor::init()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tif (entity_->pads().size() != 1) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Sensors with more than one pad are not supported\"\n> > > +\t\t\t<< std::endl;\n> >\n> > Here and in the rest of the series, LOG() does not need an std::endl\n> > unless you want an additional empty line after the message.\n>\n> Oops. Fixed.\n>\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tret = subdev_->open();\n> >\n> > Does this need to be closed before deleting it?\n>\n> I would have sworn we had a destructor for V4L2Subdevice that would\n> close the fd. I'll add an extra patch to fix this.\n>\n\nI see the patch! thanks\n\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Enumerate and cache media bus codes and sizes. */\n> > > +\tconst FormatEnum formats = subdev_->formats(0);\n> > > +\tif (formats.empty()) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"No image format found\" << std::endl;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tstd::transform(formats.begin(), formats.end(),\n> > > +\t\t std::back_inserter(mbusCodes_),\n> > > +\t\t [](decltype(*formats.begin()) f) { return f.first; });\n> > > +\n> > > +\tconst std::vector &sizes = formats.begin()->second;\n> > > +\tstd::transform(sizes.begin(), sizes.end(), std::back_inserter(sizes_),\n> > > +\t\t [](const SizeRange &range) { return range.max; });\n> >\n> > neat! I hope we could keep something similar when supporting\n> > per-format sizes...\n>\n> I pondered for some time on the usage of std::transform with a lambda\n> versus an explicit loop, and in the end decided to go for it because it\n> reminded me of Python :-)\n>\n\nindeed! this is very similar to a python's map\n\n> > > +\n> > > +\t/*\n> > > +\t * Verify the assumption that all available media bus codes support the\n> > > +\t * same frame sizes.\n> > > +\t */\n> > > +\tfor (auto it = ++formats.begin(); it != formats.end(); ++it) {\n> > > +\t\tif (it->second != sizes) {\n> > > +\t\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t\t<< \"Frame sizes differ between media bus codes\"\n> > > +\t\t\t\t<< std::endl;\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\t/* Sort the sizes. */\n> > > +\tstd::sort(sizes_.begin(), sizes_.end());\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn CameraSensor::entity()\n> > > + * \\brief Retrieve the sensor media entity\n> >\n> > I was sure we enforced a blank line here, but some classes do and some\n> > other does not :(\n>\n> I've replied to a similar comment in one of your previous reviews, let's\n> discuss it there.\n>\n> > > + * \\return The sensor media entity\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn CameraSensor::mbusCodes()\n> > > + * \\brief Retrieve the media bus codes supported by the camera sensor\n> > > + * \\return The supported media bus codes\n> > > + */\n> >\n> > mbusCode is V4L2-specific, isn't it? I can't suggest a better name\n> > though, \"formats\" is confusing, just \"codes\" is not nice... Up to\n> > you...\n>\n> It is V4L2-specific, true, but I think it's fine. The CameraSensor class\n> is a helper to handle sensors exposed through the V4L2 API, and it is\n> supposed to integrate with the rest of a V4L2-based MC pipeline. Usage\n> of V4L2-specific terms is fine in my opinion, given that the class is\n> internal to libcamera. I'll add a mention of V4L2 to the class\n> documentation.\n>\n\nI see, fine with me!\n\n> > > +\n> > > +/**\n> > > + * \\fn CameraSensor::sizes()\n> > > + * \\brief Retrieve the frame sizes supported by the camera sensor\n> > > + * \\return The supported frame sizes\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn CameraSensor::resolution()\n> > > + * \\brief Retrieve the camera sensor resolution\n> > > + * \\return The camera sensor resolution in pixels\n> > > + */\n> >\n> > This is the maximum resolution the sensor supports, right?\n>\n> Correct. Do you think \"sensor resolution\" is too vague and would require\n> a mention of maximum resolution ?\n>\n\nThis actually confused me! And I'm not even sure the maximum reported\nresolution is actually the \"sensor resolution\" if what you mean is the pixel\narray size here...\n\nI would mention \"max\" \"maximum\" in the method name, or at least in the\nmethod documentation, but it's up to you...\n\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the best sensor format for a desired output\n> > > + * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > + * \\param[in] size The desired size\n> > > + *\n> > > + * Media bus codes are selected from \\a mbusCodes, which lists all acceptable\n> > > + * codes in decreasing order of preference. This method selects the first code\n> > > + * from the list that is supported by the sensor. If none of the desired codes\n> > > + * is supported, it returns an error.\n> > > + *\n> > > + * \\a size indicates the desired size at the output of the sensor. This method\n> > > + * selects the best size supported by the sensor according to the following\n> > > + * criteria.\n> > > + *\n> > > + * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > > + * to up-scale.\n> > > + * - The sensor output size shall match the desired aspect ratio to avoid the\n> > > + * need to crop the field of view.\n> > > + * - The sensor output size shall be as small as possible to lower the required\n> > > + * bandwidth.\n> > > + *\n> > > + * The use of this method is optional, as the above criteria may not match the\n> > > + * needs of all pipeline handlers. Pipeline handlers may implement custom\n> > > + * sensor format selection when needed.\n> > > + *\n> > > + * The returned sensor output format is guaranteed to be acceptable by the\n> > > + * setFormat() method without any modification.\n> > > + *\n> > > + * \\return The best sensor output format matching the desired media bus codes\n> > > + * and size on success, or an empty format otherwise.\n> > > + */\n> > > +V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbusCodes,\n> > > +\t\t\t\t\t const Size &size) const\n> > > +{\n> > > +\tV4L2SubdeviceFormat format{};\n> > > +\n> > > +\tfor (unsigned int code : mbusCodes_) {\n> > > +\t\tif (std::any_of(mbusCodes.begin(), mbusCodes.end(),\n> > > +\t\t\t\t[code](unsigned int c) { return c == code; })) {\n> > > +\t\t\tformat.mbus_code = code;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (!format.mbus_code) {\n> > > +\t\tLOG(CameraSensor, Debug)\n> > > +\t\t\t<< \"No supported format found\" << std::endl;\n> > > +\t\treturn format;\n> > > +\t}\n> > > +\n> > > +\tunsigned int desiredArea = size.width * size.height;\n> >\n> > will we need a Size::area() ?\n>\n> Possibly, but in order to be generic, that would need to return a 64-bit\n> integer. I've limited the computation to 32-bit here on purpose as we\n> won't need more than that for the foreseable future (4GPixels sensors\n> ?).\n>\n> > > +\tunsigned int bestArea = UINT_MAX;\n> > > +\tfloat desiredRatio = static_cast(size.width) / size.height;\n> > > +\tfloat bestRatio = FLT_MAX;\n> > > +\tconst Size *bestSize = nullptr;\n> > > +\n> > > +\tfor (const Size &sz : sizes_) {\n> > > +\t\tif (sz.width < size.width || sz.height < size.height)\n> >\n> > Couldn't you just compare \"sz < size\" ?\n> >\n> > * - A size with smaller width and smaller height is smaller.\n> > * - A size with smaller area is smaller.\n> > * - A size with smaller width is smaller.\n> >\n> > doesn't it apply here?\n>\n> I did to start with, and then realized it wouldn't work. We want to\n> select a size that is larger than the size parameter in both width and\n> height. operator< would accept a size that overlaps with the size\n> parameter (has a larger height but smaller width for instance), which we\n> don't want to accept.\n>\n\nFine, thanks for clarification!\n\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> > > +\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> > > +\t\tunsigned int area = sz.width * sz.height;\n> > > +\t\tunsigned int areaDiff = area - desiredArea;\n> > > +\n> > > +\t\tif (ratioDiff > bestRatio)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> >\n> > This is puzzling, but most probably what you want. If ratio is <= than\n> > the best one, then if ratio or area is < than the current best one\n> > then select the format. So there is no priority between ratio and\n> > area differences, right?\n>\n> There's actually a priority, if the ratio is better but the area isn't,\n> the size is still picked. The area criteria is only taken into account\n> when the ratios are identical.\n>\n\nIndeed. Thanks for the clarification!\n\n> > > +\t\t\tbestRatio = ratioDiff;\n> > > +\t\t\tbestArea = areaDiff;\n> > > +\t\t\tbestSize = &sz;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (!bestSize) {\n> > > +\t\tLOG(CameraSensor, Debug)\n> > > +\t\t\t<< \"No supported size found\" << std::endl;\n> > > +\t\treturn format;\n> > > +\t}\n> > > +\n> > > +\tformat.width = bestSize->width;\n> > > +\tformat.height = bestSize->height;\n> > > +\n> > > +\treturn format;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set the sensor output format\n> > > + * \\param[in] format The desired sensor output format\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > > +{\n> > > +\treturn subdev_->setFormat(0, format);\n> > > +}\n> > > +\n> > > +std::string CameraSensor::logPrefix() const\n> > > +{\n> > > +\treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > new file mode 100644\n> > > index 000000000000..7d92af05248c\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -0,0 +1,56 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_sensor.h - A camera sensor\n> > > + */\n> > > +#ifndef __LIBCAMERA_CAMERA_SENSOR_H__\n> > > +#define __LIBCAMERA_CAMERA_SENSOR_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"v4l2_subdevice.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class MediaEntity;\n> > > +class V4L2SubdeviceFormat;\n> >\n> > As you include \"v4l2_subdevice.h\" you don't need this\n>\n> I actually don't need to include that header, I'll add a forward\n> declaration of V4L2Subdevice instead.\n>\n> > > +\n> > > +class CameraSensor : public Loggable\n> >\n> > nit: other classes that inherits Loggable inherits its protected\n> > fields only. Is this desirable?\n>\n> This isn't about inheriting the protected members only, but about\n> turning all private members of the base class into protected members of\ns/private/public ?\n> the derived class. That is, if Loggable has a public foo() method, and\n> CameraSensor derives from Loggable with the protected qualifier, then\n> the foo() method of a CameraSensor instance would be protected, not\n> public.\n>\n> It makes no difference in this case as the only public method the\n> Loggable function exposes is a virtual destructor, but I'll change it\n> for consistency.\n\nyeah, no differences, but I would change it for consistency.\n\n>\n> > > +{\n> > > +public:\n> > > +\texplicit CameraSensor(const MediaEntity *entity);\n> > > +\t~CameraSensor();\n> > > +\n> > > +\tCameraSensor(const CameraSensor &) = delete;\n> > > +\tCameraSensor &operator=(const CameraSensor &) = delete;\n> > > +\n> > > +\tint init();\n> > > +\n> > > +\tconst MediaEntity *entity() const { return entity_; }\n> > > +\tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > +\tconst std::vector &sizes() const { return sizes_; }\n> > > +\tconst Size &resolution() const { return sizes_.back(); }\n> > > +\n> > > +\tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > +\t\t\t\t const Size &size) const;\n> > > +\tint setFormat(V4L2SubdeviceFormat *format);\n> > > +\n> > > +protected:\n> > > +\tstd::string logPrefix() const;\n> > > +\n> > > +private:\n> > > +\tconst MediaEntity *entity_;\n> > > +\tV4L2Subdevice *subdev_;\n> > > +\n> > > +\tstd::vector mbusCodes_;\n> > > +\tstd::vector sizes_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_CAMERA_SENSOR_H__ */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index cd36ac307518..cf4edec05755 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -2,6 +2,7 @@ libcamera_sources = files([\n> > > 'buffer.cpp',\n> > > 'camera.cpp',\n> > > 'camera_manager.cpp',\n> > > + 'camera_sensor.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'event_dispatcher.cpp',\n> > > 'event_dispatcher_poll.cpp',\n> > > @@ -23,6 +24,7 @@ libcamera_sources = files([\n> > > ])\n> > >\n> > > libcamera_headers = files([\n> > > + 'include/camera_sensor.h',\n> > > 'include/device_enumerator.h',\n> > > 'include/event_dispatcher_poll.h',\n> > > 'include/formats.h',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E6A260DB4\n\tfor ;\n\tWed, 17 Apr 2019 09:41:13 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 835F9240004;\n\tWed, 17 Apr 2019 07:41:12 +0000 (UTC)"],"Date":"Wed, 17 Apr 2019 09:42:04 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190417074204.76f7dxssmbvozh22@uno.localdomain>","References":"<20190415165700.22970-1-laurent.pinchart@ideasonboard.com>\n\t<20190415165700.22970-10-laurent.pinchart@ideasonboard.com>\n\t<20190416162548.rufnwl5uwxc2yiyz@uno.localdomain>\n\t<20190416205242.GD4822@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"pfjhiektevxpwh7s\"","Content-Disposition":"inline","In-Reply-To":"<20190416205242.GD4822@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: camera_sensor: Add a\n\tnew class to model a camera sensor","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":"Wed, 17 Apr 2019 07:41:13 -0000"}}]