[{"id":27776,"web_url":"https://patchwork.libcamera.org/comment/27776/","msgid":"<169473031838.2843013.4523261506077905103@ping.linuxembedded.co.uk>","date":"2023-09-14T22:25:18","subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: camera: Introduce\n\tSensorConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2023-07-31 12:31:13)\n> Introduce SensorConfiguration in the libcamera API.\n> \n> The SensorConfiguration is part of the CameraConfiguration class\n> and allows applications to control the sensor settings.\n> \n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  include/libcamera/camera.h |  43 +++++++++\n>  src/libcamera/camera.cpp   | 180 +++++++++++++++++++++++++++++++++++++\n>  2 files changed, 223 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 004bc89455f5..b2aa8d467feb 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -19,6 +19,7 @@\n>  #include <libcamera/base/signal.h>\n>  \n>  #include <libcamera/controls.h>\n> +#include <libcamera/geometry.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  #include <libcamera/transform.h>\n> @@ -30,6 +31,47 @@ class FrameBufferAllocator;\n>  class PipelineHandler;\n>  class Request;\n>  \n> +class SensorConfiguration\n> +{\n> +public:\n> +       unsigned int bitDepth = 0;\n> +\n> +       Rectangle analogCrop;\n> +\n> +       struct {\n> +               unsigned int binX = 1;\n> +               unsigned int binY = 1;\n> +       } binning;\n> +\n> +       struct {\n> +               unsigned int xOddInc = 1;\n> +               unsigned int xEvenInc = 1;\n> +               unsigned int yOddInc = 1;\n> +               unsigned int yEvenInc = 1;\n> +       } skipping;\n> +\n> +       Size outputSize;\n> +\n> +       bool valid() const\n> +       {\n> +               return validate() != Invalid;\n> +       }\n> +\n> +       explicit operator bool() const\n> +       {\n> +               return validate() == Populated;\n> +       }\n> +\n> +private:\n> +       enum Status {\n> +               Unpopulated,\n> +               Populated,\n> +               Invalid,\n> +       };\n> +\n> +       Status validate() const;\n> +};\n> +\n>  class CameraConfiguration\n>  {\n>  public:\n> @@ -66,6 +108,7 @@ public:\n>         bool empty() const;\n>         std::size_t size() const;\n>  \n> +       SensorConfiguration sensorConfig;\n>         Transform transform;\n>  \n>  protected:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 0eecee766f00..e796a5da28c6 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -97,6 +97,21 @@\n>   * implemented in the above order at the hardware level. The libcamera pipeline\n>   * handlers translate the pipeline model to the real hardware configuration.\n>   *\n> + * \\subsection camera-sensor-mode Camera Sensor Model\n> + *\n> + * libcamera allows applications to control the configuration of the camera\n> + * sensor, particularly it allows to control the frame geometry and frame\n> + * delivery rate of the sensor.\n> + *\n> + * The camera sensor configuration applies to all streams produced by a camera\n> + * from the same image source.\n> + *\n> + * More details about the libcamera implemented camera sensor model are\n\nMore details about the camera sensor model implemented by libcamera are\n\n> + * available in the libcamera camera-sensor-model documentation page.\n> + *\n> + * The sensor configuration is specified by applications by populating the\n> + * CameraConfiguration::sensorConfig class member.\n> + *\n>   * \\subsection digital-zoom Digital Zoom\n>   *\n>   * Digital zoom is implemented as a combination of the cropping and scaling\n> @@ -111,6 +126,161 @@ namespace libcamera {\n>  \n>  LOG_DECLARE_CATEGORY(Camera)\n>  \n> +/**\n> + * \\class SensorConfiguration\n> + * \\brief Camera sensor configuration\n> + *\n> + * The SensorConfiguration class collects parameters to control the operations\n> + * of the camera sensor, accordingly to the abstract camera sensor model\n> + * implemented by libcamera.\n> + *\n> + * \\todo Applications shall fully populate all fields of the\n> + * CameraConfiguration::sensorConfig class members before validating the\n> + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if\n> + * any of its parameters cannot be applied to the sensor in use, the\n> + * CameraConfiguration validation process will fail and return\n> + * CameraConfiguration::Status::Invalid.\n> + *\n> + * Applications that populate the SensorConfiguration class members are\n> + * expected to be highly-specialized applications that know what sensor\n> + * they are operating with and what parameters are valid for the sensor in use.\n> + *\n> + * A detailed description of the abstract camera sensor model implemented by\n> + * libcamera and the description of its configuration parameters is available\n> + * in the libcamera documentation camera-sensor-model file.\n> + */\n> +\n> +/**\n> + * \\enum SensorConfiguration::Status\n> + * \\brief The sensor configuration validation status\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::bitDepth\n> + * \\brief The sensor image format bit depth\n> + *\n> + * The number of bits (resolution) used to represent a pixel sample.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::analogCrop\n> + * \\brief The analog crop rectangle\n> + *\n> + * The selected portion of the active pixel array used to produce the image\n> + * frame.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::binning\n> + * \\brief Sensor binning configuration\n> + *\n> + * Refer to the camera-sensor-model documentation for an accurate description\n> + * of the binning operations. Disabled by default.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::binX\n> + * \\brief Horizontal binning factor\n> + *\n> + * The horizontal binning factor. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::binY\n> + * \\brief Vertical binning factor\n> + *\n> + * The vertical binning factor. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::skipping\n> + * \\brief The sensor skipping configuration\n> + *\n> + * Refer to the camera-sensor-model documentation for an accurate description\n> + * of the skipping operations.\n> + *\n> + * If no skipping is performed, all the structure fields should be\n> + * set to 1. Disabled by default.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::xOddInc\n> + * \\brief Horizontal increment for odd rows. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::xEvenInc\n> + * \\brief Horizontal increment for even rows. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::yOddInc\n> + * \\brief Vertical increment for odd columns. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::yEvenInc\n> + * \\brief Vertical increment for even columns. Default to 1.\n> + */\n> +\n> +/**\n> + * \\var SensorConfiguration::outputSize\n> + * \\brief The frame output (visible) size\n> + *\n> + * The size of the data frame as received by the host processor.\n> + */\n> +\n> +/**\n> + * \\fn SensorConfiguration::valid() const\n> + * \\brief Validate the SensorConfiguration\n> + *\n> + * Validate the sensor configuration.\n> + *\n> + * \\todo A sensor configuration is valid (or well-formed) if it's either\n> + * completely un-populated or fully populated. For now allow applications to\n> + * populate the bitDepth and the outputSize only.\n> + *\n> + * \\return True if the SensorConfiguration is either fully populated or\n> + * un-populated, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn SensorConfiguration::operator bool() const\n> + * \\brief Test if a SensorConfiguration is fully populated\n> + * \\return True if the SensorConfiguration is fully populated\n> + */\n> +\n> +/**\n> + * \\brief Validate the sensor configuration\n> + *\n> + * \\todo A sensor configuration is valid (or well-formed) if it's either\n> + * completely un-populated or fully populated. For now allow applications to\n> + * populate the bitDepth and the outputSize only.\n> + *\n> + * \\return The sensor configuration status\n> + * \\retval Unpopulated The sensor configuration is fully unpopulated\n> + * \\retval Populated The sensor configuration is fully populated\n> + * \\retval Invalid The sensor configuration is invalid (not fully populated\n> + * and not fully unpopulated)\n> + */\n> +SensorConfiguration::Status SensorConfiguration::validate() const\n> +{\n> +       if (bitDepth && binning.binX && binning.binY &&\n> +           skipping.xOddInc && skipping.yOddInc &&\n> +           skipping.xEvenInc && skipping.yEvenInc &&\n> +           !outputSize.isNull())\n> +               return Populated;\n> +\n> +       if (!bitDepth &&\n> +           binning.binX <= 1 && binning.binY <= 1 &&\n> +           skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&\n> +           skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&\n\nThis looks like the <= is to support if the struct / class has been\nunintialised/memset or somehow allocated with \n\n\tSensorConfiguration = {}; ?\n\nOtherwise - is the <= necessary ?\n\nI don't mind keeping it - but a short comment above could explain why we\nmight accept skipping.yOddInc == 0 as Unpopulated rather than invalid.\n\n\n> +           outputSize.isNull())\n> +               return Unpopulated;\n> +\n> +       return Invalid;\n> +}\n> +\n>  /**\n>   * \\class CameraConfiguration\n>   * \\brief Hold configuration for streams of the camera\n> @@ -391,6 +561,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n>         return status;\n>  }\n>  \n> +/**\n> + * \\var CameraConfiguration::sensorConfig\n> + * \\brief The camera sensor configuration\n> + *\n> + * The sensorConfig field allows to control the configuration of the camera\n\ns/ allows to control the / allows control of the /\n\n\nBut again, most of all that is easily fixed up so then:\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> + * sensor. Refer to the camera-sensor-model documentation and to the\n> + * SensorConfiguration class documentation for details about the sensor\n> + * configuration process.\n> + */\n> +\n>  /**\n>   * \\var CameraConfiguration::transform\n>   * \\brief User-specified transform to be applied to the image\n> -- \n> 2.40.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 28073BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 14 Sep 2023 22:25:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87563628DA;\n\tFri, 15 Sep 2023 00:25:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A6B3628DA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Sep 2023 00:25:21 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F94818A1;\n\tFri, 15 Sep 2023 00:23:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694730322;\n\tbh=vI9jy5GHdEbZlhFFzV6wn09ffTHJL766ByPbnZYxNoQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3R7v+OXGJMnLTwKs6Ng+Nq5+jdk3kqQG/HOL58cRrx0Dod6HPdZ8xMnXct26o0XGT\n\tZa1WzqYpTBRPGt/0+FDe8HTF7Naj3AyUfABwSuN1f0q0F1HEfhxJGw+kbyqSzfLdSu\n\tNDZFQ/WXD0d1h0EllAN5M9ZQWd8Fl1NVbpoU5WVQZwmQjdVuQOryocgoykJf8lWSOG\n\tMXSRQj6uVO2TphZTls7JfSmXKDqO8RdY+zQfbCmnY8UGc0XZrc0KcWJ/44ICK05HcP\n\ti2n7Bt89E1zSHlEJnaLXPouLbS525+BEzncGtFyG2BC96Y2+ynvvuXaG7midVJRrO4\n\taCy2WJJ1KudbA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694730228;\n\tbh=vI9jy5GHdEbZlhFFzV6wn09ffTHJL766ByPbnZYxNoQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Vqg0CP9O5pY8l48G1xmbi+FswbkNT4jevO34kgrrDy0V7X0kSuy6pN56EsCJtYvWB\n\tzh96fJi8qaaLFb1G9ZBr8nFK/ibfQkkJpJReThr2qe2dfXuqODs5N7QkvaChdY4PoW\n\t/B/MfXOkZzvcUkp3PX0Bi/Kv6x6hvqrI32ldkXuA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Vqg0CP9O\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230731113115.5915-3-jacopo.mondi@ideasonboard.com>","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-3-jacopo.mondi@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 14 Sep 2023 23:25:18 +0100","Message-ID":"<169473031838.2843013.4523261506077905103@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: camera: Introduce\n\tSensorConfiguration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27783,"web_url":"https://patchwork.libcamera.org/comment/27783/","msgid":"<csquyb3umcs3haaczimtnxv63xbxow57ik37t5fcww3oqp6jz7@i5mhf3t7ayvp>","date":"2023-09-15T08:15:32","subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: camera: Introduce\n\tSensorConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Thu, Sep 14, 2023 at 11:25:18PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2023-07-31 12:31:13)\n> > Introduce SensorConfiguration in the libcamera API.\n> >\n> > The SensorConfiguration is part of the CameraConfiguration class\n> > and allows applications to control the sensor settings.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  include/libcamera/camera.h |  43 +++++++++\n> >  src/libcamera/camera.cpp   | 180 +++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 223 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 004bc89455f5..b2aa8d467feb 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -19,6 +19,7 @@\n> >  #include <libcamera/base/signal.h>\n> >\n> >  #include <libcamera/controls.h>\n> > +#include <libcamera/geometry.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> >  #include <libcamera/transform.h>\n> > @@ -30,6 +31,47 @@ class FrameBufferAllocator;\n> >  class PipelineHandler;\n> >  class Request;\n> >\n> > +class SensorConfiguration\n> > +{\n> > +public:\n> > +       unsigned int bitDepth = 0;\n> > +\n> > +       Rectangle analogCrop;\n> > +\n> > +       struct {\n> > +               unsigned int binX = 1;\n> > +               unsigned int binY = 1;\n> > +       } binning;\n> > +\n> > +       struct {\n> > +               unsigned int xOddInc = 1;\n> > +               unsigned int xEvenInc = 1;\n> > +               unsigned int yOddInc = 1;\n> > +               unsigned int yEvenInc = 1;\n> > +       } skipping;\n> > +\n> > +       Size outputSize;\n> > +\n> > +       bool valid() const\n> > +       {\n> > +               return validate() != Invalid;\n> > +       }\n> > +\n> > +       explicit operator bool() const\n> > +       {\n> > +               return validate() == Populated;\n> > +       }\n> > +\n> > +private:\n> > +       enum Status {\n> > +               Unpopulated,\n> > +               Populated,\n> > +               Invalid,\n> > +       };\n> > +\n> > +       Status validate() const;\n> > +};\n> > +\n> >  class CameraConfiguration\n> >  {\n> >  public:\n> > @@ -66,6 +108,7 @@ public:\n> >         bool empty() const;\n> >         std::size_t size() const;\n> >\n> > +       SensorConfiguration sensorConfig;\n> >         Transform transform;\n> >\n> >  protected:\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 0eecee766f00..e796a5da28c6 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -97,6 +97,21 @@\n> >   * implemented in the above order at the hardware level. The libcamera pipeline\n> >   * handlers translate the pipeline model to the real hardware configuration.\n> >   *\n> > + * \\subsection camera-sensor-mode Camera Sensor Model\n> > + *\n> > + * libcamera allows applications to control the configuration of the camera\n> > + * sensor, particularly it allows to control the frame geometry and frame\n> > + * delivery rate of the sensor.\n> > + *\n> > + * The camera sensor configuration applies to all streams produced by a camera\n> > + * from the same image source.\n> > + *\n> > + * More details about the libcamera implemented camera sensor model are\n>\n> More details about the camera sensor model implemented by libcamera are\n>\n> > + * available in the libcamera camera-sensor-model documentation page.\n> > + *\n> > + * The sensor configuration is specified by applications by populating the\n> > + * CameraConfiguration::sensorConfig class member.\n> > + *\n> >   * \\subsection digital-zoom Digital Zoom\n> >   *\n> >   * Digital zoom is implemented as a combination of the cropping and scaling\n> > @@ -111,6 +126,161 @@ namespace libcamera {\n> >\n> >  LOG_DECLARE_CATEGORY(Camera)\n> >\n> > +/**\n> > + * \\class SensorConfiguration\n> > + * \\brief Camera sensor configuration\n> > + *\n> > + * The SensorConfiguration class collects parameters to control the operations\n> > + * of the camera sensor, accordingly to the abstract camera sensor model\n> > + * implemented by libcamera.\n> > + *\n> > + * \\todo Applications shall fully populate all fields of the\n> > + * CameraConfiguration::sensorConfig class members before validating the\n> > + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if\n> > + * any of its parameters cannot be applied to the sensor in use, the\n> > + * CameraConfiguration validation process will fail and return\n> > + * CameraConfiguration::Status::Invalid.\n> > + *\n> > + * Applications that populate the SensorConfiguration class members are\n> > + * expected to be highly-specialized applications that know what sensor\n> > + * they are operating with and what parameters are valid for the sensor in use.\n> > + *\n> > + * A detailed description of the abstract camera sensor model implemented by\n> > + * libcamera and the description of its configuration parameters is available\n> > + * in the libcamera documentation camera-sensor-model file.\n> > + */\n> > +\n> > +/**\n> > + * \\enum SensorConfiguration::Status\n> > + * \\brief The sensor configuration validation status\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::bitDepth\n> > + * \\brief The sensor image format bit depth\n> > + *\n> > + * The number of bits (resolution) used to represent a pixel sample.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::analogCrop\n> > + * \\brief The analog crop rectangle\n> > + *\n> > + * The selected portion of the active pixel array used to produce the image\n> > + * frame.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::binning\n> > + * \\brief Sensor binning configuration\n> > + *\n> > + * Refer to the camera-sensor-model documentation for an accurate description\n> > + * of the binning operations. Disabled by default.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::binX\n> > + * \\brief Horizontal binning factor\n> > + *\n> > + * The horizontal binning factor. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::binY\n> > + * \\brief Vertical binning factor\n> > + *\n> > + * The vertical binning factor. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::skipping\n> > + * \\brief The sensor skipping configuration\n> > + *\n> > + * Refer to the camera-sensor-model documentation for an accurate description\n> > + * of the skipping operations.\n> > + *\n> > + * If no skipping is performed, all the structure fields should be\n> > + * set to 1. Disabled by default.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::xOddInc\n> > + * \\brief Horizontal increment for odd rows. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::xEvenInc\n> > + * \\brief Horizontal increment for even rows. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::yOddInc\n> > + * \\brief Vertical increment for odd columns. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::yEvenInc\n> > + * \\brief Vertical increment for even columns. Default to 1.\n> > + */\n> > +\n> > +/**\n> > + * \\var SensorConfiguration::outputSize\n> > + * \\brief The frame output (visible) size\n> > + *\n> > + * The size of the data frame as received by the host processor.\n> > + */\n> > +\n> > +/**\n> > + * \\fn SensorConfiguration::valid() const\n> > + * \\brief Validate the SensorConfiguration\n> > + *\n> > + * Validate the sensor configuration.\n> > + *\n> > + * \\todo A sensor configuration is valid (or well-formed) if it's either\n> > + * completely un-populated or fully populated. For now allow applications to\n> > + * populate the bitDepth and the outputSize only.\n> > + *\n> > + * \\return True if the SensorConfiguration is either fully populated or\n> > + * un-populated, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn SensorConfiguration::operator bool() const\n> > + * \\brief Test if a SensorConfiguration is fully populated\n> > + * \\return True if the SensorConfiguration is fully populated\n> > + */\n> > +\n> > +/**\n> > + * \\brief Validate the sensor configuration\n> > + *\n> > + * \\todo A sensor configuration is valid (or well-formed) if it's either\n> > + * completely un-populated or fully populated. For now allow applications to\n> > + * populate the bitDepth and the outputSize only.\n> > + *\n> > + * \\return The sensor configuration status\n> > + * \\retval Unpopulated The sensor configuration is fully unpopulated\n> > + * \\retval Populated The sensor configuration is fully populated\n> > + * \\retval Invalid The sensor configuration is invalid (not fully populated\n> > + * and not fully unpopulated)\n> > + */\n> > +SensorConfiguration::Status SensorConfiguration::validate() const\n> > +{\n> > +       if (bitDepth && binning.binX && binning.binY &&\n> > +           skipping.xOddInc && skipping.yOddInc &&\n> > +           skipping.xEvenInc && skipping.yEvenInc &&\n> > +           !outputSize.isNull())\n> > +               return Populated;\n> > +\n> > +       if (!bitDepth &&\n> > +           binning.binX <= 1 && binning.binY <= 1 &&\n> > +           skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&\n> > +           skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&\n>\n> This looks like the <= is to support if the struct / class has been\n> unintialised/memset or somehow allocated with\n>\n> \tSensorConfiguration = {}; ?\n>\n> Otherwise - is the <= necessary ?\n>\n> I don't mind keeping it - but a short comment above could explain why we\n> might accept skipping.yOddInc == 0 as Unpopulated rather than invalid.\n>\n\nTo be honest, this has gone through a few changes, the latest one was\nto default skipping and binning to 1, while they were previously\ninitialized to 0. That's probably where the <= comes from.\n\nHowever I think it's correct, as zero-initialized SensorConfiguration\nshould be considered unpopulated, so I can add a a comment like\n\n        /*\n\t * By default the binning and skipping factors are initialized to 1, but\n\t * a zero-initialized SensorConfiguration is considered unpopulated\n\t * as well.\n\t */\n\n>\n> > +           outputSize.isNull())\n> > +               return Unpopulated;\n> > +\n> > +       return Invalid;\n> > +}\n> > +\n> >  /**\n> >   * \\class CameraConfiguration\n> >   * \\brief Hold configuration for streams of the camera\n> > @@ -391,6 +561,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> >         return status;\n> >  }\n> >\n> > +/**\n> > + * \\var CameraConfiguration::sensorConfig\n> > + * \\brief The camera sensor configuration\n> > + *\n> > + * The sensorConfig field allows to control the configuration of the camera\n>\n> s/ allows to control the / allows control of the /\n>\n>\n> But again, most of all that is easily fixed up so then:\n>\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n\nThanks\n  j\n\n> > + * sensor. Refer to the camera-sensor-model documentation and to the\n> > + * SensorConfiguration class documentation for details about the sensor\n> > + * configuration process.\n> > + */\n> > +\n> >  /**\n> >   * \\var CameraConfiguration::transform\n> >   * \\brief User-specified transform to be applied to the image\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0AC34BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Sep 2023 08:15:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FA7262911;\n\tFri, 15 Sep 2023 10:15:38 +0200 (CEST)","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 29DD4628D8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Sep 2023 10:15:36 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF1B616B7;\n\tFri, 15 Sep 2023 10:14:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694765738;\n\tbh=IABzTb/8wSypuZZlS+0Gqt+0oMyC4tZd8/YcZzi/YAI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qHd7nvUoxXgz6/MeoJ7nRL7WhjYRA+WqlBmV0Pou6aUNKcOwt+GWszf4oPjDRBvo9\n\tBNvuZBa1cvE9N8TNBF577PiwysMm+GDJLWYdAeWoqaK3MovYsKgi5uq/5HtmkVkUXS\n\tezTlFYmS6TJ3aK0E0pRiffo08NSkI6bIAve7ONATn653quebtlMpezpCVE6hDIvsEQ\n\ttga6H8DQxd09B+0RfKUhETRT0rcMJZtex2dyZt8G28nbbkAzHGqDAjdA65zFUZ4JhM\n\toG5ew72kNuf4WmqT7rAO8VqW/TqcFjtzFEUuHjzzQoPDPNpFiMdkNzTBtJ3QbTGgk6\n\tyG2I9TQlDjMPQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694765642;\n\tbh=IABzTb/8wSypuZZlS+0Gqt+0oMyC4tZd8/YcZzi/YAI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pnfbRYc3n7Zq5nT0b7Gx25obiKKqT69v7JITdRS+VjGDWrjvlvT3ZxaVxkZtZSaW9\n\tT/e+No9po9FEl/EJe8TgCHU6P2UbSTbMuVn/I1ZYxTkZAA9P53GdTn5f4mRggZET78\n\tQ6vOmyzVyH9Uxj+yhrRHeuLjuuHTjYnHyyh70WcA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pnfbRYc3\"; dkim-atps=neutral","Date":"Fri, 15 Sep 2023 10:15:32 +0200","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<csquyb3umcs3haaczimtnxv63xbxow57ik37t5fcww3oqp6jz7@i5mhf3t7ayvp>","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-3-jacopo.mondi@ideasonboard.com>\n\t<169473031838.2843013.4523261506077905103@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<169473031838.2843013.4523261506077905103@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: camera: Introduce\n\tSensorConfiguration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]