[{"id":4050,"web_url":"https://patchwork.libcamera.org/comment/4050/","msgid":"<20200317122243.otkrrqt2vukmytol@uno.localdomain>","date":"2020-03-17T12:22:43","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: v4l2_subdevice:\n\tExtend [gs]etFormat() to specify format type","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent,\n\nOn Mon, Mar 16, 2020 at 11:43:03PM +0200, Laurent Pinchart wrote:\n> The V4L2 subdevice API exposes ACTIVE and TRY formats, but the\n> V4L2Subdevice getFormat() and setFormat() operations only apply on\n> ACTIVE formats. Extend them to support TRY formats as well.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> src/libcamera/include/v4l2_subdevice.h | 11 +++++++++--\n> src/libcamera/v4l2_subdevice.cpp | 23 +++++++++++++++++++----\n> 2 files changed, 28 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 9c077674f997..4a04eadfb1f9 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -32,6 +32,11 @@ struct V4L2SubdeviceFormat {\n> class V4L2Subdevice : public V4L2Device\n> {\n> public:\n> +\tenum Whence {\n> +\t\tActiveFormat,\n> +\t\tTryFormat,\n> +\t};\n\nJust wondering if\n ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n TryFormat = V4L2_SUBDEV_FORMAT_TRY\n\nwould not save you the ternary operator in set/get functions\n\n> +\n> \texplicit V4L2Subdevice(const MediaEntity *entity);\n> \tV4L2Subdevice(const V4L2Subdevice &) = delete;\n> \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n> @@ -46,8 +51,10 @@ public:\n>\n> \tImageFormats formats(unsigned int pad);\n>\n> -\tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> -\tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> +\tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +\t\t Whence whence = ActiveFormat);\n> +\tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +\t\t Whence whence = ActiveFormat);\n\nThis mimics the V4L2 APIs, so it's certainly safe, but considering the\nuse case for TRY is actually to set a try format to then get it back\nto see how the driver changed it, have you considered a tryFormat()\nfunction that does both steps in one call ?\n\n>\n> \tstatic V4L2Subdevice *fromEntityName(const MediaDevice *media,\n> \t\t\t\t\t const std::string &entity);\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 8b9da81e8ab3..fb9bdbce2610 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -95,6 +95,15 @@ const std::string V4L2SubdeviceFormat::toString() const\n> * any device left open will be closed, and any resources released.\n> */\n>\n> +/**\n> + * \\enum V4L2Subdevice::Whence\n> + * \\brief Specify the type of format for getFormat() and setFormat() operations\n> + * \\var V4L2Subdevice::ActiveFormat\n> + * \\brief The format operation applies to ACTIVE formats\n> + * \\var V4L2Subdevice::TryFormat\n> + * \\brief The format operation applies to TRY formats\n> + */\n> +\n> /**\n> * \\brief Create a V4L2 subdevice from a MediaEntity using its device node\n> * path\n> @@ -184,12 +193,15 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)\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> * \\param[out] format The image bus format\n> + * \\param[in] whence The format to retrieve, ACTIVE or TRY\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> +int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +\t\t\t Whence whence)\n> {\n> \tstruct v4l2_subdev_format subdevFmt = {};\n> -\tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> +\tsubdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE\n> +\t\t\t: V4L2_SUBDEV_FORMAT_TRY;\n> \tsubdevFmt.pad = pad;\n>\n> \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> @@ -211,6 +223,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> * \\brief Set an image format on one of the V4L2 subdevice pads\n> * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> * \\param[inout] format The image bus format to apply to the subdevice's pad\n> + * \\param[in] whence The format to set, ACTIVE or TRY\n\nHere and above \"ActiveFormat or TryFormat\".\nIf you could cross-reference to the enum it's even better\n\nThanks\n j\n\n> *\n> * Apply the requested image format to the desired media pad and return the\n> * actually applied format parameters, as \\ref V4L2Subdevice::getFormat would\n> @@ -218,10 +231,12 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> +int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +\t\t\t Whence whence)\n> {\n> \tstruct v4l2_subdev_format subdevFmt = {};\n> -\tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> +\tsubdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE\n> +\t\t\t: V4L2_SUBDEV_FORMAT_TRY;\n> \tsubdevFmt.pad = pad;\n> \tsubdevFmt.format.width = format->size.width;\n> \tsubdevFmt.format.height = format->size.height;\n> --\n> Regards,\n>\n> Laurent Pinchart\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 relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0345162923\n\tfor ;\n\tTue, 17 Mar 2020 13:19:49 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 446F9FF810;\n\tTue, 17 Mar 2020 12:19:48 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 17 Mar 2020 13:22:43 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Martijn Braam , \n\tMickael GUENE ,\n\tBenjamin GAIGNARD ","Message-ID":"<20200317122243.otkrrqt2vukmytol@uno.localdomain>","References":"<20200316214310.27665-1-laurent.pinchart@ideasonboard.com>\n\t<20200316214310.27665-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200316214310.27665-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: v4l2_subdevice:\n\tExtend [gs]etFormat() to specify format type","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":"Tue, 17 Mar 2020 12:19:50 -0000"}},{"id":4057,"web_url":"https://patchwork.libcamera.org/comment/4057/","msgid":"<20200317201759.GF2527@pendragon.ideasonboard.com>","date":"2020-03-17T20:17:59","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: v4l2_subdevice:\n\tExtend [gs]etFormat() to specify format type","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Mar 17, 2020 at 01:22:43PM +0100, Jacopo Mondi wrote:\n> On Mon, Mar 16, 2020 at 11:43:03PM +0200, Laurent Pinchart wrote:\n> > The V4L2 subdevice API exposes ACTIVE and TRY formats, but the\n> > V4L2Subdevice getFormat() and setFormat() operations only apply on\n> > ACTIVE formats. Extend them to support TRY formats as well.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/v4l2_subdevice.h | 11 +++++++++--\n> > src/libcamera/v4l2_subdevice.cpp | 23 +++++++++++++++++++----\n> > 2 files changed, 28 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 9c077674f997..4a04eadfb1f9 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -32,6 +32,11 @@ struct V4L2SubdeviceFormat {\n> > class V4L2Subdevice : public V4L2Device\n> > {\n> > public:\n> > +\tenum Whence {\n> > +\t\tActiveFormat,\n> > +\t\tTryFormat,\n> > +\t};\n> \n> Just wondering if\n> ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> TryFormat = V4L2_SUBDEV_FORMAT_TRY\n> \n> would not save you the ternary operator in set/get functions\n\nI would prefer keeping the ternary operator below for clarity, but your\nproposal could help the compiler optimizing the code. However, that\nrequires pulling the V4L2 kernel headers in here. Do you think it's\nworth it ?\n\n> > +\n> > \texplicit V4L2Subdevice(const MediaEntity *entity);\n> > \tV4L2Subdevice(const V4L2Subdevice &) = delete;\n> > \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n> > @@ -46,8 +51,10 @@ public:\n> >\n> > \tImageFormats formats(unsigned int pad);\n> >\n> > -\tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > -\tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format);\n> > +\tint getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +\t\t Whence whence = ActiveFormat);\n> > +\tint setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +\t\t Whence whence = ActiveFormat);\n> \n> This mimics the V4L2 APIs, so it's certainly safe, but considering the\n> use case for TRY is actually to set a try format to then get it back\n> to see how the driver changed it, have you considered a tryFormat()\n> function that does both steps in one call ?\n\nNo, because setFormat() does so already :-) It returns the formats that\nwas actually set.\n\n> >\n> > \tstatic V4L2Subdevice *fromEntityName(const MediaDevice *media,\n> > \t\t\t\t\t const std::string &entity);\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 8b9da81e8ab3..fb9bdbce2610 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -95,6 +95,15 @@ const std::string V4L2SubdeviceFormat::toString() const\n> > * any device left open will be closed, and any resources released.\n> > */\n> >\n> > +/**\n> > + * \\enum V4L2Subdevice::Whence\n> > + * \\brief Specify the type of format for getFormat() and setFormat() operations\n> > + * \\var V4L2Subdevice::ActiveFormat\n> > + * \\brief The format operation applies to ACTIVE formats\n> > + * \\var V4L2Subdevice::TryFormat\n> > + * \\brief The format operation applies to TRY formats\n> > + */\n> > +\n> > /**\n> > * \\brief Create a V4L2 subdevice from a MediaEntity using its device node\n> > * path\n> > @@ -184,12 +193,15 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)\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> > * \\param[out] format The image bus format\n> > + * \\param[in] whence The format to retrieve, ACTIVE or TRY\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> > +int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +\t\t\t Whence whence)\n> > {\n> > \tstruct v4l2_subdev_format subdevFmt = {};\n> > -\tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > +\tsubdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE\n> > +\t\t\t: V4L2_SUBDEV_FORMAT_TRY;\n> > \tsubdevFmt.pad = pad;\n> >\n> > \tint ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> > @@ -211,6 +223,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> > * \\brief Set an image format on one of the V4L2 subdevice pads\n> > * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> > * \\param[inout] format The image bus format to apply to the subdevice's pad\n> > + * \\param[in] whence The format to set, ACTIVE or TRY\n> \n> Here and above \"ActiveFormat or TryFormat\".\n> If you could cross-reference to the enum it's even better\n\nI'll fix that. It seems to require either V4L2Subdevice::ActiveFormat,\nor \\ref V4L2Subdevice::ActiveFormat \"ActiveFormat\". I'll go for the\nlatter.\n\n> > *\n> > * Apply the requested image format to the desired media pad and return the\n> > * actually applied format parameters, as \\ref V4L2Subdevice::getFormat would\n> > @@ -218,10 +231,12 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)\n> > +int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +\t\t\t Whence whence)\n> > {\n> > \tstruct v4l2_subdev_format subdevFmt = {};\n> > -\tsubdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > +\tsubdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE\n> > +\t\t\t: V4L2_SUBDEV_FORMAT_TRY;\n> > \tsubdevFmt.pad = pad;\n> > \tsubdevFmt.format.width = format->size.width;\n> > \tsubdevFmt.format.height = format->size.height;","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 E4BD262923\n\tfor ;\n\tTue, 17 Mar 2020 21:18:05 +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 5520CF9;\n\tTue, 17 Mar 2020 21:18:05 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584476285;\n\tbh=PkAOo1ynnJf9BNd3V7HwqOYeg/f/qRaVtfCXh5D5YhI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gMHlFJj8Qb3/NBTv37mkUWt7nrpIb7iLMMnyvQ2RZ6y91VpPAXAXwRdWVPZ+C5lPI\n\tIGWq1kiMXy9qW0SnpoVSFZ6ZbqcTw6uimRCjWtti2vwRLQJMRiAohC5DxPc4JiDmUy\n\tm1WL/a/QZkcVMlwSo2l+BeYpKEowvmpNwVF661TM=","Date":"Tue, 17 Mar 2020 22:17:59 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Martijn Braam , \n\tMickael GUENE ,\n\tBenjamin GAIGNARD ","Message-ID":"<20200317201759.GF2527@pendragon.ideasonboard.com>","References":"<20200316214310.27665-1-laurent.pinchart@ideasonboard.com>\n\t<20200316214310.27665-4-laurent.pinchart@ideasonboard.com>\n\t<20200317122243.otkrrqt2vukmytol@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200317122243.otkrrqt2vukmytol@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: v4l2_subdevice:\n\tExtend [gs]etFormat() to specify format type","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":"Tue, 17 Mar 2020 20:18:06 -0000"}}]