[{"id":15044,"web_url":"https://patchwork.libcamera.org/comment/15044/","msgid":"<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>","date":"2021-02-08T08:51:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Feb 08, 2021 at 03:38:05AM +0200, Laurent Pinchart wrote:\n> When a sensor can upscale the image, the native sensor resolution isn't\n> equal to the largest size reported by the sensor. Use the active area\n\nNot sure I got this one. If the sensor can upscale, wouldn't the\nlargest produced size be enumerated through the canonical\nENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ?\n\nThanks\n j\n\n> size instead, and default it to the largest enumerated size if the crop\n> rectangle targets are not supported.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/camera_sensor.h | 3 +--\n> src/libcamera/camera_sensor.cpp | 16 ++++++++--------\n> 2 files changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index c8f81882a958..74c35d1c8922 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -55,7 +55,7 @@ public:\n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n> -\tconst Size &resolution() const { return resolution_; }\n> +\tSize resolution() const { return activeArea_.size(); }\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -87,7 +87,6 @@ private:\n> \tstd::string id_;\n>\n> \tV4L2Subdevice::Formats formats_;\n> -\tSize resolution_;\n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index c9e8d49b7935..59834ffcdd94 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -234,10 +234,12 @@ int CameraSensor::init()\n> \tsizes_.erase(last, sizes_.end());\n>\n> \t/*\n> -\t * The sizes_ vector is sorted in ascending order, the resolution is\n> -\t * thus the last element of the vector.\n> +\t * Initialize the pixel array size as the largest size supported by the\n> +\t * sensor. It will be overridden later using the crop bounds. The\n> +\t * sizes_ vector is sorted in ascending order, the largest size is thus\n> +\t * the last element of the vector.\n> \t */\n> -\tresolution_ = sizes_.back();\n> +\tpixelArraySize_ = sizes_.back();\n>\n> \t/*\n> \t * VIMC is a bit special, as it does not yet support all the mandatory\n> @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver()\n> \tRectangle rect;\n> \tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> \tif (ret) {\n> -\t\trect = Rectangle(resolution());\n> \t\tLOG(CameraSensor, Warning)\n> \t\t\t<< \"The PixelArraySize property has been defaulted to \"\n> -\t\t\t<< rect.toString();\n> +\t\t\t<< pixelArraySize_.toString();\n> \t\terr = -EINVAL;\n> +\t} else {\n> +\t\tpixelArraySize_ = rect.size();\n> \t}\n> -\tpixelArraySize_.width = rect.width;\n> -\tpixelArraySize_.height = rect.height;\n>\n> \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_);\n> \tif (ret) {\n> @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver()\n> */\n> void CameraSensor::initVimcDefaultProperties()\n> {\n> -\tpixelArraySize_ = resolution();\n> \tactiveArea_ = Rectangle(pixelArraySize_);\n> }\n>\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":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 36517BD160\n\tfor ;\n\tMon, 8 Feb 2021 08:51:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B581160D0E;\n\tMon, 8 Feb 2021 09:51:29 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 519FA60301\n\tfor ;\n\tMon, 8 Feb 2021 09:51:29 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id CF5BAC000E;\n\tMon, 8 Feb 2021 08:51:28 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 8 Feb 2021 09:51:52 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>","References":"<20210208013805.30842-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210208013805.30842-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15052,"web_url":"https://patchwork.libcamera.org/comment/15052/","msgid":"","date":"2021-02-08T15:17:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote:\n> On Mon, Feb 08, 2021 at 03:38:05AM +0200, Laurent Pinchart wrote:\n> > When a sensor can upscale the image, the native sensor resolution isn't\n> > equal to the largest size reported by the sensor. Use the active area\n> \n> Not sure I got this one. If the sensor can upscale, wouldn't the\n> largest produced size be enumerated through the canonical\n> ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ?\n\nIt would, that's the issue :-) ENUM_FRAME_SIZE would report the largest\nsize the scaler can produce, which would be larger than the sensor's\nnative resolution.\n\n> > size instead, and default it to the largest enumerated size if the crop\n> > rectangle targets are not supported.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 3 +--\n> > src/libcamera/camera_sensor.cpp | 16 ++++++++--------\n> > 2 files changed, 9 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index c8f81882a958..74c35d1c8922 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -55,7 +55,7 @@ public:\n> > \tconst MediaEntity *entity() const { return entity_; }\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tconst std::vector &sizes() const { return sizes_; }\n> > -\tconst Size &resolution() const { return resolution_; }\n> > +\tSize resolution() const { return activeArea_.size(); }\n> >\n> > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > \t\t\t\t const Size &size) const;\n> > @@ -87,7 +87,6 @@ private:\n> > \tstd::string id_;\n> >\n> > \tV4L2Subdevice::Formats formats_;\n> > -\tSize resolution_;\n> > \tstd::vector mbusCodes_;\n> > \tstd::vector sizes_;\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index c9e8d49b7935..59834ffcdd94 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -234,10 +234,12 @@ int CameraSensor::init()\n> > \tsizes_.erase(last, sizes_.end());\n> >\n> > \t/*\n> > -\t * The sizes_ vector is sorted in ascending order, the resolution is\n> > -\t * thus the last element of the vector.\n> > +\t * Initialize the pixel array size as the largest size supported by the\n> > +\t * sensor. It will be overridden later using the crop bounds. The\n> > +\t * sizes_ vector is sorted in ascending order, the largest size is thus\n> > +\t * the last element of the vector.\n> > \t */\n> > -\tresolution_ = sizes_.back();\n> > +\tpixelArraySize_ = sizes_.back();\n> >\n> > \t/*\n> > \t * VIMC is a bit special, as it does not yet support all the mandatory\n> > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver()\n> > \tRectangle rect;\n> > \tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > \tif (ret) {\n> > -\t\trect = Rectangle(resolution());\n> > \t\tLOG(CameraSensor, Warning)\n> > \t\t\t<< \"The PixelArraySize property has been defaulted to \"\n> > -\t\t\t<< rect.toString();\n> > +\t\t\t<< pixelArraySize_.toString();\n> > \t\terr = -EINVAL;\n> > +\t} else {\n> > +\t\tpixelArraySize_ = rect.size();\n> > \t}\n> > -\tpixelArraySize_.width = rect.width;\n> > -\tpixelArraySize_.height = rect.height;\n> >\n> > \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_);\n> > \tif (ret) {\n> > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver()\n> > */\n> > void CameraSensor::initVimcDefaultProperties()\n> > {\n> > -\tpixelArraySize_ = resolution();\n> > \tactiveArea_ = Rectangle(pixelArraySize_);\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C00A6BD160\n\tfor ;\n\tMon, 8 Feb 2021 15:17:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D8A660D2B;\n\tMon, 8 Feb 2021 16:17:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 049EA60D24\n\tfor ;\n\tMon, 8 Feb 2021 16:17:57 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D2043D7;\n\tMon, 8 Feb 2021 16:17:56 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cE28SzEh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612797476;\n\tbh=OEDBgWAdhKsPvy5p/NjrnSSM5+Mg7lWyv46+bp4ACts=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cE28SzEhUCws5K2td825HzEtzeDItKADsXhjNPoeT6LYtjQHEH05V7pJxKp2mbFS3\n\taqDws0P3+aXnNGUlBxfPHAW5X85+0MwFnlmd3g0PzknnB19TQxN2bf324Jbm6EL0C+\n\tAkLvjGWK9Wbgce2n+/wKcouefCz4DF28VUTIUI3M=","Date":"Mon, 8 Feb 2021 17:17:32 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210208013805.30842-1-laurent.pinchart@ideasonboard.com>\n\t<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15054,"web_url":"https://patchwork.libcamera.org/comment/15054/","msgid":"<20210208154527.bgqph4h2v5iakidg@uno.localdomain>","date":"2021-02-08T15:45:27","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Feb 08, 2021 at 05:17:32PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote:\n> > On Mon, Feb 08, 2021 at 03:38:05AM +0200, Laurent Pinchart wrote:\n> > > When a sensor can upscale the image, the native sensor resolution isn't\n> > > equal to the largest size reported by the sensor. Use the active area\n> >\n> > Not sure I got this one. If the sensor can upscale, wouldn't the\n> > largest produced size be enumerated through the canonical\n> > ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ?\n>\n> It would, that's the issue :-) ENUM_FRAME_SIZE would report the largest\n> size the scaler can produce, which would be larger than the sensor's\n> native resolution.\n>\n\nRight, first email of the day, I was not fully awake probably :)\n\n> > > size instead, and default it to the largest enumerated size if the crop\n> > > rectangle targets are not supported.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 3 +--\n> > > src/libcamera/camera_sensor.cpp | 16 ++++++++--------\n> > > 2 files changed, 9 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index c8f81882a958..74c35d1c8922 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -55,7 +55,7 @@ public:\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > \tconst std::vector &sizes() const { return sizes_; }\n> > > -\tconst Size &resolution() const { return resolution_; }\n> > > +\tSize resolution() const { return activeArea_.size(); }\n> > >\n> > > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > \t\t\t\t const Size &size) const;\n> > > @@ -87,7 +87,6 @@ private:\n> > > \tstd::string id_;\n> > >\n> > > \tV4L2Subdevice::Formats formats_;\n> > > -\tSize resolution_;\n> > > \tstd::vector mbusCodes_;\n> > > \tstd::vector sizes_;\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index c9e8d49b7935..59834ffcdd94 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -234,10 +234,12 @@ int CameraSensor::init()\n> > > \tsizes_.erase(last, sizes_.end());\n> > >\n> > > \t/*\n> > > -\t * The sizes_ vector is sorted in ascending order, the resolution is\n> > > -\t * thus the last element of the vector.\n> > > +\t * Initialize the pixel array size as the largest size supported by the\n> > > +\t * sensor. It will be overridden later using the crop bounds. The\n> > > +\t * sizes_ vector is sorted in ascending order, the largest size is thus\n> > > +\t * the last element of the vector.\n> > > \t */\n> > > -\tresolution_ = sizes_.back();\n> > > +\tpixelArraySize_ = sizes_.back();\n\nHaving the pixelArraySize_ initialization here, to expect it will be\nupdated later is a bit hard to follow.\n\nWhat if you initialize it to size_.back() if retrieving CROP_BOUNDS\nfails ?\n\n> > >\n> > > \t/*\n> > > \t * VIMC is a bit special, as it does not yet support all the mandatory\n> > > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver()\n> > > \tRectangle rect;\n> > > \tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > \tif (ret) {\n> > > -\t\trect = Rectangle(resolution());\n> > > \t\tLOG(CameraSensor, Warning)\n> > > \t\t\t<< \"The PixelArraySize property has been defaulted to \"\n> > > -\t\t\t<< rect.toString();\n> > > +\t\t\t<< pixelArraySize_.toString();\n> > > \t\terr = -EINVAL;\n> > > +\t} else {\n> > > +\t\tpixelArraySize_ = rect.size();\n> > > \t}\n> > > -\tpixelArraySize_.width = rect.width;\n> > > -\tpixelArraySize_.height = rect.height;\n> > >\n> > > \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_);\n> > > \tif (ret) {\n> > > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver()\n> > > */\n> > > void CameraSensor::initVimcDefaultProperties()\n> > > {\n> > > -\tpixelArraySize_ = resolution();\n> > > \tactiveArea_ = Rectangle(pixelArraySize_);\n> > > }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D9BB4BD160\n\tfor ;\n\tMon, 8 Feb 2021 15:45:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C43560D30;\n\tMon, 8 Feb 2021 16:45:05 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 828B460D24\n\tfor ;\n\tMon, 8 Feb 2021 16:45:04 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 0B71360009;\n\tMon, 8 Feb 2021 15:45:03 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 8 Feb 2021 16:45:27 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210208154527.bgqph4h2v5iakidg@uno.localdomain>","References":"<20210208013805.30842-1-laurent.pinchart@ideasonboard.com>\n\t<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15427,"web_url":"https://patchwork.libcamera.org/comment/15427/","msgid":"","date":"2021-03-02T19:40:45","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Feb 08, 2021 at 04:45:27PM +0100, Jacopo Mondi wrote:\n> On Mon, Feb 08, 2021 at 05:17:32PM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote:\n> > > On Mon, Feb 08, 2021 at 03:38:05AM +0200, Laurent Pinchart wrote:\n> > > > When a sensor can upscale the image, the native sensor resolution isn't\n> > > > equal to the largest size reported by the sensor. Use the active area\n> > >\n> > > Not sure I got this one. If the sensor can upscale, wouldn't the\n> > > largest produced size be enumerated through the canonical\n> > > ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ?\n> >\n> > It would, that's the issue :-) ENUM_FRAME_SIZE would report the largest\n> > size the scaler can produce, which would be larger than the sensor's\n> > native resolution.\n> \n> Right, first email of the day, I was not fully awake probably :)\n\nI'll try to do better in my last e-mail of today ;-)\n\n> > > > size instead, and default it to the largest enumerated size if the crop\n> > > > rectangle targets are not supported.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 3 +--\n> > > > src/libcamera/camera_sensor.cpp | 16 ++++++++--------\n> > > > 2 files changed, 9 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index c8f81882a958..74c35d1c8922 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -55,7 +55,7 @@ public:\n> > > > \tconst MediaEntity *entity() const { return entity_; }\n> > > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > > \tconst std::vector &sizes() const { return sizes_; }\n> > > > -\tconst Size &resolution() const { return resolution_; }\n> > > > +\tSize resolution() const { return activeArea_.size(); }\n> > > >\n> > > > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > \t\t\t\t const Size &size) const;\n> > > > @@ -87,7 +87,6 @@ private:\n> > > > \tstd::string id_;\n> > > >\n> > > > \tV4L2Subdevice::Formats formats_;\n> > > > -\tSize resolution_;\n> > > > \tstd::vector mbusCodes_;\n> > > > \tstd::vector sizes_;\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index c9e8d49b7935..59834ffcdd94 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -234,10 +234,12 @@ int CameraSensor::init()\n> > > > \tsizes_.erase(last, sizes_.end());\n> > > >\n> > > > \t/*\n> > > > -\t * The sizes_ vector is sorted in ascending order, the resolution is\n> > > > -\t * thus the last element of the vector.\n> > > > +\t * Initialize the pixel array size as the largest size supported by the\n> > > > +\t * sensor. It will be overridden later using the crop bounds. The\n> > > > +\t * sizes_ vector is sorted in ascending order, the largest size is thus\n> > > > +\t * the last element of the vector.\n> > > > \t */\n> > > > -\tresolution_ = sizes_.back();\n> > > > +\tpixelArraySize_ = sizes_.back();\n> \n> Having the pixelArraySize_ initialization here, to expect it will be\n> updated later is a bit hard to follow.\n> \n> What if you initialize it to size_.back() if retrieving CROP_BOUNDS\n> fails ?\n\nI'll try that.\n\n> > > >\n> > > > \t/*\n> > > > \t * VIMC is a bit special, as it does not yet support all the mandatory\n> > > > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver()\n> > > > \tRectangle rect;\n> > > > \tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > > \tif (ret) {\n> > > > -\t\trect = Rectangle(resolution());\n> > > > \t\tLOG(CameraSensor, Warning)\n> > > > \t\t\t<< \"The PixelArraySize property has been defaulted to \"\n> > > > -\t\t\t<< rect.toString();\n> > > > +\t\t\t<< pixelArraySize_.toString();\n> > > > \t\terr = -EINVAL;\n> > > > +\t} else {\n> > > > +\t\tpixelArraySize_ = rect.size();\n> > > > \t}\n> > > > -\tpixelArraySize_.width = rect.width;\n> > > > -\tpixelArraySize_.height = rect.height;\n> > > >\n> > > > \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_);\n> > > > \tif (ret) {\n> > > > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver()\n> > > > */\n> > > > void CameraSensor::initVimcDefaultProperties()\n> > > > {\n> > > > -\tpixelArraySize_ = resolution();\n> > > > \tactiveArea_ = Rectangle(pixelArraySize_);\n> > > > }\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5C8D3BD808\n\tfor ;\n\tTue, 2 Mar 2021 19:41:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D732660106;\n\tTue, 2 Mar 2021 20:41:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 222FE60106\n\tfor ;\n\tTue, 2 Mar 2021 20:41:15 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 987EF45D;\n\tTue, 2 Mar 2021 20:41:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DSW3goSj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614714074;\n\tbh=Cfsl8pYz47+uHNhWlLEjvyTA751nTXPsKH/qrIz/hYs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DSW3goSj9DjiB67Yb5BLyQrk3dkZVd5+uDfO0uy2+yfONxunBCZ7qpwPNyGghScp5\n\tVFpgdrZRzIlQVRTFQJ3ASBAOpv7QyMnBBmClb8rbD7EWe3UA8dW2dePKsOObh5NFe5\n\tamYXNyC7Gwpki9wWYI4AICF/tF0oLnjUoBZAVOYA=","Date":"Tue, 2 Mar 2021 21:40:45 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210208013805.30842-1-laurent.pinchart@ideasonboard.com>\n\t<20210208085152.fq4pa7h7fnntevqn@uno.localdomain>\n\t\n\t<20210208154527.bgqph4h2v5iakidg@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210208154527.bgqph4h2v5iakidg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: Use active\n\tarea size as resolution","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]