[{"id":5064,"web_url":"https://patchwork.libcamera.org/comment/5064/","msgid":"<20200605193643.GM5864@oden.dyn.berto.se>","date":"2020-06-05T19:36:43","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-05-29 13:03:31 +0200, Jacopo Mondi wrote:\n> The ImageFormats class was originally designed to be used by both\n> V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their\n> image formats using V4L2PixelFormat instances, the ImageFormats class\n> cannot be used there anymore and it has been replaced by raw maps.\n> \n> Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice\n> class and its users by making ImageFormats a templated class that\n> indexes the image sizes using on keys of variadic type.\n\nThis is similar to '[RFC 2/6] libcamera: formats: Turn ImageFormats into \na template' which was dropped in favor of trying to refactor away \nImageFormats going forward.\n\nI'm not against backtracking on that decision as it's been a few months \nsince then. But I think we all need to agree what option is the best way \nforward.\n\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/libcamera/internal/camera_sensor.h  |   2 +-\n>  include/libcamera/internal/formats.h        |  64 ++++++++++-\n>  include/libcamera/internal/v4l2_subdevice.h |   2 +-\n>  src/libcamera/formats.cpp                   | 117 +++++++++++++-------\n>  src/libcamera/v4l2_subdevice.cpp            |   6 +-\n>  test/v4l2_subdevice/list_formats.cpp        |   2 +-\n>  6 files changed, 138 insertions(+), 55 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index d79bd9ce9d58..5c1d5789fe79 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -75,7 +75,7 @@ private:\n>  \n>  \tstd::string model_;\n>  \n> -\tImageFormats formats_;\n> +\tImageFormats<uint32_t> formats_;\n>  \tSize resolution_;\n>  \tstd::vector<unsigned int> mbusCodes_;\n>  \tstd::vector<Size> sizes_;\n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 4092a93ef973..e20c031e857f 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -18,18 +18,70 @@\n>  \n>  namespace libcamera {\n>  \n> +template<typename T>\n>  class ImageFormats\n>  {\n>  public:\n> -\tint addFormat(unsigned int format, const std::vector<SizeRange> &sizes);\n> +\tusing iterator = typename std::map<T, std::vector<SizeRange>>::iterator;\n> +\tusing const_iterator = typename std::map<T, std::vector<SizeRange>>::const_iterator;\n> +\tusing value_type = typename std::map<T, std::vector<SizeRange>>::value_type;\n>  \n> -\tbool isEmpty() const;\n> -\tstd::vector<unsigned int> formats() const;\n> -\tconst std::vector<SizeRange> &sizes(unsigned int format) const;\n> -\tconst std::map<unsigned int, std::vector<SizeRange>> &data() const;\n> +\titerator begin() { return data_.begin(); }\n> +\tconst_iterator begin() const { return data_.begin(); }\n> +\titerator end() { return data_.end(); }\n> +\tconst_iterator end() const { return data_.end(); }\n> +\n> +\titerator find(const T format) { return data_.find(format); }\n> +\tconst iterator find(T format) const { return data_.find(format); }\n> +\n> +\ttemplate<class... Args>\n> +\tstd::pair<iterator, bool> emplace(Args&&... args)\n> +\t{\n> +\t\treturn data_.emplace(args...);\n> +\t}\n> +\n> +\tint addFormat(T format, const std::vector<SizeRange> &sizes)\n> +\t{\n> +\t\tif (data_.find(format) != data_.end())\n> +\t\t\treturn -EEXIST;\n> +\n> +\t\tdata_[format] = sizes;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tbool isEmpty() const { return data_.empty(); }\n> +\n> +\tstd::vector<T> formats() const\n> +\t{\n> +\t\tstd::vector<T> formats;\n> +\t\tformats.reserve(data_.size());\n> +\n> +\t\t/* \\todo: Should this be cached instead of computed each time? */\n> +\t\tfor (auto const &it : data_)\n> +\t\t\tformats.push_back(it.first);\n> +\n> +\t\treturn formats;\n> +\t}\n> +\n> +\tconst std::vector<SizeRange> &sizes(T format) const\n> +\t{\n> +\t\tstatic const std::vector<SizeRange> empty;\n> +\n> +\t\tauto const &it = data_.find(format);\n> +\t\tif (it == data_.end())\n> +\t\t\treturn empty;\n> +\n> +\t\treturn it->second;\n> +\t}\n> +\n> +\tconst std::map<T, std::vector<SizeRange>> &data() const\n> +\t{\n> +\t\treturn data_;\n> +\t}\n>  \n>  private:\n> -\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> +\tstd::map<T, std::vector<SizeRange>> data_;\n>  };\n>  \n>  class PixelFormatInfo\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 1be454f0ddda..0ce6da48f58a 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -51,7 +51,7 @@ public:\n>  \tint setSelection(unsigned int pad, unsigned int target,\n>  \t\t\t Rectangle *rect);\n>  \n> -\tImageFormats formats(unsigned int pad);\n> +\tImageFormats<uint32_t> formats(unsigned int pad);\n>  \n>  \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>  \t\t      Whence whence = ActiveFormat);\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 2ac3b412ecdb..a7922077d9c5 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -22,20 +22,85 @@ LOG_DEFINE_CATEGORY(Formats)\n>  \n>  /**\n>   * \\class ImageFormats\n> - * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> + * \\brief Describe V4L2Device and V4L2SubDevice image formats and associated\n> + * image resolutions\n>   *\n> - * This class stores a list of image formats, each associated with a\n> + * This class stores a list of image formats, each of them associated with a\n>   * corresponding set of image sizes. It is used to describe the formats and\n>   * sizes supported by a V4L2Device or V4L2Subdevice.\n>   *\n> - * Formats are stored as an integer. When used for a V4L2Device, the image\n> - * formats are fourcc pixel formats. When used for a V4L2Subdevice they are\n> - * media bus codes. Both are defined by the V4L2 specification.\n> + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.\n> + * When used for a V4L2Subdevice formats are integer media bus codes.\n>   *\n>   * Sizes are stored as a list of SizeRange.\n>   */\n>  \n>  /**\n> + * \\typedef ImageFormats::iterator\n> + * \\brief Iterator for the formats map\n> + */\n> +\n> +/**\n> + * \\typedef ImageFormats::const_iterator\n> + * \\brief Const iterator for the formats map\n> + */\n> +\n> +/**\n> + * \\typedef ImageFormats::value_type\n> + * \\brief Value type of the entries in the formats map\n> + */\n> +\n> +/**\n> + * \\fn iterator ImageFormats<T>::begin()\n> + * \\brief Retrieve an iterator to the first element in the formats map\n> + * \\return An iterator to the first format map\n> + */\n> +\n> +/**\n> + * \\fn const_iterator ImageFormats<T>::begin() const\n> + * \\brief Retrieve an const iterator to the first element in the formats map\n> + * \\return A const iterator to the first format map\n> + */\n> +\n> +/**\n> + * \\fn iterator ImageFormats<T>::end()\n> + * \\brief Retrieve an iterator pointing to the past-the-end element in the\n> + * formats map\n> + * \\return An iterator to the element following the last format\n> + */\n> +\n> +/**\n> + * \\fn const_iterator ImageFormats<T>::end() const\n> + * \\brief Retrieve a const iterator pointing to the past-the-end element in the\n> + * formats map\n> + * \\return A const iterator to the element following the last format\n> + */\n> +\n> +/**\n> + * \\fn iterator ImageFormats::find(const T format)\n> + * \\brief Find an element with key equal to \\a format\n> + * \\param[in] format The format to search for\n> + * \\return An iterator to the vector of sizes associated with \\a format\n> + */\n> +\n> +/**\n> + * \\fn const_iterator ImageFormats::find(const T format) const\n> + * \\brief Find a const element with key equal to \\a format\n> + * \\param[in] format The format to search for\n> + * \\return An const iterator to the vector of sizes associated with \\a format\n> + */\n> +\n> +/**\n> + * \\fn std::pair<iterator, bool> ImageFormats::emplace(Args&&... args)\n> + * \\brief Insert a new element in the formats map constructed in place with the\n> + * given \\a args\n> + * \\param[in] args The argument pack used to construct the new entry in place\n> + * \\return A pair consisting of an iterator to the inserted element, and a bool\n> + * denoting whether the insertion took place\n> + */\n> +\n> +/**\n> + * \\fn ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)\n>   * \\brief Add a format and corresponding sizes to the description\n>   * \\param[in] format Pixel format or media bus code to describe\n>   * \\param[in] sizes List of supported size ranges for the format\n> @@ -43,42 +108,21 @@ LOG_DEFINE_CATEGORY(Formats)\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -EEXIST The format is already described\n>   */\n> -int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)\n> -{\n> -\tif (data_.find(format) != data_.end())\n> -\t\treturn -EEXIST;\n> -\n> -\tdata_[format] = sizes;\n> -\n> -\treturn 0;\n> -}\n>  \n>  /**\n> + * \\fn ImageFormats<T>::isEmpty() const\n>   * \\brief Check if the list of devices supported formats is empty\n>   * \\return True if the list of supported formats is empty\n>   */\n> -bool ImageFormats::isEmpty() const\n> -{\n> -\treturn data_.empty();\n> -}\n>  \n>  /**\n> + * \\fn ImageFormats<T>::formats() const\n>   * \\brief Retrieve a list of all supported image formats\n>   * \\return List of pixel formats or media bus codes\n>   */\n> -std::vector<unsigned int> ImageFormats::formats() const\n> -{\n> -\tstd::vector<unsigned int> formats;\n> -\tformats.reserve(data_.size());\n> -\n> -\t/* \\todo: Should this be cached instead of computed each time? */\n> -\tfor (auto const &it : data_)\n> -\t\tformats.push_back(it.first);\n> -\n> -\treturn formats;\n> -}\n>  \n>  /**\n> + * \\fn ImageFormats<T>::sizes(T format) const\n>   * \\brief Retrieve all sizes for a specific format\n>   * \\param[in] format The pixel format or mbus code\n>   *\n> @@ -88,25 +132,12 @@ std::vector<unsigned int> ImageFormats::formats() const\n>   * \\return The list of image sizes supported for \\a format, or an empty list if\n>   * the format is not supported\n>   */\n> -const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) const\n> -{\n> -\tstatic const std::vector<SizeRange> empty;\n> -\n> -\tauto const &it = data_.find(format);\n> -\tif (it == data_.end())\n> -\t\treturn empty;\n> -\n> -\treturn it->second;\n> -}\n>  \n>  /**\n> + * \\fn ImageFormats<T>::data() const\n>   * \\brief Retrieve the map that associates formats to image sizes\n>   * \\return The map that associates formats to image sizes\n>   */\n> -const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const\n> -{\n> -\treturn data_;\n> -}\n>  \n>  /**\n>   * \\class PixelFormatInfo\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 7aefc1be032d..9fa20e84a904 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n>   *\n>   * \\return A list of the supported device formats\n>   */\n> -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> +ImageFormats<uint32_t> V4L2Subdevice::formats(unsigned int pad)\n>  {\n> -\tImageFormats formats;\n> +\tImageFormats<uint32_t> formats;\n>  \n>  \tif (pad >= entity_->pads().size()) {\n>  \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n>  \t\treturn {};\n>  \t}\n>  \n> -\tfor (unsigned int code : enumPadCodes(pad)) {\n> +\tfor (uint32_t code : enumPadCodes(pad)) {\n>  \t\tstd::vector<SizeRange> sizes = enumPadSizes(pad, code);\n>  \t\tif (sizes.empty())\n>  \t\t\treturn {};\n> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> index 25503c3334e5..f66bb633fb00 100644\n> --- a/test/v4l2_subdevice/list_formats.cpp\n> +++ b/test/v4l2_subdevice/list_formats.cpp\n> @@ -48,7 +48,7 @@ void ListFormatsTest::printFormats(unsigned int pad,\n>  int ListFormatsTest::run()\n>  {\n>  \t/* List all formats available on existing \"Scaler\" pads. */\n> -\tImageFormats formats;\n> +\tImageFormats<uint32_t> formats;\n>  \n>  \tformats = scaler_->formats(0);\n>  \tif (formats.isEmpty()) {\n> -- \n> 2.26.2\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADD14603CA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  5 Jun 2020 21:36:45 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id w15so6454436lfe.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 05 Jun 2020 12:36:45 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf9sm1368608ljf.99.2020.06.05.12.36.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 05 Jun 2020 12:36:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"dXRY1gE+\"; \n\tdkim-atps=neutral","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\tbh=mO5DEoRMLG9tJiy7wpW5gnrD8ogbjO+E8RVN1rlPPgw=;\n\tb=dXRY1gE+w/uFeWBYiN0MLFeDNq4yXISrhZZXP9kkH+dQQg9OICEsjd0Emqnf1NM31D\n\tHQRgfrTheu7Gj4goAHlALdQ1bkB2BTXsGdAluenH6uJ5dbnoem834gMTM9kqZ17HM4/b\n\trqB0LpwHdhxIRrPlXDTi6gJWM9M8CRQD7E6NdZ4sI1ruJeakv7gRRR1so9wLoDFh4Ua+\n\tGyMRsO1p1XJqZ6XZbUv17R8qg+5pOtJTZ7Ebgcih9NmjCCiwJ+ArVqReShW1NHhHxRv4\n\tMepXztiCB7gU2APRXOy5ZITuHd7JGu9SjvDksimQapLcOalrkWagPun6JGUk9IKrlWnO\n\tEvJA==","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;\n\tbh=mO5DEoRMLG9tJiy7wpW5gnrD8ogbjO+E8RVN1rlPPgw=;\n\tb=lEkgcVhi7gEibKwqdEvC6GmlbgE+6EqEhXnVP6LB/Ya/wpEscshjt9ItdbPehA7JB+\n\tq86+pz9qQwvbQfKnojzNYh+FniyesHnV7/r1w/Z1BNzdrTkMj6sIUZMdjK/AF7oWfarI\n\t/gf/4ah8a/HbGjSxAmfwBY09ppjCHZQcR/E66VQmQgcLpNp2TxuEDmHwrajJweqS4UJZ\n\tyBTq2dtRyYt45OuWkPtxamC3xaYH5iG7bzDaw6tOH4kJT0HKZNTQkMjLNLfcSDP375He\n\tcKeOKCqiaxwZnQP05b1NsbrFPa+N01lgLYT3yRMHOwDSUFcWcJw5mwPOOK7CuPuJWrkZ\n\twMYw==","X-Gm-Message-State":"AOAM530jIXaP5YqpYBpWbDBa/3z+UTb1JpToVBdJuc1vXoc0SnlYevB6\n\tLmvE5KOxqaKMBKdpFkU6UfuQxGWPLJY/5Q==","X-Google-Smtp-Source":"ABdhPJxhhOQugummtFwqUQgr4tib94JKVb4PSEHYhRZGcgRnPJ7inTsHXWxrgvDMDQL+4nPt99pveg==","X-Received":"by 2002:a05:6512:62:: with SMTP id\n\ti2mr6067043lfo.152.1591385804924; \n\tFri, 05 Jun 2020 12:36:44 -0700 (PDT)","Date":"Fri, 5 Jun 2020 21:36:43 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200605193643.GM5864@oden.dyn.berto.se>","References":"<20200529110335.620503-1-jacopo@jmondi.org>\n\t<20200529110335.620503-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200529110335.620503-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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":"Fri, 05 Jun 2020 19:36:45 -0000"}},{"id":5076,"web_url":"https://patchwork.libcamera.org/comment/5076/","msgid":"<20200605231840.GP26752@pendragon.ideasonboard.com>","date":"2020-06-05T23:18:40","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Niklas,\n\nOn Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote:\n> On 2020-05-29 13:03:31 +0200, Jacopo Mondi wrote:\n> > The ImageFormats class was originally designed to be used by both\n> > V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their\n> > image formats using V4L2PixelFormat instances, the ImageFormats class\n> > cannot be used there anymore and it has been replaced by raw maps.\n> > \n> > Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice\n> > class and its users by making ImageFormats a templated class that\n> > indexes the image sizes using on keys of variadic type.\n> \n> This is similar to '[RFC 2/6] libcamera: formats: Turn ImageFormats into \n> a template' which was dropped in favor of trying to refactor away \n> ImageFormats going forward.\n> \n> I'm not against backtracking on that decision as it's been a few months \n> since then. But I think we all need to agree what option is the best way \n> forward.\n\nI agree with you. I still believe we'll be able to find a better\nabstraction in the long term. If there's value in refactoring, given how\nlong it's taking us to come up with a better solution, I think that's a\ngood intermediate step. Sorry for rejecting it initially, I thought we\ncould converge on a better solution faster.\n\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  include/libcamera/internal/camera_sensor.h  |   2 +-\n> >  include/libcamera/internal/formats.h        |  64 ++++++++++-\n> >  include/libcamera/internal/v4l2_subdevice.h |   2 +-\n> >  src/libcamera/formats.cpp                   | 117 +++++++++++++-------\n> >  src/libcamera/v4l2_subdevice.cpp            |   6 +-\n> >  test/v4l2_subdevice/list_formats.cpp        |   2 +-\n> >  6 files changed, 138 insertions(+), 55 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index d79bd9ce9d58..5c1d5789fe79 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -75,7 +75,7 @@ private:\n> >  \n> >  \tstd::string model_;\n> >  \n> > -\tImageFormats formats_;\n> > +\tImageFormats<uint32_t> formats_;\n> >  \tSize resolution_;\n> >  \tstd::vector<unsigned int> mbusCodes_;\n> >  \tstd::vector<Size> sizes_;\n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 4092a93ef973..e20c031e857f 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -18,18 +18,70 @@\n> >  \n> >  namespace libcamera {\n> >  \n> > +template<typename T>\n> >  class ImageFormats\n> >  {\n> >  public:\n> > -\tint addFormat(unsigned int format, const std::vector<SizeRange> &sizes);\n> > +\tusing iterator = typename std::map<T, std::vector<SizeRange>>::iterator;\n> > +\tusing const_iterator = typename std::map<T, std::vector<SizeRange>>::const_iterator;\n> > +\tusing value_type = typename std::map<T, std::vector<SizeRange>>::value_type;\n> >  \n> > -\tbool isEmpty() const;\n> > -\tstd::vector<unsigned int> formats() const;\n> > -\tconst std::vector<SizeRange> &sizes(unsigned int format) const;\n> > -\tconst std::map<unsigned int, std::vector<SizeRange>> &data() const;\n> > +\titerator begin() { return data_.begin(); }\n> > +\tconst_iterator begin() const { return data_.begin(); }\n> > +\titerator end() { return data_.end(); }\n> > +\tconst_iterator end() const { return data_.end(); }\n> > +\n> > +\titerator find(const T format) { return data_.find(format); }\n> > +\tconst iterator find(T format) const { return data_.find(format); }\n\nI'd split this to a separate patch. How much of the std::map API do we\nwant/need to expose ?\n\n> > +\n> > +\ttemplate<class... Args>\n> > +\tstd::pair<iterator, bool> emplace(Args&&... args)\n> > +\t{\n> > +\t\treturn data_.emplace(args...);\n> > +\t}\n> > +\n> > +\tint addFormat(T format, const std::vector<SizeRange> &sizes)\n> > +\t{\n> > +\t\tif (data_.find(format) != data_.end())\n> > +\t\t\treturn -EEXIST;\n> > +\n> > +\t\tdata_[format] = sizes;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tbool isEmpty() const { return data_.empty(); }\n> > +\n> > +\tstd::vector<T> formats() const\n> > +\t{\n> > +\t\tstd::vector<T> formats;\n> > +\t\tformats.reserve(data_.size());\n> > +\n> > +\t\t/* \\todo: Should this be cached instead of computed each time? */\n> > +\t\tfor (auto const &it : data_)\n> > +\t\t\tformats.push_back(it.first);\n> > +\n> > +\t\treturn formats;\n> > +\t}\n> > +\n> > +\tconst std::vector<SizeRange> &sizes(T format) const\n> > +\t{\n> > +\t\tstatic const std::vector<SizeRange> empty;\n> > +\n> > +\t\tauto const &it = data_.find(format);\n> > +\t\tif (it == data_.end())\n> > +\t\t\treturn empty;\n> > +\n> > +\t\treturn it->second;\n> > +\t}\n\nGiven that we'll have only two specializations of this class, could we\navoid inlining the larger functions (that is all but isEmpty() and\ndata()) ? You can move them to formats.cpp, for instance\n\ntemplate<typename T>\nstd::vector<T> ImageFormats<T>::formats() const\n{\n\tstd::vector<T> formats;\n\tformats.reserve(data_.size());\n\n\t/* \\todo: Should this be cached instead of computed each time? */\n\tfor (auto const &it : data_)\n\t\tformats.push_back(it.first);\n\n\treturn formats;\n}\n\nand at the end add\n\ntemplate class ImageFormats<uint32_t>;\ntemplate class ImageFormats<V4L2PixelFormat>;\n\n> > +\n> > +\tconst std::map<T, std::vector<SizeRange>> &data() const\n> > +\t{\n> > +\t\treturn data_;\n> > +\t}\n\nI would then make this a single line, or make isEmpty() multi-line.\n\n> >  \n> >  private:\n> > -\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> > +\tstd::map<T, std::vector<SizeRange>> data_;\n> >  };\n> >  \n> >  class PixelFormatInfo\n> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > index 1be454f0ddda..0ce6da48f58a 100644\n> > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > @@ -51,7 +51,7 @@ public:\n> >  \tint setSelection(unsigned int pad, unsigned int target,\n> >  \t\t\t Rectangle *rect);\n> >  \n> > -\tImageFormats formats(unsigned int pad);\n> > +\tImageFormats<uint32_t> formats(unsigned int pad);\n> >  \n> >  \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >  \t\t      Whence whence = ActiveFormat);\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 2ac3b412ecdb..a7922077d9c5 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -22,20 +22,85 @@ LOG_DEFINE_CATEGORY(Formats)\n> >  \n> >  /**\n> >   * \\class ImageFormats\n> > - * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> > + * \\brief Describe V4L2Device and V4L2SubDevice image formats and associated\n> > + * image resolutions\n> >   *\n> > - * This class stores a list of image formats, each associated with a\n> > + * This class stores a list of image formats, each of them associated with a\n> >   * corresponding set of image sizes. It is used to describe the formats and\n> >   * sizes supported by a V4L2Device or V4L2Subdevice.\n> >   *\n> > - * Formats are stored as an integer. When used for a V4L2Device, the image\n> > - * formats are fourcc pixel formats. When used for a V4L2Subdevice they are\n> > - * media bus codes. Both are defined by the V4L2 specification.\n> > + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.\n> > + * When used for a V4L2Subdevice formats are integer media bus codes.\n\ns/integer/uint32_t/\n\n> >   *\n> >   * Sizes are stored as a list of SizeRange.\n> >   */\n> >  \n> >  /**\n> > + * \\typedef ImageFormats::iterator\n> > + * \\brief Iterator for the formats map\n> > + */\n> > +\n> > +/**\n> > + * \\typedef ImageFormats::const_iterator\n> > + * \\brief Const iterator for the formats map\n> > + */\n> > +\n> > +/**\n> > + * \\typedef ImageFormats::value_type\n> > + * \\brief Value type of the entries in the formats map\n> > + */\n> > +\n> > +/**\n> > + * \\fn iterator ImageFormats<T>::begin()\n> > + * \\brief Retrieve an iterator to the first element in the formats map\n> > + * \\return An iterator to the first format map\n> > + */\n> > +\n> > +/**\n> > + * \\fn const_iterator ImageFormats<T>::begin() const\n> > + * \\brief Retrieve an const iterator to the first element in the formats map\n> > + * \\return A const iterator to the first format map\n> > + */\n> > +\n> > +/**\n> > + * \\fn iterator ImageFormats<T>::end()\n> > + * \\brief Retrieve an iterator pointing to the past-the-end element in the\n> > + * formats map\n> > + * \\return An iterator to the element following the last format\n> > + */\n> > +\n> > +/**\n> > + * \\fn const_iterator ImageFormats<T>::end() const\n> > + * \\brief Retrieve a const iterator pointing to the past-the-end element in the\n> > + * formats map\n> > + * \\return A const iterator to the element following the last format\n> > + */\n> > +\n> > +/**\n> > + * \\fn iterator ImageFormats::find(const T format)\n> > + * \\brief Find an element with key equal to \\a format\n> > + * \\param[in] format The format to search for\n> > + * \\return An iterator to the vector of sizes associated with \\a format\n> > + */\n> > +\n> > +/**\n> > + * \\fn const_iterator ImageFormats::find(const T format) const\n> > + * \\brief Find a const element with key equal to \\a format\n> > + * \\param[in] format The format to search for\n> > + * \\return An const iterator to the vector of sizes associated with \\a format\n> > + */\n> > +\n> > +/**\n> > + * \\fn std::pair<iterator, bool> ImageFormats::emplace(Args&&... args)\n> > + * \\brief Insert a new element in the formats map constructed in place with the\n> > + * given \\a args\n> > + * \\param[in] args The argument pack used to construct the new entry in place\n> > + * \\return A pair consisting of an iterator to the inserted element, and a bool\n> > + * denoting whether the insertion took place\n> > + */\n> > +\n> > +/**\n> > + * \\fn ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)\n> >   * \\brief Add a format and corresponding sizes to the description\n> >   * \\param[in] format Pixel format or media bus code to describe\n> >   * \\param[in] sizes List of supported size ranges for the format\n> > @@ -43,42 +108,21 @@ LOG_DEFINE_CATEGORY(Formats)\n> >   * \\return 0 on success or a negative error code otherwise\n> >   * \\retval -EEXIST The format is already described\n> >   */\n> > -int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)\n> > -{\n> > -\tif (data_.find(format) != data_.end())\n> > -\t\treturn -EEXIST;\n> > -\n> > -\tdata_[format] = sizes;\n> > -\n> > -\treturn 0;\n> > -}\n> >  \n> >  /**\n> > + * \\fn ImageFormats<T>::isEmpty() const\n> >   * \\brief Check if the list of devices supported formats is empty\n> >   * \\return True if the list of supported formats is empty\n> >   */\n> > -bool ImageFormats::isEmpty() const\n> > -{\n> > -\treturn data_.empty();\n> > -}\n> >  \n> >  /**\n> > + * \\fn ImageFormats<T>::formats() const\n> >   * \\brief Retrieve a list of all supported image formats\n> >   * \\return List of pixel formats or media bus codes\n> >   */\n> > -std::vector<unsigned int> ImageFormats::formats() const\n> > -{\n> > -\tstd::vector<unsigned int> formats;\n> > -\tformats.reserve(data_.size());\n> > -\n> > -\t/* \\todo: Should this be cached instead of computed each time? */\n> > -\tfor (auto const &it : data_)\n> > -\t\tformats.push_back(it.first);\n> > -\n> > -\treturn formats;\n> > -}\n> >  \n> >  /**\n> > + * \\fn ImageFormats<T>::sizes(T format) const\n> >   * \\brief Retrieve all sizes for a specific format\n> >   * \\param[in] format The pixel format or mbus code\n> >   *\n> > @@ -88,25 +132,12 @@ std::vector<unsigned int> ImageFormats::formats() const\n> >   * \\return The list of image sizes supported for \\a format, or an empty list if\n> >   * the format is not supported\n> >   */\n> > -const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) const\n> > -{\n> > -\tstatic const std::vector<SizeRange> empty;\n> > -\n> > -\tauto const &it = data_.find(format);\n> > -\tif (it == data_.end())\n> > -\t\treturn empty;\n> > -\n> > -\treturn it->second;\n> > -}\n> >  \n> >  /**\n> > + * \\fn ImageFormats<T>::data() const\n> >   * \\brief Retrieve the map that associates formats to image sizes\n> >   * \\return The map that associates formats to image sizes\n> >   */\n> > -const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const\n> > -{\n> > -\treturn data_;\n> > -}\n> >  \n> >  /**\n> >   * \\class PixelFormatInfo\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 7aefc1be032d..9fa20e84a904 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >   *\n> >   * \\return A list of the supported device formats\n> >   */\n> > -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > +ImageFormats<uint32_t> V4L2Subdevice::formats(unsigned int pad)\n> >  {\n> > -\tImageFormats formats;\n> > +\tImageFormats<uint32_t> formats;\n> >  \n> >  \tif (pad >= entity_->pads().size()) {\n> >  \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> >  \t\treturn {};\n> >  \t}\n> >  \n> > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > +\tfor (uint32_t code : enumPadCodes(pad)) {\n> >  \t\tstd::vector<SizeRange> sizes = enumPadSizes(pad, code);\n> >  \t\tif (sizes.empty())\n> >  \t\t\treturn {};\n> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> > index 25503c3334e5..f66bb633fb00 100644\n> > --- a/test/v4l2_subdevice/list_formats.cpp\n> > +++ b/test/v4l2_subdevice/list_formats.cpp\n> > @@ -48,7 +48,7 @@ void ListFormatsTest::printFormats(unsigned int pad,\n> >  int ListFormatsTest::run()\n> >  {\n> >  \t/* List all formats available on existing \"Scaler\" pads. */\n> > -\tImageFormats formats;\n> > +\tImageFormats<uint32_t> formats;\n> >  \n> >  \tformats = scaler_->formats(0);\n> >  \tif (formats.isEmpty()) {","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","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 4753C600F7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  6 Jun 2020 01:19:00 +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 5771227C;\n\tSat,  6 Jun 2020 01:18:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lG0VkQab\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591399139;\n\tbh=6amFfkzja9zEJzMKB1JhQjB908113PPA1iRboWE3NqQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lG0VkQab+dT4dayQXzRuRlUt3UFayEP4fmInyNptId4oNIuoBHli+1sOgK/lPGqlz\n\tsmyA9+znSytEYvj9TreuQG2nDMiTbdDlm9c+cby2mciHByBCCFt5ChW+C3awU31dBW\n\tNsLliGQzdz04X3mE7hJApZ3LtnQ+VXgrkOtxKFHY=","Date":"Sat, 6 Jun 2020 02:18:40 +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>, libcamera-devel@lists.libcamera.org","Message-ID":"<20200605231840.GP26752@pendragon.ideasonboard.com>","References":"<20200529110335.620503-1-jacopo@jmondi.org>\n\t<20200529110335.620503-2-jacopo@jmondi.org>\n\t<20200605193643.GM5864@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200605193643.GM5864@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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":"Fri, 05 Jun 2020 23:19:00 -0000"}},{"id":5118,"web_url":"https://patchwork.libcamera.org/comment/5118/","msgid":"<20200608075434.uzutaplcjg4n5z6j@uno.localdomain>","date":"2020-06-08T07:54:34","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sat, Jun 06, 2020 at 02:18:40AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Niklas,\n>\n> On Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote:\n> > On 2020-05-29 13:03:31 +0200, Jacopo Mondi wrote:\n> > > The ImageFormats class was originally designed to be used by both\n> > > V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their\n> > > image formats using V4L2PixelFormat instances, the ImageFormats class\n> > > cannot be used there anymore and it has been replaced by raw maps.\n> > >\n> > > Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice\n> > > class and its users by making ImageFormats a templated class that\n> > > indexes the image sizes using on keys of variadic type.\n> >\n> > This is similar to '[RFC 2/6] libcamera: formats: Turn ImageFormats into\n> > a template' which was dropped in favor of trying to refactor away\n> > ImageFormats going forward.\n> >\n> > I'm not against backtracking on that decision as it's been a few months\n> > since then. But I think we all need to agree what option is the best way\n> > forward.\n>\n> I agree with you. I still believe we'll be able to find a better\n> abstraction in the long term. If there's value in refactoring, given how\n> long it's taking us to come up with a better solution, I think that's a\n> good intermediate step. Sorry for rejecting it initially, I thought we\n> could converge on a better solution faster.\n\nYeah, this is very similar, and I'm aware I contributed to shot that\nproposal down, sorry about this. Howevere, we still have ImageFormats\naround, and pipeline handlers are starting to abuse it, so as it's\nstill around, I think it's worth trying to make it nicer to use\n\n\n>\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  include/libcamera/internal/camera_sensor.h  |   2 +-\n> > >  include/libcamera/internal/formats.h        |  64 ++++++++++-\n> > >  include/libcamera/internal/v4l2_subdevice.h |   2 +-\n> > >  src/libcamera/formats.cpp                   | 117 +++++++++++++-------\n> > >  src/libcamera/v4l2_subdevice.cpp            |   6 +-\n> > >  test/v4l2_subdevice/list_formats.cpp        |   2 +-\n> > >  6 files changed, 138 insertions(+), 55 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index d79bd9ce9d58..5c1d5789fe79 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -75,7 +75,7 @@ private:\n> > >\n> > >  \tstd::string model_;\n> > >\n> > > -\tImageFormats formats_;\n> > > +\tImageFormats<uint32_t> formats_;\n> > >  \tSize resolution_;\n> > >  \tstd::vector<unsigned int> mbusCodes_;\n> > >  \tstd::vector<Size> sizes_;\n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index 4092a93ef973..e20c031e857f 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -18,18 +18,70 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +template<typename T>\n> > >  class ImageFormats\n> > >  {\n> > >  public:\n> > > -\tint addFormat(unsigned int format, const std::vector<SizeRange> &sizes);\n> > > +\tusing iterator = typename std::map<T, std::vector<SizeRange>>::iterator;\n> > > +\tusing const_iterator = typename std::map<T, std::vector<SizeRange>>::const_iterator;\n> > > +\tusing value_type = typename std::map<T, std::vector<SizeRange>>::value_type;\n> > >\n> > > -\tbool isEmpty() const;\n> > > -\tstd::vector<unsigned int> formats() const;\n> > > -\tconst std::vector<SizeRange> &sizes(unsigned int format) const;\n> > > -\tconst std::map<unsigned int, std::vector<SizeRange>> &data() const;\n> > > +\titerator begin() { return data_.begin(); }\n> > > +\tconst_iterator begin() const { return data_.begin(); }\n> > > +\titerator end() { return data_.end(); }\n> > > +\tconst_iterator end() const { return data_.end(); }\n> > > +\n> > > +\titerator find(const T format) { return data_.find(format); }\n> > > +\tconst iterator find(T format) const { return data_.find(format); }\n>\n> I'd split this to a separate patch. How much of the std::map API do we\n> want/need to expose ?\n>\n> > > +\n> > > +\ttemplate<class... Args>\n> > > +\tstd::pair<iterator, bool> emplace(Args&&... args)\n> > > +\t{\n> > > +\t\treturn data_.emplace(args...);\n> > > +\t}\n> > > +\n> > > +\tint addFormat(T format, const std::vector<SizeRange> &sizes)\n> > > +\t{\n> > > +\t\tif (data_.find(format) != data_.end())\n> > > +\t\t\treturn -EEXIST;\n> > > +\n> > > +\t\tdata_[format] = sizes;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tbool isEmpty() const { return data_.empty(); }\n> > > +\n> > > +\tstd::vector<T> formats() const\n> > > +\t{\n> > > +\t\tstd::vector<T> formats;\n> > > +\t\tformats.reserve(data_.size());\n> > > +\n> > > +\t\t/* \\todo: Should this be cached instead of computed each time? */\n> > > +\t\tfor (auto const &it : data_)\n> > > +\t\t\tformats.push_back(it.first);\n> > > +\n> > > +\t\treturn formats;\n> > > +\t}\n> > > +\n> > > +\tconst std::vector<SizeRange> &sizes(T format) const\n> > > +\t{\n> > > +\t\tstatic const std::vector<SizeRange> empty;\n> > > +\n> > > +\t\tauto const &it = data_.find(format);\n> > > +\t\tif (it == data_.end())\n> > > +\t\t\treturn empty;\n> > > +\n> > > +\t\treturn it->second;\n> > > +\t}\n>\n> Given that we'll have only two specializations of this class, could we\n> avoid inlining the larger functions (that is all but isEmpty() and\n> data()) ? You can move them to formats.cpp, for instance\n>\n> template<typename T>\n> std::vector<T> ImageFormats<T>::formats() const\n> {\n> \tstd::vector<T> formats;\n> \tformats.reserve(data_.size());\n>\n> \t/* \\todo: Should this be cached instead of computed each time? */\n> \tfor (auto const &it : data_)\n> \t\tformats.push_back(it.first);\n>\n> \treturn formats;\n> }\n>\n> and at the end add\n>\n> template class ImageFormats<uint32_t>;\n> template class ImageFormats<V4L2PixelFormat>;\n>\n> > > +\n> > > +\tconst std::map<T, std::vector<SizeRange>> &data() const\n> > > +\t{\n> > > +\t\treturn data_;\n> > > +\t}\n>\n> I would then make this a single line, or make isEmpty() multi-line.\n>\n> > >\n> > >  private:\n> > > -\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> > > +\tstd::map<T, std::vector<SizeRange>> data_;\n> > >  };\n> > >\n> > >  class PixelFormatInfo\n> > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > > index 1be454f0ddda..0ce6da48f58a 100644\n> > > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > > @@ -51,7 +51,7 @@ public:\n> > >  \tint setSelection(unsigned int pad, unsigned int target,\n> > >  \t\t\t Rectangle *rect);\n> > >\n> > > -\tImageFormats formats(unsigned int pad);\n> > > +\tImageFormats<uint32_t> formats(unsigned int pad);\n> > >\n> > >  \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > >  \t\t      Whence whence = ActiveFormat);\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index 2ac3b412ecdb..a7922077d9c5 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -22,20 +22,85 @@ LOG_DEFINE_CATEGORY(Formats)\n> > >\n> > >  /**\n> > >   * \\class ImageFormats\n> > > - * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> > > + * \\brief Describe V4L2Device and V4L2SubDevice image formats and associated\n> > > + * image resolutions\n> > >   *\n> > > - * This class stores a list of image formats, each associated with a\n> > > + * This class stores a list of image formats, each of them associated with a\n> > >   * corresponding set of image sizes. It is used to describe the formats and\n> > >   * sizes supported by a V4L2Device or V4L2Subdevice.\n> > >   *\n> > > - * Formats are stored as an integer. When used for a V4L2Device, the image\n> > > - * formats are fourcc pixel formats. When used for a V4L2Subdevice they are\n> > > - * media bus codes. Both are defined by the V4L2 specification.\n> > > + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.\n> > > + * When used for a V4L2Subdevice formats are integer media bus codes.\n>\n> s/integer/uint32_t/\n>\n> > >   *\n> > >   * Sizes are stored as a list of SizeRange.\n> > >   */\n> > >\n> > >  /**\n> > > + * \\typedef ImageFormats::iterator\n> > > + * \\brief Iterator for the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef ImageFormats::const_iterator\n> > > + * \\brief Const iterator for the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef ImageFormats::value_type\n> > > + * \\brief Value type of the entries in the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats<T>::begin()\n> > > + * \\brief Retrieve an iterator to the first element in the formats map\n> > > + * \\return An iterator to the first format map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats<T>::begin() const\n> > > + * \\brief Retrieve an const iterator to the first element in the formats map\n> > > + * \\return A const iterator to the first format map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats<T>::end()\n> > > + * \\brief Retrieve an iterator pointing to the past-the-end element in the\n> > > + * formats map\n> > > + * \\return An iterator to the element following the last format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats<T>::end() const\n> > > + * \\brief Retrieve a const iterator pointing to the past-the-end element in the\n> > > + * formats map\n> > > + * \\return A const iterator to the element following the last format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats::find(const T format)\n> > > + * \\brief Find an element with key equal to \\a format\n> > > + * \\param[in] format The format to search for\n> > > + * \\return An iterator to the vector of sizes associated with \\a format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats::find(const T format) const\n> > > + * \\brief Find a const element with key equal to \\a format\n> > > + * \\param[in] format The format to search for\n> > > + * \\return An const iterator to the vector of sizes associated with \\a format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn std::pair<iterator, bool> ImageFormats::emplace(Args&&... args)\n> > > + * \\brief Insert a new element in the formats map constructed in place with the\n> > > + * given \\a args\n> > > + * \\param[in] args The argument pack used to construct the new entry in place\n> > > + * \\return A pair consisting of an iterator to the inserted element, and a bool\n> > > + * denoting whether the insertion took place\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)\n> > >   * \\brief Add a format and corresponding sizes to the description\n> > >   * \\param[in] format Pixel format or media bus code to describe\n> > >   * \\param[in] sizes List of supported size ranges for the format\n> > > @@ -43,42 +108,21 @@ LOG_DEFINE_CATEGORY(Formats)\n> > >   * \\return 0 on success or a negative error code otherwise\n> > >   * \\retval -EEXIST The format is already described\n> > >   */\n> > > -int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)\n> > > -{\n> > > -\tif (data_.find(format) != data_.end())\n> > > -\t\treturn -EEXIST;\n> > > -\n> > > -\tdata_[format] = sizes;\n> > > -\n> > > -\treturn 0;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::isEmpty() const\n> > >   * \\brief Check if the list of devices supported formats is empty\n> > >   * \\return True if the list of supported formats is empty\n> > >   */\n> > > -bool ImageFormats::isEmpty() const\n> > > -{\n> > > -\treturn data_.empty();\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::formats() const\n> > >   * \\brief Retrieve a list of all supported image formats\n> > >   * \\return List of pixel formats or media bus codes\n> > >   */\n> > > -std::vector<unsigned int> ImageFormats::formats() const\n> > > -{\n> > > -\tstd::vector<unsigned int> formats;\n> > > -\tformats.reserve(data_.size());\n> > > -\n> > > -\t/* \\todo: Should this be cached instead of computed each time? */\n> > > -\tfor (auto const &it : data_)\n> > > -\t\tformats.push_back(it.first);\n> > > -\n> > > -\treturn formats;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::sizes(T format) const\n> > >   * \\brief Retrieve all sizes for a specific format\n> > >   * \\param[in] format The pixel format or mbus code\n> > >   *\n> > > @@ -88,25 +132,12 @@ std::vector<unsigned int> ImageFormats::formats() const\n> > >   * \\return The list of image sizes supported for \\a format, or an empty list if\n> > >   * the format is not supported\n> > >   */\n> > > -const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) const\n> > > -{\n> > > -\tstatic const std::vector<SizeRange> empty;\n> > > -\n> > > -\tauto const &it = data_.find(format);\n> > > -\tif (it == data_.end())\n> > > -\t\treturn empty;\n> > > -\n> > > -\treturn it->second;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::data() const\n> > >   * \\brief Retrieve the map that associates formats to image sizes\n> > >   * \\return The map that associates formats to image sizes\n> > >   */\n> > > -const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const\n> > > -{\n> > > -\treturn data_;\n> > > -}\n> > >\n> > >  /**\n> > >   * \\class PixelFormatInfo\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 7aefc1be032d..9fa20e84a904 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > >   *\n> > >   * \\return A list of the supported device formats\n> > >   */\n> > > -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > > +ImageFormats<uint32_t> V4L2Subdevice::formats(unsigned int pad)\n> > >  {\n> > > -\tImageFormats formats;\n> > > +\tImageFormats<uint32_t> formats;\n> > >\n> > >  \tif (pad >= entity_->pads().size()) {\n> > >  \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > >  \t\treturn {};\n> > >  \t}\n> > >\n> > > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > > +\tfor (uint32_t code : enumPadCodes(pad)) {\n> > >  \t\tstd::vector<SizeRange> sizes = enumPadSizes(pad, code);\n> > >  \t\tif (sizes.empty())\n> > >  \t\t\treturn {};\n> > > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> > > index 25503c3334e5..f66bb633fb00 100644\n> > > --- a/test/v4l2_subdevice/list_formats.cpp\n> > > +++ b/test/v4l2_subdevice/list_formats.cpp\n> > > @@ -48,7 +48,7 @@ void ListFormatsTest::printFormats(unsigned int pad,\n> > >  int ListFormatsTest::run()\n> > >  {\n> > >  \t/* List all formats available on existing \"Scaler\" pads. */\n> > > -\tImageFormats formats;\n> > > +\tImageFormats<uint32_t> formats;\n> > >\n> > >  \tformats = scaler_->formats(0);\n> > >  \tif (formats.isEmpty()) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45073603C7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  8 Jun 2020 09:51:17 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 9FF5060012;\n\tMon,  8 Jun 2020 07:51:10 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Jun 2020 09:54:34 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200608075434.uzutaplcjg4n5z6j@uno.localdomain>","References":"<20200529110335.620503-1-jacopo@jmondi.org>\n\t<20200529110335.620503-2-jacopo@jmondi.org>\n\t<20200605193643.GM5864@oden.dyn.berto.se>\n\t<20200605231840.GP26752@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200605231840.GP26752@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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, 08 Jun 2020 07:51:17 -0000"}},{"id":5146,"web_url":"https://patchwork.libcamera.org/comment/5146/","msgid":"<20200608213013.vbeu5qoen2pmczqj@uno.localdomain>","date":"2020-06-08T21:30:13","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Jun 06, 2020 at 02:18:40AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Niklas,\n>\n> On Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote:\n> > On 2020-05-29 13:03:31 +0200, Jacopo Mondi wrote:\n> > > The ImageFormats class was originally designed to be used by both\n> > > V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their\n> > > image formats using V4L2PixelFormat instances, the ImageFormats class\n> > > cannot be used there anymore and it has been replaced by raw maps.\n> > >\n> > > Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice\n> > > class and its users by making ImageFormats a templated class that\n> > > indexes the image sizes using on keys of variadic type.\n> >\n> > This is similar to '[RFC 2/6] libcamera: formats: Turn ImageFormats into\n> > a template' which was dropped in favor of trying to refactor away\n> > ImageFormats going forward.\n> >\n> > I'm not against backtracking on that decision as it's been a few months\n> > since then. But I think we all need to agree what option is the best way\n> > forward.\n>\n> I agree with you. I still believe we'll be able to find a better\n> abstraction in the long term. If there's value in refactoring, given how\n> long it's taking us to come up with a better solution, I think that's a\n> good intermediate step. Sorry for rejecting it initially, I thought we\n> could converge on a better solution faster.\n>\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  include/libcamera/internal/camera_sensor.h  |   2 +-\n> > >  include/libcamera/internal/formats.h        |  64 ++++++++++-\n> > >  include/libcamera/internal/v4l2_subdevice.h |   2 +-\n> > >  src/libcamera/formats.cpp                   | 117 +++++++++++++-------\n> > >  src/libcamera/v4l2_subdevice.cpp            |   6 +-\n> > >  test/v4l2_subdevice/list_formats.cpp        |   2 +-\n> > >  6 files changed, 138 insertions(+), 55 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index d79bd9ce9d58..5c1d5789fe79 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -75,7 +75,7 @@ private:\n> > >\n> > >  \tstd::string model_;\n> > >\n> > > -\tImageFormats formats_;\n> > > +\tImageFormats<uint32_t> formats_;\n> > >  \tSize resolution_;\n> > >  \tstd::vector<unsigned int> mbusCodes_;\n> > >  \tstd::vector<Size> sizes_;\n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index 4092a93ef973..e20c031e857f 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -18,18 +18,70 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +template<typename T>\n> > >  class ImageFormats\n> > >  {\n> > >  public:\n> > > -\tint addFormat(unsigned int format, const std::vector<SizeRange> &sizes);\n> > > +\tusing iterator = typename std::map<T, std::vector<SizeRange>>::iterator;\n> > > +\tusing const_iterator = typename std::map<T, std::vector<SizeRange>>::const_iterator;\n> > > +\tusing value_type = typename std::map<T, std::vector<SizeRange>>::value_type;\n> > >\n> > > -\tbool isEmpty() const;\n> > > -\tstd::vector<unsigned int> formats() const;\n> > > -\tconst std::vector<SizeRange> &sizes(unsigned int format) const;\n> > > -\tconst std::map<unsigned int, std::vector<SizeRange>> &data() const;\n> > > +\titerator begin() { return data_.begin(); }\n> > > +\tconst_iterator begin() const { return data_.begin(); }\n> > > +\titerator end() { return data_.end(); }\n> > > +\tconst_iterator end() const { return data_.end(); }\n> > > +\n> > > +\titerator find(const T format) { return data_.find(format); }\n> > > +\tconst iterator find(T format) const { return data_.find(format); }\n>\n> I'd split this to a separate patch. How much of the std::map API do we\n> want/need to expose ?\n>\n\nIterators, begin and end for sure.\nNot sure about emplace and find, which are not used... I'll leave them\nout, but I'm not sure it's worth a separate patch to add iterators :/\n\n> > > +\n> > > +\ttemplate<class... Args>\n> > > +\tstd::pair<iterator, bool> emplace(Args&&... args)\n> > > +\t{\n> > > +\t\treturn data_.emplace(args...);\n> > > +\t}\n> > > +\n> > > +\tint addFormat(T format, const std::vector<SizeRange> &sizes)\n> > > +\t{\n> > > +\t\tif (data_.find(format) != data_.end())\n> > > +\t\t\treturn -EEXIST;\n> > > +\n> > > +\t\tdata_[format] = sizes;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tbool isEmpty() const { return data_.empty(); }\n> > > +\n> > > +\tstd::vector<T> formats() const\n> > > +\t{\n> > > +\t\tstd::vector<T> formats;\n> > > +\t\tformats.reserve(data_.size());\n> > > +\n> > > +\t\t/* \\todo: Should this be cached instead of computed each time? */\n> > > +\t\tfor (auto const &it : data_)\n> > > +\t\t\tformats.push_back(it.first);\n> > > +\n> > > +\t\treturn formats;\n> > > +\t}\n> > > +\n> > > +\tconst std::vector<SizeRange> &sizes(T format) const\n> > > +\t{\n> > > +\t\tstatic const std::vector<SizeRange> empty;\n> > > +\n> > > +\t\tauto const &it = data_.find(format);\n> > > +\t\tif (it == data_.end())\n> > > +\t\t\treturn empty;\n> > > +\n> > > +\t\treturn it->second;\n> > > +\t}\n>\n> Given that we'll have only two specializations of this class, could we\n> avoid inlining the larger functions (that is all but isEmpty() and\n> data()) ? You can move them to formats.cpp, for instance\n>\n> template<typename T>\n> std::vector<T> ImageFormats<T>::formats() const\n> {\n> \tstd::vector<T> formats;\n> \tformats.reserve(data_.size());\n>\n> \t/* \\todo: Should this be cached instead of computed each time? */\n> \tfor (auto const &it : data_)\n> \t\tformats.push_back(it.first);\n>\n> \treturn formats;\n> }\n>\n> and at the end add\n>\n> template class ImageFormats<uint32_t>;\n> template class ImageFormats<V4L2PixelFormat>;\n>\n\nSure thing, I had it this way as I had the idea to unify this with\nStreamFormats, which is a very similar construct.. But then I gave up\nas the idea of making image format public would have required to\nconvince too many people :)\n\n\nThanks\n  j\n\n> > > +\n> > > +\tconst std::map<T, std::vector<SizeRange>> &data() const\n> > > +\t{\n> > > +\t\treturn data_;\n> > > +\t}\n>\n> I would then make this a single line, or make isEmpty() multi-line.\n>\n> > >\n> > >  private:\n> > > -\tstd::map<unsigned int, std::vector<SizeRange>> data_;\n> > > +\tstd::map<T, std::vector<SizeRange>> data_;\n> > >  };\n> > >\n> > >  class PixelFormatInfo\n> > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > > index 1be454f0ddda..0ce6da48f58a 100644\n> > > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > > @@ -51,7 +51,7 @@ public:\n> > >  \tint setSelection(unsigned int pad, unsigned int target,\n> > >  \t\t\t Rectangle *rect);\n> > >\n> > > -\tImageFormats formats(unsigned int pad);\n> > > +\tImageFormats<uint32_t> formats(unsigned int pad);\n> > >\n> > >  \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > >  \t\t      Whence whence = ActiveFormat);\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index 2ac3b412ecdb..a7922077d9c5 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -22,20 +22,85 @@ LOG_DEFINE_CATEGORY(Formats)\n> > >\n> > >  /**\n> > >   * \\class ImageFormats\n> > > - * \\brief Describe V4L2Device and V4L2SubDevice image formats\n> > > + * \\brief Describe V4L2Device and V4L2SubDevice image formats and associated\n> > > + * image resolutions\n> > >   *\n> > > - * This class stores a list of image formats, each associated with a\n> > > + * This class stores a list of image formats, each of them associated with a\n> > >   * corresponding set of image sizes. It is used to describe the formats and\n> > >   * sizes supported by a V4L2Device or V4L2Subdevice.\n> > >   *\n> > > - * Formats are stored as an integer. When used for a V4L2Device, the image\n> > > - * formats are fourcc pixel formats. When used for a V4L2Subdevice they are\n> > > - * media bus codes. Both are defined by the V4L2 specification.\n> > > + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.\n> > > + * When used for a V4L2Subdevice formats are integer media bus codes.\n>\n> s/integer/uint32_t/\n>\n> > >   *\n> > >   * Sizes are stored as a list of SizeRange.\n> > >   */\n> > >\n> > >  /**\n> > > + * \\typedef ImageFormats::iterator\n> > > + * \\brief Iterator for the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef ImageFormats::const_iterator\n> > > + * \\brief Const iterator for the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef ImageFormats::value_type\n> > > + * \\brief Value type of the entries in the formats map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats<T>::begin()\n> > > + * \\brief Retrieve an iterator to the first element in the formats map\n> > > + * \\return An iterator to the first format map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats<T>::begin() const\n> > > + * \\brief Retrieve an const iterator to the first element in the formats map\n> > > + * \\return A const iterator to the first format map\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats<T>::end()\n> > > + * \\brief Retrieve an iterator pointing to the past-the-end element in the\n> > > + * formats map\n> > > + * \\return An iterator to the element following the last format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats<T>::end() const\n> > > + * \\brief Retrieve a const iterator pointing to the past-the-end element in the\n> > > + * formats map\n> > > + * \\return A const iterator to the element following the last format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn iterator ImageFormats::find(const T format)\n> > > + * \\brief Find an element with key equal to \\a format\n> > > + * \\param[in] format The format to search for\n> > > + * \\return An iterator to the vector of sizes associated with \\a format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn const_iterator ImageFormats::find(const T format) const\n> > > + * \\brief Find a const element with key equal to \\a format\n> > > + * \\param[in] format The format to search for\n> > > + * \\return An const iterator to the vector of sizes associated with \\a format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn std::pair<iterator, bool> ImageFormats::emplace(Args&&... args)\n> > > + * \\brief Insert a new element in the formats map constructed in place with the\n> > > + * given \\a args\n> > > + * \\param[in] args The argument pack used to construct the new entry in place\n> > > + * \\return A pair consisting of an iterator to the inserted element, and a bool\n> > > + * denoting whether the insertion took place\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)\n> > >   * \\brief Add a format and corresponding sizes to the description\n> > >   * \\param[in] format Pixel format or media bus code to describe\n> > >   * \\param[in] sizes List of supported size ranges for the format\n> > > @@ -43,42 +108,21 @@ LOG_DEFINE_CATEGORY(Formats)\n> > >   * \\return 0 on success or a negative error code otherwise\n> > >   * \\retval -EEXIST The format is already described\n> > >   */\n> > > -int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)\n> > > -{\n> > > -\tif (data_.find(format) != data_.end())\n> > > -\t\treturn -EEXIST;\n> > > -\n> > > -\tdata_[format] = sizes;\n> > > -\n> > > -\treturn 0;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::isEmpty() const\n> > >   * \\brief Check if the list of devices supported formats is empty\n> > >   * \\return True if the list of supported formats is empty\n> > >   */\n> > > -bool ImageFormats::isEmpty() const\n> > > -{\n> > > -\treturn data_.empty();\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::formats() const\n> > >   * \\brief Retrieve a list of all supported image formats\n> > >   * \\return List of pixel formats or media bus codes\n> > >   */\n> > > -std::vector<unsigned int> ImageFormats::formats() const\n> > > -{\n> > > -\tstd::vector<unsigned int> formats;\n> > > -\tformats.reserve(data_.size());\n> > > -\n> > > -\t/* \\todo: Should this be cached instead of computed each time? */\n> > > -\tfor (auto const &it : data_)\n> > > -\t\tformats.push_back(it.first);\n> > > -\n> > > -\treturn formats;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::sizes(T format) const\n> > >   * \\brief Retrieve all sizes for a specific format\n> > >   * \\param[in] format The pixel format or mbus code\n> > >   *\n> > > @@ -88,25 +132,12 @@ std::vector<unsigned int> ImageFormats::formats() const\n> > >   * \\return The list of image sizes supported for \\a format, or an empty list if\n> > >   * the format is not supported\n> > >   */\n> > > -const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) const\n> > > -{\n> > > -\tstatic const std::vector<SizeRange> empty;\n> > > -\n> > > -\tauto const &it = data_.find(format);\n> > > -\tif (it == data_.end())\n> > > -\t\treturn empty;\n> > > -\n> > > -\treturn it->second;\n> > > -}\n> > >\n> > >  /**\n> > > + * \\fn ImageFormats<T>::data() const\n> > >   * \\brief Retrieve the map that associates formats to image sizes\n> > >   * \\return The map that associates formats to image sizes\n> > >   */\n> > > -const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const\n> > > -{\n> > > -\treturn data_;\n> > > -}\n> > >\n> > >  /**\n> > >   * \\class PixelFormatInfo\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 7aefc1be032d..9fa20e84a904 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > >   *\n> > >   * \\return A list of the supported device formats\n> > >   */\n> > > -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > > +ImageFormats<uint32_t> V4L2Subdevice::formats(unsigned int pad)\n> > >  {\n> > > -\tImageFormats formats;\n> > > +\tImageFormats<uint32_t> formats;\n> > >\n> > >  \tif (pad >= entity_->pads().size()) {\n> > >  \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > >  \t\treturn {};\n> > >  \t}\n> > >\n> > > -\tfor (unsigned int code : enumPadCodes(pad)) {\n> > > +\tfor (uint32_t code : enumPadCodes(pad)) {\n> > >  \t\tstd::vector<SizeRange> sizes = enumPadSizes(pad, code);\n> > >  \t\tif (sizes.empty())\n> > >  \t\t\treturn {};\n> > > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> > > index 25503c3334e5..f66bb633fb00 100644\n> > > --- a/test/v4l2_subdevice/list_formats.cpp\n> > > +++ b/test/v4l2_subdevice/list_formats.cpp\n> > > @@ -48,7 +48,7 @@ void ListFormatsTest::printFormats(unsigned int pad,\n> > >  int ListFormatsTest::run()\n> > >  {\n> > >  \t/* List all formats available on existing \"Scaler\" pads. */\n> > > -\tImageFormats formats;\n> > > +\tImageFormats<uint32_t> formats;\n> > >\n> > >  \tformats = scaler_->formats(0);\n> > >  \tif (formats.isEmpty()) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B4F261027\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  8 Jun 2020 23:26:49 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 74F9740002;\n\tMon,  8 Jun 2020 21:26:48 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Jun 2020 23:30:13 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200608213013.vbeu5qoen2pmczqj@uno.localdomain>","References":"<20200529110335.620503-1-jacopo@jmondi.org>\n\t<20200529110335.620503-2-jacopo@jmondi.org>\n\t<20200605193643.GM5864@oden.dyn.berto.se>\n\t<20200605231840.GP26752@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200605231840.GP26752@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: formats: Make\n\tImageFormats a templated class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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, 08 Jun 2020 21:26:49 -0000"}}]