[{"id":989,"web_url":"https://patchwork.libcamera.org/comment/989/","msgid":"<20190301124240.GE26239@pendragon.ideasonboard.com>","date":"2019-03-01T12:42:40","subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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 Fri, Mar 01, 2019 at 12:51:35PM +0100, Jacopo Mondi wrote:\n> Implement format and size enumeration methods to list all the available\n> subdevice image resolutions and formats.\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..b6b6592bdfec 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 (minWidth,\n> + * minHeight) - (maxWidth, maxHeight) interval. If the minimum and\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> +\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 87377762664d..07035886c766 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -183,6 +183,57 @@ 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 can report either a list of discrete sizes they\n> + * support or a list of intervals expressed as a [min-max] sizes range.\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> + * an empty map on error or if the pad does not exist\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> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Invalid pad: \" << pad;\n\nThis would hold on a single line.\n\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\nThis won't make it possible to pass an error from\nVIDIOC_SUBDEV_ENUM_FRAME_SIZE to the caller. How about making\nenumPadSizes() return an int and take a pointer to the sizes vector ?\n\nWith those changes,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\t\tmbusEnum.index++;\n> +\t}\n> +\n> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> +\t\t\t<< \": \" << strerror(-ret);\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 +299,39 @@ 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> +\t\tsizeEnum.index++;\n> +\t}\n> +\n> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2Subdev, Error)\n> +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> +\t\t\t<< \": \" << strerror(-ret);\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 26159610BF\n\tfor ;\n\tFri, 1 Mar 2019 13:42:47 +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 313B149;\n\tFri, 1 Mar 2019 13:42:46 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551444166;\n\tbh=BZKA/8KlpvT5L2ozGZGsdqCYFQ75yCReGq4IvzVJBz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uewKtlUWc+m1O9y4oEbkfRsP2suyG9x/ExKXjgZoi9+bZwHnH2MQIH+64ONUqo7hg\n\tpvz+Ct6rH/Yvw93FuLGdXmfuP5ez2u1gdes6j9LU8H6AfuPA3muCtBQjduP/FIzZ+X\n\tWTH3IobiJhaqN6Ngtb2ZriMymznWG+8XC1U+8t6g=","Date":"Fri, 1 Mar 2019 14:42:40 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301124240.GE26239@pendragon.ideasonboard.com>","References":"<20190301115139.11060-1-jacopo@jmondi.org>\n\t<20190301115139.11060-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190301115139.11060-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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 12:42:47 -0000"}},{"id":992,"web_url":"https://patchwork.libcamera.org/comment/992/","msgid":"<20190301125510.l3oxwztttcs274ch@uno.localdomain>","date":"2019-03-01T12:55:10","subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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\nOn Fri, Mar 01, 2019 at 02:42:40PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Mar 01, 2019 at 12:51:35PM +0100, Jacopo Mondi wrote:\n> > Implement format and size enumeration methods to list all the available\n> > subdevice image resolutions and formats.\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..b6b6592bdfec 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 (minWidth,\n> > + * minHeight) - (maxWidth, maxHeight) interval. If the minimum and\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> > +\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 87377762664d..07035886c766 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -183,6 +183,57 @@ 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 can report either a list of discrete sizes they\n> > + * support or a list of intervals expressed as a [min-max] sizes range.\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> > + * an empty map on error or if the pad does not exist\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> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Invalid pad: \" << pad;\n>\n> This would hold on a single line.\n>\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> This won't make it possible to pass an error from\n> VIDIOC_SUBDEV_ENUM_FRAME_SIZE to the caller. How about making\n> enumPadSizes() return an int and take a pointer to the sizes vector ?\n\nI really struggled here, I wanted to keep the error visible, but also\nexploit RVO, which I presume is here used.\n\nOn error, we would return an empty size list anyone, is it worth\nreturning an int?\n\n>\n> With those changes,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\n> > +\t\tmbusEnum.index++;\n> > +\t}\n> > +\n> > +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> > +\t\t\t<< \": \" << strerror(-ret);\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 +299,39 @@ 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> > +\t\tsizeEnum.index++;\n> > +\t}\n> > +\n> > +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(V4L2Subdev, Error)\n> > +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> > +\t\t\t<< \": \" << strerror(-ret);\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 relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDAAC610BF\n\tfor ;\n\tFri, 1 Mar 2019 13:54:40 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 4C70E100005;\n\tFri, 1 Mar 2019 12:54:40 +0000 (UTC)"],"Date":"Fri, 1 Mar 2019 13:55:10 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301125510.l3oxwztttcs274ch@uno.localdomain>","References":"<20190301115139.11060-1-jacopo@jmondi.org>\n\t<20190301115139.11060-2-jacopo@jmondi.org>\n\t<20190301124240.GE26239@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yezmysk3acuiwqwn\"","Content-Disposition":"inline","In-Reply-To":"<20190301124240.GE26239@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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 12:54:41 -0000"}},{"id":996,"web_url":"https://patchwork.libcamera.org/comment/996/","msgid":"<20190301130415.GA32244@pendragon.ideasonboard.com>","date":"2019-03-01T13:04:15","subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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 01:55:10PM +0100, Jacopo Mondi wrote:\n> On Fri, Mar 01, 2019 at 02:42:40PM +0200, Laurent Pinchart wrote:\n> > On Fri, Mar 01, 2019 at 12:51:35PM +0100, Jacopo Mondi wrote:\n> >> Implement format and size enumeration methods to list all the available\n> >> subdevice image resolutions and formats.\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..b6b6592bdfec 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 (minWidth,\n> >> + * minHeight) - (maxWidth, maxHeight) interval. If the minimum and\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> >> +\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 87377762664d..07035886c766 100644\n> >> --- a/src/libcamera/v4l2_subdevice.cpp\n> >> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >> @@ -183,6 +183,57 @@ 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 can report either a list of discrete sizes they\n> >> + * support or a list of intervals expressed as a [min-max] sizes range.\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> >> + * an empty map on error or if the pad does not exist\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> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Invalid pad: \" << pad;\n> >\n> > This would hold on a single line.\n> >\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> > This won't make it possible to pass an error from\n> > VIDIOC_SUBDEV_ENUM_FRAME_SIZE to the caller. How about making\n> > enumPadSizes() return an int and take a pointer to the sizes vector ?\n> \n> I really struggled here, I wanted to keep the error visible, but also\n> exploit RVO, which I presume is here used.\n> \n> On error, we would return an empty size list anyone, is it worth\n> returning an int?\n\nAn empty map on error is fine in my opinion, but a populated map with a\nfew empty vectors would be very confusing. If you pass the vector to\nenumPadSizes() you will be able to propagate the error and clear the\nmap, while keeping the implementation efficient as you will populate the\nvector in-place.\n\n> > With those changes,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> +\n> >> +\t\tmbusEnum.index++;\n> >> +\t}\n> >> +\n> >> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Unable to enumerate formats on pad \" << pad\n> >> +\t\t\t<< \": \" << strerror(-ret);\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 +299,39 @@ 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> >> +\t\tsizeEnum.index++;\n> >> +\t}\n> >> +\n> >> +\tif (ret && (errno != EINVAL && errno != ENOTTY)) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(V4L2Subdev, Error)\n> >> +\t\t\t<< \"Unable to enumerate sizes on pad \" << pad\n> >> +\t\t\t<< \": \" << strerror(-ret);\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 A29AC610BF\n\tfor ;\n\tFri, 1 Mar 2019 14:04:21 +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 1CEF249;\n\tFri, 1 Mar 2019 14:04:21 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551445461;\n\tbh=vQK/L8tAOCSQAyVvQI0tnxqf8xf2Sb8plAP1udY44H8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kVLTpoMx9uFmn3Q9+91fsa92nmgklCFiQvJ0kjEN7/tdUk31Xq9ncD8RUzQVzn8r5\n\teo8Kg5oKhrufEtvakMG7zlRNTj4KN2Z6orIa0vvkFu4+z1SSgnsZNfV3uTQCWfskAb\n\tH3QkKAX1xt/7itO17LoA88q2HEwthKxkKv+KWdAk=","Date":"Fri, 1 Mar 2019 15:04:15 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301130415.GA32244@pendragon.ideasonboard.com>","References":"<20190301115139.11060-1-jacopo@jmondi.org>\n\t<20190301115139.11060-2-jacopo@jmondi.org>\n\t<20190301124240.GE26239@pendragon.ideasonboard.com>\n\t<20190301125510.l3oxwztttcs274ch@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190301125510.l3oxwztttcs274ch@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 1/5] 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 13:04:22 -0000"}}]