[{"id":3849,"web_url":"https://patchwork.libcamera.org/comment/3849/","msgid":"<20200228094651.d7she5ixweoun7xa@uno.localdomain>","date":"2020-02-28T09:46:51","subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Feb 28, 2020 at 04:29:09AM +0100, Niklas Söderlund wrote:\n> The ImageFormats class is used to carry both V4L2 pixelformats and mbus\n> codes as keys to lookup different supported sizes. The reason for this\n> is that both V4L2 pixelformats and mbus codes are unsigned integers and\n> the keyword PixelFormat is currently defined as such.\n>\n> Going forward PixelFormat will be reworked into a class so their is a\n> need to have ImageFormats instances with two different key types, turn\n> it into an template and update all sites to use unsigned int where a\n> mbus code is the key and PixelFormat otherwise.\n>\n\nSorry, but I'm not sure to follow.\n\nImageFormats is said to report image formats for V4L2Device and\nV4L2Subdevices at the moment and indeed it uses an unsigned int as key\nwhich could be interpreted as either a v4l2 fourcc in case we're\ndealing with a video device, or as an mbus media code if we're dealing\nwith a v4l2 subdev.\n\nPixelFormat is designed to be exposed to applications, and I would\nreally avoid create dependencies between the V4L2VideoDevice 4cc\ncodes and the application facing one.\n\nI agree ImageFormats is not a descriptive name, and I would be in\nfavour of renaming it to, say, DeviceFormat or whatever, but I'm not\nsure what we gain by making v4l2 devices tied to the same class we use\nto report image formats to application.\n\nWill I find it out later in the series ?\n\nThanks\n j\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/camera_sensor.cpp | 2 +-\n> src/libcamera/formats.cpp | 19 +++++++++++++------\n> src/libcamera/include/formats.h | 11 ++++++-----\n> src/libcamera/include/v4l2_subdevice.h | 2 +-\n> src/libcamera/include/v4l2_videodevice.h | 2 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/v4l2_subdevice.cpp | 4 ++--\n> src/libcamera/v4l2_videodevice.cpp | 4 ++--\n> test/v4l2_subdevice/list_formats.cpp | 2 +-\n> 10 files changed, 29 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2219a43074362834..23fdafb690117d91 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -131,7 +131,7 @@ int CameraSensor::init()\n> \tproperties_.set(properties::Rotation, propertyValue);\n>\n> \t/* Enumerate and cache media bus codes and sizes. */\n> -\tconst ImageFormats formats = subdev_->formats(0);\n> +\tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> \t\tLOG(CameraSensor, Error) << \"No image format found\";\n> \t\treturn -EINVAL;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 5f6552a4e06c5231..98817deee2b54c84 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -39,7 +39,8 @@ namespace libcamera {\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 &sizes)\n> +template\n> +int ImageFormats::addFormat(T format, const std::vector &sizes)\n> {\n> \tif (data_.find(format) != data_.end())\n> \t\treturn -EEXIST;\n> @@ -53,7 +54,8 @@ int ImageFormats::addFormat(unsigned int format, const std::vector &s\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> +template\n> +bool ImageFormats::isEmpty() const\n> {\n> \treturn data_.empty();\n> }\n> @@ -62,9 +64,10 @@ bool ImageFormats::isEmpty() 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 ImageFormats::formats() const\n> +template\n> +std::vector ImageFormats::formats() const\n> {\n> -\tstd::vector formats;\n> +\tstd::vector formats;\n> \tformats.reserve(data_.size());\n>\n> \t/* \\todo: Should this be cached instead of computed each time? */\n> @@ -84,7 +87,8 @@ std::vector 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 &ImageFormats::sizes(unsigned int format) const\n> +template\n> +const std::vector &ImageFormats::sizes(T format) const\n> {\n> \tstatic const std::vector empty;\n>\n> @@ -99,9 +103,12 @@ const std::vector &ImageFormats::sizes(unsigned int format) 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> &ImageFormats::data() const\n> +template\n> +const std::map> &ImageFormats::data() const\n> {\n> \treturn data_;\n> }\n>\n> +template class ImageFormats;\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> index f43bc8c004f690b4..566be9c09550a830 100644\n> --- a/src/libcamera/include/formats.h\n> +++ b/src/libcamera/include/formats.h\n> @@ -15,18 +15,19 @@\n>\n> namespace libcamera {\n>\n> +template\n> class ImageFormats\n> {\n> public:\n> -\tint addFormat(unsigned int format, const std::vector &sizes);\n> +\tint addFormat(T format, const std::vector &sizes);\n>\n> \tbool isEmpty() const;\n> -\tstd::vector formats() const;\n> -\tconst std::vector &sizes(unsigned int format) const;\n> -\tconst std::map> &data() const;\n> +\tstd::vector formats() const;\n> +\tconst std::vector &sizes(T format) const;\n> +\tconst std::map> &data() const;\n>\n> private:\n> -\tstd::map> data_;\n> +\tstd::map> data_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 9c077674f997dae6..53c73617efe4b860 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -44,7 +44,7 @@ public:\n> \tint setCrop(unsigned int pad, Rectangle *rect);\n> \tint setCompose(unsigned int pad, Rectangle *rect);\n>\n> -\tImageFormats formats(unsigned int pad);\n> +\tImageFormats formats(unsigned int pad);\n>\n> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index fcf072641420dacf..982df9d2f918e49c 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -180,7 +180,7 @@ public:\n>\n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> -\tImageFormats formats();\n> +\tImageFormats formats();\n>\n> \tint setCrop(Rectangle *rect);\n> \tint setCompose(Rectangle *rect);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index cf419e8b25a87389..8efd188e75a3d135 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -155,7 +155,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> \tif (roles.empty())\n> \t\treturn config;\n>\n> -\tImageFormats v4l2formats = data->video_->formats();\n> +\tImageFormats v4l2formats = data->video_->formats();\n> \tStreamFormats formats(v4l2formats.data());\n> \tStreamConfiguration cfg(formats);\n>\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 93d89729e1faa29f..9fbe33c626e327d4 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -175,7 +175,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \tif (roles.empty())\n> \t\treturn config;\n>\n> -\tImageFormats formats;\n> +\tImageFormats formats;\n>\n> \tfor (PixelFormat pixelformat : pixelformats) {\n> \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index f2bcd7f73c5c75a1..bec14f80c960d854 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -155,9 +155,9 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> *\n> * \\return A list of the supported device formats\n> */\n> -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> +ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> {\n> -\tImageFormats formats;\n> +\tImageFormats formats;\n>\n> \tif (pad >= entity_->pads().size()) {\n> \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 99470ce11421c77c..f84bd00570afa38c 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -837,9 +837,9 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> *\n> * \\return A list of the supported video device formats\n> */\n> -ImageFormats V4L2VideoDevice::formats()\n> +ImageFormats V4L2VideoDevice::formats()\n> {\n> -\tImageFormats formats;\n> +\tImageFormats formats;\n>\n> \tfor (unsigned int pixelformat : enumPixelformats()) {\n> \t\tstd::vector sizes = enumSizes(pixelformat);\n> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> index 067dc5ed30f4edd9..201a294c72a09b4d 100644\n> --- a/test/v4l2_subdevice/list_formats.cpp\n> +++ b/test/v4l2_subdevice/list_formats.cpp\n> @@ -47,7 +47,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 formats;\n>\n> \tformats = scaler_->formats(0);\n> \tif (formats.isEmpty()) {\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47E6F6042A\n\tfor ;\n\tFri, 28 Feb 2020 10:44:03 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id C2077E000B;\n\tFri, 28 Feb 2020 09:44:02 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Fri, 28 Feb 2020 10:46:51 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200228094651.d7she5ixweoun7xa@uno.localdomain>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gbxyqacgvlbjx7uj\"","Content-Disposition":"inline","In-Reply-To":"<20200228032913.497826-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Feb 2020 09:44:03 -0000"}},{"id":3852,"web_url":"https://patchwork.libcamera.org/comment/3852/","msgid":"<20200228113650.GB299107@oden.dyn.berto.se>","date":"2020-02-28T11:36:50","subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2020-02-28 10:46:51 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Feb 28, 2020 at 04:29:09AM +0100, Niklas Söderlund wrote:\n> > The ImageFormats class is used to carry both V4L2 pixelformats and mbus\n> > codes as keys to lookup different supported sizes. The reason for this\n> > is that both V4L2 pixelformats and mbus codes are unsigned integers and\n> > the keyword PixelFormat is currently defined as such.\n> >\n> > Going forward PixelFormat will be reworked into a class so their is a\n> > need to have ImageFormats instances with two different key types, turn\n> > it into an template and update all sites to use unsigned int where a\n> > mbus code is the key and PixelFormat otherwise.\n> >\n> \n> Sorry, but I'm not sure to follow.\n> \n> ImageFormats is said to report image formats for V4L2Device and\n> V4L2Subdevices at the moment and indeed it uses an unsigned int as key\n> which could be interpreted as either a v4l2 fourcc in case we're\n> dealing with a video device, or as an mbus media code if we're dealing\n> with a v4l2 subdev.\n> \n> PixelFormat is designed to be exposed to applications, and I would\n> really avoid create dependencies between the V4L2VideoDevice 4cc\n> codes and the application facing one.\n> \n> I agree ImageFormats is not a descriptive name, and I would be in\n> favour of renaming it to, say, DeviceFormat or whatever, but I'm not\n> sure what we gain by making v4l2 devices tied to the same class we use\n> to report image formats to application.\n> \n> Will I find it out later in the series ?\n\nYes in 3/6.\n\n> \n> Thanks\n> j\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/camera_sensor.cpp | 2 +-\n> > src/libcamera/formats.cpp | 19 +++++++++++++------\n> > src/libcamera/include/formats.h | 11 ++++++-----\n> > src/libcamera/include/v4l2_subdevice.h | 2 +-\n> > src/libcamera/include/v4l2_videodevice.h | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/v4l2_subdevice.cpp | 4 ++--\n> > src/libcamera/v4l2_videodevice.cpp | 4 ++--\n> > test/v4l2_subdevice/list_formats.cpp | 2 +-\n> > 10 files changed, 29 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a43074362834..23fdafb690117d91 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -131,7 +131,7 @@ int CameraSensor::init()\n> > \tproperties_.set(properties::Rotation, propertyValue);\n> >\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > -\tconst ImageFormats formats = subdev_->formats(0);\n> > +\tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > \t\tLOG(CameraSensor, Error) << \"No image format found\";\n> > \t\treturn -EINVAL;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 5f6552a4e06c5231..98817deee2b54c84 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -39,7 +39,8 @@ namespace libcamera {\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 &sizes)\n> > +template\n> > +int ImageFormats::addFormat(T format, const std::vector &sizes)\n> > {\n> > \tif (data_.find(format) != data_.end())\n> > \t\treturn -EEXIST;\n> > @@ -53,7 +54,8 @@ int ImageFormats::addFormat(unsigned int format, const std::vector &s\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> > +template\n> > +bool ImageFormats::isEmpty() const\n> > {\n> > \treturn data_.empty();\n> > }\n> > @@ -62,9 +64,10 @@ bool ImageFormats::isEmpty() 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 ImageFormats::formats() const\n> > +template\n> > +std::vector ImageFormats::formats() const\n> > {\n> > -\tstd::vector formats;\n> > +\tstd::vector formats;\n> > \tformats.reserve(data_.size());\n> >\n> > \t/* \\todo: Should this be cached instead of computed each time? */\n> > @@ -84,7 +87,8 @@ std::vector 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 &ImageFormats::sizes(unsigned int format) const\n> > +template\n> > +const std::vector &ImageFormats::sizes(T format) const\n> > {\n> > \tstatic const std::vector empty;\n> >\n> > @@ -99,9 +103,12 @@ const std::vector &ImageFormats::sizes(unsigned int format) 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> &ImageFormats::data() const\n> > +template\n> > +const std::map> &ImageFormats::data() const\n> > {\n> > \treturn data_;\n> > }\n> >\n> > +template class ImageFormats;\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> > index f43bc8c004f690b4..566be9c09550a830 100644\n> > --- a/src/libcamera/include/formats.h\n> > +++ b/src/libcamera/include/formats.h\n> > @@ -15,18 +15,19 @@\n> >\n> > namespace libcamera {\n> >\n> > +template\n> > class ImageFormats\n> > {\n> > public:\n> > -\tint addFormat(unsigned int format, const std::vector &sizes);\n> > +\tint addFormat(T format, const std::vector &sizes);\n> >\n> > \tbool isEmpty() const;\n> > -\tstd::vector formats() const;\n> > -\tconst std::vector &sizes(unsigned int format) const;\n> > -\tconst std::map> &data() const;\n> > +\tstd::vector formats() const;\n> > +\tconst std::vector &sizes(T format) const;\n> > +\tconst std::map> &data() const;\n> >\n> > private:\n> > -\tstd::map> data_;\n> > +\tstd::map> data_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 9c077674f997dae6..53c73617efe4b860 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -44,7 +44,7 @@ public:\n> > \tint setCrop(unsigned int pad, Rectangle *rect);\n> > \tint setCompose(unsigned int pad, Rectangle *rect);\n> >\n> > -\tImageFormats formats(unsigned int pad);\n> > +\tImageFormats formats(unsigned int pad);\n> >\n> > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index fcf072641420dacf..982df9d2f918e49c 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -180,7 +180,7 @@ public:\n> >\n> > \tint getFormat(V4L2DeviceFormat *format);\n> > \tint setFormat(V4L2DeviceFormat *format);\n> > -\tImageFormats formats();\n> > +\tImageFormats formats();\n> >\n> > \tint setCrop(Rectangle *rect);\n> > \tint setCompose(Rectangle *rect);\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index cf419e8b25a87389..8efd188e75a3d135 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -155,7 +155,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > -\tImageFormats v4l2formats = data->video_->formats();\n> > +\tImageFormats v4l2formats = data->video_->formats();\n> > \tStreamFormats formats(v4l2formats.data());\n> > \tStreamConfiguration cfg(formats);\n> >\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 93d89729e1faa29f..9fbe33c626e327d4 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -175,7 +175,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tfor (PixelFormat pixelformat : pixelformats) {\n> > \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index f2bcd7f73c5c75a1..bec14f80c960d854 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -155,9 +155,9 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> > *\n> > * \\return A list of the supported device formats\n> > */\n> > -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > +ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > {\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tif (pad >= entity_->pads().size()) {\n> > \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 99470ce11421c77c..f84bd00570afa38c 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -837,9 +837,9 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> > *\n> > * \\return A list of the supported video device formats\n> > */\n> > -ImageFormats V4L2VideoDevice::formats()\n> > +ImageFormats V4L2VideoDevice::formats()\n> > {\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tfor (unsigned int pixelformat : enumPixelformats()) {\n> > \t\tstd::vector sizes = enumSizes(pixelformat);\n> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> > index 067dc5ed30f4edd9..201a294c72a09b4d 100644\n> > --- a/test/v4l2_subdevice/list_formats.cpp\n> > +++ b/test/v4l2_subdevice/list_formats.cpp\n> > @@ -47,7 +47,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 formats;\n> >\n> > \tformats = scaler_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > --\n> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDD806272E\n\tfor ;\n\tFri, 28 Feb 2020 12:36:51 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id h18so2795390ljl.13\n\tfor ;\n\tFri, 28 Feb 2020 03:36:51 -0800 (PST)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tw1sm4729067lfe.96.2020.02.28.03.36.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 28 Feb 2020 03:36:50 -0800 (PST)"],"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=N9fZu6Oauu7EtuY6mn1FXaRHEO5EKdWFjGobRhsi+2w=;\n\tb=JTg4QP8m8JN+xBXx3n5xyVOKq+ci+AwZk6xNKzzxeMTSO/5yUwtoxMCRhVwyUpBgsR\n\tYj0AXcGw7qkHvkXBd+bQlXdYKzRb8J1SmGx5F8J5td/gRbf1A+sSJsbCSvOupDZbHdcP\n\tqkMNg9ff/9FK7+fRXF7KGVnHEmxWtQBlRtiLoPLgDeASFWzO3dQOUB1KY7YvCCPO7/lf\n\tWkZiexBspUVkadOCp7yosaV2Rwtv1irDleAdiyTELmeE16VxMSvvjEoaTdOpMxEwSIbS\n\tONfol4PUdLK0zLRehcWP4cI/dSOCxMTMMHJBHCwl74F0a+HB4ZioS9sV3R1UFm4JNnSS\n\tSZuw==","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=N9fZu6Oauu7EtuY6mn1FXaRHEO5EKdWFjGobRhsi+2w=;\n\tb=ozb/UqJ3BuCFbu5+iqbVaWeeDSirtRCfRG2w9nWpGF93IYg3FF3mQY2N6cb58AZ2ni\n\t8MHapfZELg1U+2jyEnWl99cHDIjA4+Ff1DDQySX3lcZF5j7jHosvmkXtft46FWn+cueY\n\tb5cJYP8NZF3Er05TACODNzIhkLtMZid7CL9GW/y2ZbMYGOj1TpBi3R4eFaiJMTh5uirx\n\tw62O4QqPk11XOLnZmRHdSK/KWOgYSB57JSw+qo/5TOhqqMW4qnfhem29diTtz22X6+sT\n\t9+WEx96RWoW7wOpQCW9cHyIB6z3KnYCxqt4E5f04PunfE59sSRgWt0gRctPfffOuJSRd\n\tQPjg==","X-Gm-Message-State":"ANhLgQ2fbIJbQ8NHVZLq0rW/FyljcofE0vXbYhCoCjoLFpoGT24luA4A\n\taLLMNwMlEIhjJ/MizDWArw3SXzWbQlU=","X-Google-Smtp-Source":"ADFU+vsSkXE8k68cxDyc/KvQ23+OlivQYoucfB3aqt9fmydFbThyrt2KIKy/3GtS20tqYtslhhZk6g==","X-Received":"by 2002:a2e:8e95:: with SMTP id\n\tz21mr2621436ljk.119.1582889811173; \n\tFri, 28 Feb 2020 03:36:51 -0800 (PST)","Date":"Fri, 28 Feb 2020 12:36:50 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200228113650.GB299107@oden.dyn.berto.se>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-3-niklas.soderlund@ragnatech.se>\n\t<20200228094651.d7she5ixweoun7xa@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228094651.d7she5ixweoun7xa@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Feb 2020 11:36:52 -0000"}},{"id":3855,"web_url":"https://patchwork.libcamera.org/comment/3855/","msgid":"<20200229104301.GB18738@pendragon.ideasonboard.com>","date":"2020-02-29T10:43:01","subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Feb 28, 2020 at 10:46:51AM +0100, Jacopo Mondi wrote:\n> On Fri, Feb 28, 2020 at 04:29:09AM +0100, Niklas Söderlund wrote:\n> > The ImageFormats class is used to carry both V4L2 pixelformats and mbus\n> > codes as keys to lookup different supported sizes. The reason for this\n> > is that both V4L2 pixelformats and mbus codes are unsigned integers and\n> > the keyword PixelFormat is currently defined as such.\n> >\n> > Going forward PixelFormat will be reworked into a class so their is a\n\ns/their/there/\n\n> > need to have ImageFormats instances with two different key types, turn\n> > it into an template and update all sites to use unsigned int where a\n> > mbus code is the key and PixelFormat otherwise.\n> \n> Sorry, but I'm not sure to follow.\n> \n> ImageFormats is said to report image formats for V4L2Device and\n> V4L2Subdevices at the moment and indeed it uses an unsigned int as key\n> which could be interpreted as either a v4l2 fourcc in case we're\n> dealing with a video device, or as an mbus media code if we're dealing\n> with a v4l2 subdev.\n> \n> PixelFormat is designed to be exposed to applications, and I would\n> really avoid create dependencies between the V4L2VideoDevice 4cc\n> codes and the application facing one.\n> \n> I agree ImageFormats is not a descriptive name, and I would be in\n> favour of renaming it to, say, DeviceFormat or whatever, but I'm not\n> sure what we gain by making v4l2 devices tied to the same class we use\n> to report image formats to application.\n> \n> Will I find it out later in the series ?\n\nI'm with Jacopo on this one, I don't like it much. This will essentially\nduplicate the whole class at compilation time, with lots of code that\nshouldn't depend on T. I think refactoring is needed. I'll comment\nfurther on patch 3/6.\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/camera_sensor.cpp | 2 +-\n> > src/libcamera/formats.cpp | 19 +++++++++++++------\n> > src/libcamera/include/formats.h | 11 ++++++-----\n> > src/libcamera/include/v4l2_subdevice.h | 2 +-\n> > src/libcamera/include/v4l2_videodevice.h | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/v4l2_subdevice.cpp | 4 ++--\n> > src/libcamera/v4l2_videodevice.cpp | 4 ++--\n> > test/v4l2_subdevice/list_formats.cpp | 2 +-\n> > 10 files changed, 29 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a43074362834..23fdafb690117d91 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -131,7 +131,7 @@ int CameraSensor::init()\n> > \tproperties_.set(properties::Rotation, propertyValue);\n> >\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > -\tconst ImageFormats formats = subdev_->formats(0);\n> > +\tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > \t\tLOG(CameraSensor, Error) << \"No image format found\";\n> > \t\treturn -EINVAL;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 5f6552a4e06c5231..98817deee2b54c84 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -39,7 +39,8 @@ namespace libcamera {\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 &sizes)\n> > +template\n> > +int ImageFormats::addFormat(T format, const std::vector &sizes)\n> > {\n> > \tif (data_.find(format) != data_.end())\n> > \t\treturn -EEXIST;\n> > @@ -53,7 +54,8 @@ int ImageFormats::addFormat(unsigned int format, const std::vector &s\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> > +template\n> > +bool ImageFormats::isEmpty() const\n> > {\n> > \treturn data_.empty();\n> > }\n> > @@ -62,9 +64,10 @@ bool ImageFormats::isEmpty() 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 ImageFormats::formats() const\n> > +template\n> > +std::vector ImageFormats::formats() const\n> > {\n> > -\tstd::vector formats;\n> > +\tstd::vector formats;\n> > \tformats.reserve(data_.size());\n> >\n> > \t/* \\todo: Should this be cached instead of computed each time? */\n> > @@ -84,7 +87,8 @@ std::vector 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 &ImageFormats::sizes(unsigned int format) const\n> > +template\n> > +const std::vector &ImageFormats::sizes(T format) const\n> > {\n> > \tstatic const std::vector empty;\n> >\n> > @@ -99,9 +103,12 @@ const std::vector &ImageFormats::sizes(unsigned int format) 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> &ImageFormats::data() const\n> > +template\n> > +const std::map> &ImageFormats::data() const\n> > {\n> > \treturn data_;\n> > }\n> >\n> > +template class ImageFormats;\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h\n> > index f43bc8c004f690b4..566be9c09550a830 100644\n> > --- a/src/libcamera/include/formats.h\n> > +++ b/src/libcamera/include/formats.h\n> > @@ -15,18 +15,19 @@\n> >\n> > namespace libcamera {\n> >\n> > +template\n> > class ImageFormats\n> > {\n> > public:\n> > -\tint addFormat(unsigned int format, const std::vector &sizes);\n> > +\tint addFormat(T format, const std::vector &sizes);\n> >\n> > \tbool isEmpty() const;\n> > -\tstd::vector formats() const;\n> > -\tconst std::vector &sizes(unsigned int format) const;\n> > -\tconst std::map> &data() const;\n> > +\tstd::vector formats() const;\n> > +\tconst std::vector &sizes(T format) const;\n> > +\tconst std::map> &data() const;\n> >\n> > private:\n> > -\tstd::map> data_;\n> > +\tstd::map> data_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 9c077674f997dae6..53c73617efe4b860 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -44,7 +44,7 @@ public:\n> > \tint setCrop(unsigned int pad, Rectangle *rect);\n> > \tint setCompose(unsigned int pad, Rectangle *rect);\n> >\n> > -\tImageFormats formats(unsigned int pad);\n> > +\tImageFormats formats(unsigned int pad);\n> >\n> > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index fcf072641420dacf..982df9d2f918e49c 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -180,7 +180,7 @@ public:\n> >\n> > \tint getFormat(V4L2DeviceFormat *format);\n> > \tint setFormat(V4L2DeviceFormat *format);\n> > -\tImageFormats formats();\n> > +\tImageFormats formats();\n> >\n> > \tint setCrop(Rectangle *rect);\n> > \tint setCompose(Rectangle *rect);\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index cf419e8b25a87389..8efd188e75a3d135 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -155,7 +155,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > -\tImageFormats v4l2formats = data->video_->formats();\n> > +\tImageFormats v4l2formats = data->video_->formats();\n> > \tStreamFormats formats(v4l2formats.data());\n> > \tStreamConfiguration cfg(formats);\n> >\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 93d89729e1faa29f..9fbe33c626e327d4 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -175,7 +175,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tfor (PixelFormat pixelformat : pixelformats) {\n> > \t\t/* The scaler hardcodes a x3 scale-up ratio. */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index f2bcd7f73c5c75a1..bec14f80c960d854 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -155,9 +155,9 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> > *\n> > * \\return A list of the supported device formats\n> > */\n> > -ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > +ImageFormats V4L2Subdevice::formats(unsigned int pad)\n> > {\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tif (pad >= entity_->pads().size()) {\n> > \t\tLOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 99470ce11421c77c..f84bd00570afa38c 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -837,9 +837,9 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> > *\n> > * \\return A list of the supported video device formats\n> > */\n> > -ImageFormats V4L2VideoDevice::formats()\n> > +ImageFormats V4L2VideoDevice::formats()\n> > {\n> > -\tImageFormats formats;\n> > +\tImageFormats formats;\n> >\n> > \tfor (unsigned int pixelformat : enumPixelformats()) {\n> > \t\tstd::vector sizes = enumSizes(pixelformat);\n> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp\n> > index 067dc5ed30f4edd9..201a294c72a09b4d 100644\n> > --- a/test/v4l2_subdevice/list_formats.cpp\n> > +++ b/test/v4l2_subdevice/list_formats.cpp\n> > @@ -47,7 +47,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 formats;\n> >\n> > \tformats = scaler_->formats(0);\n> > \tif (formats.isEmpty()) {","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F045B60429\n\tfor ;\n\tSat, 29 Feb 2020 11:43:24 +0100 (CET)","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 63FA3734;\n\tSat, 29 Feb 2020 11:43:24 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582973004;\n\tbh=/0aAGJJDBnYRWc/yUM/zQbzOJ2c7v0auPTzfhu+/K+c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pmyFUDQGF4G1t1yGbkYAt8CZGQ6UZeJGP5b+NYpAx0gSODzwX1rSwgrtupcf1QqPh\n\tMu4cc6tFD5Da/+KTs+EYVJQX8Y9Lk+Kpnzpl70wQVwj7lRBy3pTxca7wx2XM3KcdHi\n\tZNzgPgiwVOSN46lDu+mEtMx+VkfDvyMJ94LjKgMM=","Date":"Sat, 29 Feb 2020 12:43:01 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200229104301.GB18738@pendragon.ideasonboard.com>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-3-niklas.soderlund@ragnatech.se>\n\t<20200228094651.d7she5ixweoun7xa@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228094651.d7she5ixweoun7xa@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/6] libcamera: formats: Turn\n\tImageFormats into a template","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Feb 2020 10:43:25 -0000"}}]