[{"id":1709,"web_url":"https://patchwork.libcamera.org/comment/1709/","msgid":"<20190527100546.62q2tgyezflp7bxd@uno.localdomain>","date":"2019-05-27T10:05:46","subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:\n> Add two new objects to hold v4l2 format information for v4l2 devices and\n> subdevices. There is much code which can be shared between the two so\n> have both of them inherit from a base class to reduce code duplication.\n>\n\nI still think we should have gone this way from the very beginning, so\nI think this is the direction to go.\n\nOne question:\n1) looking at the patch, it seems to me now V4L2DeviceFormats and\nV4L2SubdeviceFormats only differns in the name of the formats map key\n(pixelcode and mbus code). I wonder if we shouldn't better unify those\n(maybe renaming DeviceFormats in just ImageFormats).\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++\n>  src/libcamera/include/formats.h |  35 +++++++\n>  2 files changed, 193 insertions(+)\n>\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -24,4 +24,162 @@ namespace libcamera {\n>   * resolutions represented by SizeRange items.\n>   */\n>\n> +\n> +/**\n> + * \\class DeviceFormats\n> + * \\brief Base class for V4L2Device and V4L2SubDevice Formats\n> + *\n> + * The base class holds common functionallity between V4L2DeviceFormats and\n\ns/functionallity/functionalities/\n\n> + * V4L2SubDeviceForamts.\n\ns/Foramats/Formats/ here an in other places\n> + *\n> + * \\sa V4L2DeviceFormats\n> + * \\sa V4L2SubDeviceForamts\n> + */\n> +\n> +/**\n> + * \\brief Create an empty DeviceFormats\n> + */\n> +DeviceFormats::DeviceFormats()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Create a DeviceFormats with data\n\nwith data/using the formats and size provided by \\a data\n\nmissing the data argument documentation.\n\nDocumentation apart, we're now creating a DeviceFormats from an\nalready built map, which require users of this class to build the map\nby themselves\n\nHave you considered the possibility of creating an empty DeviceFormats\nand later populated it with:\n\n        addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)\n\nOtherwise the interface to populate the formats stays as awkward to\nuse as it is today.\n\n> + */\n> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> +\t: data_(data)\n\ns/data/map ?\n\n> +{\n> +}\n> +\n> +DeviceFormats::~DeviceFormats()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve all sizes for a specific format\n> + * \\param[in] format A pixelformat or mbus code\n> + *\n> + * Retrieves all SizeRanges for a specific format. For V4L2Device \\a format is a\n\nWe usually use \"Retrieve\"\n\n> + * pixelformoat while for a V4L2Subdevice \\a format is a media bus code.\n\npixelformoat/pixel format code/\n\nAs the difference is the name of the field only, I would rather unify\nthe two derived classes.\n\n> + *\n> + * \\return List of SizeRanges for \\a format, empty list if format is not valid\n\nThe list of image sizes produced using \\a format, or an empty list if format is not supported.\n\n> + */\n> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const\n> +{\n> +\tauto const &it = data_.find(format);\n> +\tif (it == data_.end())\n> +\t\treturn {};\n> +\n> +\treturn it->second;\n> +}\n> +\n> +/**\n> + * \\brief Check if the device formats is empty\n\nCheck if the list of devices supported formats is empty\n\n> + * \\return True if the formats are empty\n\n\"True if the list of supported formats is empty\"\n\n> + */\n> +bool DeviceFormats::empty() const\n> +{\n> +\treturn data_.empty();\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the raw dataA\n\ns/dataA/data\nand I would\ns/data/the map that associates formats to image sizes/\n\nI would rather not let users access the whole map. Do you have an use\ncase for this?\n\n> + * \\return Raw map containgin formats to SizeRanges\n\nThe map that associates formats to image sizes\n\n> + */\n> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()\n> +{\n> +\treturn data_;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve a list all contained formats\n\nRetrieve a list of all supported image formats\n\n> + *\n> + * This is a helper function intended to be used by V4L2DeviceFormats and\n> + * V4L2SubdeviceFormats.\n> + *\n> + * \\return A list of formats contained\n\n\"of supported formats\"\n\n> + */\n> +std::vector<unsigned int> DeviceFormats::formats() const\n> +{\n> +\tstd::vector<unsigned int> formats;\n> +\n> +\tfor (auto const &it : data_)\n> +\t\tformats.push_back(it.first);\n> +\n> +\treturn formats;\n\nI'm very disappointed std::map does not have a keys() nor a values()\nmethod.\n\n> +}\n> +\n> +/**\n> + * \\var DeviceFormats::data_\n> + * \\brief The map holding format and SizeRange information\n\nAssociating image formats to sizes.\n\n> + */\n> +\n> +/**\n> + * \\class V4L2SubdeviceFormats\n> + * \\brief Holds media bus codes to frame sizes information for a v4l2 subdevice\n> + *\n> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The\n> + * intended user of this object is pipeline handlers which can create it\n> + * from a V4L2Subdevice and use it to describe and validate formats.\n> + */\n> +\n> +/**\n> + * \\brief Create an empty V4L2SubdeviceFormats\n> + */\n> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()\n> +\t: DeviceFormats()\n> +{\n> +}\n\nHow do you fill in an empty V4L2SubdeviceFormats ?\n\n> +\n> +/**\n> + * \\brief Create an V4L2SubdeviceFormats with data\n> + */\n> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> +\t: DeviceFormats(data)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve media bus codes which are described\nwhich are described ... ?\n\nI would just \"Retrieve the list of supported media bus codes\"\n\n> + * \\return List of media bus codes\n> + */\n> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const\n> +{\n> +\treturn formats();\n> +}\n> +\n> +/**\n> + * \\class V4L2DeviceFormats\n> + * \\brief Holds pixelformats to frame sizes information for a v4l2 device\n> + *\n> + * Hold pixelformats and frame sizes which describes a v4l2 device. The\n> + * intended user of this object is pipeline handlers which can create it\n> + * from a V4L2Device and use it to describe and validate formats.\n> + */\n> +\n> +/**\n> + * \\brief Create an empty V4L2DeviceFormats\n> + */\n> +V4L2DeviceFormats::V4L2DeviceFormats()\n> +\t: DeviceFormats()\n> +{\n> +}\n\nSame question\n\n> +\n> +/**\n> + * \\brief Create an V4L2DeviceFormats with data\n> + */\n> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> +\t: DeviceFormats(data)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve pixelformats which are described\n> + * \\return List of pixelformats\n> + */\n> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const\n> +{\n> +\treturn formats();\n> +}\n\nAnd same comment again: do we now need V4L2Device and V4L2Subdevice\nformats? I think it's easier for pipeline handler to deal with formats\nand sizes through a single interface, even if we loose the\npixelcode/media bus code/ distinction, which is by the way only\nspecified at documentation level.\n\nThanks\n  j\n\n\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> index a73772b1eda068b4..372f6e6d71b236dd 100644\n> --- a/src/libcamera/include/formats.h\n> +++ b/src/libcamera/include/formats.h\n> @@ -17,6 +17,41 @@ namespace libcamera {\n>\n>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;\n>\n> +class DeviceFormats\n> +{\n> +public:\n> +\tvirtual ~DeviceFormats();\n> +\n> +\tstd::vector<SizeRange> sizes(unsigned int format) const;\n> +\tbool empty() const;\n> +\tconst std::map<unsigned int, std::vector<SizeRange>> &data();\n> +\n> +protected:\n> +\tDeviceFormats();\n> +\tDeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> +\tstd::vector<unsigned int> formats() const;\n> +\n> +\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> +};\n> +\n> +class V4L2SubdeviceFormats : public DeviceFormats\n> +{\n> +public:\n> +\tV4L2SubdeviceFormats();\n> +\tV4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> +\n> +\tstd::vector<unsigned int> codes() const;\n> +};\n> +\n> +class V4L2DeviceFormats : public DeviceFormats\n> +{\n> +public:\n> +\tV4L2DeviceFormats();\n> +\tV4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> +\n> +\tstd::vector<unsigned int> pixelformats() const;\n> +};\n> +\n>  } /* namespace libcamera */\n>\n>  #endif /* __LIBCAMERA_FORMATS_H__ */\n> --\n> 2.21.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9773960E47\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 May 2019 12:04:38 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 249A41C0006;\n\tMon, 27 May 2019 10:04:37 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 27 May 2019 12:05:46 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190527100546.62q2tgyezflp7bxd@uno.localdomain>","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"aek37qbk6gfelaaj\"","Content-Disposition":"inline","In-Reply-To":"<20190527001543.13593-6-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 27 May 2019 10:04:38 -0000"}},{"id":1725,"web_url":"https://patchwork.libcamera.org/comment/1725/","msgid":"<4371fdc7-7828-2925-af2b-036ac8c1e591@ideasonboard.com>","date":"2019-05-29T21:36:56","subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 27/05/2019 11:05, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:\n>> Add two new objects to hold v4l2 format information for v4l2 devices and\n>> subdevices. There is much code which can be shared between the two so\n>> have both of them inherit from a base class to reduce code duplication.\n>>\n> \n> I still think we should have gone this way from the very beginning, so\n> I think this is the direction to go.\n> \n> One question:\n> 1) looking at the patch, it seems to me now V4L2DeviceFormats and\n> V4L2SubdeviceFormats only differns in the name of the formats map key\n> (pixelcode and mbus code). I wonder if we shouldn't better unify those\n> (maybe renaming DeviceFormats in just ImageFormats).\n> \n\nAlso, I'm a bit weary that we will now have V4L2DeviceFormat and\nV4L2DeviceFormats which are quite different\n\n (xxxxFormats is not a direct array of xxxFormat)\n\nSame for both V4L2DeviceFormats and V4L2SubdeviceFormats.\n\nDo you think this is a problem? Or will this be ok?\n\nOr perhaps it's a moot point, as I think Jacopo's comments look like\nsuggesting unifying this some how so the naming will be less ambiguous\nthen anyway.\n\n--\nKieran\n\n\n>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n>> ---\n>>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++\n>>  src/libcamera/include/formats.h |  35 +++++++\n>>  2 files changed, 193 insertions(+)\n>>\n>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n>> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644\n>> --- a/src/libcamera/formats.cpp\n>> +++ b/src/libcamera/formats.cpp\n>> @@ -24,4 +24,162 @@ namespace libcamera {\n>>   * resolutions represented by SizeRange items.\n>>   */\n>>\n>> +\n>> +/**\n>> + * \\class DeviceFormats\n>> + * \\brief Base class for V4L2Device and V4L2SubDevice Formats\n>> + *\n>> + * The base class holds common functionallity between V4L2DeviceFormats and\n> \n> s/functionallity/functionalities/\n> \n>> + * V4L2SubDeviceForamts.\n> \n> s/Foramats/Formats/ here an in other places\n>> + *\n>> + * \\sa V4L2DeviceFormats\n>> + * \\sa V4L2SubDeviceForamts\n>> + */\n>> +\n>> +/**\n>> + * \\brief Create an empty DeviceFormats\n>> + */\n>> +DeviceFormats::DeviceFormats()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create a DeviceFormats with data\n> \n> with data/using the formats and size provided by \\a data\n> \n> missing the data argument documentation.\n> \n> Documentation apart, we're now creating a DeviceFormats from an\n> already built map, which require users of this class to build the map\n> by themselves\n> \n> Have you considered the possibility of creating an empty DeviceFormats\n> and later populated it with:\n> \n>         addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)\n> \n> Otherwise the interface to populate the formats stays as awkward to\n> use as it is today.\n> \n>> + */\n>> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n>> +\t: data_(data)\n> \n> s/data/map ?\n> \n>> +{\n>> +}\n>> +\n>> +DeviceFormats::~DeviceFormats()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve all sizes for a specific format\n>> + * \\param[in] format A pixelformat or mbus code\n>> + *\n>> + * Retrieves all SizeRanges for a specific format. For V4L2Device \\a format is a\n> \n> We usually use \"Retrieve\"\n> \n>> + * pixelformoat while for a V4L2Subdevice \\a format is a media bus code.\n> \n> pixelformoat/pixel format code/\n> \n> As the difference is the name of the field only, I would rather unify\n> the two derived classes.\n> \n>> + *\n>> + * \\return List of SizeRanges for \\a format, empty list if format is not valid\n> \n> The list of image sizes produced using \\a format, or an empty list if format is not supported.\n> \n>> + */\n>> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const\n>> +{\n>> +\tauto const &it = data_.find(format);\n>> +\tif (it == data_.end())\n>> +\t\treturn {};\n>> +\n>> +\treturn it->second;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Check if the device formats is empty\n> \n> Check if the list of devices supported formats is empty\n> \n>> + * \\return True if the formats are empty\n> \n> \"True if the list of supported formats is empty\"\n> \n>> + */\n>> +bool DeviceFormats::empty() const\n>> +{\n>> +\treturn data_.empty();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the raw dataA\n> \n> s/dataA/data\n> and I would\n> s/data/the map that associates formats to image sizes/\n> \n> I would rather not let users access the whole map. Do you have an use\n> case for this?\n> \n>> + * \\return Raw map containgin formats to SizeRanges\n> \n> The map that associates formats to image sizes\n> \n>> + */\n>> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()\n>> +{\n>> +\treturn data_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve a list all contained formats\n> \n> Retrieve a list of all supported image formats\n> \n>> + *\n>> + * This is a helper function intended to be used by V4L2DeviceFormats and\n>> + * V4L2SubdeviceFormats.\n>> + *\n>> + * \\return A list of formats contained\n> \n> \"of supported formats\"\n> \n>> + */\n>> +std::vector<unsigned int> DeviceFormats::formats() const\n>> +{\n>> +\tstd::vector<unsigned int> formats;\n>> +\n>> +\tfor (auto const &it : data_)\n>> +\t\tformats.push_back(it.first);\n>> +\n>> +\treturn formats;\n> \n> I'm very disappointed std::map does not have a keys() nor a values()\n> method.\n> \n>> +}\n>> +\n>> +/**\n>> + * \\var DeviceFormats::data_\n>> + * \\brief The map holding format and SizeRange information\n> \n> Associating image formats to sizes.\n> \n>> + */\n>> +\n>> +/**\n>> + * \\class V4L2SubdeviceFormats\n>> + * \\brief Holds media bus codes to frame sizes information for a v4l2 subdevice\n>> + *\n>> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The\n>> + * intended user of this object is pipeline handlers which can create it\n>> + * from a V4L2Subdevice and use it to describe and validate formats.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Create an empty V4L2SubdeviceFormats\n>> + */\n>> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()\n>> +\t: DeviceFormats()\n>> +{\n>> +}\n> \n> How do you fill in an empty V4L2SubdeviceFormats ?\n> \n>> +\n>> +/**\n>> + * \\brief Create an V4L2SubdeviceFormats with data\n>> + */\n>> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n>> +\t: DeviceFormats(data)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve media bus codes which are described\n> which are described ... ?\n> \n> I would just \"Retrieve the list of supported media bus codes\"\n> \n>> + * \\return List of media bus codes\n>> + */\n>> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const\n>> +{\n>> +\treturn formats();\n>> +}\n>> +\n>> +/**\n>> + * \\class V4L2DeviceFormats\n>> + * \\brief Holds pixelformats to frame sizes information for a v4l2 device\n>> + *\n>> + * Hold pixelformats and frame sizes which describes a v4l2 device. The\n>> + * intended user of this object is pipeline handlers which can create it\n>> + * from a V4L2Device and use it to describe and validate formats.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Create an empty V4L2DeviceFormats\n>> + */\n>> +V4L2DeviceFormats::V4L2DeviceFormats()\n>> +\t: DeviceFormats()\n>> +{\n>> +}\n> \n> Same question\n> \n>> +\n>> +/**\n>> + * \\brief Create an V4L2DeviceFormats with data\n>> + */\n>> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n>> +\t: DeviceFormats(data)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve pixelformats which are described\n>> + * \\return List of pixelformats\n>> + */\n>> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const\n>> +{\n>> +\treturn formats();\n>> +}\n> \n> And same comment again: do we now need V4L2Device and V4L2Subdevice\n> formats? I think it's easier for pipeline handler to deal with formats\n> and sizes through a single interface, even if we loose the\n> pixelcode/media bus code/ distinction, which is by the way only\n> specified at documentation level.\n> \n> Thanks\n>   j\n> \n> \n>> +\n>>  } /* namespace libcamera */\n>> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n>> index a73772b1eda068b4..372f6e6d71b236dd 100644\n>> --- a/src/libcamera/include/formats.h\n>> +++ b/src/libcamera/include/formats.h\n>> @@ -17,6 +17,41 @@ namespace libcamera {\n>>\n>>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;\n>>\n>> +class DeviceFormats\n>> +{\n>> +public:\n>> +\tvirtual ~DeviceFormats();\n>> +\n>> +\tstd::vector<SizeRange> sizes(unsigned int format) const;\n>> +\tbool empty() const;\n>> +\tconst std::map<unsigned int, std::vector<SizeRange>> &data();\n>> +\n>> +protected:\n>> +\tDeviceFormats();\n>> +\tDeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n>> +\tstd::vector<unsigned int> formats() const;\n>> +\n>> +\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n>> +};\n>> +\n>> +class V4L2SubdeviceFormats : public DeviceFormats\n>> +{\n>> +public:\n>> +\tV4L2SubdeviceFormats();\n>> +\tV4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n>> +\n>> +\tstd::vector<unsigned int> codes() const;\n>> +};\n>> +\n>> +class V4L2DeviceFormats : public DeviceFormats\n>> +{\n>> +public:\n>> +\tV4L2DeviceFormats();\n>> +\tV4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n>> +\n>> +\tstd::vector<unsigned int> pixelformats() const;\n>> +};\n>> +\n>>  } /* namespace libcamera */\n>>\n>>  #endif /* __LIBCAMERA_FORMATS_H__ */\n>> --\n>> 2.21.0\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97D9A60E46\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 May 2019 23:36:59 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1987524;\n\tWed, 29 May 2019 23:36:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559165819;\n\tbh=IpXYdW/UH9snueoG9Cfly5YB45ZBvLJ0j0qF7hO6LC4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=lqcR9W0sChCZBGiqx+xIRB8D6Mh8LRuNQHrQ9xtcuyidx4iDknEVWqjMqOS6fbywX\n\tLZbQFhuJQRknQT0YUlORZ18kADao+afZ63AyJyn9PQ99LDjT/oHigxegVLkcmA3bGP\n\t5A58jwclevnXrl1LPF9XkySU1GJlrFzRH78fBhVg=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, =?utf-8?q?Niklas_S=C3=B6derlund?=\n\t<niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-6-niklas.soderlund@ragnatech.se>\n\t<20190527100546.62q2tgyezflp7bxd@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<4371fdc7-7828-2925-af2b-036ac8c1e591@ideasonboard.com>","Date":"Wed, 29 May 2019 22:36:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.6.1","MIME-Version":"1.0","In-Reply-To":"<20190527100546.62q2tgyezflp7bxd@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 29 May 2019 21:36:59 -0000"}},{"id":1802,"web_url":"https://patchwork.libcamera.org/comment/1802/","msgid":"<20190609120537.GD4778@pendragon.ideasonboard.com>","date":"2019-06-09T12:05:37","subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, May 29, 2019 at 10:36:56PM +0100, Kieran Bingham wrote:\n> On 27/05/2019 11:05, Jacopo Mondi wrote:\n> > On Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:\n> >> Add two new objects to hold v4l2 format information for v4l2 devices and\n> >> subdevices. There is much code which can be shared between the two so\n> >> have both of them inherit from a base class to reduce code duplication.\n> > \n> > I still think we should have gone this way from the very beginning, so\n> > I think this is the direction to go.\n> > \n> > One question:\n> > 1) looking at the patch, it seems to me now V4L2DeviceFormats and\n> > V4L2SubdeviceFormats only differns in the name of the formats map key\n> > (pixelcode and mbus code). I wonder if we shouldn't better unify those\n> > (maybe renaming DeviceFormats in just ImageFormats).\n> \n> Also, I'm a bit weary that we will now have V4L2DeviceFormat and\n> V4L2DeviceFormats which are quite different\n> \n>  (xxxxFormats is not a direct array of xxxFormat)\n> \n> Same for both V4L2DeviceFormats and V4L2SubdeviceFormats.\n> \n> Do you think this is a problem? Or will this be ok?\n> \n> Or perhaps it's a moot point, as I think Jacopo's comments look like\n> suggesting unifying this some how so the naming will be less ambiguous\n> then anyway.\n\nI also agree that, given how similar the two classes are, unifying them\nwould be a good idea.\n\n> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> >> ---\n> >>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++\n> >>  src/libcamera/include/formats.h |  35 +++++++\n> >>  2 files changed, 193 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> >> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644\n> >> --- a/src/libcamera/formats.cpp\n> >> +++ b/src/libcamera/formats.cpp\n> >> @@ -24,4 +24,162 @@ namespace libcamera {\n> >>   * resolutions represented by SizeRange items.\n> >>   */\n> >>\n> >> +\n\nExtra blank line ?\n\n> >> +/**\n> >> + * \\class DeviceFormats\n> >> + * \\brief Base class for V4L2Device and V4L2SubDevice Formats\n> >> + *\n> >> + * The base class holds common functionallity between V4L2DeviceFormats and\n> > \n> > s/functionallity/functionalities/\n> > \n> >> + * V4L2SubDeviceForamts.\n> > \n> > s/Foramats/Formats/ here an in other places\n> >> + *\n> >> + * \\sa V4L2DeviceFormats\n> >> + * \\sa V4L2SubDeviceForamts\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create an empty DeviceFormats\n> >> + */\n> >> +DeviceFormats::DeviceFormats()\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Create a DeviceFormats with data\n> > \n> > with data/using the formats and size provided by \\a data\n> > \n> > missing the data argument documentation.\n> > \n> > Documentation apart, we're now creating a DeviceFormats from an\n> > already built map, which require users of this class to build the map\n> > by themselves\n> > \n> > Have you considered the possibility of creating an empty DeviceFormats\n> > and later populated it with:\n> > \n> >         addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)\n> > \n> > Otherwise the interface to populate the formats stays as awkward to\n> > use as it is today.\n\nI have yet to review the patches that make use of this class, but I\nagree that this constructor is a bit awkward to use.\n\n> >> + */\n> >> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> >> +\t: data_(data)\n> > \n> > s/data/map ?\n> > \n> >> +{\n> >> +}\n> >> +\n> >> +DeviceFormats::~DeviceFormats()\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve all sizes for a specific format\n> >> + * \\param[in] format A pixelformat or mbus code\n> >> + *\n> >> + * Retrieves all SizeRanges for a specific format. For V4L2Device \\a format is a\n> > \n> > We usually use \"Retrieve\"\n> > \n> >> + * pixelformoat while for a V4L2Subdevice \\a format is a media bus code.\n> > \n> > pixelformoat/pixel format code/\n> > \n> > As the difference is the name of the field only, I would rather unify\n> > the two derived classes.\n> > \n> >> + *\n> >> + * \\return List of SizeRanges for \\a format, empty list if format is not valid\n> > \n> > The list of image sizes produced using \\a format, or an empty list if format is not supported.\n> > \n> >> + */\n> >> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const\n> >> +{\n> >> +\tauto const &it = data_.find(format);\n> >> +\tif (it == data_.end())\n> >> +\t\treturn {};\n> >> +\n> >> +\treturn it->second;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Check if the device formats is empty\n> > \n> > Check if the list of devices supported formats is empty\n> > \n> >> + * \\return True if the formats are empty\n> > \n> > \"True if the list of supported formats is empty\"\n> > \n> >> + */\n> >> +bool DeviceFormats::empty() const\n> >> +{\n> >> +\treturn data_.empty();\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve the raw dataA\n> > \n> > s/dataA/data\n> > and I would\n> > s/data/the map that associates formats to image sizes/\n> > \n> > I would rather not let users access the whole map. Do you have an use\n> > case for this?\n\nWhy would you prefer not exposing it ?\n\nAn alternative could also be to expose custom iterators on the\nDeviceFormats class.\n\n> >> + * \\return Raw map containgin formats to SizeRanges\n> > \n> > The map that associates formats to image sizes\n> > \n> >> + */\n> >> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()\n> >> +{\n> >> +\treturn data_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve a list all contained formats\n> > \n> > Retrieve a list of all supported image formats\n> > \n> >> + *\n> >> + * This is a helper function intended to be used by V4L2DeviceFormats and\n> >> + * V4L2SubdeviceFormats.\n> >> + *\n> >> + * \\return A list of formats contained\n> > \n> > \"of supported formats\"\n> > \n> >> + */\n> >> +std::vector<unsigned int> DeviceFormats::formats() const\n> >> +{\n> >> +\tstd::vector<unsigned int> formats;\n\nYou should call formats.reserve(data_.size()); to optimise memory\nallocation.\n\n> >> +\n> >> +\tfor (auto const &it : data_)\n> >> +\t\tformats.push_back(it.first);\n> >> +\n> >> +\treturn formats;\n> > \n> > I'm very disappointed std::map does not have a keys() nor a values()\n> > method.\n\nAnother option is\n\n\tstd::transform(data_.begin(), data_.end(), std::back_inserter(formats),\n\t\t       [](decltype(data_)::value_type const &pair) {\n\t\t\t       return pair.first;\n\t\t       });\n\nI'm not sure which one is more efficient.\n\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\var DeviceFormats::data_\n> >> + * \\brief The map holding format and SizeRange information\n> > \n> > Associating image formats to sizes.\n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class V4L2SubdeviceFormats\n> >> + * \\brief Holds media bus codes to frame sizes information for a v4l2 subdevice\n> >> + *\n> >> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The\n> >> + * intended user of this object is pipeline handlers which can create it\n> >> + * from a V4L2Subdevice and use it to describe and validate formats.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create an empty V4L2SubdeviceFormats\n> >> + */\n> >> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()\n> >> +\t: DeviceFormats()\n> >> +{\n> >> +}\n> > \n> > How do you fill in an empty V4L2SubdeviceFormats ?\n> > \n> >> +\n> >> +/**\n> >> + * \\brief Create an V4L2SubdeviceFormats with data\n> >> + */\n> >> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> >> +\t: DeviceFormats(data)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve media bus codes which are described\n> > which are described ... ?\n> > \n> > I would just \"Retrieve the list of supported media bus codes\"\n> > \n> >> + * \\return List of media bus codes\n> >> + */\n> >> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const\n> >> +{\n> >> +\treturn formats();\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class V4L2DeviceFormats\n> >> + * \\brief Holds pixelformats to frame sizes information for a v4l2 device\n> >> + *\n> >> + * Hold pixelformats and frame sizes which describes a v4l2 device. The\n> >> + * intended user of this object is pipeline handlers which can create it\n> >> + * from a V4L2Device and use it to describe and validate formats.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create an empty V4L2DeviceFormats\n> >> + */\n> >> +V4L2DeviceFormats::V4L2DeviceFormats()\n> >> +\t: DeviceFormats()\n> >> +{\n> >> +}\n> > \n> > Same question\n> > \n> >> +\n> >> +/**\n> >> + * \\brief Create an V4L2DeviceFormats with data\n> >> + */\n> >> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)\n> >> +\t: DeviceFormats(data)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve pixelformats which are described\n> >> + * \\return List of pixelformats\n> >> + */\n> >> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const\n> >> +{\n> >> +\treturn formats();\n> >> +}\n> > \n> > And same comment again: do we now need V4L2Device and V4L2Subdevice\n> > formats? I think it's easier for pipeline handler to deal with formats\n> > and sizes through a single interface, even if we loose the\n> > pixelcode/media bus code/ distinction, which is by the way only\n> > specified at documentation level.\n> > \n> >> +\n> >>  } /* namespace libcamera */\n> >> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> >> index a73772b1eda068b4..372f6e6d71b236dd 100644\n> >> --- a/src/libcamera/include/formats.h\n> >> +++ b/src/libcamera/include/formats.h\n> >> @@ -17,6 +17,41 @@ namespace libcamera {\n> >>\n> >>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;\n> >>\n> >> +class DeviceFormats\n> >> +{\n> >> +public:\n> >> +\tvirtual ~DeviceFormats();\n> >> +\n> >> +\tstd::vector<SizeRange> sizes(unsigned int format) const;\n\nShould this return a const reference instead of a copy ?\n\n> >> +\tbool empty() const;\n\nisEmpty() ?\n\n> >> +\tconst std::map<unsigned int, std::vector<SizeRange>> &data();\n\nI think you can inline this function.\n\n> >> +\n> >> +protected:\n> >> +\tDeviceFormats();\n> >> +\tDeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> >> +\tstd::vector<unsigned int> formats() const;\n> >> +\n> >> +\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> >> +};\n> >> +\n> >> +class V4L2SubdeviceFormats : public DeviceFormats\n> >> +{\n> >> +public:\n> >> +\tV4L2SubdeviceFormats();\n> >> +\tV4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> >> +\n> >> +\tstd::vector<unsigned int> codes() const;\n> >> +};\n> >> +\n> >> +class V4L2DeviceFormats : public DeviceFormats\n> >> +{\n> >> +public:\n> >> +\tV4L2DeviceFormats();\n> >> +\tV4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);\n> >> +\n> >> +\tstd::vector<unsigned int> pixelformats() const;\n> >> +};\n> >> +\n> >>  } /* namespace libcamera */\n> >>\n> >>  #endif /* __LIBCAMERA_FORMATS_H__ */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10AE861F66\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  9 Jun 2019 14:05:52 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86A015D;\n\tSun,  9 Jun 2019 14:05:51 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560081951;\n\tbh=Q+9B9lx6wTFMePINjQzcN13oSsBbUSvdvPY/M/yRaKs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KoCJMAfd5rEh+4uZd98r0RyAU/eaS/Mg4w0CowOrSfHTax/Es4dMmCWn04bOA53gs\n\tTgtAKp0VrWnkoj8L9WrhIrrdjKblyYBP4oEP7+eAeh6/oAmWHIleGfRo6mJ/rbu43v\n\tYTXRidNajHRwzNwXhoMXyXFjHcHQzHgsafAbj/Fg=","Date":"Sun, 9 Jun 2019 15:05:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Jacopo Mondi <jacopo@jmondi.org>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190609120537.GD4778@pendragon.ideasonboard.com>","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-6-niklas.soderlund@ragnatech.se>\n\t<20190527100546.62q2tgyezflp7bxd@uno.localdomain>\n\t<4371fdc7-7828-2925-af2b-036ac8c1e591@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4371fdc7-7828-2925-af2b-036ac8c1e591@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/17] libcamera: formats: Add\n\tV4L2DeviceFormats and V4L2DeviceFormats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 09 Jun 2019 12:05:52 -0000"}}]