[{"id":2516,"web_url":"https://patchwork.libcamera.org/comment/2516/","msgid":"<20190828114810.GR28351@bigcity.dyn.berto.se>","date":"2019-08-28T11:48:10","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> Retrieve the camera sensor pixel array sizes and the active pixel area\n> using the V4L2Subdevice selection API.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/geometry.h | 1 +\n> src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> src/libcamera/geometry.cpp | 24 +++++++++++++\n> src/libcamera/include/camera_sensor.h | 4 +++\n> 4 files changed, 80 insertions(+)\n\nI think you should split the changes to geometry.{cpp,h} into its own \npatch.\n\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 52f4d010de76..b91fcfa1dd17 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -42,6 +42,7 @@ struct Size {\n> \tunsigned int height;\n> \n> \tconst std::string toString() const;\n> +\tbool contains(const Rectangle &rectangle) const;\n> };\n> \n> bool operator==(const Size &lhs, const Size &rhs);\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index fc7fdcdcaf5b..b6ccaae0930b 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -128,6 +128,28 @@ int CameraSensor::init()\n> \t}\n> \trotation_ = value;\n> \n> +\t/*\n> +\t * Retrieve and store the sensor pixel array size and active area\n> +\t * rectangle.\n> +\t */\n> +\tRectangle rect;\n> +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\tpixelArray_.width = rect.w;\n> +\tpixelArray_.height = rect.h;\n> +\n> +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (!pixelArray_.contains(rect)) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tactiveArea_ = rect;\n> +\n> \t/* Enumerate and cache media bus codes and sizes. */\n> \tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> \treturn sizes_.back();\n> }\n> \n> +/**\n> + * \\fn CameraSensor::pixelArray()\n> + * \\brief Retrieve the camera sensor pixel array dimension\n> + * \\return The number of horizontal and vertical pixel units installed on the\n> + * camera sensor\n> + *\n> + * The returned pixel array size is the number of pixels units physically\n> + * installed on the sensor, including non-active ones, such as pixels used\n> + * for calibration or testing.\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::activeArea()\n> + * \\brief Retrieve the active pixels area\n> + * \\return A rectangle describing the active pixel area\n> + *\n> + * The returned rectangle is contained in the larger pixel array area,\n> + * returned by the CameraSensor::pixelArray() operation.\n> + *\n> + * \\todo This operation collides with CameraSensor::resolution() as they\n> + * both reports the same information (bugs in the driver code apart).\n> + * Also, for some sensors, the maximum available resolution might depend on\n> + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> + * is different from the maximum available resolution for 16:9 format). This\n> + * has to be sorted out as well.\n\nShould we remove CameraSensor::resolution() in favor if new more \n\"correct\" way of finding out the sensor resolution?\n\n> + *\n> + * \\sa Size::contains()\n> + */\n> +\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 32b0faeadc63..9b60768a57a7 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> }\n> \n> +/**\n> + * \\fn Size::contains()\n> + * \\param rectangle The rectangle to verify\n> + * \\return True if \\a rectangle is contained in the area represented by this\n> + * Size, false otherwise\n\nReturn comes at the end of the documentation block.\n\n> + *\n> + * Return true if \\a rectangle is completely contained in the area represented\n> + * by the width and height of this Size, with its top left corner assumed to be\n> + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> + * horizontal and vertical positive displacement from the Size top left corner,\n> + * from where the rectangle's horizontal and vertical sizes are calculated.\n> + */\n> +bool Size::contains(const Rectangle &rectangle) const\n> +{\n> +\tif (rectangle.x < 0 || rectangle.y < 0)\n> +\t\treturn false;\n> +\n> +\tif (rectangle.x + rectangle.w > width ||\n> +\t rectangle.y + rectangle.h > height)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> /**\n> * \\brief Compare sizes for equality\n> * \\return True if the two sizes are equal, false otherwise\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 32d39127b275..abf5344cf9d8 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -38,6 +38,8 @@ public:\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n> \tconst Size &resolution() const;\n> +\tconst Size pixelArray() const { return pixelArray_; }\n> +\tconst Rectangle activeArea() const { return activeArea_; }\n> \n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -59,6 +61,8 @@ private:\n> \n> \tCameraLocation location_;\n> \tunsigned int rotation_;\n> +\tSize pixelArray_;\n> +\tRectangle activeArea_;\n> };\n> \n> } /* namespace libcamera */\n> -- \n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9830760BF6\n\tfor ;\n\tWed, 28 Aug 2019 13:48:11 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id o11so1854954lfb.12\n\tfor ;\n\tWed, 28 Aug 2019 04:48:11 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\td7sm771632lfa.86.2019.08.28.04.48.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Aug 2019 04:48:10 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=5krKqnRYLkv29lNykRrtZWMnmpocH8688sjfkaYROAk=;\n\tb=VAAZUzcrLYOe1CZfso2fTQJ6d2d5l/lrqlL3yGHy+JO275lJ/uMSEM8zB4PciwNGrW\n\t6bpNEdCfz104Mm0p02xDJteFKPxKEaDjbXXffzrRe//Ujx+bdKH31Nno+gbxhAOmE384\n\tqHJ7iSwtSTEWxRKeZj7PltuIEjjCPhmiP3+MkZ6M1x2AHgGxMMbsPQLR+tBe+hwnFL9I\n\ti+zf4lE2n7w5WMg6cc5yA4B77QjXvYIhwIZyaU/caWK8cNx9sJm6NZhUPPYLJ1DHf5GO\n\tvBnXd7RwRPRj9T79U4FZiTBZCevqA0aCFHOB4HFLShMm81FB9apEADxkQexx1JjZxibl\n\tLDhw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=5krKqnRYLkv29lNykRrtZWMnmpocH8688sjfkaYROAk=;\n\tb=RSbdf4fYeIPVPl/5nqLy2K8qnyZmmG5BnLYwjW4ow8EyOpotXt5Vfu5aetzX2JjEYx\n\tRHeZHXHF9KRHEHSBWkMtY66O+umlqmzI5mAiSj/Xrrjd9icQqRYxMGtt4ncU5b+7B3S/\n\tKCR7t9xooqSrauDNOZoSi1IGXmsOemwBeXfBqVOuNYkLZov0TjpwVVkFTzhhQWieQhql\n\t5riJ9SO5l1TRVjAiPeE5YVJ/pZtsjF5bXxTbfJu2Fk6H2dpr7wdlrmzh30FxOhTLHJhz\n\tgR4g47a3mBq/DRpcEC5HcmzZ/GH0KfEXXLIhsnSAcr3HxZASfV+IGHHtMGsV9fzYUJBi\n\t6u8A==","X-Gm-Message-State":"APjAAAUbUI45Qc5cWY8rONqjZMn5XXt75X2FPrleq9UjPPzQhDkQlBGj\n\tX4Usn7gAIoSWvm4hmgwPJFJTAz3A8JE=","X-Google-Smtp-Source":"APXvYqzW35Fmv28UgZOtcZNcQhYKTyFrKr6qLDMOF4bC/uWTNyzk46j5zh4w9p25s6zs9VfskXAZug==","X-Received":"by 2002:ac2:4835:: with SMTP id 21mr2330829lft.121.1566992891051;\n\tWed, 28 Aug 2019 04:48:11 -0700 (PDT)","Date":"Wed, 28 Aug 2019 13:48:10 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828114810.GR28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190827095008.11405-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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":"Wed, 28 Aug 2019 11:48:11 -0000"}},{"id":2527,"web_url":"https://patchwork.libcamera.org/comment/2527/","msgid":"<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>","date":"2019-08-28T16:34:06","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> > Retrieve the camera sensor pixel array sizes and the active pixel area\n> > using the V4L2Subdevice selection API.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/geometry.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> > src/libcamera/geometry.cpp | 24 +++++++++++++\n> > src/libcamera/include/camera_sensor.h | 4 +++\n> > 4 files changed, 80 insertions(+)\n>\n> I think you should split the changes to geometry.{cpp,h} into its own\n> patch.\n>\nMmmm, I know we stand on two opposite sides here... the change is\nrather trivial and it makes sense to me to show its usage in the same\npatch to avoid people jumping from one patch to another during review.\nI could split if this bothers you, but it's not worth it in my opinion\n\n> >\n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index 52f4d010de76..b91fcfa1dd17 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -42,6 +42,7 @@ struct Size {\n> > \tunsigned int height;\n> >\n> > \tconst std::string toString() const;\n> > +\tbool contains(const Rectangle &rectangle) const;\n> > };\n> >\n> > bool operator==(const Size &lhs, const Size &rhs);\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index fc7fdcdcaf5b..b6ccaae0930b 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -128,6 +128,28 @@ int CameraSensor::init()\n> > \t}\n> > \trotation_ = value;\n> >\n> > +\t/*\n> > +\t * Retrieve and store the sensor pixel array size and active area\n> > +\t * rectangle.\n> > +\t */\n> > +\tRectangle rect;\n> > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\tpixelArray_.width = rect.w;\n> > +\tpixelArray_.height = rect.h;\n> > +\n> > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tif (!pixelArray_.contains(rect)) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tactiveArea_ = rect;\n> > +\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > \tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> > \treturn sizes_.back();\n> > }\n> >\n> > +/**\n> > + * \\fn CameraSensor::pixelArray()\n> > + * \\brief Retrieve the camera sensor pixel array dimension\n> > + * \\return The number of horizontal and vertical pixel units installed on the\n> > + * camera sensor\n> > + *\n> > + * The returned pixel array size is the number of pixels units physically\n> > + * installed on the sensor, including non-active ones, such as pixels used\n> > + * for calibration or testing.\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::activeArea()\n> > + * \\brief Retrieve the active pixels area\n> > + * \\return A rectangle describing the active pixel area\n> > + *\n> > + * The returned rectangle is contained in the larger pixel array area,\n> > + * returned by the CameraSensor::pixelArray() operation.\n> > + *\n> > + * \\todo This operation collides with CameraSensor::resolution() as they\n> > + * both reports the same information (bugs in the driver code apart).\n> > + * Also, for some sensors, the maximum available resolution might depend on\n> > + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> > + * is different from the maximum available resolution for 16:9 format). This\n> > + * has to be sorted out as well.\n>\n> Should we remove CameraSensor::resolution() in favor if new more\n> \"correct\" way of finding out the sensor resolution?\n>\n\nGood question.. another option would be to get the maximum resolution\nin regard to a specific aspect ratio... I would say I should better\nlook into the current usages of this operation...\n\n> > + *\n> > + * \\sa Size::contains()\n> > + */\n> > +\n> > /**\n> > * \\brief Retrieve the best sensor format for a desired output\n> > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index 32b0faeadc63..9b60768a57a7 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> > \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> > }\n> >\n> > +/**\n> > + * \\fn Size::contains()\n> > + * \\param rectangle The rectangle to verify\n> > + * \\return True if \\a rectangle is contained in the area represented by this\n> > + * Size, false otherwise\n>\n> Return comes at the end of the documentation block.\n>\n\nTrue, thanks for spotting\n\n> > + *\n> > + * Return true if \\a rectangle is completely contained in the area represented\n> > + * by the width and height of this Size, with its top left corner assumed to be\n> > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> > + * horizontal and vertical positive displacement from the Size top left corner,\n> > + * from where the rectangle's horizontal and vertical sizes are calculated.\n> > + */\n> > +bool Size::contains(const Rectangle &rectangle) const\n> > +{\n> > +\tif (rectangle.x < 0 || rectangle.y < 0)\n> > +\t\treturn false;\n> > +\n> > +\tif (rectangle.x + rectangle.w > width ||\n> > +\t rectangle.y + rectangle.h > height)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > /**\n> > * \\brief Compare sizes for equality\n> > * \\return True if the two sizes are equal, false otherwise\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index 32d39127b275..abf5344cf9d8 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -38,6 +38,8 @@ public:\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tconst std::vector &sizes() const { return sizes_; }\n> > \tconst Size &resolution() const;\n> > +\tconst Size pixelArray() const { return pixelArray_; }\n> > +\tconst Rectangle activeArea() const { return activeArea_; }\n> >\n> > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > \t\t\t\t const Size &size) const;\n> > @@ -59,6 +61,8 @@ private:\n> >\n> > \tCameraLocation location_;\n> > \tunsigned int rotation_;\n> > +\tSize pixelArray_;\n> > +\tRectangle activeArea_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C864460BF6\n\tfor ;\n\tWed, 28 Aug 2019 18:32:34 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 4F6C9240004;\n\tWed, 28 Aug 2019 16:32:34 +0000 (UTC)"],"Date":"Wed, 28 Aug 2019 18:34:06 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>\n\t<20190828114810.GR28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jnyivk2fzzuwt3sm\"","Content-Disposition":"inline","In-Reply-To":"<20190828114810.GR28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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":"Wed, 28 Aug 2019 16:32:34 -0000"}},{"id":2532,"web_url":"https://patchwork.libcamera.org/comment/2532/","msgid":"<20190829084945.GV28351@bigcity.dyn.berto.se>","date":"2019-08-29T08:49:45","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> > > Retrieve the camera sensor pixel array sizes and the active pixel area\n> > > using the V4L2Subdevice selection API.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/geometry.h | 1 +\n> > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> > > src/libcamera/geometry.cpp | 24 +++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > 4 files changed, 80 insertions(+)\n> >\n> > I think you should split the changes to geometry.{cpp,h} into its own\n> > patch.\n> >\n> Mmmm, I know we stand on two opposite sides here... the change is\n> rather trivial and it makes sense to me to show its usage in the same\n> patch to avoid people jumping from one patch to another during review.\n> I could split if this bothers you, but it's not worth it in my opinion\n\nI agree this is a fight you and I will enjoy for many years I hope, \nuntil I win in the end of course ;-P I don't feel strongly about it so \ndon't spend energy on it if you don't want to for trivial patches. For \nmid to large size patches I feel very strongly on this topic tho.\n\nI know you feel it's easier to review larger patches and not \"jump\" \nbetween multiple ones. For me it's the opposite, I spend 4 times longer \nreviewing a patch like this because my warning light go off asking why \nthe patch do two or more things. I try to find the reason for this but \ncan't, and once I realise the patch do more then one thing without it \nstrictly needing to I comment on it to release my frustration ;-)\n\nI think this is some sort of meta-bikesheeding discussion, not sure if \nthat is a good thing or not :-)\n\n> \n> > >\n> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > index 52f4d010de76..b91fcfa1dd17 100644\n> > > --- a/include/libcamera/geometry.h\n> > > +++ b/include/libcamera/geometry.h\n> > > @@ -42,6 +42,7 @@ struct Size {\n> > > \tunsigned int height;\n> > >\n> > > \tconst std::string toString() const;\n> > > +\tbool contains(const Rectangle &rectangle) const;\n> > > };\n> > >\n> > > bool operator==(const Size &lhs, const Size &rhs);\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index fc7fdcdcaf5b..b6ccaae0930b 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -128,6 +128,28 @@ int CameraSensor::init()\n> > > \t}\n> > > \trotation_ = value;\n> > >\n> > > +\t/*\n> > > +\t * Retrieve and store the sensor pixel array size and active area\n> > > +\t * rectangle.\n> > > +\t */\n> > > +\tRectangle rect;\n> > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\tpixelArray_.width = rect.w;\n> > > +\tpixelArray_.height = rect.h;\n> > > +\n> > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tif (!pixelArray_.contains(rect)) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tactiveArea_ = rect;\n> > > +\n> > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > \tif (formats.isEmpty()) {\n> > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> > > \treturn sizes_.back();\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn CameraSensor::pixelArray()\n> > > + * \\brief Retrieve the camera sensor pixel array dimension\n> > > + * \\return The number of horizontal and vertical pixel units installed on the\n> > > + * camera sensor\n> > > + *\n> > > + * The returned pixel array size is the number of pixels units physically\n> > > + * installed on the sensor, including non-active ones, such as pixels used\n> > > + * for calibration or testing.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn CameraSensor::activeArea()\n> > > + * \\brief Retrieve the active pixels area\n> > > + * \\return A rectangle describing the active pixel area\n> > > + *\n> > > + * The returned rectangle is contained in the larger pixel array area,\n> > > + * returned by the CameraSensor::pixelArray() operation.\n> > > + *\n> > > + * \\todo This operation collides with CameraSensor::resolution() as they\n> > > + * both reports the same information (bugs in the driver code apart).\n> > > + * Also, for some sensors, the maximum available resolution might depend on\n> > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> > > + * is different from the maximum available resolution for 16:9 format). This\n> > > + * has to be sorted out as well.\n> >\n> > Should we remove CameraSensor::resolution() in favor if new more\n> > \"correct\" way of finding out the sensor resolution?\n> >\n> \n> Good question.. another option would be to get the maximum resolution\n> in regard to a specific aspect ratio... I would say I should better\n> look into the current usages of this operation...\n> \n> > > + *\n> > > + * \\sa Size::contains()\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Retrieve the best sensor format for a desired output\n> > > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > index 32b0faeadc63..9b60768a57a7 100644\n> > > --- a/src/libcamera/geometry.cpp\n> > > +++ b/src/libcamera/geometry.cpp\n> > > @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> > > \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn Size::contains()\n> > > + * \\param rectangle The rectangle to verify\n> > > + * \\return True if \\a rectangle is contained in the area represented by this\n> > > + * Size, false otherwise\n> >\n> > Return comes at the end of the documentation block.\n> >\n> \n> True, thanks for spotting\n> \n> > > + *\n> > > + * Return true if \\a rectangle is completely contained in the area represented\n> > > + * by the width and height of this Size, with its top left corner assumed to be\n> > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> > > + * horizontal and vertical positive displacement from the Size top left corner,\n> > > + * from where the rectangle's horizontal and vertical sizes are calculated.\n> > > + */\n> > > +bool Size::contains(const Rectangle &rectangle) const\n> > > +{\n> > > +\tif (rectangle.x < 0 || rectangle.y < 0)\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (rectangle.x + rectangle.w > width ||\n> > > +\t rectangle.y + rectangle.h > height)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compare sizes for equality\n> > > * \\return True if the two sizes are equal, false otherwise\n> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > index 32d39127b275..abf5344cf9d8 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -38,6 +38,8 @@ public:\n> > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > \tconst std::vector &sizes() const { return sizes_; }\n> > > \tconst Size &resolution() const;\n> > > +\tconst Size pixelArray() const { return pixelArray_; }\n> > > +\tconst Rectangle activeArea() const { return activeArea_; }\n> > >\n> > > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > \t\t\t\t const Size &size) const;\n> > > @@ -59,6 +61,8 @@ private:\n> > >\n> > > \tCameraLocation location_;\n> > > \tunsigned int rotation_;\n> > > +\tSize pixelArray_;\n> > > +\tRectangle activeArea_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.23.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A9AA60BCF\n\tfor ;\n\tThu, 29 Aug 2019 10:49:48 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id l11so1869974lfk.6\n\tfor ;\n\tThu, 29 Aug 2019 01:49:48 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\ty20sm248626ljy.55.2019.08.29.01.49.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 01:49:46 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=rarytMCxVzJJdntbz82xcRM4XEjwH18a/UPCjj+veEI=;\n\tb=b0rmXf+46VVoZPp+Ioko3Uq6bWNmGGO4KsTJ761qgwmXj5NHWoydXqMLnPpULkiNeV\n\t7foW1lcGRHr4MEC8fCcp30LYIURdj1OOSKMfn45qKQ5rUcWIRQ/ixPW1JOmN1MzvQ/+6\n\t4/MlLKcgujCLcxZtKUDIiujmA+Rf2i8r4DsgCAU2JC/aEacUSOrKylNxft8ZLlRE7j/c\n\tD2Cvw1JyAFz+dqij2qWA4miVcTGQkoq+bORBHxQsvlWuC6URpKDOOA5PKhOhuv5C+lcV\n\tfIOa/JTeO+L3safpuoz95ft8Po4Nm3LhmD4uPST0mguHS43kvYRqu0pUtvkhZC7zhNic\n\t/ahQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=rarytMCxVzJJdntbz82xcRM4XEjwH18a/UPCjj+veEI=;\n\tb=L+c5uaYfDSFDSIp/PzNCphdSTvNkMrYHmNWTPP+q7XrUQj9HcfmuxI4jVqbrmmSGLm\n\tii87GAkIETgM0fIX8l3gsN1SGP6E6lty5zBdk+GZd3jEaC4nyPlsnIdLElOnpZhYTFsz\n\tR59ieuLqTfxwbu+MIStbK8MDAByEJKHZSQLaJ0hoJuEvubFE5yIL8sWizh4fJVybLdti\n\twZiVhoEHXw6qF/4YeBAcV6fkhXpSZPJQEbnnksZJxrQUm1UkZE+oqijcn8955DwfCP91\n\tAErux+twOCRQ/ohepdKVMp6zB/v3L/3DcYeNcy/R/0iSYaCij49ANlqt303XRqEZUXEk\n\tLuNQ==","X-Gm-Message-State":"APjAAAUmXElxTzGQEmwGXXhmv0ZzCLUd2lH4zpLEivJh7CNQ4lpIn7/P\n\t861hQOagPx+AGDhpw1bjdyMWXQ==","X-Google-Smtp-Source":"APXvYqxYBScO80T/UNWnaFgAT55HxSv2jkeWoiGhIAMKj84WjfUJfR9ccplk1D+rROk5eGrUq3HR3g==","X-Received":"by 2002:a19:ec16:: with SMTP id b22mr5418853lfa.1.1567068587514; \n\tThu, 29 Aug 2019 01:49:47 -0700 (PDT)","Date":"Thu, 29 Aug 2019 10:49:45 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829084945.GV28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>\n\t<20190828114810.GR28351@bigcity.dyn.berto.se>\n\t<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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, 29 Aug 2019 08:49:48 -0000"}},{"id":2535,"web_url":"https://patchwork.libcamera.org/comment/2535/","msgid":"<20190829090736.dtjdch365e52v2w6@uno.localdomain>","date":"2019-08-29T09:07:36","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> > > > Retrieve the camera sensor pixel array sizes and the active pixel area\n> > > > using the V4L2Subdevice selection API.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/geometry.h | 1 +\n> > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> > > > src/libcamera/geometry.cpp | 24 +++++++++++++\n> > > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > > 4 files changed, 80 insertions(+)\n> > >\n> > > I think you should split the changes to geometry.{cpp,h} into its own\n> > > patch.\n> > >\n> > Mmmm, I know we stand on two opposite sides here... the change is\n> > rather trivial and it makes sense to me to show its usage in the same\n> > patch to avoid people jumping from one patch to another during review.\n> > I could split if this bothers you, but it's not worth it in my opinion\n>\n> I agree this is a fight you and I will enjoy for many years I hope,\n> until I win in the end of course ;-P I don't feel strongly about it so\n> don't spend energy on it if you don't want to for trivial patches. For\n> mid to large size patches I feel very strongly on this topic tho.\n>\n> I know you feel it's easier to review larger patches and not \"jump\"\n> between multiple ones. For me it's the opposite, I spend 4 times longer\n> reviewing a patch like this because my warning light go off asking why\n> the patch do two or more things. I try to find the reason for this but\n> can't, and once I realise the patch do more then one thing without it\n> strictly needing to I comment on it to release my frustration ;-)\n>\n> I think this is some sort of meta-bikesheeding discussion, not sure if\n> that is a good thing or not :-)\n>\nSo I'm glad we have demonstrated we're both right here (or both wrong)\nas by definition bikeshedding discussions are made to disappoint all\nthe involved parties.\n\nFor bigger patches I agree, but please let me keep this one the way it\nis...\n\nOne less metaphysical question below..\n\n> >\n> > > >\n> > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > index 52f4d010de76..b91fcfa1dd17 100644\n> > > > --- a/include/libcamera/geometry.h\n> > > > +++ b/include/libcamera/geometry.h\n> > > > @@ -42,6 +42,7 @@ struct Size {\n> > > > \tunsigned int height;\n> > > >\n> > > > \tconst std::string toString() const;\n> > > > +\tbool contains(const Rectangle &rectangle) const;\n> > > > };\n> > > >\n> > > > bool operator==(const Size &lhs, const Size &rhs);\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index fc7fdcdcaf5b..b6ccaae0930b 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -128,6 +128,28 @@ int CameraSensor::init()\n> > > > \t}\n> > > > \trotation_ = value;\n> > > >\n> > > > +\t/*\n> > > > +\t * Retrieve and store the sensor pixel array size and active area\n> > > > +\t * rectangle.\n> > > > +\t */\n> > > > +\tRectangle rect;\n> > > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\tpixelArray_.width = rect.w;\n> > > > +\tpixelArray_.height = rect.h;\n> > > > +\n> > > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tif (!pixelArray_.contains(rect)) {\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\tactiveArea_ = rect;\n\nI hoped for a comment here, as what's here happening is that we're\nvalidating the kernel driver reported parameters. Is this something\nwe want to do or should we assume the kernel side is, as we all know\nvery well, perfect ?\n\nThanks\n j\n\n> > > > +\n> > > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > > \tif (formats.isEmpty()) {\n> > > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> > > > \treturn sizes_.back();\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\fn CameraSensor::pixelArray()\n> > > > + * \\brief Retrieve the camera sensor pixel array dimension\n> > > > + * \\return The number of horizontal and vertical pixel units installed on the\n> > > > + * camera sensor\n> > > > + *\n> > > > + * The returned pixel array size is the number of pixels units physically\n> > > > + * installed on the sensor, including non-active ones, such as pixels used\n> > > > + * for calibration or testing.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn CameraSensor::activeArea()\n> > > > + * \\brief Retrieve the active pixels area\n> > > > + * \\return A rectangle describing the active pixel area\n> > > > + *\n> > > > + * The returned rectangle is contained in the larger pixel array area,\n> > > > + * returned by the CameraSensor::pixelArray() operation.\n> > > > + *\n> > > > + * \\todo This operation collides with CameraSensor::resolution() as they\n> > > > + * both reports the same information (bugs in the driver code apart).\n> > > > + * Also, for some sensors, the maximum available resolution might depend on\n> > > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> > > > + * is different from the maximum available resolution for 16:9 format). This\n> > > > + * has to be sorted out as well.\n> > >\n> > > Should we remove CameraSensor::resolution() in favor if new more\n> > > \"correct\" way of finding out the sensor resolution?\n> > >\n> >\n> > Good question.. another option would be to get the maximum resolution\n> > in regard to a specific aspect ratio... I would say I should better\n> > look into the current usages of this operation...\n> >\n> > > > + *\n> > > > + * \\sa Size::contains()\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > index 32b0faeadc63..9b60768a57a7 100644\n> > > > --- a/src/libcamera/geometry.cpp\n> > > > +++ b/src/libcamera/geometry.cpp\n> > > > @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> > > > \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\fn Size::contains()\n> > > > + * \\param rectangle The rectangle to verify\n> > > > + * \\return True if \\a rectangle is contained in the area represented by this\n> > > > + * Size, false otherwise\n> > >\n> > > Return comes at the end of the documentation block.\n> > >\n> >\n> > True, thanks for spotting\n> >\n> > > > + *\n> > > > + * Return true if \\a rectangle is completely contained in the area represented\n> > > > + * by the width and height of this Size, with its top left corner assumed to be\n> > > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> > > > + * horizontal and vertical positive displacement from the Size top left corner,\n> > > > + * from where the rectangle's horizontal and vertical sizes are calculated.\n> > > > + */\n> > > > +bool Size::contains(const Rectangle &rectangle) const\n> > > > +{\n> > > > +\tif (rectangle.x < 0 || rectangle.y < 0)\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\tif (rectangle.x + rectangle.w > width ||\n> > > > +\t rectangle.y + rectangle.h > height)\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Compare sizes for equality\n> > > > * \\return True if the two sizes are equal, false otherwise\n> > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > > index 32d39127b275..abf5344cf9d8 100644\n> > > > --- a/src/libcamera/include/camera_sensor.h\n> > > > +++ b/src/libcamera/include/camera_sensor.h\n> > > > @@ -38,6 +38,8 @@ public:\n> > > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > > \tconst std::vector &sizes() const { return sizes_; }\n> > > > \tconst Size &resolution() const;\n> > > > +\tconst Size pixelArray() const { return pixelArray_; }\n> > > > +\tconst Rectangle activeArea() const { return activeArea_; }\n> > > >\n> > > > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > \t\t\t\t const Size &size) const;\n> > > > @@ -59,6 +61,8 @@ private:\n> > > >\n> > > > \tCameraLocation location_;\n> > > > \tunsigned int rotation_;\n> > > > +\tSize pixelArray_;\n> > > > +\tRectangle activeArea_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > --\n> > > > 2.23.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BA0560BCF\n\tfor ;\n\tThu, 29 Aug 2019 11:06:04 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id DE79C20000D;\n\tThu, 29 Aug 2019 09:06:03 +0000 (UTC)"],"Date":"Thu, 29 Aug 2019 11:07:36 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829090736.dtjdch365e52v2w6@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>\n\t<20190828114810.GR28351@bigcity.dyn.berto.se>\n\t<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>\n\t<20190829084945.GV28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vyk3ilx6cw4675u3\"","Content-Disposition":"inline","In-Reply-To":"<20190829084945.GV28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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, 29 Aug 2019 09:06:04 -0000"}},{"id":2546,"web_url":"https://patchwork.libcamera.org/comment/2546/","msgid":"<20190829142722.GY28351@bigcity.dyn.berto.se>","date":"2019-08-29T14:27:22","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-08-29 11:07:36 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> > > > > Retrieve the camera sensor pixel array sizes and the active pixel area\n> > > > > using the V4L2Subdevice selection API.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/geometry.h | 1 +\n> > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> > > > > src/libcamera/geometry.cpp | 24 +++++++++++++\n> > > > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > > > 4 files changed, 80 insertions(+)\n> > > >\n> > > > I think you should split the changes to geometry.{cpp,h} into its own\n> > > > patch.\n> > > >\n> > > Mmmm, I know we stand on two opposite sides here... the change is\n> > > rather trivial and it makes sense to me to show its usage in the same\n> > > patch to avoid people jumping from one patch to another during review.\n> > > I could split if this bothers you, but it's not worth it in my opinion\n> >\n> > I agree this is a fight you and I will enjoy for many years I hope,\n> > until I win in the end of course ;-P I don't feel strongly about it so\n> > don't spend energy on it if you don't want to for trivial patches. For\n> > mid to large size patches I feel very strongly on this topic tho.\n> >\n> > I know you feel it's easier to review larger patches and not \"jump\"\n> > between multiple ones. For me it's the opposite, I spend 4 times longer\n> > reviewing a patch like this because my warning light go off asking why\n> > the patch do two or more things. I try to find the reason for this but\n> > can't, and once I realise the patch do more then one thing without it\n> > strictly needing to I comment on it to release my frustration ;-)\n> >\n> > I think this is some sort of meta-bikesheeding discussion, not sure if\n> > that is a good thing or not :-)\n> >\n> So I'm glad we have demonstrated we're both right here (or both wrong)\n> as by definition bikeshedding discussions are made to disappoint all\n> the involved parties.\n\n:-)\n\n> \n> For bigger patches I agree, but please let me keep this one the way it\n> is...\n\nAbsolutely, no worries. Sorry if that was not clear in my previous mail \nthat I do not feel strongly about it in regard to this patch.\n\n> \n> One less metaphysical question below..\n> \n> > >\n> > > > >\n> > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > > index 52f4d010de76..b91fcfa1dd17 100644\n> > > > > --- a/include/libcamera/geometry.h\n> > > > > +++ b/include/libcamera/geometry.h\n> > > > > @@ -42,6 +42,7 @@ struct Size {\n> > > > > \tunsigned int height;\n> > > > >\n> > > > > \tconst std::string toString() const;\n> > > > > +\tbool contains(const Rectangle &rectangle) const;\n> > > > > };\n> > > > >\n> > > > > bool operator==(const Size &lhs, const Size &rhs);\n> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > index fc7fdcdcaf5b..b6ccaae0930b 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -128,6 +128,28 @@ int CameraSensor::init()\n> > > > > \t}\n> > > > > \trotation_ = value;\n> > > > >\n> > > > > +\t/*\n> > > > > +\t * Retrieve and store the sensor pixel array size and active area\n> > > > > +\t * rectangle.\n> > > > > +\t */\n> > > > > +\tRectangle rect;\n> > > > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> > > > > +\tif (ret)\n> > > > > +\t\treturn ret;\n> > > > > +\tpixelArray_.width = rect.w;\n> > > > > +\tpixelArray_.height = rect.h;\n> > > > > +\n> > > > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > > > > +\tif (ret)\n> > > > > +\t\treturn ret;\n> > > > > +\n> > > > > +\tif (!pixelArray_.contains(rect)) {\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\tactiveArea_ = rect;\n> \n> I hoped for a comment here, as what's here happening is that we're\n> validating the kernel driver reported parameters. Is this something\n> we want to do or should we assume the kernel side is, as we all know\n> very well, perfect ?\n\nI'm divided, it's good to have checks but I also thinks the kernel \nshould report the correct thing. And if not and things so south it \nshould be fixed in the kernel.\n\n> \n> Thanks\n> j\n> \n> > > > > +\n> > > > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > > > \tif (formats.isEmpty()) {\n> > > > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> > > > > \treturn sizes_.back();\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn CameraSensor::pixelArray()\n> > > > > + * \\brief Retrieve the camera sensor pixel array dimension\n> > > > > + * \\return The number of horizontal and vertical pixel units installed on the\n> > > > > + * camera sensor\n> > > > > + *\n> > > > > + * The returned pixel array size is the number of pixels units physically\n> > > > > + * installed on the sensor, including non-active ones, such as pixels used\n> > > > > + * for calibration or testing.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn CameraSensor::activeArea()\n> > > > > + * \\brief Retrieve the active pixels area\n> > > > > + * \\return A rectangle describing the active pixel area\n> > > > > + *\n> > > > > + * The returned rectangle is contained in the larger pixel array area,\n> > > > > + * returned by the CameraSensor::pixelArray() operation.\n> > > > > + *\n> > > > > + * \\todo This operation collides with CameraSensor::resolution() as they\n> > > > > + * both reports the same information (bugs in the driver code apart).\n> > > > > + * Also, for some sensors, the maximum available resolution might depend on\n> > > > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> > > > > + * is different from the maximum available resolution for 16:9 format). This\n> > > > > + * has to be sorted out as well.\n> > > >\n> > > > Should we remove CameraSensor::resolution() in favor if new more\n> > > > \"correct\" way of finding out the sensor resolution?\n> > > >\n> > >\n> > > Good question.. another option would be to get the maximum resolution\n> > > in regard to a specific aspect ratio... I would say I should better\n> > > look into the current usages of this operation...\n> > >\n> > > > > + *\n> > > > > + * \\sa Size::contains()\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Retrieve the best sensor format for a desired output\n> > > > > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > > index 32b0faeadc63..9b60768a57a7 100644\n> > > > > --- a/src/libcamera/geometry.cpp\n> > > > > +++ b/src/libcamera/geometry.cpp\n> > > > > @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> > > > > \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn Size::contains()\n> > > > > + * \\param rectangle The rectangle to verify\n> > > > > + * \\return True if \\a rectangle is contained in the area represented by this\n> > > > > + * Size, false otherwise\n> > > >\n> > > > Return comes at the end of the documentation block.\n> > > >\n> > >\n> > > True, thanks for spotting\n> > >\n> > > > > + *\n> > > > > + * Return true if \\a rectangle is completely contained in the area represented\n> > > > > + * by the width and height of this Size, with its top left corner assumed to be\n> > > > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> > > > > + * horizontal and vertical positive displacement from the Size top left corner,\n> > > > > + * from where the rectangle's horizontal and vertical sizes are calculated.\n> > > > > + */\n> > > > > +bool Size::contains(const Rectangle &rectangle) const\n> > > > > +{\n> > > > > +\tif (rectangle.x < 0 || rectangle.y < 0)\n> > > > > +\t\treturn false;\n> > > > > +\n> > > > > +\tif (rectangle.x + rectangle.w > width ||\n> > > > > +\t rectangle.y + rectangle.h > height)\n> > > > > +\t\treturn false;\n> > > > > +\n> > > > > +\treturn true;\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Compare sizes for equality\n> > > > > * \\return True if the two sizes are equal, false otherwise\n> > > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > > > index 32d39127b275..abf5344cf9d8 100644\n> > > > > --- a/src/libcamera/include/camera_sensor.h\n> > > > > +++ b/src/libcamera/include/camera_sensor.h\n> > > > > @@ -38,6 +38,8 @@ public:\n> > > > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > > > > \tconst std::vector &sizes() const { return sizes_; }\n> > > > > \tconst Size &resolution() const;\n> > > > > +\tconst Size pixelArray() const { return pixelArray_; }\n> > > > > +\tconst Rectangle activeArea() const { return activeArea_; }\n> > > > >\n> > > > > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > > > > \t\t\t\t const Size &size) const;\n> > > > > @@ -59,6 +61,8 @@ private:\n> > > > >\n> > > > > \tCameraLocation location_;\n> > > > > \tunsigned int rotation_;\n> > > > > +\tSize pixelArray_;\n> > > > > +\tRectangle activeArea_;\n> > > > > };\n> > > > >\n> > > > > } /* namespace libcamera */\n> > > > > --\n> > > > > 2.23.0\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards,\n> > > > Niklas Söderlund\n> >\n> >\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3F0A60BB0\n\tfor ;\n\tThu, 29 Aug 2019 16:27:24 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id r134so1796737lff.12\n\tfor ;\n\tThu, 29 Aug 2019 07:27:24 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\tx13sm374474ljm.7.2019.08.29.07.27.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 07:27:23 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=+vt1t2bnAPM4ofU76xCCDWs2EKE6RCyfID3x8s3sLSI=;\n\tb=kfKgmIVXOjdp9FvqZE0rtlbPSlPw5UtK10ArCHfuxaJXhNNSBhEC0LmYmfV6sLHM0x\n\tUlnyWeqLN1q/FsC21TWFCgnFJbVPzEUxsCNMNbAD6Bz00MkhbznUIiF75E2NAtyOCi0y\n\t8puFUDOotb7+zWQnUdPa1f5Wn+FDrXeAktP6MOBtJ8kSeOpLcHDOheVrRNXQ1AqDOddY\n\tRRbscfHjfvvyG4SFDXt29AYLNlWQO209hJJ4CFZmA3fTjiltH22iwee671QIzzRbDdma\n\tIGsb2lg2joNqpg2U34MIojQJfly4KXtd6O4PmkdtzmREpW7974mjQ2pLyBNyWxYgJb/J\n\tQpCA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=+vt1t2bnAPM4ofU76xCCDWs2EKE6RCyfID3x8s3sLSI=;\n\tb=dJa9gPzKG8gXotU4PzWvbpEUUCZ0b2NEbfyokixony1p/jZYND5GIt496XyEwlxqgP\n\tRKppwJCg8+L3Ky1yDLqoft4jByvJC5+xBAE0Nnb6xM2sCIObm5EtAfbtrSpXgIQUz8sC\n\tl8BCKv3cx71Kpm4zP6FWljCEBj0md6wUN5H3ZQ3tmUAuVU3D5+IyFnnhfqVOpz1b4fEV\n\tP4iElYz4yKefsZwI0M2KmwriMdOJq5ipaSsv0DQ78vWeRFUhmMHidPrtxrY5fx9y9gCL\n\tSf+DzZrSQz1WrVtDwUvMsQdG0dhZZsPcv5JWlv90iFlKom4Qcd4sdB21Ri2Y7LNRrMDs\n\t2SrA==","X-Gm-Message-State":"APjAAAWD+4sbjw7b8/+joosSPm7lCaNWa1IDWLB3mgoRfTKtukajUW95\n\tILDcIJQFN/aoqiW2n7O1bErGrw==","X-Google-Smtp-Source":"APXvYqwn0Y9tlNkbeJ/6kqey+FSbC/57rqcjKgfzJKIKNo9UO095VsZPtXFt7Ans7QIgUfbCCNSZOg==","X-Received":"by 2002:a19:428f:: with SMTP id\n\tp137mr6534877lfa.149.1567088843972; \n\tThu, 29 Aug 2019 07:27:23 -0700 (PDT)","Date":"Thu, 29 Aug 2019 16:27:22 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829142722.GY28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>\n\t<20190828114810.GR28351@bigcity.dyn.berto.se>\n\t<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>\n\t<20190829084945.GV28351@bigcity.dyn.berto.se>\n\t<20190829090736.dtjdch365e52v2w6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190829090736.dtjdch365e52v2w6@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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, 29 Aug 2019 14:27:25 -0000"}},{"id":2576,"web_url":"https://patchwork.libcamera.org/comment/2576/","msgid":"<20190903201741.GE4788@pendragon.ideasonboard.com>","date":"2019-09-03T20:17:41","subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor sizes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Aug 29, 2019 at 04:27:22PM +0200, Niklas Söderlund wrote:\n> On 2019-08-29 11:07:36 +0200, Jacopo Mondi wrote:\n> > On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote:\n> >> On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote:\n> >>> On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:\n> >>>> On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:\n> >>>>> Retrieve the camera sensor pixel array sizes and the active pixel area\n\ns/sizes/size/\n\n> >>>>> using the V4L2Subdevice selection API.\n> >>>>>\n> >>>>> Signed-off-by: Jacopo Mondi \n> >>>>> ---\n> >>>>> include/libcamera/geometry.h | 1 +\n> >>>>> src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++\n> >>>>> src/libcamera/geometry.cpp | 24 +++++++++++++\n> >>>>> src/libcamera/include/camera_sensor.h | 4 +++\n> >>>>> 4 files changed, 80 insertions(+)\n> >>>>\n> >>>> I think you should split the changes to geometry.{cpp,h} into its own\n> >>>> patch.\n> >>>>\n> >>> Mmmm, I know we stand on two opposite sides here... the change is\n> >>> rather trivial and it makes sense to me to show its usage in the same\n> >>> patch to avoid people jumping from one patch to another during review.\n> >>> I could split if this bothers you, but it's not worth it in my opinion\n> >>\n> >> I agree this is a fight you and I will enjoy for many years I hope,\n> >> until I win in the end of course ;-P I don't feel strongly about it so\n> >> don't spend energy on it if you don't want to for trivial patches. For\n> >> mid to large size patches I feel very strongly on this topic tho.\n> >>\n> >> I know you feel it's easier to review larger patches and not \"jump\"\n> >> between multiple ones. For me it's the opposite, I spend 4 times longer\n> >> reviewing a patch like this because my warning light go off asking why\n> >> the patch do two or more things. I try to find the reason for this but\n> >> can't, and once I realise the patch do more then one thing without it\n> >> strictly needing to I comment on it to release my frustration ;-)\n> >>\n> >> I think this is some sort of meta-bikesheeding discussion, not sure if\n> >> that is a good thing or not :-)\n> >>\n> > So I'm glad we have demonstrated we're both right here (or both wrong)\n> > as by definition bikeshedding discussions are made to disappoint all\n> > the involved parties.\n> \n> :-)\n\nI'm afraid I will be as disappointed as Niklas here :-) My preference\nwould also have been to split the geometry change.\n\n> > For bigger patches I agree, but please let me keep this one the way it\n> > is...\n> \n> Absolutely, no worries. Sorry if that was not clear in my previous mail \n> that I do not feel strongly about it in regard to this patch.\n> \n> > One less metaphysical question below..\n> > \n> >>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> >>>>> index 52f4d010de76..b91fcfa1dd17 100644\n> >>>>> --- a/include/libcamera/geometry.h\n> >>>>> +++ b/include/libcamera/geometry.h\n> >>>>> @@ -42,6 +42,7 @@ struct Size {\n> >>>>> \tunsigned int height;\n> >>>>>\n> >>>>> \tconst std::string toString() const;\n> >>>>> +\tbool contains(const Rectangle &rectangle) const;\n> >>>>> };\n> >>>>>\n> >>>>> bool operator==(const Size &lhs, const Size &rhs);\n> >>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>>>> index fc7fdcdcaf5b..b6ccaae0930b 100644\n> >>>>> --- a/src/libcamera/camera_sensor.cpp\n> >>>>> +++ b/src/libcamera/camera_sensor.cpp\n> >>>>> @@ -128,6 +128,28 @@ int CameraSensor::init()\n> >>>>> \t}\n> >>>>> \trotation_ = value;\n> >>>>>\n> >>>>> +\t/*\n> >>>>> +\t * Retrieve and store the sensor pixel array size and active area\n> >>>>> +\t * rectangle.\n> >>>>> +\t */\n> >>>>> +\tRectangle rect;\n> >>>>> +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);\n> >>>>> +\tif (ret)\n> >>>>> +\t\treturn ret;\n> >>>>> +\tpixelArray_.width = rect.w;\n> >>>>> +\tpixelArray_.height = rect.h;\n\nA Rectangle::size() method would be nice, you could then write\n\n\tpixelArray_ = rect.size();\n\n> >>>>> +\n> >>>>> +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> >>>>> +\tif (ret)\n> >>>>> +\t\treturn ret;\n> >>>>> +\n> >>>>> +\tif (!pixelArray_.contains(rect)) {\n> >>>>> +\t\tLOG(CameraSensor, Error)\n> >>>>> +\t\t\t<< \"Invalid camera sensor pixel array dimension\";\n> >>>>> +\t\treturn -EINVAL;\n> >>>>> +\t}\n> >>>>> +\tactiveArea_ = rect;\n> > \n> > I hoped for a comment here, as what's here happening is that we're\n> > validating the kernel driver reported parameters. Is this something\n> > we want to do or should we assume the kernel side is, as we all know\n> > very well, perfect ?\n> \n> I'm divided, it's good to have checks but I also thinks the kernel \n> should report the correct thing. And if not and things so south it \n> should be fixed in the kernel.\n\nI wouldn't have thought about guarding against this personally. I don't\nmind the check much, but I wonder how far we should go checking that the\nkernel behaves correctly.\n\n> >>>>> +\n> >>>>> \t/* Enumerate and cache media bus codes and sizes. */\n> >>>>> \tconst ImageFormats formats = subdev_->formats(0);\n> >>>>> \tif (formats.isEmpty()) {\n> >>>>> @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const\n> >>>>> \treturn sizes_.back();\n> >>>>> }\n> >>>>>\n> >>>>> +/**\n> >>>>> + * \\fn CameraSensor::pixelArray()\n> >>>>> + * \\brief Retrieve the camera sensor pixel array dimension\n> >>>>> + * \\return The number of horizontal and vertical pixel units installed on the\n> >>>>> + * camera sensor\n> >>>>> + *\n> >>>>> + * The returned pixel array size is the number of pixels units physically\n> >>>>> + * installed on the sensor, including non-active ones, such as pixels used\n> >>>>> + * for calibration or testing.\n\nI think we need a bit more details here, or possibly in the\ndocumentation of the CameraSensor class itself. We should have a diagram\nof how we model a sensor, and show when the pixel array, active area and\nresolution are.\n\nFurthermore, I would mention optical black pixels explicitly. To be\nprecise, wouldn't they qualify as active pixels, but not exposed. What\nother non-exposed or non-active pixels would we have (and what is a\nnon-active pixel) ?\n\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn CameraSensor::activeArea()\n> >>>>> + * \\brief Retrieve the active pixels area\n> >>>>> + * \\return A rectangle describing the active pixel area\n> >>>>> + *\n> >>>>> + * The returned rectangle is contained in the larger pixel array area,\n> >>>>> + * returned by the CameraSensor::pixelArray() operation.\n> >>>>> + *\n> >>>>> + * \\todo This operation collides with CameraSensor::resolution() as they\n> >>>>> + * both reports the same information (bugs in the driver code apart).\n> >>>>> + * Also, for some sensors, the maximum available resolution might depend on\n> >>>>> + * the image size ratio (ie the maximumx resolution for images in 4:3 format\n> >>>>> + * is different from the maximum available resolution for 16:9 format). This\n> >>>>> + * has to be sorted out as well.\n> >>>>\n> >>>> Should we remove CameraSensor::resolution() in favor if new more\n> >>>> \"correct\" way of finding out the sensor resolution?\n> >>>\n> >>> Good question.. another option would be to get the maximum resolution\n> >>> in regard to a specific aspect ratio... I would say I should better\n> >>> look into the current usages of this operation...\n\nCould you have a look ? I think we should address this todo item already\nif possible.\n\n> >>>>> + *\n> >>>>> + * \\sa Size::contains()\n\nIs this useful ?\n\n> >>>>> + */\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\brief Retrieve the best sensor format for a desired output\n> >>>>> * \\param[in] mbusCodes The list of acceptable media bus codes\n> >>>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> >>>>> index 32b0faeadc63..9b60768a57a7 100644\n> >>>>> --- a/src/libcamera/geometry.cpp\n> >>>>> +++ b/src/libcamera/geometry.cpp\n> >>>>> @@ -116,6 +116,30 @@ const std::string Size::toString() const\n> >>>>> \treturn std::to_string(width) + \"x\" + std::to_string(height);\n> >>>>> }\n> >>>>>\n> >>>>> +/**\n> >>>>> + * \\fn Size::contains()\n> >>>>> + * \\param rectangle The rectangle to verify\n> >>>>> + * \\return True if \\a rectangle is contained in the area represented by this\n> >>>>> + * Size, false otherwise\n> >>>>\n> >>>> Return comes at the end of the documentation block.\n> >>>\n> >>> True, thanks for spotting\n\nAnd the \\brief is missing.\n\n> >>>>> + *\n> >>>>> + * Return true if \\a rectangle is completely contained in the area represented\n> >>>>> + * by the width and height of this Size, with its top left corner assumed to be\n> >>>>> + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the\n> >>>>> + * horizontal and vertical positive displacement from the Size top left corner,\n> >>>>> + * from where the rectangle's horizontal and vertical sizes are calculated.\n\nI don't think this is the way to go. A rectangle \"contained\" in a size\nseems weird, and I would have intuitively thought this would check the\nrectangle size against the size.\n\nA better API would be to construct a Rectangle from a Point and a Size\n(as constructing it from just a size seems weird and ill-defined too),\nand add a bool Rectangle::contains(const Rectangle &rect) method.\n\nYou will also need to define if rectangles that have overlapping borders\nwould be considered as being contained or not. Could a.contains(b) &&\nb.contains(a) ever be true ?\n\nYou can add both the Point class, the new Rectangle constructor and the\nRectangle::contains() method in the same patch if you split them from\nthis one ;-)\n\n> >>>>> + */\n> >>>>> +bool Size::contains(const Rectangle &rectangle) const\n> >>>>> +{\n> >>>>> +\tif (rectangle.x < 0 || rectangle.y < 0)\n> >>>>> +\t\treturn false;\n> >>>>> +\n> >>>>> +\tif (rectangle.x + rectangle.w > width ||\n> >>>>> +\t rectangle.y + rectangle.h > height)\n> >>>>> +\t\treturn false;\n> >>>>> +\n> >>>>> +\treturn true;\n> >>>>> +}\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\brief Compare sizes for equality\n> >>>>> * \\return True if the two sizes are equal, false otherwise\n> >>>>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> >>>>> index 32d39127b275..abf5344cf9d8 100644\n> >>>>> --- a/src/libcamera/include/camera_sensor.h\n> >>>>> +++ b/src/libcamera/include/camera_sensor.h\n> >>>>> @@ -38,6 +38,8 @@ public:\n> >>>>> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> >>>>> \tconst std::vector &sizes() const { return sizes_; }\n> >>>>> \tconst Size &resolution() const;\n> >>>>> +\tconst Size pixelArray() const { return pixelArray_; }\n> >>>>> +\tconst Rectangle activeArea() const { return activeArea_; }\n> >>>>>\n> >>>>> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> >>>>> \t\t\t\t const Size &size) const;\n> >>>>> @@ -59,6 +61,8 @@ private:\n> >>>>>\n> >>>>> \tCameraLocation location_;\n> >>>>> \tunsigned int rotation_;\n> >>>>> +\tSize pixelArray_;\n> >>>>> +\tRectangle activeArea_;\n> >>>>> };\n> >>>>>\n> >>>>> } /* namespace libcamera */","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 03FB260BCF\n\tfor ;\n\tTue, 3 Sep 2019 22:17:49 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-18-41-nat.elisa-mobile.fi\n\t[85.76.18.41])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63221542;\n\tTue, 3 Sep 2019 22:17:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567541868;\n\tbh=SFEHPQnhQ8jtCqdG9GbkdZvF6AGQ+J+QjsaSXq4uddE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iRF7rmdcGoXVFU+kEhL7lJbcr1ltiRehZn7gmYLMLnSSMpVZPgLyIVEPLeGp8pjZU\n\trNofRBZOxHDMi1UASB4Yy0tIUinAcrlIUJtU97Q2EgmAS6qvsDso4xms30yzY5X4xE\n\tXP17eJN070H6Aa6rvqhw5ucJwzvvnA4kp93dp9aE=","Date":"Tue, 3 Sep 2019 23:17:41 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190903201741.GE4788@pendragon.ideasonboard.com>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-8-jacopo@jmondi.org>\n\t<20190828114810.GR28351@bigcity.dyn.berto.se>\n\t<20190828163406.w3k7l5e6pn3lrjtp@uno.localdomain>\n\t<20190829084945.GV28351@bigcity.dyn.berto.se>\n\t<20190829090736.dtjdch365e52v2w6@uno.localdomain>\n\t<20190829142722.GY28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190829142722.GY28351@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor:\n\tRetrieve sensor 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":"Tue, 03 Sep 2019 20:17:49 -0000"}}]