[{"id":970,"web_url":"https://patchwork.libcamera.org/comment/970/","msgid":"<20190228220350.GM7811@pendragon.ideasonboard.com>","date":"2019-02-28T22:03:50","subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:\n> Implement enumFormat() methods to enumerate the available image\n> resolutions on the subdevice.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/geometry.cpp | 34 +++++++++++\n> src/libcamera/include/geometry.h | 12 ++++\n> src/libcamera/include/v4l2_subdevice.h | 11 +++-\n> src/libcamera/v4l2_subdevice.cpp | 84 ++++++++++++++++++++++++++\n> 4 files changed, 139 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 57f4fc7716d9..f41f6975d4ce 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -46,4 +46,38 @@ namespace libcamera {\n> * \\brief The distance between the top and bottom sides\n> */\n> \n> +/**\n> + * \\struct SizeRange\n> + * \\brief Describe a range of image sizes\n> + *\n> + * SizeRange describes a range of image sizes included in the (maxWidth,\n> + * maxHeight) - (minWidth, minHeight) interval. If the minimum and\n\nCould you swap min and max ? Intervals are usually represented with\nminimum first.\n\n> + * maximum sizes are identical it represents a single image resolution.\n> + */\n> +\n> +/**\n> + * \\fn SizeRange::SizeRange()\n> + * \\brief Construct a size range\n> + */\n> +\n> +/**\n> + * \\var SizeRange::minWidth\n> + * \\brief The minimum image width\n> + */\n> +\n> +/**\n> + * \\var SizeRange::minHeight\n> + * \\brief The minimum image height\n> + */\n> +\n> +/**\n> + * \\var SizeRange::maxWidth\n> + * \\brief The maximum image width\n> + */\n> +\n> +/**\n> + * \\var SizeRange::maxHeight\n> + * \\brief The maximum image height\n> + */\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h\n> index cc146da7cb0d..eadc4ed4f9cb 100644\n> --- a/src/libcamera/include/geometry.h\n> +++ b/src/libcamera/include/geometry.h\n> @@ -17,6 +17,18 @@ struct Rectangle {\n> \tunsigned int h;\n> };\n> \n> +struct SizeRange {\n> +\tSizeRange(unsigned int minW, unsigned int minH,\n> +\t\t unsigned int maxW, unsigned int maxH)\n> +\t\t: minWidth(minW), minHeight(minH), maxWidth(maxW),\n> +\t\t maxHeight(maxH) {}\n\nDo we need a constructor ?\n\n> +\n> +\tunsigned int minWidth;\n> +\tunsigned int minHeight;\n> +\tunsigned int maxWidth;\n> +\tunsigned int maxHeight;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_GEOMETRY_H__ */\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 669e79f9a9fd..aa7451e86962 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -7,15 +7,16 @@\n> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> \n> +#include \n> #include \n> +#include \n> \n> +#include \"geometry.h\"\n> #include \"log.h\"\n> #include \"media_object.h\"\n> \n> namespace libcamera {\n> \n> -struct Rectangle;\n> -\n> struct V4L2SubdeviceFormat {\n> \tuint32_t mbus_code;\n> \tuint32_t width;\n> @@ -39,6 +40,9 @@ public:\n> \tint setCrop(unsigned int pad, Rectangle *rect);\n> \tint setCompose(unsigned int pad, Rectangle *rect);\n> \n> +\tconst std::map>\n> +\t\t\t\t\t\tformats(unsigned int pad);\n> +\n> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> \n> @@ -46,6 +50,9 @@ protected:\n> \tstd::string logPrefix() const;\n> \n> private:\n> +\tstd::vector enumPadSizes(unsigned int pad,\n> +\t\t\t\t\t unsigned int code);\n> +\n> \tint setSelection(unsigned int pad, unsigned int target,\n> \t\t\t Rectangle *rect);\n> \n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 4fe59c45f000..7f191e072c61 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> \treturn setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);\n> }\n> \n> +/**\n> + * \\brief List the sub-device image resolutions and formats on \\a pad\n> + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> + *\n> + * Retrieve a list of image formats and sizes on the \\a pad of a video\n> + * subdevice. Subdevices are free to report a list of discrete sizes they\n\ns/ Subdevices are free to report/ Subdevices can report either/\n\n> + * support, or a single size interval expressed as a [min-max] sizes range.\n\nCan't subdevices report a list of intervals ?\n\n> + *\n> + * Each image size list is associated with a media bus pixel code for which\n> + * the reported resolutions are supported.\n> + *\n> + * \\return A, map of image formats associated with a list of image sizes, or\n\ns/A, /A /\n\n> + * an empty map on error or if the pad does not exists\n\ns/exists/exist/\n\n> + * \\sa SizeRange\n\nI don't think you need this, doesn't Doxygen link to SizeRange as it is\nused in the return type.\n\n> + */\n> +const std::map>\n> +V4L2Subdevice::formats(unsigned int pad)\n> +{\n> +\tstd::map> formatMap = {};\n> +\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> +\tint ret;\n> +\n> +\tif (pad > entity_->pads().size()) {\n\nShouldn't this be >= ?\n\n> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Format enumeration required on a non-existing pad: \"\n\nDid you mean \"requested\" ? How about just \"Invalid pad \" << pad (as the\nlog message will contain the function name already) ?\n\n> +\t\t\t<< pad;\n> +\t\treturn formatMap;\n> +\t}\n> +\n> +\tmbusEnum.pad = pad;\n> +\tmbusEnum.index = 0;\n> +\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> +\twhile (true) {\n> +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\n> +\t\tformatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);\n> +\n> +\t\tmbusEnum.index++;\n> +\t}\n> +\n> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n\n\t\tret = -errnor;\n\nand use strerror(-ret).\n\n> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Unable to enumerate format on pad \" << pad\n\ns/format/formats/\n\n> +\t\t\t<< \": \" << strerror(errno);\n> +\t\tformatMap.clear();\n> +\t}\n> +\n> +\treturn formatMap;\n> +}\n> +\n> /**\n> * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> \treturn 0;\n> }\n> \n> +std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> +\t\t\t\t\t\t unsigned int code)\n> +{\n> +\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> +\tstd::vector sizes = {};\n> +\tint ret;\n> +\n> +\tsizeEnum.index = 0;\n> +\tsizeEnum.pad = pad;\n> +\tsizeEnum.code = code;\n> +\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> +\twhile (true) {\n> +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\n> +\t\tsizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,\n> +\t\t\t\t sizeEnum.max_width, sizeEnum.max_height);\n\nIf you used\n\n\t\tsizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,\n\t\t\t\t sizeEnum.max_width, sizeEnum.max_height });\n\nI think you could remove the SizeRange constructor.\n\n> +\n> +\t\tsizeEnum.index++;\n> +\t}\n> +\n> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n\nSame errno dance here.\n\n> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> +\t\t\t<< \": \" << strerror(errno);\n> +\t\tsizes.clear();\n> +\t}\n> +\n> +\treturn sizes;\n> +}\n> +\n> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \t\t\t\tRectangle *rect)\n> {","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 E26F2610B6\n\tfor ;\n\tThu, 28 Feb 2019 23:03:55 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66B8349;\n\tThu, 28 Feb 2019 23:03:55 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551391435;\n\tbh=OUKFWoetw7ySN8T1H6PrTVTPntWgATG8Mpa2BkmPW9k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kpXyH22P9QsWGvJ7qckvVsgNvyTyox3pDiagzoymi4Z93J+yw4gTJbbLKev5IO32v\n\tjIqLPbme4OP9QHPLJW9IyFBfk0rZv79ZZCMCLW3ZG7Xvv7HPjwuZ8ct1cJ4VfesL5+\n\t9V7/hyVLuRr35eYZHFtWIbncPB6NlPyzpgMIAA9c=","Date":"Fri, 1 Mar 2019 00:03:50 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228220350.GM7811@pendragon.ideasonboard.com>","References":"<20190228200151.2948-1-jacopo@jmondi.org>\n\t<20190228200151.2948-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190228200151.2948-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 22:03:56 -0000"}},{"id":979,"web_url":"https://patchwork.libcamera.org/comment/979/","msgid":"<20190301091624.pjhz7zeh22ulfhpv@uno.localdomain>","date":"2019-03-01T09:16:24","subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for your comments\n\nOn Fri, Mar 01, 2019 at 12:03:50AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:\n> > Implement enumFormat() methods to enumerate the available image\n> > resolutions on the subdevice.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/geometry.cpp | 34 +++++++++++\n> > src/libcamera/include/geometry.h | 12 ++++\n> > src/libcamera/include/v4l2_subdevice.h | 11 +++-\n> > src/libcamera/v4l2_subdevice.cpp | 84 ++++++++++++++++++++++++++\n> > 4 files changed, 139 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index 57f4fc7716d9..f41f6975d4ce 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -46,4 +46,38 @@ namespace libcamera {\n> > * \\brief The distance between the top and bottom sides\n> > */\n> >\n> > +/**\n> > + * \\struct SizeRange\n> > + * \\brief Describe a range of image sizes\n> > + *\n> > + * SizeRange describes a range of image sizes included in the (maxWidth,\n> > + * maxHeight) - (minWidth, minHeight) interval. If the minimum and\n>\n> Could you swap min and max ? Intervals are usually represented with\n> minimum first.\n\nYes, I initially had it like this, then I swapped it to (max - min) as\nit would represent a differnce between two sizes, but I agree it is\nmore canonically seen with min first.\n\n>\n> > + * maximum sizes are identical it represents a single image resolution.\n> > + */\n> > +\n> > +/**\n> > + * \\fn SizeRange::SizeRange()\n> > + * \\brief Construct a size range\n> > + */\n> > +\n> > +/**\n> > + * \\var SizeRange::minWidth\n> > + * \\brief The minimum image width\n> > + */\n> > +\n> > +/**\n> > + * \\var SizeRange::minHeight\n> > + * \\brief The minimum image height\n> > + */\n> > +\n> > +/**\n> > + * \\var SizeRange::maxWidth\n> > + * \\brief The maximum image width\n> > + */\n> > +\n> > +/**\n> > + * \\var SizeRange::maxHeight\n> > + * \\brief The maximum image height\n> > + */\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h\n> > index cc146da7cb0d..eadc4ed4f9cb 100644\n> > --- a/src/libcamera/include/geometry.h\n> > +++ b/src/libcamera/include/geometry.h\n> > @@ -17,6 +17,18 @@ struct Rectangle {\n> > \tunsigned int h;\n> > };\n> >\n> > +struct SizeRange {\n> > +\tSizeRange(unsigned int minW, unsigned int minH,\n> > +\t\t unsigned int maxW, unsigned int maxH)\n> > +\t\t: minWidth(minW), minHeight(minH), maxWidth(maxW),\n> > +\t\t maxHeight(maxH) {}\n>\n> Do we need a constructor ?\n>\n\nIf I use emplace_back like I did yes. With your below suggestion\nprobably not.\n\n> > +\n> > +\tunsigned int minWidth;\n> > +\tunsigned int minHeight;\n> > +\tunsigned int maxWidth;\n> > +\tunsigned int maxHeight;\n> > +};\n> > +\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_GEOMETRY_H__ */\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 669e79f9a9fd..aa7451e86962 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -7,15 +7,16 @@\n> > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> >\n> > +#include \n> > #include \n> > +#include \n> >\n> > +#include \"geometry.h\"\n> > #include \"log.h\"\n> > #include \"media_object.h\"\n> >\n> > namespace libcamera {\n> >\n> > -struct Rectangle;\n> > -\n> > struct V4L2SubdeviceFormat {\n> > \tuint32_t mbus_code;\n> > \tuint32_t width;\n> > @@ -39,6 +40,9 @@ public:\n> > \tint setCrop(unsigned int pad, Rectangle *rect);\n> > \tint setCompose(unsigned int pad, Rectangle *rect);\n> >\n> > +\tconst std::map>\n> > +\t\t\t\t\t\tformats(unsigned int pad);\n> > +\n> > \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> >\n> > @@ -46,6 +50,9 @@ protected:\n> > \tstd::string logPrefix() const;\n> >\n> > private:\n> > +\tstd::vector enumPadSizes(unsigned int pad,\n> > +\t\t\t\t\t unsigned int code);\n> > +\n> > \tint setSelection(unsigned int pad, unsigned int target,\n> > \t\t\t Rectangle *rect);\n> >\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 4fe59c45f000..7f191e072c61 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> > \treturn setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);\n> > }\n> >\n> > +/**\n> > + * \\brief List the sub-device image resolutions and formats on \\a pad\n> > + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > + *\n> > + * Retrieve a list of image formats and sizes on the \\a pad of a video\n> > + * subdevice. Subdevices are free to report a list of discrete sizes they\n>\n> s/ Subdevices are free to report/ Subdevices can report either/\n>\n> > + * support, or a single size interval expressed as a [min-max] sizes range.\n>\n> Can't subdevices report a list of intervals ?\n>\n\nPossibly, yes, I'll change this.\n\n> > + *\n> > + * Each image size list is associated with a media bus pixel code for which\n> > + * the reported resolutions are supported.\n> > + *\n> > + * \\return A, map of image formats associated with a list of image sizes, or\n>\n> s/A, /A /\n>\n> > + * an empty map on error or if the pad does not exists\n>\n> s/exists/exist/\n>\n> > + * \\sa SizeRange\n>\n> I don't think you need this, doesn't Doxygen link to SizeRange as it is\n> used in the return type.\n>\n> > + */\n> > +const std::map>\n> > +V4L2Subdevice::formats(unsigned int pad)\n> > +{\n> > +\tstd::map> formatMap = {};\n> > +\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> > +\tint ret;\n> > +\n> > +\tif (pad > entity_->pads().size()) {\n>\n> Shouldn't this be >= ?\n>\n\nUps, sorry, I had a different check here and forgot to update this.\n\n\n> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Format enumeration required on a non-existing pad: \"\n>\n> Did you mean \"requested\" ? How about just \"Invalid pad \" << pad (as the\n> log message will contain the function name already) ?\n>\n> > +\t\t\t<< pad;\n> > +\t\treturn formatMap;\n> > +\t}\n> > +\n> > +\tmbusEnum.pad = pad;\n> > +\tmbusEnum.index = 0;\n> > +\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > +\twhile (true) {\n> > +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n> > +\t\tif (ret)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tformatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);\n> > +\n> > +\t\tmbusEnum.index++;\n> > +\t}\n> > +\n> > +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n>\n> \t\tret = -errnor;\n>\n> and use strerror(-ret).\n>\n> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Unable to enumerate format on pad \" << pad\n>\n> s/format/formats/\n>\n> > +\t\t\t<< \": \" << strerror(errno);\n> > +\t\tformatMap.clear();\n> > +\t}\n> > +\n> > +\treturn formatMap;\n> > +}\n> > +\n> > /**\n> > * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> > \treturn 0;\n> > }\n> >\n> > +std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > +\t\t\t\t\t\t unsigned int code)\n> > +{\n> > +\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> > +\tstd::vector sizes = {};\n> > +\tint ret;\n> > +\n> > +\tsizeEnum.index = 0;\n> > +\tsizeEnum.pad = pad;\n> > +\tsizeEnum.code = code;\n> > +\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > +\twhile (true) {\n> > +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n> > +\t\tif (ret)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tsizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,\n> > +\t\t\t\t sizeEnum.max_width, sizeEnum.max_height);\n>\n> If you used\n>\n> \t\tsizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,\n> \t\t\t\t sizeEnum.max_width, sizeEnum.max_height });\n>\n> I think you could remove the SizeRange constructor.\n\nI'll try, but are you sure you're not constructing a SizeRange item\nand copying it in the vector in this way, instead of letting the\nemplace_back method construct it?\n\n>\n> > +\n> > +\t\tsizeEnum.index++;\n> > +\t}\n> > +\n> > +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n>\n> Same errno dance here.\n\nYes, thank you.\n\nOnce clarified the emplace_back() thing, I'll re-submit.\n\n>\n> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> > +\t\t\t<< \": \" << strerror(errno);\n> > +\t\tsizes.clear();\n> > +\t}\n> > +\n> > +\treturn sizes;\n> > +}\n> > +\n> > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \t\t\t\tRectangle *rect)\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6BD8610B2\n\tfor ;\n\tFri, 1 Mar 2019 10:15:54 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 346952000A;\n\tFri, 1 Mar 2019 09:15:53 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 1 Mar 2019 10:16:24 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301091624.pjhz7zeh22ulfhpv@uno.localdomain>","References":"<20190228200151.2948-1-jacopo@jmondi.org>\n\t<20190228200151.2948-4-jacopo@jmondi.org>\n\t<20190228220350.GM7811@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"zksz7tsmj5ldgodk\"","Content-Disposition":"inline","In-Reply-To":"<20190228220350.GM7811@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 01 Mar 2019 09:15:54 -0000"}},{"id":985,"web_url":"https://patchwork.libcamera.org/comment/985/","msgid":"<20190301100105.GB25460@pendragon.ideasonboard.com>","date":"2019-03-01T10:01:05","subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Mar 01, 2019 at 10:16:24AM +0100, Jacopo Mondi wrote:\n> On Fri, Mar 01, 2019 at 12:03:50AM +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 28, 2019 at 09:01:45PM +0100, Jacopo Mondi wrote:\n> >> Implement enumFormat() methods to enumerate the available image\n> >> resolutions on the subdevice.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/geometry.cpp | 34 +++++++++++\n> >> src/libcamera/include/geometry.h | 12 ++++\n> >> src/libcamera/include/v4l2_subdevice.h | 11 +++-\n> >> src/libcamera/v4l2_subdevice.cpp | 84 ++++++++++++++++++++++++++\n> >> 4 files changed, 139 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> >> index 57f4fc7716d9..f41f6975d4ce 100644\n> >> --- a/src/libcamera/geometry.cpp\n> >> +++ b/src/libcamera/geometry.cpp\n> >> @@ -46,4 +46,38 @@ namespace libcamera {\n> >> * \\brief The distance between the top and bottom sides\n> >> */\n> >>\n> >> +/**\n> >> + * \\struct SizeRange\n> >> + * \\brief Describe a range of image sizes\n> >> + *\n> >> + * SizeRange describes a range of image sizes included in the (maxWidth,\n> >> + * maxHeight) - (minWidth, minHeight) interval. If the minimum and\n> >\n> > Could you swap min and max ? Intervals are usually represented with\n> > minimum first.\n> \n> Yes, I initially had it like this, then I swapped it to (max - min) as\n> it would represent a differnce between two sizes, but I agree it is\n> more canonically seen with min first.\n> \n> >> + * maximum sizes are identical it represents a single image resolution.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn SizeRange::SizeRange()\n> >> + * \\brief Construct a size range\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var SizeRange::minWidth\n> >> + * \\brief The minimum image width\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var SizeRange::minHeight\n> >> + * \\brief The minimum image height\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var SizeRange::maxWidth\n> >> + * \\brief The maximum image width\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var SizeRange::maxHeight\n> >> + * \\brief The maximum image height\n> >> + */\n> >> +\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/include/geometry.h b/src/libcamera/include/geometry.h\n> >> index cc146da7cb0d..eadc4ed4f9cb 100644\n> >> --- a/src/libcamera/include/geometry.h\n> >> +++ b/src/libcamera/include/geometry.h\n> >> @@ -17,6 +17,18 @@ struct Rectangle {\n> >> \tunsigned int h;\n> >> };\n> >>\n> >> +struct SizeRange {\n> >> +\tSizeRange(unsigned int minW, unsigned int minH,\n> >> +\t\t unsigned int maxW, unsigned int maxH)\n> >> +\t\t: minWidth(minW), minHeight(minH), maxWidth(maxW),\n> >> +\t\t maxHeight(maxH) {}\n> >\n> > Do we need a constructor ?\n> \n> If I use emplace_back like I did yes. With your below suggestion\n> probably not.\n> \n> >> +\n> >> +\tunsigned int minWidth;\n> >> +\tunsigned int minHeight;\n> >> +\tunsigned int maxWidth;\n> >> +\tunsigned int maxHeight;\n> >> +};\n> >> +\n> >> } /* namespace libcamera */\n> >>\n> >> #endif /* __LIBCAMERA_GEOMETRY_H__ */\n> >> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> >> index 669e79f9a9fd..aa7451e86962 100644\n> >> --- a/src/libcamera/include/v4l2_subdevice.h\n> >> +++ b/src/libcamera/include/v4l2_subdevice.h\n> >> @@ -7,15 +7,16 @@\n> >> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> >> #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> >>\n> >> +#include \n> >> #include \n> >> +#include \n> >>\n> >> +#include \"geometry.h\"\n> >> #include \"log.h\"\n> >> #include \"media_object.h\"\n> >>\n> >> namespace libcamera {\n> >>\n> >> -struct Rectangle;\n> >> -\n> >> struct V4L2SubdeviceFormat {\n> >> \tuint32_t mbus_code;\n> >> \tuint32_t width;\n> >> @@ -39,6 +40,9 @@ public:\n> >> \tint setCrop(unsigned int pad, Rectangle *rect);\n> >> \tint setCompose(unsigned int pad, Rectangle *rect);\n> >>\n> >> +\tconst std::map>\n> >> +\t\t\t\t\t\tformats(unsigned int pad);\n> >> +\n> >> \tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> >> \tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> >>\n> >> @@ -46,6 +50,9 @@ protected:\n> >> \tstd::string logPrefix() const;\n> >>\n> >> private:\n> >> +\tstd::vector enumPadSizes(unsigned int pad,\n> >> +\t\t\t\t\t unsigned int code);\n> >> +\n> >> \tint setSelection(unsigned int pad, unsigned int target,\n> >> \t\t\t Rectangle *rect);\n> >>\n> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >> index 4fe59c45f000..7f191e072c61 100644\n> >> --- a/src/libcamera/v4l2_subdevice.cpp\n> >> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >> @@ -183,6 +183,58 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)\n> >> \treturn setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);\n> >> }\n> >>\n> >> +/**\n> >> + * \\brief List the sub-device image resolutions and formats on \\a pad\n> >> + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> >> + *\n> >> + * Retrieve a list of image formats and sizes on the \\a pad of a video\n> >> + * subdevice. Subdevices are free to report a list of discrete sizes they\n> >\n> > s/ Subdevices are free to report/ Subdevices can report either/\n> >\n> >> + * support, or a single size interval expressed as a [min-max] sizes range.\n> >\n> > Can't subdevices report a list of intervals ?\n> >\n> \n> Possibly, yes, I'll change this.\n> \n> >> + *\n> >> + * Each image size list is associated with a media bus pixel code for which\n> >> + * the reported resolutions are supported.\n> >> + *\n> >> + * \\return A, map of image formats associated with a list of image sizes, or\n> >\n> > s/A, /A /\n> >\n> >> + * an empty map on error or if the pad does not exists\n> >\n> > s/exists/exist/\n> >\n> >> + * \\sa SizeRange\n> >\n> > I don't think you need this, doesn't Doxygen link to SizeRange as it is\n> > used in the return type.\n> >\n> >> + */\n> >> +const std::map>\n> >> +V4L2Subdevice::formats(unsigned int pad)\n> >> +{\n> >> +\tstd::map> formatMap = {};\n> >> +\tstruct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> >> +\tint ret;\n> >> +\n> >> +\tif (pad > entity_->pads().size()) {\n> >\n> > Shouldn't this be >= ?\n> >\n> \n> Ups, sorry, I had a different check here and forgot to update this.\n> \n> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Format enumeration required on a non-existing pad: \"\n> >\n> > Did you mean \"requested\" ? How about just \"Invalid pad \" << pad (as the\n> > log message will contain the function name already) ?\n> >\n> >> +\t\t\t<< pad;\n> >> +\t\treturn formatMap;\n> >> +\t}\n> >> +\n> >> +\tmbusEnum.pad = pad;\n> >> +\tmbusEnum.index = 0;\n> >> +\tmbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >> +\twhile (true) {\n> >> +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);\n> >> +\t\tif (ret)\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tformatMap[mbusEnum.code] = enumPadSizes(pad, mbusEnum.code);\n> >> +\n> >> +\t\tmbusEnum.index++;\n> >> +\t}\n> >> +\n> >> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> >\n> > \t\tret = -errnor;\n> >\n> > and use strerror(-ret).\n> >\n> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Unable to enumerate format on pad \" << pad\n> >\n> > s/format/formats/\n> >\n> >> +\t\t\t<< \": \" << strerror(errno);\n> >> +\t\tformatMap.clear();\n> >> +\t}\n> >> +\n> >> +\treturn formatMap;\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> >> * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> >> @@ -248,6 +300,38 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> >> \treturn 0;\n> >> }\n> >>\n> >> +std::vector V4L2Subdevice::enumPadSizes(unsigned int pad,\n> >> +\t\t\t\t\t\t unsigned int code)\n> >> +{\n> >> +\tstruct v4l2_subdev_frame_size_enum sizeEnum = {};\n> >> +\tstd::vector sizes = {};\n> >> +\tint ret;\n> >> +\n> >> +\tsizeEnum.index = 0;\n> >> +\tsizeEnum.pad = pad;\n> >> +\tsizeEnum.code = code;\n> >> +\tsizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >> +\twhile (true) {\n> >> +\t\tret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);\n> >> +\t\tif (ret)\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tsizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,\n> >> +\t\t\t\t sizeEnum.max_width, sizeEnum.max_height);\n> >\n> > If you used\n> >\n> > \t\tsizes.emplace_back({ sizeEnum.min_width, sizeEnum.min_height,\n> > \t\t\t\t sizeEnum.max_width, sizeEnum.max_height });\n> >\n> > I think you could remove the SizeRange constructor.\n> \n> I'll try, but are you sure you're not constructing a SizeRange item\n> and copying it in the vector in this way, instead of letting the\n> emplace_back method construct it?\n\nYou're right, it doesn't work. Let's keep the constructor then.\n\n> >> +\n> >> +\t\tsizeEnum.index++;\n> >> +\t}\n> >> +\n> >> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> >\n> > Same errno dance here.\n> \n> Yes, thank you.\n> \n> Once clarified the emplace_back() thing, I'll re-submit.\n> \n> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> >> +\t\t\t<< \": \" << strerror(errno);\n> >> +\t\tsizes.clear();\n> >> +\t}\n> >> +\n> >> +\treturn sizes;\n> >> +}\n> >> +\n> >> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >> \t\t\t\tRectangle *rect)\n> >> {","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D405F610B2\n\tfor ;\n\tFri, 1 Mar 2019 11:01:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33C1249;\n\tFri, 1 Mar 2019 11:01:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551434471;\n\tbh=Oq0LfBVGKZBaPy2S+1m5ELnwKmDIym+9tO0RAByZAzY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PwANHu8xAtFrYq044ZFWxM9MX4YCuQgIkMjrcR+WawXOKVisgEWDPPt1m+i3Bigx/\n\t3lp+c2LNuCR6je2g+qiSgGz59BaIwB07mKY10kt6JE9j8OHTrOqVNjQ2UoYz2nqviI\n\t0po2rB2rLDRy5kuTXfi3uV01pb8lwAJZiEYRlqTk=","Date":"Fri, 1 Mar 2019 12:01:05 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301100105.GB25460@pendragon.ideasonboard.com>","References":"<20190228200151.2948-1-jacopo@jmondi.org>\n\t<20190228200151.2948-4-jacopo@jmondi.org>\n\t<20190228220350.GM7811@pendragon.ideasonboard.com>\n\t<20190301091624.pjhz7zeh22ulfhpv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190301091624.pjhz7zeh22ulfhpv@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 3/9] libcamera: v4l2_subdevice:\n\tImplement ENUM_FRAME_SIZES","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 01 Mar 2019 10:01:12 -0000"}}]