[{"id":4505,"web_url":"https://patchwork.libcamera.org/comment/4505/","msgid":"<20200425020511.GB19672@pendragon.ideasonboard.com>","date":"2020-04-25T02:05:11","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> Add definition of pixel array related properties.\n> \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> 1 file changed, 155 insertions(+)\n> \n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index ce627fa042ba..8f6797723a9d 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -386,4 +386,159 @@ controls:\n> | |\n> | |\n> +--------------------+\n> +\n> + - PixelArraySize:\n> + type: float\n> + size: [2]\n> + description: |\n> + The physical sizes of the pixel array (width and height), in\n> + millimeters.\n\nOnce we'll have categories for properties I think this should become\nPhysicalSize in the sensor category. We could already rename it now to\nPhysicalSize or PixelArrayPhysicalSize if you think it would be a good\nidea. I don't mind much either way, but if we keep the current name, how\nabout adding a comment to remember this ?\n\n # \\todo Rename this property to PhysicalSize once we will have property\n # categories\n\n> +\n> + - PixelArray:\n\nI was going to say we should rename this to PixelArraySize as it's the\nsize of the pixel array, and then realized that the previous property\nhas the same name :-) Maybe we should rename both ?\n\n> + type: int32_t\n> + size: [2]\n> + description: |\n> + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> +\n> + The property describes a rectangle with its top-left corner in position\n> + (0, 0) and width and height described by the first and second values\n> + of this property.\n> +\n> + The PixelArray property defines the rectangle that includes all possible\n> + rectangles defined by the ActiveAreas property, and describes the full\n> + pixel array, including non-active pixels, black level calibration\n> + pixels etc.\n\nIt's not entirely clear to me what pixels should be included in this\nrectangle. I'm not blaming your documentation here, but rather my lack\nof knowledge of what else than non-active pixels and black level\ncalibration pixels there are :-) Just an idea, instead of linking this\nproperty to ActiveAreas, would it make sense to link it to\nPixelArraySize ? Maybe something along the lines of\n\n\tThe PixelArraySize property defines the size of the full pixel\n\tarray, covered by the PixelArrayPhysicalSize area. This may\n\tinclude inactive or optical black pixels.\n\n> +\n> + - ActiveAreas:\n> + type: int32_t\n> + size: [4 x n]\n> + description: |\n> + The camera sensor active pixel area rectangles, represented as\n> + rectangles contained in the one described by the PixelArray property.\n> +\n> + This property describes an arbitrary number of overlapping rectangles,\n> + representing the active pixel portions of the camera sensor pixel array.\n> +\n> + Each rectangle is defined by its displacement from pixel (0, 0) of\n> + the rectangle described by the PixelArray property, a width and an\n> + height.\n\ns/an height/a height/\n\n> +\n> + Each rectangle described by this property represents the maximum image\n> + size that the camera module can produce for a given image resolution.\n\nHow about s/for a given image resolution/for a particular aspect ratio/\n?\n\nI would also add, taken from below,\n\n When multiple rectangles are reported, they shall be ordered\n\tfrom the tallest to the shortest.\n\n> +\n> + Example 1.\n> + A sensor which only produces images in the 4:3 image resolution will\n> + report a single ActiveArea rectangle, from which all other image formats\n> + are obtained by either cropping the field-of-view and/or applying pixel\n> + sub-sampling techniques such as pixel skipping or binning.\n> +\n> + PixelArray(0)\n> + /-----------------/\n> + x1 x2\n> + (0,0)-> +-o------------o-+ /\n> + y1 o +------------+ | |\n> + | |////////////| | |\n> + | |////////////| | | PixelArray(1)\n> + | |////////////| | |\n> + y2 o +------------+ | |\n> + +----------------+ /\n> +\n> + The property reports a single rectangle\n> +\n> + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))\n\nIf the rectangle is defined through width and height, this should be\n\n ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)\n\nAlternatively you could use coordinates:\n\n ActiveArea = (x1, y1, x2, y2)\n\n> +\n> + Example 2\n> + A camera sensor which can produce images in different native\n\ns/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.\n\n> + resolutions, will report several overlapping rectangle, one for each\n\ns/resolutions,/resolutions/\ns/rectangle/rectangles/\n\n> + natively supported resolution, ordered from the tallest to the shortest\n> + one.\n> +\n> + PixelArray(0)\n> + /-----------------/\n> + x1 x2 x3 x4\n> + (0,0)-> +o---o------o---o+ /\n> + y1 | +------+ | |\n> + | |//////| | |\n> + y2 o+---+------+---+| |\n> + ||///|//////|///|| | PixelArray(1)\n> + y3 o+---+------+---+| |\n> + | |//////| | |\n> + y4 | +------+ | |\n\nI think you need a o next to y1 and y4.\n\n> + +----+------+----+ /\n> +\n> + The property reports two rectangles\n> +\n> + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),\n\ns/y4 - 1/y4 - y1/\n\n> + (x1, y2, (x4 - x1), (y3 - y2))\n\nAnd comment as above regarding the width.\n\n> +\n> + The first rectangle describes the maximum field-of-view of all image\n> + formats in the 4:3 resolutions, while the second one describes the\n> + maximum field of view for all image formats in the 16:9 resolutions.\n> +\n> + - BayerFilterArrangement:\n> + type: int32_t\n> + description: |\n> + The pixel array color filter displacement.\n\nI could be wrong, but is \"displacement\" the right term ? Maybe\narrangement ?\n\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\nAs we could support sensors with non-Bayer CFAs, how about already\nrenaming the control to ColorFilterArrangement, and remove Bayer from\nhere ?\n\n> +\n> + Color filters are usually displaced in line-alternating fashion on the\n\ns/displaced/arranged/ ?\n\n> + sensor pixel array. In example, one line might be composed of Red-Green\n\ns/In example/For example/\n\n> + while the successive is composed of Blue-Green color information.\n> +\n> + The value of this property represents the arrangement of color filters\n> + in the top-left 2x2 pixel square.\n\nAs this is only valid for Bayer, maybe\n\n For Bayer filters, the value of this property represents the arrangement\n of color filters in the top-left 2x2 pixel square.\n\n> +\n> + For example, for a sensor with the following color filter displacement\n\ns/displacement/arrangement/\n\n(you could also use pattern instead of arrangement here or in other\nplaces)\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 filter arrangement is represented by the BGGR value, which\n> + correspond to the pixel readout sequence in line interleaved mode.\n> +\n> + enum:\n> + - name: BayerFilterRGGB\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\nShould these be sorted alphabetically ?\n\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\nI would drop this value for now, as we should instead report which\nfilter arrangement is used. We could already add known patterns, such as\nthe ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,\nbut I think that's overkill, we can extend it later when/if needed.\n\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 (or later) standard.\n\nIs this the ISO speed or the Standard Output Sensitivity (SOS) ? I think\nthis property needs a bit more research, should we leave it out for now\nto avoid blocking the rest ?\n\nOverall, good work ! No major issue, so the next version should be the\nlast one.\n\n> +\n> ...","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4622D603F8\n\tfor ;\n\tSat, 25 Apr 2020 04:05:27 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0D714F7;\n\tSat, 25 Apr 2020 04:05:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"u5+7QEM+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587780326;\n\tbh=NazeX5yhECrJ27uy/uR0OwlCeXPtTsDVIfpMix3dLxU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u5+7QEM+Cws9SXtmwQH75RBNBIV0GCETYHEK3BtQQ11jJQH/QOn+exZfdUtioeVw5\n\tfmoJVEO7PZlBEb5CxS2eDG46vJlGDfLadghtYuixy/U42qSrxwHUeu/EZaqo6x5Gha\n\tuTJqXNzjzuaEzY6mw0tKEdPuo8nA3T//mgFlf7s8=","Date":"Sat, 25 Apr 2020 05:05:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425020511.GB19672@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200424215304.558317-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 25 Apr 2020 02:05:27 -0000"}},{"id":4513,"web_url":"https://patchwork.libcamera.org/comment/4513/","msgid":"<20200425135845.cqg7yac3aivev2se@uno.localdomain>","date":"2020-04-25T13:58:45","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for review\n\nOn Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> > Add definition of pixel array related properties.\n> >\n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> > 1 file changed, 155 insertions(+)\n> >\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index ce627fa042ba..8f6797723a9d 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -386,4 +386,159 @@ controls:\n> > | |\n> > | |\n> > +--------------------+\n> > +\n> > + - PixelArraySize:\n> > + type: float\n> > + size: [2]\n> > + description: |\n> > + The physical sizes of the pixel array (width and height), in\n> > + millimeters.\n>\n> Once we'll have categories for properties I think this should become\n> PhysicalSize in the sensor category. We could already rename it now to\n> PhysicalSize or PixelArrayPhysicalSize if you think it would be a good\n> idea. I don't mind much either way, but if we keep the current name, how\n> about adding a comment to remember this ?\n\nI like PhysicalSize.\n\n>\n> # \\todo Rename this property to PhysicalSize once we will have property\n> # categories\n>\n> > +\n> > + - PixelArray:\n>\n> I was going to say we should rename this to PixelArraySize as it's the\n> size of the pixel array, and then realized that the previous property\n> has the same name :-) Maybe we should rename both ?\n\nLet's rename both\n\n>\n> > + type: int32_t\n> > + size: [2]\n> > + description: |\n> > + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> > +\n> > + The property describes a rectangle with its top-left corner in position\n> > + (0, 0) and width and height described by the first and second values\n> > + of this property.\n> > +\n> > + The PixelArray property defines the rectangle that includes all possible\n> > + rectangles defined by the ActiveAreas property, and describes the full\n> > + pixel array, including non-active pixels, black level calibration\n> > + pixels etc.\n>\n> It's not entirely clear to me what pixels should be included in this\n> rectangle. I'm not blaming your documentation here, but rather my lack\n> of knowledge of what else than non-active pixels and black level\n> calibration pixels there are :-) Just an idea, instead of linking this\n> property to ActiveAreas, would it make sense to link it to\n> PixelArraySize ? Maybe something along the lines of\n\nI would just drop rectangle maybe, as that property transports a widht\nand an height.\n\n>\n> \tThe PixelArraySize property defines the size of the full pixel\n> \tarray, covered by the PixelArrayPhysicalSize area. This may\n> \tinclude inactive or optical black pixels.\n>\n\nI'll take this in, but I'm not sure if it's a good idea to link this\none to the PhysicalArraySize, as they are expressed with two different\nmeasure units. So I would keep at least the first line of the existing\ndescription, and s/may include/includes/ in your proposed change, as\nthis is not optional, the property should report inactive pixels too.\n\n> > +\n> > + - ActiveAreas:\n> > + type: int32_t\n> > + size: [4 x n]\n> > + description: |\n> > + The camera sensor active pixel area rectangles, represented as\n> > + rectangles contained in the one described by the PixelArray property.\n> > +\n> > + This property describes an arbitrary number of overlapping rectangles,\n> > + representing the active pixel portions of the camera sensor pixel array.\n> > +\n> > + Each rectangle is defined by its displacement from pixel (0, 0) of\n> > + the rectangle described by the PixelArray property, a width and an\n> > + height.\n>\n> s/an height/a height/\n>\n> > +\n> > + Each rectangle described by this property represents the maximum image\n> > + size that the camera module can produce for a given image resolution.\n>\n> How about s/for a given image resolution/for a particular aspect ratio/\n> ?\n>\n> I would also add, taken from below,\n>\n> When multiple rectangles are reported, they shall be ordered\n> \tfrom the tallest to the shortest.\n\nack, I though I had it in previous versions..\n\n>\n> > +\n> > + Example 1.\n> > + A sensor which only produces images in the 4:3 image resolution will\n> > + report a single ActiveArea rectangle, from which all other image formats\n> > + are obtained by either cropping the field-of-view and/or applying pixel\n> > + sub-sampling techniques such as pixel skipping or binning.\n> > +\n> > + PixelArray(0)\n> > + /-----------------/\n> > + x1 x2\n> > + (0,0)-> +-o------------o-+ /\n> > + y1 o +------------+ | |\n> > + | |////////////| | |\n> > + | |////////////| | | PixelArray(1)\n> > + | |////////////| | |\n> > + y2 o +------------+ | |\n> > + +----------------+ /\n> > +\n> > + The property reports a single rectangle\n> > +\n> > + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))\n>\n> If the rectangle is defined through width and height, this should be\n>\n> ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)\n>\n> Alternatively you could use coordinates:\n>\n> ActiveArea = (x1, y1, x2, y2)\n\nthis is not correct, as y2 is the vertical distance from pixel (0,0)\nwhile the active area vertical size is (y2 - y1 +1). Same for the\nhorizontal size.\n\n>\n> > +\n> > + Example 2\n> > + A camera sensor which can produce images in different native\n>\n> s/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.\n\nI would keep using 'camera sensor' whenever possible.\n\n>\n> > + resolutions, will report several overlapping rectangle, one for each\n>\n> s/resolutions,/resolutions/\n> s/rectangle/rectangles/\n>\n> > + natively supported resolution, ordered from the tallest to the shortest\n> > + one.\n> > +\n> > + PixelArray(0)\n> > + /-----------------/\n> > + x1 x2 x3 x4\n> > + (0,0)-> +o---o------o---o+ /\n> > + y1 | +------+ | |\n> > + | |//////| | |\n> > + y2 o+---+------+---+| |\n> > + ||///|//////|///|| | PixelArray(1)\n> > + y3 o+---+------+---+| |\n> > + | |//////| | |\n> > + y4 | +------+ | |\n>\n> I think you need a o next to y1 and y4.\n>\n\nYes!\n\n> > + +----+------+----+ /\n> > +\n> > + The property reports two rectangles\n> > +\n> > + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),\n>\n> s/y4 - 1/y4 - y1/\n\nups, yes\n\n>\n> > + (x1, y2, (x4 - x1), (y3 - y2))\n>\n> And comment as above regarding the width.\n>\n\nI'll add '- 1'\n\n> > +\n> > + The first rectangle describes the maximum field-of-view of all image\n> > + formats in the 4:3 resolutions, while the second one describes the\n> > + maximum field of view for all image formats in the 16:9 resolutions.\n> > +\n> > + - BayerFilterArrangement:\n> > + type: int32_t\n> > + description: |\n> > + The pixel array color filter displacement.\n>\n> I could be wrong, but is \"displacement\" the right term ? Maybe\n> arrangement ?\n>\n\nProbably yes.\n\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> As we could support sensors with non-Bayer CFAs, how about already\n> renaming the control to ColorFilterArrangement, and remove Bayer from\n> here ?\n>\n\nAck\n\n> > +\n> > + Color filters are usually displaced in line-alternating fashion on the\n>\n> s/displaced/arranged/ ?\n>\n> > + sensor pixel array. In example, one line might be composed of Red-Green\n>\n> s/In example/For example/\n>\n> > + while the successive is composed of Blue-Green color information.\n> > +\n> > + The value of this property represents the arrangement of color filters\n> > + in the top-left 2x2 pixel square.\n>\n> As this is only valid for Bayer, maybe\n>\n> For Bayer filters, the value of this property represents the arrangement\n> of color filters in the top-left 2x2 pixel square.\n\nAck\n\n>\n> > +\n> > + For example, for a sensor with the following color filter displacement\n>\n> s/displacement/arrangement/\n>\n> (you could also use pattern instead of arrangement here or in other\n> places)\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 filter arrangement is represented by the BGGR value, which\n> > + correspond to the pixel readout sequence in line interleaved mode.\n> > +\n> > + enum:\n> > + - name: BayerFilterRGGB\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> Should these be sorted alphabetically ?\n>\n\nWe could, yes\n\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> I would drop this value for now, as we should instead report which\n> filter arrangement is used. We could already add known patterns, such as\n> the ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,\n> but I think that's overkill, we can extend it later when/if needed.\n>\n\nAck\n\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 (or later) standard.\n>\n> Is this the ISO speed or the Standard Output Sensitivity (SOS) ? I think\n> this property needs a bit more research, should we leave it out for now\n> to avoid blocking the rest ?\n\nYes, this is barely copied without going into much detail. Let's leave\nit out for now.\n\n>\n> Overall, good work ! No major issue, so the next version should be the\n> last one.\n\nThanks and thanks for your comments.\n\n>\n> > +\n> > ...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95732603FB\n\tfor ;\n\tSat, 25 Apr 2020 15:55:37 +0200 (CEST)","from uno.localdomain\n\t(host240-55-dynamic.3-87-r.retail.telecomitalia.it [87.3.55.240])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 155FCE000A;\n\tSat, 25 Apr 2020 13:55:35 +0000 (UTC)"],"X-Originating-IP":"87.3.55.240","Date":"Sat, 25 Apr 2020 15:58:45 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425135845.cqg7yac3aivev2se@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>\n\t<20200425020511.GB19672@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425020511.GB19672@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 25 Apr 2020 13:55:37 -0000"}},{"id":4514,"web_url":"https://patchwork.libcamera.org/comment/4514/","msgid":"<20200425165011.GA11157@pendragon.ideasonboard.com>","date":"2020-04-25T16:50:11","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sat, Apr 25, 2020 at 03:58:45PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> >> Add definition of pixel array related properties.\n> >>\n> >> Reviewed-by: Niklas Söderlund \n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> >> 1 file changed, 155 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> >> index ce627fa042ba..8f6797723a9d 100644\n> >> --- a/src/libcamera/property_ids.yaml\n> >> +++ b/src/libcamera/property_ids.yaml\n> >> @@ -386,4 +386,159 @@ controls:\n> >> | |\n> >> | |\n> >> +--------------------+\n> >> +\n> >> + - PixelArraySize:\n> >> + type: float\n> >> + size: [2]\n> >> + description: |\n> >> + The physical sizes of the pixel array (width and height), in\n> >> + millimeters.\n> >\n> > Once we'll have categories for properties I think this should become\n> > PhysicalSize in the sensor category. We could already rename it now to\n> > PhysicalSize or PixelArrayPhysicalSize if you think it would be a good\n> > idea. I don't mind much either way, but if we keep the current name, how\n> > about adding a comment to remember this ?\n> \n> I like PhysicalSize.\n\nBTW, is mm precise enough, or should we use µm ?\n\n> >\n> > # \\todo Rename this property to PhysicalSize once we will have property\n> > # categories\n> >\n> >> +\n> >> + - PixelArray:\n> >\n> > I was going to say we should rename this to PixelArraySize as it's the\n> > size of the pixel array, and then realized that the previous property\n> > has the same name :-) Maybe we should rename both ?\n> \n> Let's rename both\n> \n> >> + type: int32_t\n> >> + size: [2]\n> >> + description: |\n> >> + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> >> +\n> >> + The property describes a rectangle with its top-left corner in position\n> >> + (0, 0) and width and height described by the first and second values\n> >> + of this property.\n> >> +\n> >> + The PixelArray property defines the rectangle that includes all possible\n> >> + rectangles defined by the ActiveAreas property, and describes the full\n> >> + pixel array, including non-active pixels, black level calibration\n> >> + pixels etc.\n> >\n> > It's not entirely clear to me what pixels should be included in this\n> > rectangle. I'm not blaming your documentation here, but rather my lack\n> > of knowledge of what else than non-active pixels and black level\n> > calibration pixels there are :-) Just an idea, instead of linking this\n> > property to ActiveAreas, would it make sense to link it to\n> > PixelArraySize ? Maybe something along the lines of\n> \n> I would just drop rectangle maybe, as that property transports a widht\n> and an height.\n> \n> > \tThe PixelArraySize property defines the size of the full pixel\n> > \tarray, covered by the PixelArrayPhysicalSize area. This may\n> > \tinclude inactive or optical black pixels.\n> \n> I'll take this in, but I'm not sure if it's a good idea to link this\n> one to the PhysicalArraySize, as they are expressed with two different\n> measure units.\n\nI think linking the two are important, but we need to figure out what\nthe link is. Otherwise, does the physical size represent the size of the\nchip (quite unlikelu), the size of the full pixel array, the size of the\noptical window, ... ?\n\nI'm looking at the IMX219 datasheet, and it reports\n\n- Image size: diagonal 4.60mm\n- Total number of pixels: 3296x2512\n- Number of effective pixels: 3296x2480\n- Number of active pixels: 3280x2464\n- Unit cell size: 1.12µm x 1.12µm\n\nCalculating the diagonal leads to 4641µm, 4620µm and 4595µm for the\nthree rectangles respectively. The last one match 4.60mm.\n\nWith the above definition of the physical size, the diagonal would be\n4641µm (for a size of 3692µm x 2813µm). I wonder if we should report the\nactive image size instead (3674µm x 2760µm for a diagonal of 4595µm).\nIt's not difficult to convert between the two (assuming all the pixels\nhave the same size), but I'd like to pick the one that makes the most\nsense. We can of course reconsider that decision later if we find out we\nmade a mistake.\n\nMy reasoning for picking the size of the active area is that the\nphysical size will most likely be used to make optics calculation, and\nthe lens should be selected and positioned according to the active area.\nHowever, there could be systems with a different lens positioning, and\nI'm not sure yet what that would imply.\n\nAndroid seems to report the physical size of the full pixel array,\nincluding inactive pixels. I'm not sure what the rationale is, and maybe\nthe difference between the two is lost in the measurement error noise in\nthe end, so I wouldn't rule out that they may not have been picked the\nbest option (but I also don't imply I know better :-)).\n\nAs a conclusion, let's pick one option (physical size covering the\nactive area or the full array), and document it.\n\n> So I would keep at least the first line of the existing\n> description, and s/may include/includes/ in your proposed change, as\n> this is not optional, the property should report inactive pixels too.\n\nI think it would be better to refer to the pixel array size property in\nthe definition of ActiveAreas below, not the other way around, otherwise\nyou'll have a cycle :-)\n\nThe sensors I've seen typically have the following structure, from top\nto bottom:\n\n- A few dummy lines, for unknown purpose\n- Optical black lines, split in three areas (invalid, valid, invalid)\n- Exposed lines, split in three areas (invalid, valid, invalid)\n\nExposed lines are sometimes called effective, and \"active\" pixels can\nrefer to either all the exposed lines or to the valid exposed lines,\ndepending on datasheets.\n\nThe invalid lines are considered not to be usable, usually because\nthey're too close to the edges and thus can contain artifacts. Some of\nthem may be used for internal purpose. The dummy lines, optical black\nlines (both invalid and valid) and invalid exposed lines may or may not\nbe readable, depending on the sensor.\n\nThe question now is what else than the valid exposed pixels need to be\nreported in this property. I think we need to look at it from the point\nof view of what the sensor can readout (assuming we map the physical\nsize to the valid active pixels), otherwise I don't really see the point\nin giving information about something that would have absolutely no\ninfluence on the receiver side.\n\n> >> +\n> >> + - ActiveAreas:\n> >> + type: int32_t\n> >> + size: [4 x n]\n> >> + description: |\n> >> + The camera sensor active pixel area rectangles, represented as\n> >> + rectangles contained in the one described by the PixelArray property.\n> >> +\n> >> + This property describes an arbitrary number of overlapping rectangles,\n> >> + representing the active pixel portions of the camera sensor pixel array.\n> >> +\n> >> + Each rectangle is defined by its displacement from pixel (0, 0) of\n> >> + the rectangle described by the PixelArray property, a width and an\n> >> + height.\n> >\n> > s/an height/a height/\n> >\n> >> +\n> >> + Each rectangle described by this property represents the maximum image\n> >> + size that the camera module can produce for a given image resolution.\n> >\n> > How about s/for a given image resolution/for a particular aspect ratio/\n> > ?\n> >\n> > I would also add, taken from below,\n> >\n> > When multiple rectangles are reported, they shall be ordered\n> > \tfrom the tallest to the shortest.\n> \n> ack, I though I had it in previous versions..\n> \n> >> +\n> >> + Example 1.\n> >> + A sensor which only produces images in the 4:3 image resolution will\n> >> + report a single ActiveArea rectangle, from which all other image formats\n> >> + are obtained by either cropping the field-of-view and/or applying pixel\n> >> + sub-sampling techniques such as pixel skipping or binning.\n> >> +\n> >> + PixelArray(0)\n> >> + /-----------------/\n> >> + x1 x2\n> >> + (0,0)-> +-o------------o-+ /\n> >> + y1 o +------------+ | |\n> >> + | |////////////| | |\n> >> + | |////////////| | | PixelArray(1)\n> >> + | |////////////| | |\n> >> + y2 o +------------+ | |\n> >> + +----------------+ /\n> >> +\n> >> + The property reports a single rectangle\n> >> +\n> >> + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))\n> >\n> > If the rectangle is defined through width and height, this should be\n> >\n> > ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)\n> >\n> > Alternatively you could use coordinates:\n> >\n> > ActiveArea = (x1, y1, x2, y2)\n> \n> this is not correct, as y2 is the vertical distance from pixel (0,0)\n> while the active area vertical size is (y2 - y1 +1). Same for the\n> horizontal size.\n\nMy point was that we could define the rectangle through the coordinates\nof the top-left and bottom-right pixels (x1, y1, x2, y2), or through the\ncoordinates of the top-left pixel and its size (x1, y1, x2 - x1 + 1, y2\n- y1 + 1). I don't have a preference.\n\n> >> +\n> >> + Example 2\n> >> + A camera sensor which can produce images in different native\n> >\n> > s/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.\n> \n> I would keep using 'camera sensor' whenever possible.\n> \n> >> + resolutions, will report several overlapping rectangle, one for each\n> >\n> > s/resolutions,/resolutions/\n> > s/rectangle/rectangles/\n> >\n> >> + natively supported resolution, ordered from the tallest to the shortest\n> >> + one.\n> >> +\n> >> + PixelArray(0)\n> >> + /-----------------/\n> >> + x1 x2 x3 x4\n> >> + (0,0)-> +o---o------o---o+ /\n> >> + y1 | +------+ | |\n> >> + | |//////| | |\n> >> + y2 o+---+------+---+| |\n> >> + ||///|//////|///|| | PixelArray(1)\n> >> + y3 o+---+------+---+| |\n> >> + | |//////| | |\n> >> + y4 | +------+ | |\n> >\n> > I think you need a o next to y1 and y4.\n> \n> Yes!\n> \n> >> + +----+------+----+ /\n> >> +\n> >> + The property reports two rectangles\n> >> +\n> >> + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),\n> >\n> > s/y4 - 1/y4 - y1/\n> \n> ups, yes\n> \n> >> + (x1, y2, (x4 - x1), (y3 - y2))\n> >\n> > And comment as above regarding the width.\n> \n> I'll add '- 1'\n> \n> >> +\n> >> + The first rectangle describes the maximum field-of-view of all image\n> >> + formats in the 4:3 resolutions, while the second one describes the\n> >> + maximum field of view for all image formats in the 16:9 resolutions.\n> >> +\n> >> + - BayerFilterArrangement:\n> >> + type: int32_t\n> >> + description: |\n> >> + The pixel array color filter displacement.\n> >\n> > I could be wrong, but is \"displacement\" the right term ? Maybe\n> > arrangement ?\n> \n> Probably yes.\n> \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> > As we could support sensors with non-Bayer CFAs, how about already\n> > renaming the control to ColorFilterArrangement, and remove Bayer from\n> >\n> \n> Ack\n> \n> >> +\n> >> + Color filters are usually displaced in line-alternating fashion on the\n> >\n> > s/displaced/arranged/ ?\n> >\n> >> + sensor pixel array. In example, one line might be composed of Red-Green\n> >\n> > s/In example/For example/\n> >\n> >> + while the successive is composed of Blue-Green color information.\n> >> +\n> >> + The value of this property represents the arrangement of color filters\n> >> + in the top-left 2x2 pixel square.\n> >\n> > As this is only valid for Bayer, maybe\n> >\n> > For Bayer filters, the value of this property represents the arrangement\n> > of color filters in the top-left 2x2 pixel square.\n> \n> Ack\n> \n> >> +\n> >> + For example, for a sensor with the following color filter displacement\n> >\n> > s/displacement/arrangement/\n> >\n> > (you could also use pattern instead of arrangement here or in other\n> > places)\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 filter arrangement is represented by the BGGR value, which\n> >> + correspond to the pixel readout sequence in line interleaved mode.\n> >> +\n> >> + enum:\n> >> + - name: BayerFilterRGGB\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> > Should these be sorted alphabetically ?\n> \n> We could, yes\n> \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> > I would drop this value for now, as we should instead report which\n> > filter arrangement is used. We could already add known patterns, such as\n> > the ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,\n> > but I think that's overkill, we can extend it later when/if needed.\n> \n> Ack\n> \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 (or later) standard.\n> >\n> > Is this the ISO speed or the Standard Output Sensitivity (SOS) ? I think\n> > this property needs a bit more research, should we leave it out for now\n> > to avoid blocking the rest ?\n> \n> Yes, this is barely copied without going into much detail. Let's leave\n> it out for now.\n> \n> > Overall, good work ! No major issue, so the next version should be the\n> > last one.\n> \n> Thanks and thanks for your comments.","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 AB734603FC\n\tfor ;\n\tSat, 25 Apr 2020 18:50:26 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E432D4F7;\n\tSat, 25 Apr 2020 18:50:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jMq0mecZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587833426;\n\tbh=oLTn36SWa7hm/XT6KXbR6C0jSX1OxjOBrxS6husS9Sc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jMq0mecZdKu3z5uj9RDrsWnG5KmeVP6HFENx/LhBZu7xhf4DJ0tc7L5imQrGvC33/\n\trqtnChY1WVGCSbJ7gBaXTVeVDXGjwc/i/c/wfdSfQSPATY4yhD7ELqLG+d8PpQQEgm\n\t8n7NEY9PLtWF+DNHA25lyZV0o1EMVvscAsk6tMME=","Date":"Sat, 25 Apr 2020 19:50:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425165011.GA11157@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>\n\t<20200425020511.GB19672@pendragon.ideasonboard.com>\n\t<20200425135845.cqg7yac3aivev2se@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425135845.cqg7yac3aivev2se@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 25 Apr 2020 16:50:26 -0000"}},{"id":4522,"web_url":"https://patchwork.libcamera.org/comment/4522/","msgid":"<20200425211933.GG10975@pendragon.ideasonboard.com>","date":"2020-04-25T21:19:33","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"(Adding Sakari to CC)\n\nOn Sat, Apr 25, 2020 at 07:50:12PM +0300, Laurent Pinchart wrote:\n> On Sat, Apr 25, 2020 at 03:58:45PM +0200, Jacopo Mondi wrote:\n> > On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:\n> > > On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> > >> Add definition of pixel array related properties.\n> > >>\n> > >> Reviewed-by: Niklas Söderlund \n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> > >> 1 file changed, 155 insertions(+)\n> > >>\n> > >> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > >> index ce627fa042ba..8f6797723a9d 100644\n> > >> --- a/src/libcamera/property_ids.yaml\n> > >> +++ b/src/libcamera/property_ids.yaml\n> > >> @@ -386,4 +386,159 @@ controls:\n> > >> | |\n> > >> | |\n> > >> +--------------------+\n> > >> +\n> > >> + - PixelArraySize:\n> > >> + type: float\n> > >> + size: [2]\n> > >> + description: |\n> > >> + The physical sizes of the pixel array (width and height), in\n> > >> + millimeters.\n> > >\n> > > Once we'll have categories for properties I think this should become\n> > > PhysicalSize in the sensor category. We could already rename it now to\n> > > PhysicalSize or PixelArrayPhysicalSize if you think it would be a good\n> > > idea. I don't mind much either way, but if we keep the current name, how\n> > > about adding a comment to remember this ?\n> > \n> > I like PhysicalSize.\n> \n> BTW, is mm precise enough, or should we use µm ?\n> \n> > >\n> > > # \\todo Rename this property to PhysicalSize once we will have property\n> > > # categories\n> > >\n> > >> +\n> > >> + - PixelArray:\n> > >\n> > > I was going to say we should rename this to PixelArraySize as it's the\n> > > size of the pixel array, and then realized that the previous property\n> > > has the same name :-) Maybe we should rename both ?\n> > \n> > Let's rename both\n> > \n> > >> + type: int32_t\n> > >> + size: [2]\n> > >> + description: |\n> > >> + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> > >> +\n> > >> + The property describes a rectangle with its top-left corner in position\n> > >> + (0, 0) and width and height described by the first and second values\n> > >> + of this property.\n> > >> +\n> > >> + The PixelArray property defines the rectangle that includes all possible\n> > >> + rectangles defined by the ActiveAreas property, and describes the full\n> > >> + pixel array, including non-active pixels, black level calibration\n> > >> + pixels etc.\n> > >\n> > > It's not entirely clear to me what pixels should be included in this\n> > > rectangle. I'm not blaming your documentation here, but rather my lack\n> > > of knowledge of what else than non-active pixels and black level\n> > > calibration pixels there are :-) Just an idea, instead of linking this\n> > > property to ActiveAreas, would it make sense to link it to\n> > > PixelArraySize ? Maybe something along the lines of\n> > \n> > I would just drop rectangle maybe, as that property transports a widht\n> > and an height.\n> > \n> > > \tThe PixelArraySize property defines the size of the full pixel\n> > > \tarray, covered by the PixelArrayPhysicalSize area. This may\n> > > \tinclude inactive or optical black pixels.\n> > \n> > I'll take this in, but I'm not sure if it's a good idea to link this\n> > one to the PhysicalArraySize, as they are expressed with two different\n> > measure units.\n> \n> I think linking the two are important, but we need to figure out what\n> the link is. Otherwise, does the physical size represent the size of the\n> chip (quite unlikelu), the size of the full pixel array, the size of the\n> optical window, ... ?\n> \n> I'm looking at the IMX219 datasheet, and it reports\n> \n> - Image size: diagonal 4.60mm\n> - Total number of pixels: 3296x2512\n> - Number of effective pixels: 3296x2480\n> - Number of active pixels: 3280x2464\n> - Unit cell size: 1.12µm x 1.12µm\n> \n> Calculating the diagonal leads to 4641µm, 4620µm and 4595µm for the\n> three rectangles respectively. The last one match 4.60mm.\n> \n> With the above definition of the physical size, the diagonal would be\n> 4641µm (for a size of 3692µm x 2813µm). I wonder if we should report the\n> active image size instead (3674µm x 2760µm for a diagonal of 4595µm).\n> It's not difficult to convert between the two (assuming all the pixels\n> have the same size), but I'd like to pick the one that makes the most\n> sense. We can of course reconsider that decision later if we find out we\n> made a mistake.\n> \n> My reasoning for picking the size of the active area is that the\n> physical size will most likely be used to make optics calculation, and\n> the lens should be selected and positioned according to the active area.\n> However, there could be systems with a different lens positioning, and\n> I'm not sure yet what that would imply.\n> \n> Android seems to report the physical size of the full pixel array,\n> including inactive pixels. I'm not sure what the rationale is, and maybe\n> the difference between the two is lost in the measurement error noise in\n> the end, so I wouldn't rule out that they may not have been picked the\n> best option (but I also don't imply I know better :-)).\n> \n> As a conclusion, let's pick one option (physical size covering the\n> active area or the full array), and document it.\n> \n> > So I would keep at least the first line of the existing\n> > description, and s/may include/includes/ in your proposed change, as\n> > this is not optional, the property should report inactive pixels too.\n> \n> I think it would be better to refer to the pixel array size property in\n> the definition of ActiveAreas below, not the other way around, otherwise\n> you'll have a cycle :-)\n> \n> The sensors I've seen typically have the following structure, from top\n> to bottom:\n> \n> - A few dummy lines, for unknown purpose\n> - Optical black lines, split in three areas (invalid, valid, invalid)\n> - Exposed lines, split in three areas (invalid, valid, invalid)\n> \n> Exposed lines are sometimes called effective, and \"active\" pixels can\n> refer to either all the exposed lines or to the valid exposed lines,\n> depending on datasheets.\n> \n> The invalid lines are considered not to be usable, usually because\n> they're too close to the edges and thus can contain artifacts. Some of\n> them may be used for internal purpose. The dummy lines, optical black\n> lines (both invalid and valid) and invalid exposed lines may or may not\n> be readable, depending on the sensor.\n> \n> The question now is what else than the valid exposed pixels need to be\n> reported in this property. I think we need to look at it from the point\n> of view of what the sensor can readout (assuming we map the physical\n> size to the valid active pixels), otherwise I don't really see the point\n> in giving information about something that would have absolutely no\n> influence on the receiver side.\n> \n> > >> +\n> > >> + - ActiveAreas:\n> > >> + type: int32_t\n> > >> + size: [4 x n]\n> > >> + description: |\n> > >> + The camera sensor active pixel area rectangles, represented as\n> > >> + rectangles contained in the one described by the PixelArray property.\n> > >> +\n> > >> + This property describes an arbitrary number of overlapping rectangles,\n> > >> + representing the active pixel portions of the camera sensor pixel array.\n> > >> +\n> > >> + Each rectangle is defined by its displacement from pixel (0, 0) of\n> > >> + the rectangle described by the PixelArray property, a width and an\n> > >> + height.\n> > >\n> > > s/an height/a height/\n> > >\n> > >> +\n> > >> + Each rectangle described by this property represents the maximum image\n> > >> + size that the camera module can produce for a given image resolution.\n> > >\n> > > How about s/for a given image resolution/for a particular aspect ratio/\n> > > ?\n> > >\n> > > I would also add, taken from below,\n> > >\n> > > When multiple rectangles are reported, they shall be ordered\n> > > \tfrom the tallest to the shortest.\n> > \n> > ack, I though I had it in previous versions..\n> > \n> > >> +\n> > >> + Example 1.\n> > >> + A sensor which only produces images in the 4:3 image resolution will\n> > >> + report a single ActiveArea rectangle, from which all other image formats\n> > >> + are obtained by either cropping the field-of-view and/or applying pixel\n> > >> + sub-sampling techniques such as pixel skipping or binning.\n> > >> +\n> > >> + PixelArray(0)\n> > >> + /-----------------/\n> > >> + x1 x2\n> > >> + (0,0)-> +-o------------o-+ /\n> > >> + y1 o +------------+ | |\n> > >> + | |////////////| | |\n> > >> + | |////////////| | | PixelArray(1)\n> > >> + | |////////////| | |\n> > >> + y2 o +------------+ | |\n> > >> + +----------------+ /\n> > >> +\n> > >> + The property reports a single rectangle\n> > >> +\n> > >> + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))\n> > >\n> > > If the rectangle is defined through width and height, this should be\n> > >\n> > > ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)\n> > >\n> > > Alternatively you could use coordinates:\n> > >\n> > > ActiveArea = (x1, y1, x2, y2)\n> > \n> > this is not correct, as y2 is the vertical distance from pixel (0,0)\n> > while the active area vertical size is (y2 - y1 +1). Same for the\n> > horizontal size.\n> \n> My point was that we could define the rectangle through the coordinates\n> of the top-left and bottom-right pixels (x1, y1, x2, y2), or through the\n> coordinates of the top-left pixel and its size (x1, y1, x2 - x1 + 1, y2\n> - y1 + 1). I don't have a preference.\n> \n> > >> +\n> > >> + Example 2\n> > >> + A camera sensor which can produce images in different native\n> > >\n> > > s/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.\n> > \n> > I would keep using 'camera sensor' whenever possible.\n> > \n> > >> + resolutions, will report several overlapping rectangle, one for each\n> > >\n> > > s/resolutions,/resolutions/\n> > > s/rectangle/rectangles/\n> > >\n> > >> + natively supported resolution, ordered from the tallest to the shortest\n> > >> + one.\n> > >> +\n> > >> + PixelArray(0)\n> > >> + /-----------------/\n> > >> + x1 x2 x3 x4\n> > >> + (0,0)-> +o---o------o---o+ /\n> > >> + y1 | +------+ | |\n> > >> + | |//////| | |\n> > >> + y2 o+---+------+---+| |\n> > >> + ||///|//////|///|| | PixelArray(1)\n> > >> + y3 o+---+------+---+| |\n> > >> + | |//////| | |\n> > >> + y4 | +------+ | |\n> > >\n> > > I think you need a o next to y1 and y4.\n> > \n> > Yes!\n> > \n> > >> + +----+------+----+ /\n> > >> +\n> > >> + The property reports two rectangles\n> > >> +\n> > >> + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),\n> > >\n> > > s/y4 - 1/y4 - y1/\n> > \n> > ups, yes\n> > \n> > >> + (x1, y2, (x4 - x1), (y3 - y2))\n> > >\n> > > And comment as above regarding the width.\n> > \n> > I'll add '- 1'\n> > \n> > >> +\n> > >> + The first rectangle describes the maximum field-of-view of all image\n> > >> + formats in the 4:3 resolutions, while the second one describes the\n> > >> + maximum field of view for all image formats in the 16:9 resolutions.\n> > >> +\n> > >> + - BayerFilterArrangement:\n> > >> + type: int32_t\n> > >> + description: |\n> > >> + The pixel array color filter displacement.\n> > >\n> > > I could be wrong, but is \"displacement\" the right term ? Maybe\n> > > arrangement ?\n> > \n> > Probably yes.\n> > \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> > > As we could support sensors with non-Bayer CFAs, how about already\n> > > renaming the control to ColorFilterArrangement, and remove Bayer from\n> > >\n> > \n> > Ack\n> > \n> > >> +\n> > >> + Color filters are usually displaced in line-alternating fashion on the\n> > >\n> > > s/displaced/arranged/ ?\n> > >\n> > >> + sensor pixel array. In example, one line might be composed of Red-Green\n> > >\n> > > s/In example/For example/\n> > >\n> > >> + while the successive is composed of Blue-Green color information.\n> > >> +\n> > >> + The value of this property represents the arrangement of color filters\n> > >> + in the top-left 2x2 pixel square.\n> > >\n> > > As this is only valid for Bayer, maybe\n> > >\n> > > For Bayer filters, the value of this property represents the arrangement\n> > > of color filters in the top-left 2x2 pixel square.\n> > \n> > Ack\n> > \n> > >> +\n> > >> + For example, for a sensor with the following color filter displacement\n> > >\n> > > s/displacement/arrangement/\n> > >\n> > > (you could also use pattern instead of arrangement here or in other\n> > > places)\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 filter arrangement is represented by the BGGR value, which\n> > >> + correspond to the pixel readout sequence in line interleaved mode.\n> > >> +\n> > >> + enum:\n> > >> + - name: BayerFilterRGGB\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> > > Should these be sorted alphabetically ?\n> > \n> > We could, yes\n> > \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> > > I would drop this value for now, as we should instead report which\n> > > filter arrangement is used. We could already add known patterns, such as\n> > > the ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,\n> > > but I think that's overkill, we can extend it later when/if needed.\n> > \n> > Ack\n> > \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 (or later) standard.\n> > >\n> > > Is this the ISO speed or the Standard Output Sensitivity (SOS) ? I think\n> > > this property needs a bit more research, should we leave it out for now\n> > > to avoid blocking the rest ?\n> > \n> > Yes, this is barely copied without going into much detail. Let's leave\n> > it out for now.\n> > \n> > > Overall, good work ! No major issue, so the next version should be the\n> > > last one.\n> > \n> > Thanks and thanks for your comments.","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 A17EE603FC\n\tfor ;\n\tSat, 25 Apr 2020 23:19:48 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D216B4F7;\n\tSat, 25 Apr 2020 23:19:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"N2oaIVIX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587849588;\n\tbh=2NuGBN1P8sxsaP/nrCjuUWk65K2LZG+ekI/HgcMaN3I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N2oaIVIXYxot+AnbG6taGr6OU/NJJwSnOxyk8KiJCU22jNmzjvGpfjptk8gni/Q8t\n\tAvhDIBD+QYSAwSItKwtrGDPJI12XjuspXzfCAFFY+30IiSXSNb2GNXc8Vy+z2+4zv/\n\t9skZhD/D1SGLoza22vX/BgZG9abhCND+wUfXrh/I=","Date":"Sun, 26 Apr 2020 00:19:33 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Sakari Ailus ","Message-ID":"<20200425211933.GG10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>\n\t<20200425020511.GB19672@pendragon.ideasonboard.com>\n\t<20200425135845.cqg7yac3aivev2se@uno.localdomain>\n\t<20200425165011.GA11157@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425165011.GA11157@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 25 Apr 2020 21:19:48 -0000"}},{"id":4869,"web_url":"https://patchwork.libcamera.org/comment/4869/","msgid":"<20200519155007.ztrwwj55mhyukkmj@uno.localdomain>","date":"2020-05-19T15:50:07","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n I just re-started looking into this\n\nOn Sat, Apr 25, 2020 at 07:50:11PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sat, Apr 25, 2020 at 03:58:45PM +0200, Jacopo Mondi wrote:\n> > On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:\n> > > On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> > >> Add definition of pixel array related properties.\n> > >>\n> > >> Reviewed-by: Niklas Söderlund \n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> > >> 1 file changed, 155 insertions(+)\n> > >>\n> > >> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > >> index ce627fa042ba..8f6797723a9d 100644\n> > >> --- a/src/libcamera/property_ids.yaml\n> > >> +++ b/src/libcamera/property_ids.yaml\n> > >> @@ -386,4 +386,159 @@ controls:\n> > >> | |\n> > >> | |\n> > >> +--------------------+\n> > >> +\n> > >> + - PixelArraySize:\n> > >> + type: float\n> > >> + size: [2]\n> > >> + description: |\n> > >> + The physical sizes of the pixel array (width and height), in\n> > >> + millimeters.\n> > >\n> > > Once we'll have categories for properties I think this should become\n> > > PhysicalSize in the sensor category. We could already rename it now to\n> > > PhysicalSize or PixelArrayPhysicalSize if you think it would be a good\n> > > idea. I don't mind much either way, but if we keep the current name, how\n> > > about adding a comment to remember this ?\n> >\n> > I like PhysicalSize.\n>\n> BTW, is mm precise enough, or should we use µm ?\n>\n\nI had a look at a few manuals of recent-ish sensors and the their\n(active or full pixel array size) range from ~2 to ~6 mm, with a\nprecision of usually 3 more digits.\n\nConsidering the single pixel size will shrink, even if resolution\nwould likely go up, we might go below 1mm, and anyway, it's easier to\nexpress 1234 um than 1,234 mm\n\n> > >\n> > > # \\todo Rename this property to PhysicalSize once we will have property\n> > > # categories\n> > >\n> > >> +\n> > >> + - PixelArray:\n> > >\n> > > I was going to say we should rename this to PixelArraySize as it's the\n> > > size of the pixel array, and then realized that the previous property\n> > > has the same name :-) Maybe we should rename both ?\n> >\n> > Let's rename both\n> >\n> > >> + type: int32_t\n> > >> + size: [2]\n> > >> + description: |\n> > >> + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> > >> +\n> > >> + The property describes a rectangle with its top-left corner in position\n> > >> + (0, 0) and width and height described by the first and second values\n> > >> + of this property.\n> > >> +\n> > >> + The PixelArray property defines the rectangle that includes all possible\n> > >> + rectangles defined by the ActiveAreas property, and describes the full\n> > >> + pixel array, including non-active pixels, black level calibration\n> > >> + pixels etc.\n> > >\n> > > It's not entirely clear to me what pixels should be included in this\n> > > rectangle. I'm not blaming your documentation here, but rather my lack\n> > > of knowledge of what else than non-active pixels and black level\n> > > calibration pixels there are :-) Just an idea, instead of linking this\n> > > property to ActiveAreas, would it make sense to link it to\n> > > PixelArraySize ? Maybe something along the lines of\n> >\n> > I would just drop rectangle maybe, as that property transports a widht\n> > and an height.\n> >\n> > > \tThe PixelArraySize property defines the size of the full pixel\n> > > \tarray, covered by the PixelArrayPhysicalSize area. This may\n> > > \tinclude inactive or optical black pixels.\n> >\n> > I'll take this in, but I'm not sure if it's a good idea to link this\n> > one to the PhysicalArraySize, as they are expressed with two different\n> > measure units.\n>\n> I think linking the two are important, but we need to figure out what\n> the link is. Otherwise, does the physical size represent the size of the\n> chip (quite unlikelu), the size of the full pixel array, the size of the\n> optical window, ... ?\n\nA few manuals and products briefs from OV report the chip size (I\nassume, at least, as it varies depending on the package). But as you\nnoticed below, I presume the size is mostly used for optical\ncaluculations, so the chip area size is less relevant indeed.\n\n>\n> I'm looking at the IMX219 datasheet, and it reports\n>\n> - Image size: diagonal 4.60mm\n> - Total number of pixels: 3296x2512\n> - Number of effective pixels: 3296x2480\n> - Number of active pixels: 3280x2464\n> - Unit cell size: 1.12µm x 1.12µm\n>\n> Calculating the diagonal leads to 4641µm, 4620µm and 4595µm for the\n> three rectangles respectively. The last one match 4.60mm.\n>\n> With the above definition of the physical size, the diagonal would be\n> 4641µm (for a size of 3692µm x 2813µm). I wonder if we should report the\n> active image size instead (3674µm x 2760µm for a diagonal of 4595µm).\n> It's not difficult to convert between the two (assuming all the pixels\n> have the same size), but I'd like to pick the one that makes the most\n> sense. We can of course reconsider that decision later if we find out we\n> made a mistake.\n>\n> My reasoning for picking the size of the active area is that the\n> physical size will most likely be used to make optics calculation, and\n> the lens should be selected and positioned according to the active area.\n> However, there could be systems with a different lens positioning, and\n> I'm not sure yet what that would imply.\n>\n\nI have checked the same for the ov13858, and it reports\nthe physical size of the 'active area'.\n\nov5670 instead reports as phyisical size an area larger than the\nactive one.\n\nI fear there is no standard but I agree with linking the physical area\nsize to the active pixel area size, unfortunaly defining the 'active\narea' itself might not be trivial\n\n> Android seems to report the physical size of the full pixel array,\n> including inactive pixels. I'm not sure what the rationale is, and maybe\n> the difference between the two is lost in the measurement error noise in\n> the end, so I wouldn't rule out that they may not have been picked the\n> best option (but I also don't imply I know better :-)).\n>\n> As a conclusion, let's pick one option (physical size covering the\n> active area or the full array), and document it.\n>\n> > So I would keep at least the first line of the existing\n> > description, and s/may include/includes/ in your proposed change, as\n> > this is not optional, the property should report inactive pixels too.\n>\n> I think it would be better to refer to the pixel array size property in\n> the definition of ActiveAreas below, not the other way around, otherwise\n> you'll have a cycle :-)\n>\n> The sensors I've seen typically have the following structure, from top\n> to bottom:\n>\n> - A few dummy lines, for unknown purpose\n> - Optical black lines, split in three areas (invalid, valid, invalid)\n\nwhat is a 'valid' black line compared to an 'invalid' one ? I think\nthe same applies to column\n\n> - Exposed lines, split in three areas (invalid, valid, invalid)\n>\n> Exposed lines are sometimes called effective, and \"active\" pixels can\n> refer to either all the exposed lines or to the valid exposed lines,\n> depending on datasheets.\n>\n> The invalid lines are considered not to be usable, usually because\n> they're too close to the edges and thus can contain artifacts. Some of\n> them may be used for internal purpose. The dummy lines, optical black\n> lines (both invalid and valid) and invalid exposed lines may or may not\n> be readable, depending on the sensor.\n>\n> The question now is what else than the valid exposed pixels need to be\n> reported in this property. I think we need to look at it from the point\n> of view of what the sensor can readout (assuming we map the physical\n> size to the valid active pixels), otherwise I don't really see the point\n> in giving information about something that would have absolutely no\n> influence on the receiver side.\n\nI would consider invalid but exposed lines as part of the\nPixelArraySize, usually pixel (0,0) is part of an invalid but exposed\nline/column, isn't it ?\n\nI would then just drop the mention of 'optical blank' from the\nproperty definition, but keep it moslty like it is.\n\n>\n> > >> +\n> > >> + - ActiveAreas:\n> > >> + type: int32_t\n> > >> + size: [4 x n]\n> > >> + description: |\n> > >> + The camera sensor active pixel area rectangles, represented as\n> > >> + rectangles contained in the one described by the PixelArray property.\n> > >> +\n> > >> + This property describes an arbitrary number of overlapping rectangles,\n> > >> + representing the active pixel portions of the camera sensor pixel array.\n> > >> +\n> > >> + Each rectangle is defined by its displacement from pixel (0, 0) of\n> > >> + the rectangle described by the PixelArray property, a width and an\n> > >> + height.\n> > >\n> > > s/an height/a height/\n> > >\n> > >> +\n> > >> + Each rectangle described by this property represents the maximum image\n> > >> + size that the camera module can produce for a given image resolution.\n> > >\n> > > How about s/for a given image resolution/for a particular aspect ratio/\n> > > ?\n> > >\n> > > I would also add, taken from below,\n> > >\n> > > When multiple rectangles are reported, they shall be ordered\n> > > \tfrom the tallest to the shortest.\n> >\n> > ack, I though I had it in previous versions..\n> >\n> > >> +\n> > >> + Example 1.\n> > >> + A sensor which only produces images in the 4:3 image resolution will\n> > >> + report a single ActiveArea rectangle, from which all other image formats\n> > >> + are obtained by either cropping the field-of-view and/or applying pixel\n> > >> + sub-sampling techniques such as pixel skipping or binning.\n> > >> +\n> > >> + PixelArray(0)\n> > >> + /-----------------/\n> > >> + x1 x2\n> > >> + (0,0)-> +-o------------o-+ /\n> > >> + y1 o +------------+ | |\n> > >> + | |////////////| | |\n> > >> + | |////////////| | | PixelArray(1)\n> > >> + | |////////////| | |\n> > >> + y2 o +------------+ | |\n> > >> + +----------------+ /\n> > >> +\n> > >> + The property reports a single rectangle\n> > >> +\n> > >> + ActiveArea = (x1, y1, (x2 - x1), (y2 - y1))\n> > >\n> > > If the rectangle is defined through width and height, this should be\n> > >\n> > > ActiveArea = (x1, y1, x2 - x1 + 1, y2 - y1 + 1)\n> > >\n> > > Alternatively you could use coordinates:\n> > >\n> > > ActiveArea = (x1, y1, x2, y2)\n> >\n> > this is not correct, as y2 is the vertical distance from pixel (0,0)\n> > while the active area vertical size is (y2 - y1 +1). Same for the\n> > horizontal size.\n>\n> My point was that we could define the rectangle through the coordinates\n> of the top-left and bottom-right pixels (x1, y1, x2, y2), or through the\n> coordinates of the top-left pixel and its size (x1, y1, x2 - x1 + 1, y2\n> - y1 + 1). I don't have a preference.\n\nx2 != (x2 - x1)\n\n>\n> > >> +\n> > >> + Example 2\n> > >> + A camera sensor which can produce images in different native\n> > >\n> > > s/camera sensor/sensor/ here or s/sensor/camera sensor/ in example 1.\n> >\n> > I would keep using 'camera sensor' whenever possible.\n> >\n> > >> + resolutions, will report several overlapping rectangle, one for each\n> > >\n> > > s/resolutions,/resolutions/\n> > > s/rectangle/rectangles/\n> > >\n> > >> + natively supported resolution, ordered from the tallest to the shortest\n> > >> + one.\n> > >> +\n> > >> + PixelArray(0)\n> > >> + /-----------------/\n> > >> + x1 x2 x3 x4\n> > >> + (0,0)-> +o---o------o---o+ /\n> > >> + y1 | +------+ | |\n> > >> + | |//////| | |\n> > >> + y2 o+---+------+---+| |\n> > >> + ||///|//////|///|| | PixelArray(1)\n> > >> + y3 o+---+------+---+| |\n> > >> + | |//////| | |\n> > >> + y4 | +------+ | |\n> > >\n> > > I think you need a o next to y1 and y4.\n> >\n> > Yes!\n> >\n> > >> + +----+------+----+ /\n> > >> +\n> > >> + The property reports two rectangles\n> > >> +\n> > >> + PixelArray = ( (x2, y1, (x3 - x2), (y4 - 1),\n> > >\n> > > s/y4 - 1/y4 - y1/\n> >\n> > ups, yes\n> >\n> > >> + (x1, y2, (x4 - x1), (y3 - y2))\n> > >\n> > > And comment as above regarding the width.\n> >\n> > I'll add '- 1'\n> >\n> > >> +\n> > >> + The first rectangle describes the maximum field-of-view of all image\n> > >> + formats in the 4:3 resolutions, while the second one describes the\n> > >> + maximum field of view for all image formats in the 16:9 resolutions.\n> > >> +\n> > >> + - BayerFilterArrangement:\n> > >> + type: int32_t\n> > >> + description: |\n> > >> + The pixel array color filter displacement.\n> > >\n> > > I could be wrong, but is \"displacement\" the right term ? Maybe\n> > > arrangement ?\n> >\n> > Probably yes.\n> >\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> > > As we could support sensors with non-Bayer CFAs, how about already\n> > > renaming the control to ColorFilterArrangement, and remove Bayer from\n> > >\n> >\n> > Ack\n> >\n> > >> +\n> > >> + Color filters are usually displaced in line-alternating fashion on the\n> > >\n> > > s/displaced/arranged/ ?\n> > >\n> > >> + sensor pixel array. In example, one line might be composed of Red-Green\n> > >\n> > > s/In example/For example/\n> > >\n> > >> + while the successive is composed of Blue-Green color information.\n> > >> +\n> > >> + The value of this property represents the arrangement of color filters\n> > >> + in the top-left 2x2 pixel square.\n> > >\n> > > As this is only valid for Bayer, maybe\n> > >\n> > > For Bayer filters, the value of this property represents the arrangement\n> > > of color filters in the top-left 2x2 pixel square.\n> >\n> > Ack\n> >\n> > >> +\n> > >> + For example, for a sensor with the following color filter displacement\n> > >\n> > > s/displacement/arrangement/\n> > >\n> > > (you could also use pattern instead of arrangement here or in other\n> > > places)\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 filter arrangement is represented by the BGGR value, which\n> > >> + correspond to the pixel readout sequence in line interleaved mode.\n> > >> +\n> > >> + enum:\n> > >> + - name: BayerFilterRGGB\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> > > Should these be sorted alphabetically ?\n> >\n> > We could, yes\n> >\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> > > I would drop this value for now, as we should instead report which\n> > > filter arrangement is used. We could already add known patterns, such as\n> > > the ones from https://en.wikipedia.org/wiki/Bayer_filter for instance,\n> > > but I think that's overkill, we can extend it later when/if needed.\n> >\n> > Ack\n> >\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 (or later) standard.\n> > >\n> > > Is this the ISO speed or the Standard Output Sensitivity (SOS) ? I think\n> > > this property needs a bit more research, should we leave it out for now\n> > > to avoid blocking the rest ?\n> >\n> > Yes, this is barely copied without going into much detail. Let's leave\n> > it out for now.\n> >\n> > > Overall, good work ! No major issue, so the next version should be the\n> > > last one.\n> >\n> > Thanks and thanks for your comments.\n\nThere is a v4 out, but it was probably sent before these comments were\nreceived. I'll send a v5, maybe of this patch only to fast-track it\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87144603DA\n\tfor ;\n\tTue, 19 May 2020 17:46:49 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id F3C2C1C0013;\n\tTue, 19 May 2020 15:46:48 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 19 May 2020 17:50:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200519155007.ztrwwj55mhyukkmj@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>\n\t<20200425020511.GB19672@pendragon.ideasonboard.com>\n\t<20200425135845.cqg7yac3aivev2se@uno.localdomain>\n\t<20200425165011.GA11157@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425165011.GA11157@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 19 May 2020 15:46:49 -0000"}},{"id":4874,"web_url":"https://patchwork.libcamera.org/comment/4874/","msgid":"<20200520104244.rnuxlleqi2awtgen@uno.localdomain>","date":"2020-05-20T10:42:44","subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n\nOn Tue, May 19, 2020 at 05:50:07PM +0200, Jacopo Mondi wrote:\n> Hi Laurent,\n> I just re-started looking into this\n>\n> On Sat, Apr 25, 2020 at 07:50:11PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Sat, Apr 25, 2020 at 03:58:45PM +0200, Jacopo Mondi wrote:\n> > > On Sat, Apr 25, 2020 at 05:05:11AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Apr 24, 2020 at 11:52:53PM +0200, Jacopo Mondi wrote:\n> > > >> Add definition of pixel array related properties.\n> > > >>\n> > > >> Reviewed-by: Niklas Söderlund \n> > > >> Signed-off-by: Jacopo Mondi \n> > > >> ---\n> > > >> src/libcamera/property_ids.yaml | 155 ++++++++++++++++++++++++++++++++\n> > > >> 1 file changed, 155 insertions(+)\n> > > >>\n> > > >> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > >> index ce627fa042ba..8f6797723a9d 100644\n> > > >> --- a/src/libcamera/property_ids.yaml\n> > > >> +++ b/src/libcamera/property_ids.yaml\n> > > >> @@ -386,4 +386,159 @@ controls:\n> > > >> | |\n> > > >> | |\n> > > >> +--------------------+\n> > > >> +\n> > > >> + - PixelArraySize:\n> > > >> + type: float\n> > > >> + size: [2]\n> > > >> + description: |\n> > > >> + The physical sizes of the pixel array (width and height), in\n> > > >> + millimeters.\n> > > >\n> > > > Once we'll have categories for properties I think this should become\n> > > > PhysicalSize in the sensor category. We could already rename it now to\n> > > > PhysicalSize or PixelArrayPhysicalSize if you think it would be a good\n> > > > idea. I don't mind much either way, but if we keep the current name, how\n> > > > about adding a comment to remember this ?\n> > >\n> > > I like PhysicalSize.\n> >\n> > BTW, is mm precise enough, or should we use µm ?\n> >\n>\n> I had a look at a few manuals of recent-ish sensors and the their\n> (active or full pixel array size) range from ~2 to ~6 mm, with a\n> precision of usually 3 more digits.\n>\n> Considering the single pixel size will shrink, even if resolution\n> would likely go up, we might go below 1mm, and anyway, it's easier to\n> express 1234 um than 1,234 mm\n>\n> > > >\n> > > > # \\todo Rename this property to PhysicalSize once we will have property\n> > > > # categories\n> > > >\n> > > >> +\n> > > >> + - PixelArray:\n> > > >\n> > > > I was going to say we should rename this to PixelArraySize as it's the\n> > > > size of the pixel array, and then realized that the previous property\n> > > > has the same name :-) Maybe we should rename both ?\n> > >\n> > > Let's rename both\n> > >\n> > > >> + type: int32_t\n> > > >> + size: [2]\n> > > >> + description: |\n> > > >> + The camera sensor pixel array vertical and horizontal sizes, in pixels.\n> > > >> +\n> > > >> + The property describes a rectangle with its top-left corner in position\n> > > >> + (0, 0) and width and height described by the first and second values\n> > > >> + of this property.\n> > > >> +\n> > > >> + The PixelArray property defines the rectangle that includes all possible\n> > > >> + rectangles defined by the ActiveAreas property, and describes the full\n> > > >> + pixel array, including non-active pixels, black level calibration\n> > > >> + pixels etc.\n> > > >\n> > > > It's not entirely clear to me what pixels should be included in this\n> > > > rectangle. I'm not blaming your documentation here, but rather my lack\n> > > > of knowledge of what else than non-active pixels and black level\n> > > > calibration pixels there are :-) Just an idea, instead of linking this\n> > > > property to ActiveAreas, would it make sense to link it to\n> > > > PixelArraySize ? Maybe something along the lines of\n> > >\n> > > I would just drop rectangle maybe, as that property transports a widht\n> > > and an height.\n> > >\n> > > > \tThe PixelArraySize property defines the size of the full pixel\n> > > > \tarray, covered by the PixelArrayPhysicalSize area. This may\n> > > > \tinclude inactive or optical black pixels.\n> > >\n> > > I'll take this in, but I'm not sure if it's a good idea to link this\n> > > one to the PhysicalArraySize, as they are expressed with two different\n> > > measure units.\n> >\n> > I think linking the two are important, but we need to figure out what\n> > the link is. Otherwise, does the physical size represent the size of the\n> > chip (quite unlikelu), the size of the full pixel array, the size of the\n> > optical window, ... ?\n>\n> A few manuals and products briefs from OV report the chip size (I\n> assume, at least, as it varies depending on the package). But as you\n> noticed below, I presume the size is mostly used for optical\n> caluculations, so the chip area size is less relevant indeed.\n>\n> >\n> > I'm looking at the IMX219 datasheet, and it reports\n> >\n> > - Image size: diagonal 4.60mm\n> > - Total number of pixels: 3296x2512\n> > - Number of effective pixels: 3296x2480\n> > - Number of active pixels: 3280x2464\n> > - Unit cell size: 1.12µm x 1.12µm\n> >\n> > Calculating the diagonal leads to 4641µm, 4620µm and 4595µm for the\n> > three rectangles respectively. The last one match 4.60mm.\n> >\n> > With the above definition of the physical size, the diagonal would be\n> > 4641µm (for a size of 3692µm x 2813µm). I wonder if we should report the\n> > active image size instead (3674µm x 2760µm for a diagonal of 4595µm).\n> > It's not difficult to convert between the two (assuming all the pixels\n> > have the same size), but I'd like to pick the one that makes the most\n> > sense. We can of course reconsider that decision later if we find out we\n> > made a mistake.\n> >\n> > My reasoning for picking the size of the active area is that the\n> > physical size will most likely be used to make optics calculation, and\n> > the lens should be selected and positioned according to the active area.\n> > However, there could be systems with a different lens positioning, and\n> > I'm not sure yet what that would imply.\n> >\n>\n> I have checked the same for the ov13858, and it reports\n> the physical size of the 'active area'.\n>\n> ov5670 instead reports as phyisical size an area larger than the\n> active one.\n>\n> I fear there is no standard but I agree with linking the physical area\n> size to the active pixel area size, unfortunaly defining the 'active\n> area' itself might not be trivial\n>\n\nNow that I re-read that, I wonder: do we need to expose the already\ncalculated physical size ? V4L2 already offers a control to retrieve\nthe unit cell area, we expose the rectangle(s) in unit cells for each\narea, application can do the calculation by themselves, if they want\nto, right ?\n\nIn this way, my proposal now would be:\n1) Expose the unit cell size (there's a V4L2 control already)\n2) Expose three rectangles (in pixel units)\n - Total number of pixels\n Includes optically blank pixels, exposed but not valid, and valid\n pixels\n - Effective pixels\n Total number of exposed but not valid pixels, includes all the\n possible 'valid' rectangles\n - Valid(/active) pixels\n The rectangle(s) of exposed and valid pixels, contained in the\n effective pixels rectangles, which represents the maximum image\n size for an image resolution.\n\nApplication can calculate the phyisical area of interestes with these\nproperties.\n\nThe Android HAL will do the calculations to report what android needs.\n\nThe only drawback I see is that we require one more information from\nthe sensor driver, the UNIT_CELL_SIZE control.\n\nIdeally, if we could match the above three rectangles with the V4L2\nselection targets it would be lovely. I fear that part is\nunder-specified, and for the moment we would like to keep an array of\nper-sensor static information to report those values.\n\nAck to explore this direction ?\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F30B603F3\n\tfor ;\n\tWed, 20 May 2020 12:39:25 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 4A62710000C;\n\tWed, 20 May 2020 10:39:24 +0000 (UTC)"],"Date":"Wed, 20 May 2020 12:42:44 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tSakari Ailus ","Message-ID":"<20200520104244.rnuxlleqi2awtgen@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-3-jacopo@jmondi.org>\n\t<20200425020511.GB19672@pendragon.ideasonboard.com>\n\t<20200425135845.cqg7yac3aivev2se@uno.localdomain>\n\t<20200425165011.GA11157@pendragon.ideasonboard.com>\n\t<20200519155007.ztrwwj55mhyukkmj@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200519155007.ztrwwj55mhyukkmj@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 02/13] libcamera: properties:\n\tDefine pixel array properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 20 May 2020 10:39:25 -0000"}}]