[{"id":4518,"web_url":"https://patchwork.libcamera.org/comment/4518/","msgid":"<20200425201445.GC10975@pendragon.ideasonboard.com>","date":"2020-04-25T20:14:45","subject":"Re: [libcamera-devel] [PATCH v3 07/13] libcamera: camera_sensor:\n\tDefine CameraSensorInfo","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:58PM +0200, Jacopo Mondi wrote:\n> Define the CameraSensorInfo structure that reports the current image sensor\n> configuration.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 99 +++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 13 ++++\n> 2 files changed, 112 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index a54751fecf5a..e39f277622ae 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -28,6 +28,105 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(CameraSensor);\n> \n> +/**\n> + * \\struct CameraSensorInfo\n> + * \\brief Report the image sensor characteristics\n> + *\n> + * The structure reports image sensor characteristics used by IPA modules to\n> + * tune their algorithms based on the image sensor model currently in use and\n> + * its configuration.\n> + *\n> + * The here reported information describe the sensor's intrinsics\n> + * characteristics, such as its pixel array size and the sensor model name,\n> + * as well as information relative to the currently configured mode, such as\n> + * the produced image size and the bit depth of the requested image format.\n> + *\n> + * Instances of this structure are meant to be assembled by the CameraSensor\n> + * class and its specialized subclasses by inspecting the sensor static\n\nI think we can now drop \"and its specialized subclasses\".\n\n> + * properties as well as the currently configured sensor mode.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::name\n> + * \\brief The image sensor name\n\nThis is a tad ambiguous. We both know that this is the sensor model\nname, in practice taken from the entity name with bus information\nremoved. Let's make it explicit:\n\n * \\brief The image sensor model name\n *\n * The sensor model name is a free-formed string that uniquely identifies the\n * sensor model.\n */\n\nShould we also rename the \"name\" field to \"model\" ?\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::bitsPerPixel\n> + * \\brief The bits per-pixel of the image format produced by the image sensor\n\ns/bits per-pixel/bits per pixel/ or \"number of bits per pixel\".\n\nDo we need to also convey the CFA pattern ? I briefly thought we could\nuse the media bus code to do both, but I think not exposing V4L2\nconcepts here is a better idea, so bitsPerPixel is fine.\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::activeAreaSize\n> + * \\brief The size of the active pixel array area of the sensor\n\ns/active pixel array area/pixel array active area/ to match the term\nused below ?\n\n> + * \\sa properties::PixelArray\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::analogCrop\n> + * \\brief The portion of the pixel array active area which is read-out and\n> + * processed\n> + *\n> + * The analog crop rectangle top-left corner is defined as the displacement\n> + * from the top-left corner of the pixel array active area. The rectangle\n> + * horizontal and vertical sizes define the portion of the pixel matrix which\n\ns/matrix/array/ ?\n\n> + * is read-out and provided to the sensor's on-board processing pipeline, before\n\nMaybe s/on-board/internal/ as it's more on-chip than on-board ? :-)\n\n> + * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> + * take place.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::outputSize\n> + * \\brief The size of the images produced by the camera sensor\n> + *\n> + * The output image size defines the horizontal and vertical sizes of the images\n> + * produced by the image sensor. The final output image size is defined as the\n\nI would s/final // as otherwise one could wonder if \"final output image\nsize\" is different that \"output image size\".\n\n> + * end result of the sensor's on-board image processing pipeline stages, applied\n\nSame here regarding on-board.\n\n> + * on the pixel array portion defined by the analog crop rectangle. Each image\n> + * processing stage that performs pixel sub-sampling techniques, such as pixel\n> + * binning or skipping, or perform any additional digital scaling concur in\n> + * the definition of the final output image size.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::lineLength\n> + * \\brief Total line length in pixels\n\ns/in pixels/in pixel clock periods/ (and same in the next line) as\ntechnically speaking there's no pixels during the blanking period\n(especially on buses such as CSI-2).\n\n> + *\n> + * The total line length in pixels, including blanking and synchronism signal\n\ns/synchronism/synchronization/\n\n> + * active state duration.\n\nI would however drop \" and synchronization ...\" completely. The\nsynchronization signals are defined by the bus protocol, they're not an\nadditional part of the line time in addition to the active length and\nthe blanking. The full line length is the sum of the active and blanking\nlengths, there's nothing else.\n\n> + *\n> + * In example:\n\ns/In example/For example/ or just s/In example/Example/\n\n> + *\n> + * \\verbatim\n> + _______ _______ ___\n> + HREF ... ___| |___| |___| ...\n> + _ _ _\n> + HSYNC ... _| |_________| |_________| |__ ...\n> +\n> + Valid ... ....xxxxxxxx....xxxxxxxx....xxx ...\n> + Data\n> +\n> + Line\t ... /-----------/-----------/--- ...\n> + Length\n> +\n> + \\endverbatim\n\nIf we drop the mention of synchronization signals the example may not be\nneeded anymore.\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorInfo::pixelClock\n> + * \\brief The pixel clock in Hz\n> + *\n> + * The pixel read out frequency in Hz. The property describes how many pixel\n\ns/how many pixel/how many pixels/\n\n> + * per second are produced by the camera sensor, including blankings and\n> + * synchronism signal active state duration.\n\nSame comment as above.\n\n> + *\n> + * To obtain the read-out time in second of a full line:\n> + *\n> + * \\verbatim\n> +\tLineDuration(s) = lineLength(pixel) / pixelClock(Hz)\n\ns/LineDuration/lineDuration/\n\n> + \\endverbatim\n> + */\n\nlineLength and pixelClock should be swapped here, the order in the\nstructure below is the inverse.\n\n> +\n> /**\n> * \\class CameraSensor\n> * \\brief A camera sensor based on V4L2 subdevices\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 8fa4d450f959..e19852d4cabe 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -22,6 +22,19 @@ class V4L2Subdevice;\n> \n> struct V4L2SubdeviceFormat;\n> \n> +struct CameraSensorInfo {\n> +\tstd::string name;\n> +\n> +\tuint32_t bitsPerPixel;\n> +\n> +\tSize activeAreaSize;\n> +\tRectangle analogCrop;\n> +\tSize outputSize;\n> +\n> +\tuint32_t pixelClock;\n> +\tuint32_t lineLength;\n> +};\n> +\n> class CameraSensor : protected Loggable\n> {\n> public:","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 4C92E603FC\n\tfor ;\n\tSat, 25 Apr 2020 22:15:01 +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 AA9D14F7;\n\tSat, 25 Apr 2020 22:15:00 +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=\"iGSFF1WR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587845700;\n\tbh=LChGKWxC7DMzVBml8pWw7jGrGlkUmKYhM+SyrOJp9AE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iGSFF1WRx3h3xA5CRRJOYPkZ+k6o97mnC+CkH+QMTaHbMX3qHlNYcO/YarigLPqUi\n\t3u8VhFu12F/Z9fRPY+atAQuEZgs1Z8AD6G1cMos4zE+ohJXn5p8LT5XjLfwqBvfbUc\n\tATzny99kD45PBIEFkv564myP/j/JWDKWfVQ0JZRg=","Date":"Sat, 25 Apr 2020 23:14:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425201445.GC10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200424215304.558317-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 07/13] libcamera: camera_sensor:\n\tDefine CameraSensorInfo","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 20:15:01 -0000"}}]