[{"id":4262,"web_url":"https://patchwork.libcamera.org/comment/4262/","msgid":"<04582b61-647d-95ce-28c7-24c02faaed41@ideasonboard.com>","date":"2020-03-24T13:59:43","subject":"Re: [libcamera-devel] [PATCH v3 1/6] libcamera: properties: Define\n\tpixel array properties","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 09/03/2020 18:04, Jacopo Mondi wrote:\n> Add definition of pixel array related properties.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> ---\n> v2 -> v3:\n> - Pluralize PixelAreaSizes\n> - Use the new 'size' property in place of 'compound'.\n>   I tried to set meaningful values but it's not easy to get them right..\n> ---\n>  src/libcamera/property_ids.yaml | 177 ++++++++++++++++++++++++++++++++\n>  1 file changed, 177 insertions(+)\n> \n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index ce627fa042ba..4cecb5ad9ac3 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -386,4 +386,181 @@ controls:\n>                                |                    |\n>                                |                    |\n>                                +--------------------+\n> +\n> +  - PixelArraySize:\n> +      type: float\n\nDo we really need a float to store millimetres?\n\nHrm ... actually yes - A quick google for instance \"IMX219 sensor size\"\nreturns 4.60 mm (diagonal) ... :-)\n\n\n\n> +      size: [2]\n> +      description: |\n> +        The physical sizes of the pixel array (width and height), in\n> +        millimeters.\n> +\n\nLooking at the IMX219 product brief/datasheet:\n\nDevice Structure\n* CMOS image sensor\n* Image size : Diagonal 4.60 mm (Type 1/4.0)\n* Total number of pixels : 3296 (H) × 2512 (V) approx. 8.28 M pixels\n* Number of effective pixels : 3296 (H) × 2480 (V) approx. 8.17 M pixels\n* Number of active pixels : 3280 (H) × 2464 (V) approx. 8.08 M pixels\n* Chip size : 5.095 mm (H) × 4.930 mm (V) (w/ Scribe)\n* Unit cell size : 1.12 μm (H) × 1.12 μm (V)\n* Substrate material : Silicon\n\nThis is stating an Image Size (as a diagonal...) Presumably someone will\nhave to convert/identify that to a width/height appropriately?\n\nI assume the chip-size is not relevant here.\n\nFor instance, the OV5647 specifies:\n\n* image area: 3673.6 um x 2738.4um,\n\nas well as die-dimensions.\n\n\n\n\n\n> +  - PixelArrayBounds:\n> +      type: int32_t\n> +      size: [2]\n> +      description: |\n> +        The camera sensor pixel array bounding rectangle vertical and\n> +        horizontal sizes.\n\nThat reads like a string of unassociated words.\n\nPerhaps some hyphenation would help?\n\nThe camera-sensor pixel-array bounding-rectangle vertical and horizontal\nsizes.\n\n\nOr:\n\nThe vertical and horizontal sizes for the maximum bounding rectangle of\nthe pixel array on the camera sensor.\n\n> +\n> +        For image sensors with a rectangular pixel array the sizes described by\n\n/array/array,/\n\n> +        this property are the same as the PixelAreaSize property size.\n> +\n> +        For image sensors with more complex pixel array displacements (such as\n> +        cross-shaped pixel arrays, non symmetrical pixel arrays etc) this\n> +        property represents the bounding rectangle in which all pixel array\n> +        dimensions are inscribed into.\n> +\n> +        In example, the bounding rectangle sizes for image sensor with a\n\n/In/For/\n\n> +        cross-shaped pixel array is described as\n> +\n> +\n> +                     PixelArrayBound(0) = width\n> +                    /-----------------/\n> +\n> +            (0,0)-> +----+------+----+  /\n> +                    |    |//////|    |  |\n> +                    +----+//////+----+  |\n> +                    |////////////////|  | PixelArrayBound(1) = height\n> +                    +----+//////+----+  |\n> +                    |    |//////|    |  |\n> +                    +----+------+----+  /\n> +                            |\n> +                             -> Cross-shaped pixel area\n> +\n> +  - PixelArrays:\n> +      type: int32_t\n> +      size: [8]\n> +      description: |\n> +        The sensor pixel array rectangles, relative to the rectangle described\n> +        by the PixelArrayBounds property.\n> +\n> +        This property describes an arbitrary number of (likely overlapping)\n> +        rectangles, representing the pixel array areas the sensor is composed\n> +        of.\n> +\n> +        Each rectangle is defined by its displacement from pixel (0, 0) of\n> +        the bounding rectangle described by the PixelArrayBound property.\n> +\n> +        For image sensors with a rectangular pixel array, a single rectangle\n> +        is required. For sensors with more complex pixel array displacements\n> +        multiple rectangles shall be specified, ordered from the tallest to the\n\nShould this state 'multiple overlapping rectangles' ?\n\n> +        shorter one.\n> +\n> +        For each rectangle, this property reports the full pixel array size,\n> +        including non-active pixels, black level calibration pixels etc.\n> +\n> +        In example, a simple sensor with a rectangular pixel array is described\n> +        as\n> +\n> +                     PixelArrayBound(0) = width\n> +                    /-----------------/\n> +                      x1          x2\n> +            (0,0)-> +-o------------o-+  /\n> +                 y1 o +------------+ |  |\n> +                    | |////////////| |  |\n> +                    | |////////////| |  | PixelArrayBound(1) = height\n> +                    | |////////////| |  |\n> +                 y2 o +------------+ |  |\n> +                    +----------------+  /\n> +\n> +                 PixelArray = (x1, y1, (x2 - x1), (y2 - y1))\n\nSo for a single 'described rect', this is\nstruct rect {\n\tint32_t x;\n\tint32_t y;\n\n\tint32_t width;\n\tint32_t height;\n}\n\nWhat are the remaining 4 values? 0?\n\n(Is there a means to recognise when or when not to parse the second 'rect'?)\n\n\n> +\n> +        A more complex sensor, with a cross shaped pixel array displacement\n> +        is described with 2 rectangles, with the vertical rectangle\n> +        described first\n> +\n> +                     PixelArrayBound(0) = width\n> +                    /-----------------/\n> +                    x1  x2     x3  x4 W\n> +            (0,0)-> +o---o------o---o+  /\n> +                    |    |//////|    |  |\n> +                 y1 o+---+------+---+|  |\n> +                    ||///|//////|///||  | PixelArrayBound(1) = height\n> +                 y2 o+---+------+---+|  |\n> +                    |    |//////|    |  |\n> +                 H  +----+------+----+  /\n> +\n> +\n> +                PixelArray = ( (x2, 0, (x3 - x2), H),\n> +                               (x1, y1, (x4 - x1), (y2 - y1))\n\nWouldn't it make sense to show y1,y2,y3,y4 as well, and use y4-y1 in\nplace of H? or are you trying to show that the W and H are not required\nparameters, and the rectangles do not have to utilise the full width or\nheight of the sensor?\n\n\nBut why would the width of the sensor be larger than the width of the\nlargest horizontal rectangle ...\n\n\nSo we are really expressing:\n\n> +                     PixelArrayBound(0) = width\n> +                    /-----------------/\n> +                    x1  x2     x3  x4 W\n> +            (0,0)-> +o---o------o---o+  /\n> +                    |    |//////|    |  |\n> +                 y1 o    +------+    |  |\n> +                    |    |//////|    |  | PixelArrayBound(1) = height\n> +                 y2 o    +------+    |  |\n> +                    |    |//////|    |  |\n> +                 H  +----+------+----+  /\n\nfollowed by:\n\n> +                     PixelArrayBound(0) = width\n> +                    /-----------------/\n> +                    x1  x2     x3  x4 W\n> +            (0,0)-> +o---o------o---o+  /\n> +                    |                |  |\n> +                 y1 o+---+------+---+|  |\n> +                    ||///|//////|///||  | PixelArrayBound(1) = height\n> +                 y2 o+---+------+---+|  |\n> +                    |                |  |\n> +                 H  +----+------+----+  /\n\n\nSo really, PixelArrays describes up to two rectangles...\n\nWhat happens if the shape is more complex? Why do we support a cross\nshaped pixel array, but not a hexagonal pixel array?\n\n(perhaps cross shaped pixel arrays are a standard thing, I don't know,\nor maybe its' just because we only support a maximum of two rects...,\nand a polygon would be more bytes to encode...)\n\n\nQuerying this it seems it's for advanced sensors which have layouts to\nsupport different aspect ratios. It would be good to express that here\nsomewhere, but I'm not sure how yet.\n\n\n> +\n> +  - ActiveAreaSizes:\n> +      type: int32_t\n> +      size: [8]\n> +      description: |\n> +        The sensor active pixel area sizes, represented as rectangles\n\n/sensor/sensor's/ (the active pixel area belongs to the sensor)\n\n\n> +        inscribed in the ones described by the PixelArrays property.\n> +\n> +        One ActiveAreaSize rectangle per each rectangle described in the\n\n/per/for/\n\n> +        PixelArrays property is required. As a consequence, the two properties\n> +        shall transport the same number of elements.\n> +\n> +        The ActiveAreaSize rectangles represent the maximum image sizes the\n> +        sensor can produce.\n> +\n> +  - BayerFilterArrangement:\n> +      type: int32_t\n> +      description: |\n> +        The pixel array color filter displacement.\n> +\n> +        This property describes the arrangement and readout sequence of the\n> +        three RGB color components of the sensor's Bayer Color Filter Array\n> +        (CFA).\n> +\n> +        Color filters are usually displaced in line-alternating fashion on the\n> +        sensor pixel array. In example, one line might be composed of Red-Green\n> +        while the successive is composed of Blue-Green color information.\n> +\n> +        The value of this property represent the arrangement of color filters\n\n/represent/represents/\n\n> +        in the top-left 2x2 pixel square.\n> +\n> +        In example, for a sensor with the following color filter displacement\n\n/In example,/For example,/\n\n> +\n> +                 (0, 0)               (max-col)\n> +           +---+    +--------------...---+\n> +           |B|G|<---|B|G|B|G|B|G|B|...B|G|\n> +           |G|R|<---|G|R|G|R|G|R|G|...G|R|\n> +           +---+    |B|G|B|G|B|G|B|...B|G|\n> +                    ...                  ..\n> +                    ...                  ..\n> +                    |G|R|G|R|G|R|G|...G|R|\n> +                    |B|G|B|G|B|G|B|...B|G|   (max-lines)\n> +                    +--------------...---+\n> +\n> +        The filer arrangement is represented by the BGGR value, which correspond\n\n/filer/filter/\n/correspond/corresponds/\n\n\n> +        to the pixel readout sequence in line interleaved mode.\n> +\n> +      enum:\n> +        - name: BayerFilterRGGB\n\nDo we need to prefix these enum values (BayerFilerXXXX) for namespacing?\nOr are they already namespaced by being specific to BayerFilterArrangement?\n\nI.e. can we arrange that these become:\n\nBayerFilterArrangement::RGGB ?\n\nRather than:\n\nBayerFilterArrangement::BayerFilterRGGB ?\n\n> +          value: 0\n> +          description: |\n> +            Color filter array displacement is Red-Green/Green-Blue\n> +\n> +        - name: BayerFilterGRBG\n> +          value: 1\n> +          description: |\n> +            Color filter array displacement is Green-Red/Blue-Green\n> +\n> +        - name: BayerFilterGBRG\n> +          value: 2\n> +          description: |\n> +            Color filter array displacement is Green-Blue/Red-Green\n> +\n> +        - name: BayerFilterBGGR\n> +          value: 3\n> +          description: |\n> +            Color filter array displacement is Blue-Green/Green-Red\n> +\n> +        - name: BayerFilterNonStandard\n> +          value: 4\n> +          description: |\n> +            The pixel array color filter does not use the standard Bayer RGB\n> +            color model\n> +\n> +  - ISOSensitivityRange:\n> +      type: int32_t\n> +      size: [2]\n> +      description: |\n> +        The range of supported ISO sensitivities, as documented by the\n> +        ISO 12232:2006 standard\n\nThe ISO 1232:2006 appears to be superseded by ISO 12232:2019... and\nitself supersedes the :2006 version. Do we need to specify versions? Or\nshould we state 'or later' .... or ... ?\n\n (I see that \"Speed ratings greater than 10000 have finally been defined\nin ISO 12232:2019\"...)\n\n\nThis is 2 int32_t entries. Presumably, thus one is the minimum, and one\nis the maximum. I can guess which order they are - but do they need to\nbe specified explicitly? (maybe not, but I ask as devils advocate at least)\n\n\n> +\n>  ...\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC6D860411\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 24 Mar 2020 14:59:47 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63A8DACB;\n\tTue, 24 Mar 2020 14:59:47 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585058387;\n\tbh=33VvdZoSEb1Hod4UhvxW1ibx6QBNiwHU1Gb77gugAys=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=E30jTsPyxvVnbXxh+HXpei5tHKfzNqd4I4e6lMNnNIaOPG6oPN9etaPJaIT/XbrdN\n\tWtBqPWv2lwI5QBDyuEeZy/MbI1K4R84/rAM6cF34Cm7Hz3dgJdGpZepXY/2IlC38J1\n\tekeC9FkWdTHEZ7L1IbSU4GrOjJZVWuCMzzsEJG5M=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20200309180444.725757-1-jacopo@jmondi.org>\n\t<20200309180444.725757-2-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<04582b61-647d-95ce-28c7-24c02faaed41@ideasonboard.com>","Date":"Tue, 24 Mar 2020 13:59:43 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200309180444.725757-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 1/6] libcamera: properties: Define\n\tpixel array properties","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>","X-List-Received-Date":"Tue, 24 Mar 2020 13:59:48 -0000"}},{"id":4300,"web_url":"https://patchwork.libcamera.org/comment/4300/","msgid":"<20200326121448.w4ltw6r742maeyub@uno.localdomain>","date":"2020-03-26T12:14:48","subject":"Re: [libcamera-devel] [PATCH v3 1/6] libcamera: properties: Define\n\tpixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Tue, Mar 24, 2020 at 01:59:43PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 09/03/2020 18:04, Jacopo Mondi wrote:\n> > Add definition of pixel array related properties.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> >\n> > ---\n> > v2 -> v3:\n> > - Pluralize PixelAreaSizes\n> > - Use the new 'size' property in place of 'compound'.\n> >   I tried to set meaningful values but it's not easy to get them right..\n> > ---\n> >  src/libcamera/property_ids.yaml | 177 ++++++++++++++++++++++++++++++++\n> >  1 file changed, 177 insertions(+)\n> >\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index ce627fa042ba..4cecb5ad9ac3 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -386,4 +386,181 @@ controls:\n> >                                |                    |\n> >                                |                    |\n> >                                +--------------------+\n> > +\n> > +  - PixelArraySize:\n> > +      type: float\n>\n> Do we really need a float to store millimetres?\n>\n> Hrm ... actually yes - A quick google for instance \"IMX219 sensor size\"\n> returns 4.60 mm (diagonal) ... :-)\n>\n>\n>\n> > +      size: [2]\n> > +      description: |\n> > +        The physical sizes of the pixel array (width and height), in\n> > +        millimeters.\n> > +\n>\n> Looking at the IMX219 product brief/datasheet:\n>\n> Device Structure\n> * CMOS image sensor\n> * Image size : Diagonal 4.60 mm (Type 1/4.0)\n> * Total number of pixels : 3296 (H) × 2512 (V) approx. 8.28 M pixels\n> * Number of effective pixels : 3296 (H) × 2480 (V) approx. 8.17 M pixels\n> * Number of active pixels : 3280 (H) × 2464 (V) approx. 8.08 M pixels\n> * Chip size : 5.095 mm (H) × 4.930 mm (V) (w/ Scribe)\n> * Unit cell size : 1.12 μm (H) × 1.12 μm (V)\n> * Substrate material : Silicon\n>\n> This is stating an Image Size (as a diagonal...) Presumably someone will\n> have to convert/identify that to a width/height appropriately?\n>\n> I assume the chip-size is not relevant here.\n\nYou are right, I have used that chip size in place of the pixel array\nsize, which can be calculated from Fig.3 of the chip manual instead.\n\n>\n> For instance, the OV5647 specifies:\n>\n> * image area: 3673.6 um x 2738.4um,\n>\n> as well as die-dimensions.\n>\n>\n>\n>\n>\n> > +  - PixelArrayBounds:\n> > +      type: int32_t\n> > +      size: [2]\n> > +      description: |\n> > +        The camera sensor pixel array bounding rectangle vertical and\n> > +        horizontal sizes.\n>\n> That reads like a string of unassociated words.\n>\n> Perhaps some hyphenation would help?\n>\n> The camera-sensor pixel-array bounding-rectangle vertical and horizontal\n> sizes.\n>\n>\n> Or:\n>\n> The vertical and horizontal sizes for the maximum bounding rectangle of\n> the pixel array on the camera sensor.\n\nAs clarified with Sakari, this property will be dropped. Cross-shaped\nsensors expose different overlapping rectangles as supported modes,\nbut according to his opinion, there is nothing like a cross-shaped\npixel matrix like the below reported one\n\n>\n> > +\n> > +        For image sensors with a rectangular pixel array the sizes described by\n>\n> /array/array,/\n>\n> > +        this property are the same as the PixelAreaSize property size.\n> > +\n> > +        For image sensors with more complex pixel array displacements (such as\n> > +        cross-shaped pixel arrays, non symmetrical pixel arrays etc) this\n> > +        property represents the bounding rectangle in which all pixel array\n> > +        dimensions are inscribed into.\n> > +\n> > +        In example, the bounding rectangle sizes for image sensor with a\n>\n> /In/For/\n>\n> > +        cross-shaped pixel array is described as\n> > +\n> > +\n> > +                     PixelArrayBound(0) = width\n> > +                    /-----------------/\n> > +\n> > +            (0,0)-> +----+------+----+  /\n> > +                    |    |//////|    |  |\n> > +                    +----+//////+----+  |\n> > +                    |////////////////|  | PixelArrayBound(1) = height\n> > +                    +----+//////+----+  |\n> > +                    |    |//////|    |  |\n> > +                    +----+------+----+  /\n> > +                            |\n> > +                             -> Cross-shaped pixel area\n> > +\n> > +  - PixelArrays:\n> > +      type: int32_t\n> > +      size: [8]\n> > +      description: |\n> > +        The sensor pixel array rectangles, relative to the rectangle described\n> > +        by the PixelArrayBounds property.\n> > +\n> > +        This property describes an arbitrary number of (likely overlapping)\n> > +        rectangles, representing the pixel array areas the sensor is composed\n> > +        of.\n> > +\n> > +        Each rectangle is defined by its displacement from pixel (0, 0) of\n> > +        the bounding rectangle described by the PixelArrayBound property.\n> > +\n> > +        For image sensors with a rectangular pixel array, a single rectangle\n> > +        is required. For sensors with more complex pixel array displacements\n> > +        multiple rectangles shall be specified, ordered from the tallest to the\n>\n> Should this state 'multiple overlapping rectangles' ?\n>\n\nyes\n\n> > +        shorter one.\n> > +\n> > +        For each rectangle, this property reports the full pixel array size,\n> > +        including non-active pixels, black level calibration pixels etc.\n> > +\n> > +        In example, a simple sensor with a rectangular pixel array is described\n> > +        as\n> > +\n> > +                     PixelArrayBound(0) = width\n> > +                    /-----------------/\n> > +                      x1          x2\n> > +            (0,0)-> +-o------------o-+  /\n> > +                 y1 o +------------+ |  |\n> > +                    | |////////////| |  |\n> > +                    | |////////////| |  | PixelArrayBound(1) = height\n> > +                    | |////////////| |  |\n> > +                 y2 o +------------+ |  |\n> > +                    +----------------+  /\n> > +\n> > +                 PixelArray = (x1, y1, (x2 - x1), (y2 - y1))\n>\n> So for a single 'described rect', this is\n> struct rect {\n> \tint32_t x;\n> \tint32_t y;\n>\n> \tint32_t width;\n> \tint32_t height;\n> }\n>\n> What are the remaining 4 values? 0?\n\nThere are no other values.\nI recently realized the value to assign to 'size:' is totally\narbitrary, while I interpreted it as an upper bound (hence, the note\nin the patch commit message)\n\n - Use the new 'size' property in place of 'compound'.\n   I tried to set meaningful values but it's not easy to get them right..\n\n>\n> (Is there a means to recognise when or when not to parse the second 'rect'?)\n\nYou'll get it from the property size.\n\n>\n>\n> > +\n> > +        A more complex sensor, with a cross shaped pixel array displacement\n> > +        is described with 2 rectangles, with the vertical rectangle\n> > +        described first\n> > +\n> > +                     PixelArrayBound(0) = width\n> > +                    /-----------------/\n> > +                    x1  x2     x3  x4 W\n> > +            (0,0)-> +o---o------o---o+  /\n> > +                    |    |//////|    |  |\n> > +                 y1 o+---+------+---+|  |\n> > +                    ||///|//////|///||  | PixelArrayBound(1) = height\n> > +                 y2 o+---+------+---+|  |\n> > +                    |    |//////|    |  |\n> > +                 H  +----+------+----+  /\n> > +\n> > +\n> > +                PixelArray = ( (x2, 0, (x3 - x2), H),\n> > +                               (x1, y1, (x4 - x1), (y2 - y1))\n>\n> Wouldn't it make sense to show y1,y2,y3,y4 as well, and use y4-y1 in\n> place of H? or are you trying to show that the W and H are not required\n> parameters, and the rectangles do not have to utilise the full width or\n> height of the sensor?\n\nyes, rectangles can span the whole width or height or not, that's why\nthe two dimensions are described differently.\n\n>\n>\n> But why would the width of the sensor be larger than the width of the\n> largest horizontal rectangle ...\n\nThis property will have to be changed as there are no more bounds to\ntake into account\n\n>\n>\n> So we are really expressing:\n>\n> > +                     PixelArrayBound(0) = width\n> > +                    /-----------------/\n> > +                    x1  x2     x3  x4 W\n> > +            (0,0)-> +o---o------o---o+  /\n> > +                    |    |//////|    |  |\n> > +                 y1 o    +------+    |  |\n> > +                    |    |//////|    |  | PixelArrayBound(1) = height\n> > +                 y2 o    +------+    |  |\n> > +                    |    |//////|    |  |\n> > +                 H  +----+------+----+  /\n>\n> followed by:\n>\n> > +                     PixelArrayBound(0) = width\n> > +                    /-----------------/\n> > +                    x1  x2     x3  x4 W\n> > +            (0,0)-> +o---o------o---o+  /\n> > +                    |                |  |\n> > +                 y1 o+---+------+---+|  |\n> > +                    ||///|//////|///||  | PixelArrayBound(1) = height\n> > +                 y2 o+---+------+---+|  |\n> > +                    |                |  |\n> > +                 H  +----+------+----+  /\n>\n>\n> So really, PixelArrays describes up to two rectangles...\n>\n> What happens if the shape is more complex? Why do we support a cross\n> shaped pixel array, but not a hexagonal pixel array?\n\nI have heard from Sakari and Laurent abount possible 'cross shaped'\npixel arrays (which after the clarification, are actually just sensors\nwith modes that maximize the FOV in one direction).\n\n>\n> (perhaps cross shaped pixel arrays are a standard thing, I don't know,\n> or maybe its' just because we only support a maximum of two rects...,\n> and a polygon would be more bytes to encode...)\n>\n\nYes, let's leave it out\n\n>\n> Querying this it seems it's for advanced sensors which have layouts to\n> support different aspect ratios. It would be good to express that here\n> somewhere, but I'm not sure how yet.\n\nI'll try to capture that, but I don't think it's necessary. If an\nintegrator has a cross-shaped sensor to deal with, I'm sure she knows\nwhat's that for\n\n>\n>\n> > +\n> > +  - ActiveAreaSizes:\n> > +      type: int32_t\n> > +      size: [8]\n> > +      description: |\n> > +        The sensor active pixel area sizes, represented as rectangles\n>\n> /sensor/sensor's/ (the active pixel area belongs to the sensor)\n>\n>\n> > +        inscribed in the ones described by the PixelArrays property.\n> > +\n> > +        One ActiveAreaSize rectangle per each rectangle described in the\n>\n> /per/for/\n>\n\nack\n\n> > +        PixelArrays property is required. As a consequence, the two properties\n> > +        shall transport the same number of elements.\n> > +\n> > +        The ActiveAreaSize rectangles represent the maximum image sizes the\n> > +        sensor can produce.\n> > +\n> > +  - BayerFilterArrangement:\n> > +      type: int32_t\n> > +      description: |\n> > +        The pixel array color filter displacement.\n> > +\n> > +        This property describes the arrangement and readout sequence of the\n> > +        three RGB color components of the sensor's Bayer Color Filter Array\n> > +        (CFA).\n> > +\n> > +        Color filters are usually displaced in line-alternating fashion on the\n> > +        sensor pixel array. In example, one line might be composed of Red-Green\n> > +        while the successive is composed of Blue-Green color information.\n> > +\n> > +        The value of this property represent the arrangement of color filters\n>\n> /represent/represents/\n>\n> > +        in the top-left 2x2 pixel square.\n> > +\n> > +        In example, for a sensor with the following color filter displacement\n>\n> /In example,/For example,/\n>\n> > +\n> > +                 (0, 0)               (max-col)\n> > +           +---+    +--------------...---+\n> > +           |B|G|<---|B|G|B|G|B|G|B|...B|G|\n> > +           |G|R|<---|G|R|G|R|G|R|G|...G|R|\n> > +           +---+    |B|G|B|G|B|G|B|...B|G|\n> > +                    ...                  ..\n> > +                    ...                  ..\n> > +                    |G|R|G|R|G|R|G|...G|R|\n> > +                    |B|G|B|G|B|G|B|...B|G|   (max-lines)\n> > +                    +--------------...---+\n> > +\n> > +        The filer arrangement is represented by the BGGR value, which correspond\n>\n> /filer/filter/\n> /correspond/corresponds/\n>\n>\n> > +        to the pixel readout sequence in line interleaved mode.\n> > +\n> > +      enum:\n> > +        - name: BayerFilterRGGB\n>\n> Do we need to prefix these enum values (BayerFilerXXXX) for namespacing?\n> Or are they already namespaced by being specific to BayerFilterArrangement?\n>\n> I.e. can we arrange that these become:\n>\n> BayerFilterArrangement::RGGB ?\n>\n> Rather than:\n>\n> BayerFilterArrangement::BayerFilterRGGB ?\n>\n\nLooking at the generated property_ids.cpp\n\n\n/**\n * \\enum BayerFilterArrangementValues\n * \\brief Supported BayerFilterArrangement values\n *\n * \\var BayerFilterArrangementValues::BayerFilterRGGB\n * \\brief Color filter array displacement is Red-Green/Green-Blue\n *\n * \\var BayerFilterArrangementValues::BayerFilterGRBG\n * \\brief Color filter array displacement is Green-Red/Blue-Green\n *\n * \\var BayerFilterArrangementValues::BayerFilterGBRG\n * \\brief Color filter array displacement is Green-Blue/Red-Green\n *\n * \\var BayerFilterArrangementValues::BayerFilterBGGR\n * \\brief Color filter array displacement is Blue-Green/Green-Red\n *\n * \\var BayerFilterArrangementValues::BayerFilterNonStandard\n * \\brief The pixel array color filter does not use the standard Bayer RGB\n * color model\n */\n\nShortening the enum entries names would allow them to be accessed as\n'RGGB' only, which seems bad. And really, why does this matter ?\n\n> > +          value: 0\n> > +          description: |\n> > +            Color filter array displacement is Red-Green/Green-Blue\n> > +\n> > +        - name: BayerFilterGRBG\n> > +          value: 1\n> > +          description: |\n> > +            Color filter array displacement is Green-Red/Blue-Green\n> > +\n> > +        - name: BayerFilterGBRG\n> > +          value: 2\n> > +          description: |\n> > +            Color filter array displacement is Green-Blue/Red-Green\n> > +\n> > +        - name: BayerFilterBGGR\n> > +          value: 3\n> > +          description: |\n> > +            Color filter array displacement is Blue-Green/Green-Red\n> > +\n> > +        - name: BayerFilterNonStandard\n> > +          value: 4\n> > +          description: |\n> > +            The pixel array color filter does not use the standard Bayer RGB\n> > +            color model\n> > +\n> > +  - ISOSensitivityRange:\n> > +      type: int32_t\n> > +      size: [2]\n> > +      description: |\n> > +        The range of supported ISO sensitivities, as documented by the\n> > +        ISO 12232:2006 standard\n>\n> The ISO 1232:2006 appears to be superseded by ISO 12232:2019... and\n> itself supersedes the :2006 version. Do we need to specify versions? Or\n> should we state 'or later' .... or ... ?\n>\n>  (I see that \"Speed ratings greater than 10000 have finally been defined\n> in ISO 12232:2019\"...)\n\nThanks, I got that from the android definition. Should we use the most\nrecent one. I premit I have no idea what I'm talking about.\n\n>\n>\n> This is 2 int32_t entries. Presumably, thus one is the minimum, and one\n> is the maximum. I can guess which order they are - but do they need to\n> be specified explicitly? (maybe not, but I ask as devils advocate at least)\n\nWell, one will be less than the other, right ? We can specify it, but\nseems really obvious to me to specify a range as [min, max] and not\n[max, min].\n\nThanks for the detailed review\n\n>\n>\n> > +\n> >  ...\n> >\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2801F60410\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 26 Mar 2020 13:11:50 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 7D61B1BF207;\n\tThu, 26 Mar 2020 12:11:49 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 26 Mar 2020 13:14:48 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200326121448.w4ltw6r742maeyub@uno.localdomain>","References":"<20200309180444.725757-1-jacopo@jmondi.org>\n\t<20200309180444.725757-2-jacopo@jmondi.org>\n\t<04582b61-647d-95ce-28c7-24c02faaed41@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<04582b61-647d-95ce-28c7-24c02faaed41@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/6] libcamera: properties: Define\n\tpixel array properties","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>","X-List-Received-Date":"Thu, 26 Mar 2020 12:11:50 -0000"}}]