[{"id":32372,"web_url":"https://patchwork.libcamera.org/comment/32372/","msgid":"","date":"2024-11-25T19:57:55","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> In preparation for adding support for querying direction information\n> from controls, populate the corresponding field in the control ID\n> defintions.\n\nI wish I could save you the usual bikeshedding on names, but...\n\nAre 'out' and 'in' the best terms ? Android divides 'controls' (from\napp to camera) from 'metadata' (from camera to app) and I quite like\nit (and I think most people in the industry is also accustomed to\nthem, but I agree it makes hard to express 'both' easily, so yeah 'in'\n'out' and 'inout' makes sense. How do we document that ?\n\nAlso, the absence of the field indicates 'inout'. This should be\ndocumented, and I wonder if 'inout' is valid as a value if it's\nimplicitly the default.\n\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> src/libcamera/control_ids_draft.yaml | 7 +++++++\n> 2 files changed, 19 insertions(+)\n>\n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index d34a2d068b60..c0e91d1852cd 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n\nAt the beginning of the file we have\n\n# Unless otherwise stated, all controls are bi-directional, i.e. they can be\n# set through Request::controls() and returned out through Request::metadata().\n\nCould you expand it by mentioning 'direction' ?\n\n> @@ -17,6 +17,7 @@ controls:\n>\n> - AeLocked:\n> type: bool\n> + direction: out\n> description: |\n> Report the lock status of a running AE algorithm.\n>\n> @@ -236,6 +237,7 @@ controls:\n>\n> - AeFlickerDetected:\n> type: int32_t\n> + direction: out\n> description: |\n> Flicker period detected in microseconds.\n>\n> @@ -274,6 +276,7 @@ controls:\n>\n> - Lux:\n> type: float\n> + direction: out\n> description: |\n> Report an estimate of the current illuminance level in lux.\n>\n> @@ -324,6 +327,7 @@ controls:\n>\n> - AwbLocked:\n> type: bool\n> + direction: out\n> description: |\n> Report the lock status of a running AWB algorithm.\n>\n\nColourTemperature is missing\n\n Report the estimate of the colour temperature for the frame, in kelvin.\n\n The ColourTemperature control can only be returned in metadata.\n\n\n> @@ -361,6 +365,7 @@ controls:\n>\n> - SensorBlackLevels:\n> type: int32_t\n> + direction: out\n> description: |\n> Reports the sensor black levels used for processing a frame.\n>\n> @@ -385,6 +390,7 @@ controls:\n>\n> - FocusFoM:\n> type: int32_t\n> + direction: out\n> description: |\n> Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n>\n> @@ -442,6 +448,7 @@ controls:\n>\n> - FrameDuration:\n> type: int64_t\n> + direction: out\n> description: |\n> The instantaneous frame duration from start of frame exposure to start\n> of next exposure, expressed in microseconds.\n> @@ -450,6 +457,7 @@ controls:\n>\n> - FrameDurationLimits:\n> type: int64_t\n> + direction: in\n\nI read\n\n When reported in\n metadata, the control expresses the minimum and maximum frame\n durations used after being clipped to the sensor provided frame\n duration limits.\n\n> description: |\n> The minimum and maximum (in that order) frame duration, expressed in\n> microseconds.\n> @@ -487,6 +495,7 @@ controls:\n>\n> - SensorTemperature:\n> type: float\n> + direction: out\n> description: |\n> Temperature measure from the camera sensor in Celsius.\n>\n> @@ -499,6 +508,7 @@ controls:\n>\n> - SensorTimestamp:\n> type: int64_t\n> + direction: out\n> description: |\n> The time when the first row of the image sensor active array is exposed.\n>\n> @@ -667,6 +677,7 @@ controls:\n>\n> - AfTrigger:\n> type: int32_t\n> + direction: in\n> description: |\n> Start an autofocus scan.\n>\n> @@ -692,6 +703,7 @@ controls:\n>\n> - AfPause:\n> type: int32_t\n> + direction: in\n> description: |\n> Pause lens movements when in continuous autofocus mode.\n\nShould AfState be out only ?\nAlso AfPauseState seems to be a metadata only.\n\nHdrChannel I read\n\n This value is reported back to the application so that it can discover\n whether this capture corresponds to the short or long exposure image\n (or any other image used by the HDR procedure). An application can\n monitor the HDR channel to discover when the differently exposed images\n have arrived.\n\nwhich suggests me it's a metadata only\n\nand DebugMetadataEnable is possibily input only ?\n\n>\n> diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> index 1b284257f601..59aa6141c25c 100644\n> --- a/src/libcamera/control_ids_draft.yaml\n> +++ b/src/libcamera/control_ids_draft.yaml\n> @@ -79,6 +79,7 @@ controls:\n>\n> - AeState:\n> type: int32_t\n> + direction: out\n> description: |\n> Control to report the current AE algorithm state. Currently identical to\n> ANDROID_CONTROL_AE_STATE.\n> @@ -108,6 +109,7 @@ controls:\n>\n> - AwbState:\n> type: int32_t\n> + direction: out\n> description: |\n> Control to report the current AWB algorithm state. Currently identical\n> to ANDROID_CONTROL_AWB_STATE.\n> @@ -129,6 +131,7 @@ controls:\n>\n> - SensorRollingShutterSkew:\n> type: int64_t\n> + direction: out\n> description: |\n> Control to report the time between the start of exposure of the first\n> row and the start of exposure of the last row. Currently identical to\n> @@ -262,6 +265,7 @@ controls:\n>\n> - FaceDetectFaceRectangles:\n> type: Rectangle\n> + direction: out\n> description: |\n> Boundary rectangles of the detected faces. The number of values is\n> the number of detected faces.\n> @@ -273,6 +277,7 @@ controls:\n>\n> - FaceDetectFaceScores:\n> type: uint8_t\n> + direction: out\n> description: |\n> Confidence score of each of the detected faces. The range of score is\n> [0, 100]. The number of values should be the number of faces reported\n> @@ -285,6 +290,7 @@ controls:\n>\n> - FaceDetectFaceLandmarks:\n> type: Point\n> + direction: out\n> description: |\n> Array of human face landmark coordinates in format [..., left_eye_i,\n> right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> @@ -298,6 +304,7 @@ controls:\n>\n> - FaceDetectFaceIds:\n> type: int32_t\n> + direction: out\n> description: |\n> Each detected face is given a unique ID that is valid for as long as the\n> face is visible to the camera device. A face that leaves the field of\n\nHave you gone through control_ids_rpi.yaml and control_ids_debug.yaml\nas well ?\n\n> --\n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 42E30C3274\n\tfor ;\n\tMon, 25 Nov 2024 19:58:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30EF86604A;\n\tMon, 25 Nov 2024 20:58:00 +0100 (CET)","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 35F5B65F7E\n\tfor ;\n\tMon, 25 Nov 2024 20:57:59 +0100 (CET)","from ideasonboard.com (mob-5-90-139-188.net.vodafone.it\n\t[5.90.139.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 408454AD;\n\tMon, 25 Nov 2024 20:57:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"e3J+A6+E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732564657;\n\tbh=9UWsn2F+EtwNPxVUVdTC2Cv4zeyl4KzrjhYce8jN3No=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e3J+A6+EZNcBxAz5pSXU62reSHKk4PodnuzZPp5iUwU1Av0QeQijPT1rVTvZyWcSU\n\tjneLEaCl/aPLsgJk+XVvg6yCvKtWDTpknMyouNdBYjtg74dkAQkXIj7v/IEGoRvitJ\n\tLTVWA1oF7TmzQOFCuqYo5lr7dFv+OHowUoFjZeOc=","Date":"Mon, 25 Nov 2024 20:57:55 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241125153003.3309066-2-paul.elder@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32373,"web_url":"https://patchwork.libcamera.org/comment/32373/","msgid":"","date":"2024-11-25T20:11:17","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the review.\n\nOn Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > In preparation for adding support for querying direction information\n> > from controls, populate the corresponding field in the control ID\n> > defintions.\n> \n> I wish I could save you the usual bikeshedding on names, but...\n> \n> Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> app to camera) from 'metadata' (from camera to app) and I quite like\n> it (and I think most people in the industry is also accustomed to\n> them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> 'out' and 'inout' makes sense. How do we document that ?\n\nI personally thought that in and out were less confusing than controls\nand metadata, mainly because both controls and metadata go in a\nControlList, and then we talk about setting controls in metadata that is\nreturned in a ControlList, while we can also set controls in controls.\n\nAlso I got the idea from doxygen \\params[inout]\n\n> Also, the absence of the field indicates 'inout'. This should be\n> documented, and I wonder if 'inout' is valid as a value if it's\n> implicitly the default.\n\nIf you want to be explicit you could write it? I thought it was good\njust to leave as an option.\n\n> \n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > 2 files changed, 19 insertions(+)\n> >\n> > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > index d34a2d068b60..c0e91d1852cd 100644\n> > --- a/src/libcamera/control_ids_core.yaml\n> > +++ b/src/libcamera/control_ids_core.yaml\n> \n> At the beginning of the file we have\n> \n> # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> # set through Request::controls() and returned out through Request::metadata().\n> \n> Could you expand it by mentioning 'direction' ?\n\nIt's already mentioned: \"bi-*direction*al\" :)\n\nNo but seriously, that was what I used to validate my uncertainty about\nusing the word \"direction\".\n\n> \n> > @@ -17,6 +17,7 @@ controls:\n> >\n> > - AeLocked:\n> > type: bool\n> > + direction: out\n> > description: |\n> > Report the lock status of a running AE algorithm.\n> >\n> > @@ -236,6 +237,7 @@ controls:\n> >\n> > - AeFlickerDetected:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Flicker period detected in microseconds.\n> >\n> > @@ -274,6 +276,7 @@ controls:\n> >\n> > - Lux:\n> > type: float\n> > + direction: out\n> > description: |\n> > Report an estimate of the current illuminance level in lux.\n> >\n> > @@ -324,6 +327,7 @@ controls:\n> >\n> > - AwbLocked:\n> > type: bool\n> > + direction: out\n> > description: |\n> > Report the lock status of a running AWB algorithm.\n> >\n> \n> ColourTemperature is missing\n> \n> Report the estimate of the colour temperature for the frame, in kelvin.\n> \n> The ColourTemperature control can only be returned in metadata.\n\nWhat do you mean, I thought this was settable as of...\n\nhttps://patchwork.libcamera.org/patch/20894/\n\nOh, it's not merged yet. Ok.\n\n> \n> \n> > @@ -361,6 +365,7 @@ controls:\n> >\n> > - SensorBlackLevels:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Reports the sensor black levels used for processing a frame.\n> >\n> > @@ -385,6 +390,7 @@ controls:\n> >\n> > - FocusFoM:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> >\n> > @@ -442,6 +448,7 @@ controls:\n> >\n> > - FrameDuration:\n> > type: int64_t\n> > + direction: out\n> > description: |\n> > The instantaneous frame duration from start of frame exposure to start\n> > of next exposure, expressed in microseconds.\n> > @@ -450,6 +457,7 @@ controls:\n> >\n> > - FrameDurationLimits:\n> > type: int64_t\n> > + direction: in\n> \n> I read\n> \n> When reported in\n> metadata, the control expresses the minimum and maximum frame\n> durations used after being clipped to the sensor provided frame\n> duration limits.\n\nAnd I missed that. Nice catch.\n\n> \n> > description: |\n> > The minimum and maximum (in that order) frame duration, expressed in\n> > microseconds.\n> > @@ -487,6 +495,7 @@ controls:\n> >\n> > - SensorTemperature:\n> > type: float\n> > + direction: out\n> > description: |\n> > Temperature measure from the camera sensor in Celsius.\n> >\n> > @@ -499,6 +508,7 @@ controls:\n> >\n> > - SensorTimestamp:\n> > type: int64_t\n> > + direction: out\n> > description: |\n> > The time when the first row of the image sensor active array is exposed.\n> >\n> > @@ -667,6 +677,7 @@ controls:\n> >\n> > - AfTrigger:\n> > type: int32_t\n> > + direction: in\n> > description: |\n> > Start an autofocus scan.\n> >\n> > @@ -692,6 +703,7 @@ controls:\n> >\n> > - AfPause:\n> > type: int32_t\n> > + direction: in\n> > description: |\n> > Pause lens movements when in continuous autofocus mode.\n> \n> Should AfState be out only ?\n> Also AfPauseState seems to be a metadata only.\n\nThey indeed should, yes.\n\n> \n> HdrChannel I read\n> \n> This value is reported back to the application so that it can discover\n> whether this capture corresponds to the short or long exposure image\n> (or any other image used by the HDR procedure). An application can\n> monitor the HDR channel to discover when the differently exposed images\n> have arrived.\n> \n> which suggests me it's a metadata only\n\nI think you're right.\n\n> \n> and DebugMetadataEnable is possibily input only ?\n\nMy thought process was that all enable-type controls should be both, as\nyou'd probably want to know if what you've set has actually been set.\n\n> >\n> > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > index 1b284257f601..59aa6141c25c 100644\n> > --- a/src/libcamera/control_ids_draft.yaml\n> > +++ b/src/libcamera/control_ids_draft.yaml\n> > @@ -79,6 +79,7 @@ controls:\n> >\n> > - AeState:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Control to report the current AE algorithm state. Currently identical to\n> > ANDROID_CONTROL_AE_STATE.\n> > @@ -108,6 +109,7 @@ controls:\n> >\n> > - AwbState:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Control to report the current AWB algorithm state. Currently identical\n> > to ANDROID_CONTROL_AWB_STATE.\n> > @@ -129,6 +131,7 @@ controls:\n> >\n> > - SensorRollingShutterSkew:\n> > type: int64_t\n> > + direction: out\n> > description: |\n> > Control to report the time between the start of exposure of the first\n> > row and the start of exposure of the last row. Currently identical to\n> > @@ -262,6 +265,7 @@ controls:\n> >\n> > - FaceDetectFaceRectangles:\n> > type: Rectangle\n> > + direction: out\n> > description: |\n> > Boundary rectangles of the detected faces. The number of values is\n> > the number of detected faces.\n> > @@ -273,6 +277,7 @@ controls:\n> >\n> > - FaceDetectFaceScores:\n> > type: uint8_t\n> > + direction: out\n> > description: |\n> > Confidence score of each of the detected faces. The range of score is\n> > [0, 100]. The number of values should be the number of faces reported\n> > @@ -285,6 +290,7 @@ controls:\n> >\n> > - FaceDetectFaceLandmarks:\n> > type: Point\n> > + direction: out\n> > description: |\n> > Array of human face landmark coordinates in format [..., left_eye_i,\n> > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > @@ -298,6 +304,7 @@ controls:\n> >\n> > - FaceDetectFaceIds:\n> > type: int32_t\n> > + direction: out\n> > description: |\n> > Each detected face is given a unique ID that is valid for as long as the\n> > face is visible to the camera device. A face that leaves the field of\n> \n> Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> as well ?\n\ndebug is empty and rpi... I missed Bcm2835StatsOutput should be out\nonly.\n\n\nThanks,\n\nPaul\n\n> \n> > --\n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1A752BD160\n\tfor ;\n\tMon, 25 Nov 2024 20:11:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EC7D6604A;\n\tMon, 25 Nov 2024 21:11:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E923565F87\n\tfor ;\n\tMon, 25 Nov 2024 21:11:24 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:9d06:dcbb:7303:4be6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08CE76EC;\n\tMon, 25 Nov 2024 21:11:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YVZE76rh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732565463;\n\tbh=3Ycrbodvkk52u9enNndyFU8nY06qFkfNLCM/QhoMwlk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YVZE76rhJCfQ705CndAWku37d6AG85bqtYcfEOa6yrHDlPYYHh6x7+4gBLpJTelir\n\t0+bWVre1zIZON+bmHc3TzAFdxEVWVjHG8xYSiIBYqR9tsa5kT/eOFlNJRJrjd4lD4R\n\tynQKhdeVocg8QnrZTVFDQ3ZDqYEW6wiDeRrAv664=","Date":"Tue, 26 Nov 2024 05:11:17 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32376,"web_url":"https://patchwork.libcamera.org/comment/32376/","msgid":"<20241125230417.GY19381@pendragon.ideasonboard.com>","date":"2024-11-25T23:04:17","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > In preparation for adding support for querying direction information\n> > > from controls, populate the corresponding field in the control ID\n> > > defintions.\n> > \n> > I wish I could save you the usual bikeshedding on names, but...\n> > \n> > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > app to camera) from 'metadata' (from camera to app) and I quite like\n> > it (and I think most people in the industry is also accustomed to\n> > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > 'out' and 'inout' makes sense. How do we document that ?\n> \n> I personally thought that in and out were less confusing than controls\n> and metadata, mainly because both controls and metadata go in a\n> ControlList, and then we talk about setting controls in metadata that is\n> returned in a ControlList, while we can also set controls in controls.\n> \n> Also I got the idea from doxygen \\params[inout]\n\nIf our Control* classes were named using a generic term other than\ncontrol, then we could use \"control\" and \"metadata\". Or if\nRequest::controls() was named something other than \"controls\". But for\nnow we're using these names, so I think it would just add to the\nconfusion. \"in\" and \"out\" are probably not the absolute best but I think\nthey can do for now.\n\n> > Also, the absence of the field indicates 'inout'. This should be\n> > documented, and I wonder if 'inout' is valid as a value if it's\n> > implicitly the default.\n> \n> If you want to be explicit you could write it? I thought it was good\n> just to leave as an option.\n\nThe advantage of being explicit is that we can catch missing information\neasily.\n\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > 2 files changed, 19 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > index d34a2d068b60..c0e91d1852cd 100644\n> > > --- a/src/libcamera/control_ids_core.yaml\n> > > +++ b/src/libcamera/control_ids_core.yaml\n> > \n> > At the beginning of the file we have\n> > \n> > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > # set through Request::controls() and returned out through Request::metadata().\n> > \n> > Could you expand it by mentioning 'direction' ?\n> \n> It's already mentioned: \"bi-*direction*al\" :)\n> \n> No but seriously, that was what I used to validate my uncertainty about\n> using the word \"direction\".\n> \n> > \n> > > @@ -17,6 +17,7 @@ controls:\n> > >\n> > > - AeLocked:\n> > > type: bool\n> > > + direction: out\n> > > description: |\n> > > Report the lock status of a running AE algorithm.\n> > >\n> > > @@ -236,6 +237,7 @@ controls:\n> > >\n> > > - AeFlickerDetected:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Flicker period detected in microseconds.\n> > >\n> > > @@ -274,6 +276,7 @@ controls:\n> > >\n> > > - Lux:\n> > > type: float\n> > > + direction: out\n> > > description: |\n> > > Report an estimate of the current illuminance level in lux.\n> > >\n> > > @@ -324,6 +327,7 @@ controls:\n> > >\n> > > - AwbLocked:\n> > > type: bool\n> > > + direction: out\n> > > description: |\n> > > Report the lock status of a running AWB algorithm.\n> > >\n> > \n> > ColourTemperature is missing\n> > \n> > Report the estimate of the colour temperature for the frame, in kelvin.\n> > \n> > The ColourTemperature control can only be returned in metadata.\n> \n> What do you mean, I thought this was settable as of...\n> \n> https://patchwork.libcamera.org/patch/20894/\n> \n> Oh, it's not merged yet. Ok.\n> \n> > > @@ -361,6 +365,7 @@ controls:\n> > >\n> > > - SensorBlackLevels:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Reports the sensor black levels used for processing a frame.\n> > >\n> > > @@ -385,6 +390,7 @@ controls:\n> > >\n> > > - FocusFoM:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > >\n> > > @@ -442,6 +448,7 @@ controls:\n> > >\n> > > - FrameDuration:\n> > > type: int64_t\n> > > + direction: out\n> > > description: |\n> > > The instantaneous frame duration from start of frame exposure to start\n> > > of next exposure, expressed in microseconds.\n> > > @@ -450,6 +457,7 @@ controls:\n> > >\n> > > - FrameDurationLimits:\n> > > type: int64_t\n> > > + direction: in\n> > \n> > I read\n> > \n> > When reported in\n> > metadata, the control expresses the minimum and maximum frame\n> > durations used after being clipped to the sensor provided frame\n> > duration limits.\n> \n> And I missed that. Nice catch.\n> \n> > > description: |\n> > > The minimum and maximum (in that order) frame duration, expressed in\n> > > microseconds.\n> > > @@ -487,6 +495,7 @@ controls:\n> > >\n> > > - SensorTemperature:\n> > > type: float\n> > > + direction: out\n> > > description: |\n> > > Temperature measure from the camera sensor in Celsius.\n> > >\n> > > @@ -499,6 +508,7 @@ controls:\n> > >\n> > > - SensorTimestamp:\n> > > type: int64_t\n> > > + direction: out\n> > > description: |\n> > > The time when the first row of the image sensor active array is exposed.\n> > >\n> > > @@ -667,6 +677,7 @@ controls:\n> > >\n> > > - AfTrigger:\n> > > type: int32_t\n> > > + direction: in\n> > > description: |\n> > > Start an autofocus scan.\n> > >\n> > > @@ -692,6 +703,7 @@ controls:\n> > >\n> > > - AfPause:\n> > > type: int32_t\n> > > + direction: in\n> > > description: |\n> > > Pause lens movements when in continuous autofocus mode.\n> > \n> > Should AfState be out only ?\n> > Also AfPauseState seems to be a metadata only.\n> \n> They indeed should, yes.\n> \n> > \n> > HdrChannel I read\n> > \n> > This value is reported back to the application so that it can discover\n> > whether this capture corresponds to the short or long exposure image\n> > (or any other image used by the HDR procedure). An application can\n> > monitor the HDR channel to discover when the differently exposed images\n> > have arrived.\n> > \n> > which suggests me it's a metadata only\n> \n> I think you're right.\n> \n> > \n> > and DebugMetadataEnable is possibily input only ?\n> \n> My thought process was that all enable-type controls should be both, as\n> you'd probably want to know if what you've set has actually been set.\n> \n> > >\n> > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > index 1b284257f601..59aa6141c25c 100644\n> > > --- a/src/libcamera/control_ids_draft.yaml\n> > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > @@ -79,6 +79,7 @@ controls:\n> > >\n> > > - AeState:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Control to report the current AE algorithm state. Currently identical to\n> > > ANDROID_CONTROL_AE_STATE.\n> > > @@ -108,6 +109,7 @@ controls:\n> > >\n> > > - AwbState:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Control to report the current AWB algorithm state. Currently identical\n> > > to ANDROID_CONTROL_AWB_STATE.\n> > > @@ -129,6 +131,7 @@ controls:\n> > >\n> > > - SensorRollingShutterSkew:\n> > > type: int64_t\n> > > + direction: out\n> > > description: |\n> > > Control to report the time between the start of exposure of the first\n> > > row and the start of exposure of the last row. Currently identical to\n> > > @@ -262,6 +265,7 @@ controls:\n> > >\n> > > - FaceDetectFaceRectangles:\n> > > type: Rectangle\n> > > + direction: out\n> > > description: |\n> > > Boundary rectangles of the detected faces. The number of values is\n> > > the number of detected faces.\n> > > @@ -273,6 +277,7 @@ controls:\n> > >\n> > > - FaceDetectFaceScores:\n> > > type: uint8_t\n> > > + direction: out\n> > > description: |\n> > > Confidence score of each of the detected faces. The range of score is\n> > > [0, 100]. The number of values should be the number of faces reported\n> > > @@ -285,6 +290,7 @@ controls:\n> > >\n> > > - FaceDetectFaceLandmarks:\n> > > type: Point\n> > > + direction: out\n> > > description: |\n> > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > @@ -298,6 +304,7 @@ controls:\n> > >\n> > > - FaceDetectFaceIds:\n> > > type: int32_t\n> > > + direction: out\n> > > description: |\n> > > Each detected face is given a unique ID that is valid for as long as the\n> > > face is visible to the camera device. A face that leaves the field of\n> > \n> > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > as well ?\n> \n> debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> only.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C4F21C3274\n\tfor ;\n\tMon, 25 Nov 2024 23:04:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0640866058;\n\tTue, 26 Nov 2024 00:04:31 +0100 (CET)","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 1BCA465FF5\n\tfor ;\n\tTue, 26 Nov 2024 00:04:29 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DCB6E526;\n\tTue, 26 Nov 2024 00:04:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nDY2tVpo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732575847;\n\tbh=fZqQerSu4kszhF05tZFeDhoi7gkjUHVcq58vBOPIdoM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nDY2tVpomAKyh7YouJuOhwdjmiNIiNniIy4j7E7089KUkn00yQ2r6U2T5M3S7zW4x\n\ta2rXoXyooodlDR9jS5VQ7B6yFCWtRfOIfdLqd5gxInjhOGogYblnb613rbdTMHWgS9\n\tPYBNglJEdjcQvuJHlMXfIZpGioOEhLtsMuj3HkPw=","Date":"Tue, 26 Nov 2024 01:04:17 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"<20241125230417.GY19381@pendragon.ideasonboard.com>","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32388,"web_url":"https://patchwork.libcamera.org/comment/32388/","msgid":"","date":"2024-11-26T15:58:03","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart\n wrote:\n>\n> On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > In preparation for adding support for querying direction information\n> > > > from controls, populate the corresponding field in the control ID\n> > > > defintions.\n> > >\n> > > I wish I could save you the usual bikeshedding on names, but...\n> > >\n> > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > it (and I think most people in the industry is also accustomed to\n> > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > 'out' and 'inout' makes sense. How do we document that ?\n> >\n> > I personally thought that in and out were less confusing than controls\n> > and metadata, mainly because both controls and metadata go in a\n> > ControlList, and then we talk about setting controls in metadata that is\n> > returned in a ControlList, while we can also set controls in controls.\n> >\n> > Also I got the idea from doxygen \\params[inout]\n>\n> If our Control* classes were named using a generic term other than\n> control, then we could use \"control\" and \"metadata\". Or if\n> Request::controls() was named something other than \"controls\". But for\n> now we're using these names, so I think it would just add to the\n> confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> they can do for now.\n>\n> > > Also, the absence of the field indicates 'inout'. This should be\n> > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > implicitly the default.\n> >\n> > If you want to be explicit you could write it? I thought it was good\n> > just to leave as an option.\n>\n> The advantage of being explicit is that we can catch missing information\n> easily.\n\nTo be honest, I am not sure about \"in\" and \"out\" either. I have only\na slight preference of using \"read\" and \"write\" instead, but I'm not\nconvinced it's much better.\n\nNaush\n\n>\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > 2 files changed, 19 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > +++ b/src/libcamera/control_ids_core.yaml\n> > >\n> > > At the beginning of the file we have\n> > >\n> > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > # set through Request::controls() and returned out through Request::metadata().\n> > >\n> > > Could you expand it by mentioning 'direction' ?\n> >\n> > It's already mentioned: \"bi-*direction*al\" :)\n> >\n> > No but seriously, that was what I used to validate my uncertainty about\n> > using the word \"direction\".\n> >\n> > >\n> > > > @@ -17,6 +17,7 @@ controls:\n> > > >\n> > > > - AeLocked:\n> > > > type: bool\n> > > > + direction: out\n> > > > description: |\n> > > > Report the lock status of a running AE algorithm.\n> > > >\n> > > > @@ -236,6 +237,7 @@ controls:\n> > > >\n> > > > - AeFlickerDetected:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Flicker period detected in microseconds.\n> > > >\n> > > > @@ -274,6 +276,7 @@ controls:\n> > > >\n> > > > - Lux:\n> > > > type: float\n> > > > + direction: out\n> > > > description: |\n> > > > Report an estimate of the current illuminance level in lux.\n> > > >\n> > > > @@ -324,6 +327,7 @@ controls:\n> > > >\n> > > > - AwbLocked:\n> > > > type: bool\n> > > > + direction: out\n> > > > description: |\n> > > > Report the lock status of a running AWB algorithm.\n> > > >\n> > >\n> > > ColourTemperature is missing\n> > >\n> > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > >\n> > > The ColourTemperature control can only be returned in metadata.\n> >\n> > What do you mean, I thought this was settable as of...\n> >\n> > https://patchwork.libcamera.org/patch/20894/\n> >\n> > Oh, it's not merged yet. Ok.\n> >\n> > > > @@ -361,6 +365,7 @@ controls:\n> > > >\n> > > > - SensorBlackLevels:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Reports the sensor black levels used for processing a frame.\n> > > >\n> > > > @@ -385,6 +390,7 @@ controls:\n> > > >\n> > > > - FocusFoM:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > >\n> > > > @@ -442,6 +448,7 @@ controls:\n> > > >\n> > > > - FrameDuration:\n> > > > type: int64_t\n> > > > + direction: out\n> > > > description: |\n> > > > The instantaneous frame duration from start of frame exposure to start\n> > > > of next exposure, expressed in microseconds.\n> > > > @@ -450,6 +457,7 @@ controls:\n> > > >\n> > > > - FrameDurationLimits:\n> > > > type: int64_t\n> > > > + direction: in\n> > >\n> > > I read\n> > >\n> > > When reported in\n> > > metadata, the control expresses the minimum and maximum frame\n> > > durations used after being clipped to the sensor provided frame\n> > > duration limits.\n> >\n> > And I missed that. Nice catch.\n> >\n> > > > description: |\n> > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > microseconds.\n> > > > @@ -487,6 +495,7 @@ controls:\n> > > >\n> > > > - SensorTemperature:\n> > > > type: float\n> > > > + direction: out\n> > > > description: |\n> > > > Temperature measure from the camera sensor in Celsius.\n> > > >\n> > > > @@ -499,6 +508,7 @@ controls:\n> > > >\n> > > > - SensorTimestamp:\n> > > > type: int64_t\n> > > > + direction: out\n> > > > description: |\n> > > > The time when the first row of the image sensor active array is exposed.\n> > > >\n> > > > @@ -667,6 +677,7 @@ controls:\n> > > >\n> > > > - AfTrigger:\n> > > > type: int32_t\n> > > > + direction: in\n> > > > description: |\n> > > > Start an autofocus scan.\n> > > >\n> > > > @@ -692,6 +703,7 @@ controls:\n> > > >\n> > > > - AfPause:\n> > > > type: int32_t\n> > > > + direction: in\n> > > > description: |\n> > > > Pause lens movements when in continuous autofocus mode.\n> > >\n> > > Should AfState be out only ?\n> > > Also AfPauseState seems to be a metadata only.\n> >\n> > They indeed should, yes.\n> >\n> > >\n> > > HdrChannel I read\n> > >\n> > > This value is reported back to the application so that it can discover\n> > > whether this capture corresponds to the short or long exposure image\n> > > (or any other image used by the HDR procedure). An application can\n> > > monitor the HDR channel to discover when the differently exposed images\n> > > have arrived.\n> > >\n> > > which suggests me it's a metadata only\n> >\n> > I think you're right.\n> >\n> > >\n> > > and DebugMetadataEnable is possibily input only ?\n> >\n> > My thought process was that all enable-type controls should be both, as\n> > you'd probably want to know if what you've set has actually been set.\n> >\n> > > >\n> > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > index 1b284257f601..59aa6141c25c 100644\n> > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > @@ -79,6 +79,7 @@ controls:\n> > > >\n> > > > - AeState:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Control to report the current AE algorithm state. Currently identical to\n> > > > ANDROID_CONTROL_AE_STATE.\n> > > > @@ -108,6 +109,7 @@ controls:\n> > > >\n> > > > - AwbState:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Control to report the current AWB algorithm state. Currently identical\n> > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > @@ -129,6 +131,7 @@ controls:\n> > > >\n> > > > - SensorRollingShutterSkew:\n> > > > type: int64_t\n> > > > + direction: out\n> > > > description: |\n> > > > Control to report the time between the start of exposure of the first\n> > > > row and the start of exposure of the last row. Currently identical to\n> > > > @@ -262,6 +265,7 @@ controls:\n> > > >\n> > > > - FaceDetectFaceRectangles:\n> > > > type: Rectangle\n> > > > + direction: out\n> > > > description: |\n> > > > Boundary rectangles of the detected faces. The number of values is\n> > > > the number of detected faces.\n> > > > @@ -273,6 +277,7 @@ controls:\n> > > >\n> > > > - FaceDetectFaceScores:\n> > > > type: uint8_t\n> > > > + direction: out\n> > > > description: |\n> > > > Confidence score of each of the detected faces. The range of score is\n> > > > [0, 100]. The number of values should be the number of faces reported\n> > > > @@ -285,6 +290,7 @@ controls:\n> > > >\n> > > > - FaceDetectFaceLandmarks:\n> > > > type: Point\n> > > > + direction: out\n> > > > description: |\n> > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > @@ -298,6 +304,7 @@ controls:\n> > > >\n> > > > - FaceDetectFaceIds:\n> > > > type: int32_t\n> > > > + direction: out\n> > > > description: |\n> > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > face is visible to the camera device. A face that leaves the field of\n> > >\n> > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > as well ?\n> >\n> > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > only.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 37837BD16B\n\tfor ;\n\tTue, 26 Nov 2024 15:58:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DC3065FC9;\n\tTue, 26 Nov 2024 16:58:23 +0100 (CET)","from mail-yb1-xb2c.google.com (mail-yb1-xb2c.google.com\n\t[IPv6:2607:f8b0:4864:20::b2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3CDC65F9E\n\tfor ;\n\tTue, 26 Nov 2024 16:58:21 +0100 (CET)","by mail-yb1-xb2c.google.com with SMTP id\n\t3f1490d57ef6-e387195210eso798064276.0\n\tfor ;\n\tTue, 26 Nov 2024 07:58:21 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"i7/IImJh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1732636700; x=1733241500;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tamgWmlRQ+BPKIR0ckXBCFvayLD8qx82OjxmF/lPuI8=;\n\tb=i7/IImJhF/FJKvNJm1GpxHImv16IF0TLu6B2jXpqWKyIT/nc0O98b89HgVCxGoR/5z\n\tTGUe/er7GpKVN43h6oyjNfNnZOPbp2VfLJXihGJAHOGvkEpAOJSZjGvnjUTj4JiOOEUA\n\ty9WLjTiViQ1UOxUsCkEXkKHtv36Vm3+U6OQ2RRGDUJRpQ8udruEFPKH0bPJzrnbeN+3b\n\tBTDPGGTiO2fLd9H0baHCaiNygIC5KWdB1avEsq4TMDXS7eqLjEfjm7N3O+tnC5dLuD++\n\tvjN+a9FJHI3p88A3eqK17HKg5Rq7MpagYeOQuKUMglLz6FpUWNYHkLAs/I2tUHjzKCC1\n\ta87g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732636700; x=1733241500;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=tamgWmlRQ+BPKIR0ckXBCFvayLD8qx82OjxmF/lPuI8=;\n\tb=UXjB5VIJsjVLjA5M4OIojvq1kAJ+9x6atQf1GNvhJx3M4MVOnVAKrMYI05HWZh2KuY\n\tckrX1xVnkWvSXcX+p3Hb8f6oEN4vEDWJAL6mTxywhIlzDC4NaoHtUAdm6udyovWhKR82\n\t81KZKCuwI4FrSQSqcehuT+rxrOht1umudSIkWyY+l8+QSVvT+9J4qBIl2rX5Li8vasa7\n\tPc4N+ZEW35NIBROH+ldYsjJyW4c3BJStbSYuLvPQS/0yDo7TidUhkfSmWFv8JhoZ8OOk\n\t7jcIrMsXXf4l5lNURoevgD62jg7RTwcBUloAVNuWlo2m/FYz0d8EQ0Thv1NPRiHQyEEF\n\tB1fA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXyLXLMRytE7Yn+YmZWpLyavgIMX0kk08YPA+6V/t7AL8rAqVRKdFuLc+716nsZCqVh7ZrMGL1FrX5NbzovpXE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yyol3vE9kk/muV+5y72cDi2+sG7KRDR9q0y+zCkietNlbl6iVdc\n\tPVI9n2rSbhOAuSucIo/Bx1DO4urIWdH+j39oXzCybAf0gjosN4/3jOHQ/VNeBIYoQnyoT5/EDnU\n\tb0jaOYBZitXCcPbKm3HYEt2ugDtfWGkk48eF2qBinxFm9CKOi","X-Gm-Gg":"ASbGncuGHKns9Mywckv+AT+3Jg4FOwgDIRu58ZElPZn/znW1X9I3oHdL62nBrXX31ip\n\tE3RD3JNenl2Q8x78Zt05SjbtFnhiM","X-Google-Smtp-Source":"AGHT+IHGDMEswEgQMncdlwl526PtLSHnfXqDgpRHFZMzbIq3wQWUtp5J/iWNkifk78jyEtMOmWM/80d+XOb/BmZbgco=","X-Received":"by 2002:a05:690c:fc5:b0:6ee:eebc:2663 with SMTP id\n\t00721157ae682-6ef2282c6d9mr19647027b3.6.1732636700354;\n\tTue, 26 Nov 2024 07:58:20 -0800 (PST)","MIME-Version":"1.0","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>","In-Reply-To":"<20241125230417.GY19381@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 26 Nov 2024 15:58:03 +0000","Message-ID":"","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","To":"Laurent Pinchart ","Cc":"Paul Elder , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32389,"web_url":"https://patchwork.libcamera.org/comment/32389/","msgid":"","date":"2024-11-26T16:04:47","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote:\n> On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart\n> wrote:\n> >\n> > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > > In preparation for adding support for querying direction information\n> > > > > from controls, populate the corresponding field in the control ID\n> > > > > defintions.\n> > > >\n> > > > I wish I could save you the usual bikeshedding on names, but...\n> > > >\n> > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > > it (and I think most people in the industry is also accustomed to\n> > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > > 'out' and 'inout' makes sense. How do we document that ?\n> > >\n> > > I personally thought that in and out were less confusing than controls\n> > > and metadata, mainly because both controls and metadata go in a\n> > > ControlList, and then we talk about setting controls in metadata that is\n> > > returned in a ControlList, while we can also set controls in controls.\n> > >\n> > > Also I got the idea from doxygen \\params[inout]\n> >\n> > If our Control* classes were named using a generic term other than\n> > control, then we could use \"control\" and \"metadata\". Or if\n> > Request::controls() was named something other than \"controls\". But for\n> > now we're using these names, so I think it would just add to the\n> > confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> > they can do for now.\n> >\n> > > > Also, the absence of the field indicates 'inout'. This should be\n> > > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > > implicitly the default.\n> > >\n> > > If you want to be explicit you could write it? I thought it was good\n> > > just to leave as an option.\n> >\n> > The advantage of being explicit is that we can catch missing information\n> > easily.\n>\n> To be honest, I am not sure about \"in\" and \"out\" either. I have only\n> a slight preference of using \"read\" and \"write\" instead, but I'm not\n> convinced it's much better.\n\nWhat matter the most, and I forgot to mention it in the reply, is that\nwe document the direction.\n\nIn/write means \"from applications to the camera\"\nOut/read means \"from the camera to the application\"\n\n(if I got your suggested usage of read/write correctly)\n\nIf I'm not mistaken I haven't seen this being clearly specified, and I\nthink it's worth it as for application developer 'in' could potentialy\nmean \"it comes from the camera\"\n\nThanks\n j\n\n>\n> Naush\n>\n> >\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > > 2 files changed, 19 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > >\n> > > > At the beginning of the file we have\n> > > >\n> > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > > # set through Request::controls() and returned out through Request::metadata().\n> > > >\n> > > > Could you expand it by mentioning 'direction' ?\n> > >\n> > > It's already mentioned: \"bi-*direction*al\" :)\n> > >\n> > > No but seriously, that was what I used to validate my uncertainty about\n> > > using the word \"direction\".\n> > >\n> > > >\n> > > > > @@ -17,6 +17,7 @@ controls:\n> > > > >\n> > > > > - AeLocked:\n> > > > > type: bool\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report the lock status of a running AE algorithm.\n> > > > >\n> > > > > @@ -236,6 +237,7 @@ controls:\n> > > > >\n> > > > > - AeFlickerDetected:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Flicker period detected in microseconds.\n> > > > >\n> > > > > @@ -274,6 +276,7 @@ controls:\n> > > > >\n> > > > > - Lux:\n> > > > > type: float\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report an estimate of the current illuminance level in lux.\n> > > > >\n> > > > > @@ -324,6 +327,7 @@ controls:\n> > > > >\n> > > > > - AwbLocked:\n> > > > > type: bool\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report the lock status of a running AWB algorithm.\n> > > > >\n> > > >\n> > > > ColourTemperature is missing\n> > > >\n> > > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > > >\n> > > > The ColourTemperature control can only be returned in metadata.\n> > >\n> > > What do you mean, I thought this was settable as of...\n> > >\n> > > https://patchwork.libcamera.org/patch/20894/\n> > >\n> > > Oh, it's not merged yet. Ok.\n> > >\n> > > > > @@ -361,6 +365,7 @@ controls:\n> > > > >\n> > > > > - SensorBlackLevels:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Reports the sensor black levels used for processing a frame.\n> > > > >\n> > > > > @@ -385,6 +390,7 @@ controls:\n> > > > >\n> > > > > - FocusFoM:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > > >\n> > > > > @@ -442,6 +448,7 @@ controls:\n> > > > >\n> > > > > - FrameDuration:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > The instantaneous frame duration from start of frame exposure to start\n> > > > > of next exposure, expressed in microseconds.\n> > > > > @@ -450,6 +457,7 @@ controls:\n> > > > >\n> > > > > - FrameDurationLimits:\n> > > > > type: int64_t\n> > > > > + direction: in\n> > > >\n> > > > I read\n> > > >\n> > > > When reported in\n> > > > metadata, the control expresses the minimum and maximum frame\n> > > > durations used after being clipped to the sensor provided frame\n> > > > duration limits.\n> > >\n> > > And I missed that. Nice catch.\n> > >\n> > > > > description: |\n> > > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > > microseconds.\n> > > > > @@ -487,6 +495,7 @@ controls:\n> > > > >\n> > > > > - SensorTemperature:\n> > > > > type: float\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Temperature measure from the camera sensor in Celsius.\n> > > > >\n> > > > > @@ -499,6 +508,7 @@ controls:\n> > > > >\n> > > > > - SensorTimestamp:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > The time when the first row of the image sensor active array is exposed.\n> > > > >\n> > > > > @@ -667,6 +677,7 @@ controls:\n> > > > >\n> > > > > - AfTrigger:\n> > > > > type: int32_t\n> > > > > + direction: in\n> > > > > description: |\n> > > > > Start an autofocus scan.\n> > > > >\n> > > > > @@ -692,6 +703,7 @@ controls:\n> > > > >\n> > > > > - AfPause:\n> > > > > type: int32_t\n> > > > > + direction: in\n> > > > > description: |\n> > > > > Pause lens movements when in continuous autofocus mode.\n> > > >\n> > > > Should AfState be out only ?\n> > > > Also AfPauseState seems to be a metadata only.\n> > >\n> > > They indeed should, yes.\n> > >\n> > > >\n> > > > HdrChannel I read\n> > > >\n> > > > This value is reported back to the application so that it can discover\n> > > > whether this capture corresponds to the short or long exposure image\n> > > > (or any other image used by the HDR procedure). An application can\n> > > > monitor the HDR channel to discover when the differently exposed images\n> > > > have arrived.\n> > > >\n> > > > which suggests me it's a metadata only\n> > >\n> > > I think you're right.\n> > >\n> > > >\n> > > > and DebugMetadataEnable is possibily input only ?\n> > >\n> > > My thought process was that all enable-type controls should be both, as\n> > > you'd probably want to know if what you've set has actually been set.\n> > >\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > > index 1b284257f601..59aa6141c25c 100644\n> > > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > > @@ -79,6 +79,7 @@ controls:\n> > > > >\n> > > > > - AeState:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the current AE algorithm state. Currently identical to\n> > > > > ANDROID_CONTROL_AE_STATE.\n> > > > > @@ -108,6 +109,7 @@ controls:\n> > > > >\n> > > > > - AwbState:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the current AWB algorithm state. Currently identical\n> > > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > > @@ -129,6 +131,7 @@ controls:\n> > > > >\n> > > > > - SensorRollingShutterSkew:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the time between the start of exposure of the first\n> > > > > row and the start of exposure of the last row. Currently identical to\n> > > > > @@ -262,6 +265,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceRectangles:\n> > > > > type: Rectangle\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Boundary rectangles of the detected faces. The number of values is\n> > > > > the number of detected faces.\n> > > > > @@ -273,6 +277,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceScores:\n> > > > > type: uint8_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Confidence score of each of the detected faces. The range of score is\n> > > > > [0, 100]. The number of values should be the number of faces reported\n> > > > > @@ -285,6 +290,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceLandmarks:\n> > > > > type: Point\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > > @@ -298,6 +304,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceIds:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > > face is visible to the camera device. A face that leaves the field of\n> > > >\n> > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > > as well ?\n> > >\n> > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > > only.\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1EEE9C3200\n\tfor ;\n\tTue, 26 Nov 2024 16:04:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 441CC65FD5;\n\tTue, 26 Nov 2024 17:04:55 +0100 (CET)","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 4A31C65FA0\n\tfor ;\n\tTue, 26 Nov 2024 17:04:53 +0100 (CET)","from ideasonboard.com (mob-5-90-139-188.net.vodafone.it\n\t[5.90.139.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D5F5526;\n\tTue, 26 Nov 2024 17:04:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tnWTaVWG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732637070;\n\tbh=IiwkAZGWl5XTChbZHvF+nMoReB+qfYXMu9Svba+zTgg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tnWTaVWGCAvMaUccd1tcaJRkIKMl7yz6u0YV429z5j1xX2ChBJtmLJYBvSjPsGAOn\n\tHu3Y9fMjQCAwWgDeglFHns+bXJvtICwCRLAUKt6+xmYUqhJQ7zvQL+i9Q62mK6ALUe\n\tK8L7ZWa3Sbl0KpvwfhAWWyCFzRTBe5xwg0kiwQkY=","Date":"Tue, 26 Nov 2024 17:04:47 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"Laurent Pinchart , \n\tPaul Elder ,\n\tJacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32390,"web_url":"https://patchwork.libcamera.org/comment/32390/","msgid":"<173263716933.1605529.16246026126566571780@ping.linuxembedded.co.uk>","date":"2024-11-26T16:06:09","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2024-11-26 15:58:03)\n> On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart\n> wrote:\n> >\n> > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > > In preparation for adding support for querying direction information\n> > > > > from controls, populate the corresponding field in the control ID\n> > > > > defintions.\n> > > >\n> > > > I wish I could save you the usual bikeshedding on names, but...\n> > > >\n> > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > > it (and I think most people in the industry is also accustomed to\n> > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > > 'out' and 'inout' makes sense. How do we document that ?\n> > >\n> > > I personally thought that in and out were less confusing than controls\n> > > and metadata, mainly because both controls and metadata go in a\n> > > ControlList, and then we talk about setting controls in metadata that is\n> > > returned in a ControlList, while we can also set controls in controls.\n> > >\n> > > Also I got the idea from doxygen \\params[inout]\n> >\n> > If our Control* classes were named using a generic term other than\n> > control, then we could use \"control\" and \"metadata\". Or if\n> > Request::controls() was named something other than \"controls\". But for\n> > now we're using these names, so I think it would just add to the\n> > confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> > they can do for now.\n> >\n> > > > Also, the absence of the field indicates 'inout'. This should be\n> > > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > > implicitly the default.\n> > >\n> > > If you want to be explicit you could write it? I thought it was good\n> > > just to leave as an option.\n> >\n> > The advantage of being explicit is that we can catch missing information\n> > easily.\n> \n> To be honest, I am not sure about \"in\" and \"out\" either. I have only\n> a slight preference of using \"read\" and \"write\" instead, but I'm not\n> convinced it's much better.\n\nAs there's a shed to be painted...\n\nI think I would be in the R/W camp here too\n\nProperties and Metadata are R only\nControls are W or potentially RW ...?\n\nOtherwise direction feels odd to me. Which direction is it framed from ?\nAre metadata 'in' to the applciation or 'out' from libcamera ? (Yes, the\ndocumentation would say that), while a Read/Write 'action' conveys the\ndirection is from the actor...\n\n--\nKieran\n\n> \n> Naush\n> \n> >\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > > 2 files changed, 19 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > >\n> > > > At the beginning of the file we have\n> > > >\n> > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > > # set through Request::controls() and returned out through Request::metadata().\n> > > >\n> > > > Could you expand it by mentioning 'direction' ?\n> > >\n> > > It's already mentioned: \"bi-*direction*al\" :)\n> > >\n> > > No but seriously, that was what I used to validate my uncertainty about\n> > > using the word \"direction\".\n> > >\n> > > >\n> > > > > @@ -17,6 +17,7 @@ controls:\n> > > > >\n> > > > > - AeLocked:\n> > > > > type: bool\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report the lock status of a running AE algorithm.\n> > > > >\n> > > > > @@ -236,6 +237,7 @@ controls:\n> > > > >\n> > > > > - AeFlickerDetected:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Flicker period detected in microseconds.\n> > > > >\n> > > > > @@ -274,6 +276,7 @@ controls:\n> > > > >\n> > > > > - Lux:\n> > > > > type: float\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report an estimate of the current illuminance level in lux.\n> > > > >\n> > > > > @@ -324,6 +327,7 @@ controls:\n> > > > >\n> > > > > - AwbLocked:\n> > > > > type: bool\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Report the lock status of a running AWB algorithm.\n> > > > >\n> > > >\n> > > > ColourTemperature is missing\n> > > >\n> > > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > > >\n> > > > The ColourTemperature control can only be returned in metadata.\n> > >\n> > > What do you mean, I thought this was settable as of...\n> > >\n> > > https://patchwork.libcamera.org/patch/20894/\n> > >\n> > > Oh, it's not merged yet. Ok.\n> > >\n> > > > > @@ -361,6 +365,7 @@ controls:\n> > > > >\n> > > > > - SensorBlackLevels:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Reports the sensor black levels used for processing a frame.\n> > > > >\n> > > > > @@ -385,6 +390,7 @@ controls:\n> > > > >\n> > > > > - FocusFoM:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > > >\n> > > > > @@ -442,6 +448,7 @@ controls:\n> > > > >\n> > > > > - FrameDuration:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > The instantaneous frame duration from start of frame exposure to start\n> > > > > of next exposure, expressed in microseconds.\n> > > > > @@ -450,6 +457,7 @@ controls:\n> > > > >\n> > > > > - FrameDurationLimits:\n> > > > > type: int64_t\n> > > > > + direction: in\n> > > >\n> > > > I read\n> > > >\n> > > > When reported in\n> > > > metadata, the control expresses the minimum and maximum frame\n> > > > durations used after being clipped to the sensor provided frame\n> > > > duration limits.\n> > >\n> > > And I missed that. Nice catch.\n> > >\n> > > > > description: |\n> > > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > > microseconds.\n> > > > > @@ -487,6 +495,7 @@ controls:\n> > > > >\n> > > > > - SensorTemperature:\n> > > > > type: float\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Temperature measure from the camera sensor in Celsius.\n> > > > >\n> > > > > @@ -499,6 +508,7 @@ controls:\n> > > > >\n> > > > > - SensorTimestamp:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > The time when the first row of the image sensor active array is exposed.\n> > > > >\n> > > > > @@ -667,6 +677,7 @@ controls:\n> > > > >\n> > > > > - AfTrigger:\n> > > > > type: int32_t\n> > > > > + direction: in\n> > > > > description: |\n> > > > > Start an autofocus scan.\n> > > > >\n> > > > > @@ -692,6 +703,7 @@ controls:\n> > > > >\n> > > > > - AfPause:\n> > > > > type: int32_t\n> > > > > + direction: in\n> > > > > description: |\n> > > > > Pause lens movements when in continuous autofocus mode.\n> > > >\n> > > > Should AfState be out only ?\n> > > > Also AfPauseState seems to be a metadata only.\n> > >\n> > > They indeed should, yes.\n> > >\n> > > >\n> > > > HdrChannel I read\n> > > >\n> > > > This value is reported back to the application so that it can discover\n> > > > whether this capture corresponds to the short or long exposure image\n> > > > (or any other image used by the HDR procedure). An application can\n> > > > monitor the HDR channel to discover when the differently exposed images\n> > > > have arrived.\n> > > >\n> > > > which suggests me it's a metadata only\n> > >\n> > > I think you're right.\n> > >\n> > > >\n> > > > and DebugMetadataEnable is possibily input only ?\n> > >\n> > > My thought process was that all enable-type controls should be both, as\n> > > you'd probably want to know if what you've set has actually been set.\n> > >\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > > index 1b284257f601..59aa6141c25c 100644\n> > > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > > @@ -79,6 +79,7 @@ controls:\n> > > > >\n> > > > > - AeState:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the current AE algorithm state. Currently identical to\n> > > > > ANDROID_CONTROL_AE_STATE.\n> > > > > @@ -108,6 +109,7 @@ controls:\n> > > > >\n> > > > > - AwbState:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the current AWB algorithm state. Currently identical\n> > > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > > @@ -129,6 +131,7 @@ controls:\n> > > > >\n> > > > > - SensorRollingShutterSkew:\n> > > > > type: int64_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Control to report the time between the start of exposure of the first\n> > > > > row and the start of exposure of the last row. Currently identical to\n> > > > > @@ -262,6 +265,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceRectangles:\n> > > > > type: Rectangle\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Boundary rectangles of the detected faces. The number of values is\n> > > > > the number of detected faces.\n> > > > > @@ -273,6 +277,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceScores:\n> > > > > type: uint8_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Confidence score of each of the detected faces. The range of score is\n> > > > > [0, 100]. The number of values should be the number of faces reported\n> > > > > @@ -285,6 +290,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceLandmarks:\n> > > > > type: Point\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > > @@ -298,6 +304,7 @@ controls:\n> > > > >\n> > > > > - FaceDetectFaceIds:\n> > > > > type: int32_t\n> > > > > + direction: out\n> > > > > description: |\n> > > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > > face is visible to the camera device. A face that leaves the field of\n> > > >\n> > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > > as well ?\n> > >\n> > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > > only.\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 55081C3200\n\tfor ;\n\tTue, 26 Nov 2024 16:06:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DE7D66077;\n\tTue, 26 Nov 2024 17:06:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E98F765FA0\n\tfor ;\n\tTue, 26 Nov 2024 17:06:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 852F0526;\n\tTue, 26 Nov 2024 17:05:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Te2exJnP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732637149;\n\tbh=RIg6qXem6q4+QaoTmuSUrrOVglH/pEuUyMffr7hGtLA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Te2exJnPph1eAycP0a2iSz9gWj1pymi2amfRnqqw5M4XIYmLJCsZq8hxP4tsoV5NV\n\tcnbs6PHo06lgvEMRW0HrDJBxY5Db7mvc96se3qU/kI/eriJh210ZVPlSG5LXNDmLj1\n\tEHb9ty61xRWoZu5buYbqo/Sj4bfU3RnUg7I5Gb/M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tNaushir Patuck ","Date":"Tue, 26 Nov 2024 16:06:09 +0000","Message-ID":"<173263716933.1605529.16246026126566571780@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32391,"web_url":"https://patchwork.libcamera.org/comment/32391/","msgid":"","date":"2024-11-26T16:07:31","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Tue, 26 Nov 2024 at 16:04, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote:\n> > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart\n> > wrote:\n> > >\n> > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > > > In preparation for adding support for querying direction information\n> > > > > > from controls, populate the corresponding field in the control ID\n> > > > > > defintions.\n> > > > >\n> > > > > I wish I could save you the usual bikeshedding on names, but...\n> > > > >\n> > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > > > it (and I think most people in the industry is also accustomed to\n> > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > > > 'out' and 'inout' makes sense. How do we document that ?\n> > > >\n> > > > I personally thought that in and out were less confusing than controls\n> > > > and metadata, mainly because both controls and metadata go in a\n> > > > ControlList, and then we talk about setting controls in metadata that is\n> > > > returned in a ControlList, while we can also set controls in controls.\n> > > >\n> > > > Also I got the idea from doxygen \\params[inout]\n> > >\n> > > If our Control* classes were named using a generic term other than\n> > > control, then we could use \"control\" and \"metadata\". Or if\n> > > Request::controls() was named something other than \"controls\". But for\n> > > now we're using these names, so I think it would just add to the\n> > > confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> > > they can do for now.\n> > >\n> > > > > Also, the absence of the field indicates 'inout'. This should be\n> > > > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > > > implicitly the default.\n> > > >\n> > > > If you want to be explicit you could write it? I thought it was good\n> > > > just to leave as an option.\n> > >\n> > > The advantage of being explicit is that we can catch missing information\n> > > easily.\n> >\n> > To be honest, I am not sure about \"in\" and \"out\" either. I have only\n> > a slight preference of using \"read\" and \"write\" instead, but I'm not\n> > convinced it's much better.\n>\n> What matter the most, and I forgot to mention it in the reply, is that\n> we document the direction.\n>\n> In/write means \"from applications to the camera\"\n> Out/read means \"from the camera to the application\"\n>\n> (if I got your suggested usage of read/write correctly)\n\nThat's correct, my read/write is from the reference of the application\ndeveloper.\n\nNaush\n\n>\n> If I'm not mistaken I haven't seen this being clearly specified, and I\n> think it's worth it as for application developer 'in' could potentialy\n> mean \"it comes from the camera\"\n>\n> Thanks\n> j\n>\n> >\n> > Naush\n> >\n> > >\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > > > 2 files changed, 19 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > >\n> > > > > At the beginning of the file we have\n> > > > >\n> > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > > > # set through Request::controls() and returned out through Request::metadata().\n> > > > >\n> > > > > Could you expand it by mentioning 'direction' ?\n> > > >\n> > > > It's already mentioned: \"bi-*direction*al\" :)\n> > > >\n> > > > No but seriously, that was what I used to validate my uncertainty about\n> > > > using the word \"direction\".\n> > > >\n> > > > >\n> > > > > > @@ -17,6 +17,7 @@ controls:\n> > > > > >\n> > > > > > - AeLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AE algorithm.\n> > > > > >\n> > > > > > @@ -236,6 +237,7 @@ controls:\n> > > > > >\n> > > > > > - AeFlickerDetected:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Flicker period detected in microseconds.\n> > > > > >\n> > > > > > @@ -274,6 +276,7 @@ controls:\n> > > > > >\n> > > > > > - Lux:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report an estimate of the current illuminance level in lux.\n> > > > > >\n> > > > > > @@ -324,6 +327,7 @@ controls:\n> > > > > >\n> > > > > > - AwbLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AWB algorithm.\n> > > > > >\n> > > > >\n> > > > > ColourTemperature is missing\n> > > > >\n> > > > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > > > >\n> > > > > The ColourTemperature control can only be returned in metadata.\n> > > >\n> > > > What do you mean, I thought this was settable as of...\n> > > >\n> > > > https://patchwork.libcamera.org/patch/20894/\n> > > >\n> > > > Oh, it's not merged yet. Ok.\n> > > >\n> > > > > > @@ -361,6 +365,7 @@ controls:\n> > > > > >\n> > > > > > - SensorBlackLevels:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports the sensor black levels used for processing a frame.\n> > > > > >\n> > > > > > @@ -385,6 +390,7 @@ controls:\n> > > > > >\n> > > > > > - FocusFoM:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > > > >\n> > > > > > @@ -442,6 +448,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDuration:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The instantaneous frame duration from start of frame exposure to start\n> > > > > > of next exposure, expressed in microseconds.\n> > > > > > @@ -450,6 +457,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDurationLimits:\n> > > > > > type: int64_t\n> > > > > > + direction: in\n> > > > >\n> > > > > I read\n> > > > >\n> > > > > When reported in\n> > > > > metadata, the control expresses the minimum and maximum frame\n> > > > > durations used after being clipped to the sensor provided frame\n> > > > > duration limits.\n> > > >\n> > > > And I missed that. Nice catch.\n> > > >\n> > > > > > description: |\n> > > > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > > > microseconds.\n> > > > > > @@ -487,6 +495,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTemperature:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Temperature measure from the camera sensor in Celsius.\n> > > > > >\n> > > > > > @@ -499,6 +508,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTimestamp:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The time when the first row of the image sensor active array is exposed.\n> > > > > >\n> > > > > > @@ -667,6 +677,7 @@ controls:\n> > > > > >\n> > > > > > - AfTrigger:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Start an autofocus scan.\n> > > > > >\n> > > > > > @@ -692,6 +703,7 @@ controls:\n> > > > > >\n> > > > > > - AfPause:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Pause lens movements when in continuous autofocus mode.\n> > > > >\n> > > > > Should AfState be out only ?\n> > > > > Also AfPauseState seems to be a metadata only.\n> > > >\n> > > > They indeed should, yes.\n> > > >\n> > > > >\n> > > > > HdrChannel I read\n> > > > >\n> > > > > This value is reported back to the application so that it can discover\n> > > > > whether this capture corresponds to the short or long exposure image\n> > > > > (or any other image used by the HDR procedure). An application can\n> > > > > monitor the HDR channel to discover when the differently exposed images\n> > > > > have arrived.\n> > > > >\n> > > > > which suggests me it's a metadata only\n> > > >\n> > > > I think you're right.\n> > > >\n> > > > >\n> > > > > and DebugMetadataEnable is possibily input only ?\n> > > >\n> > > > My thought process was that all enable-type controls should be both, as\n> > > > you'd probably want to know if what you've set has actually been set.\n> > > >\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > > > index 1b284257f601..59aa6141c25c 100644\n> > > > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > > > @@ -79,6 +79,7 @@ controls:\n> > > > > >\n> > > > > > - AeState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AE algorithm state. Currently identical to\n> > > > > > ANDROID_CONTROL_AE_STATE.\n> > > > > > @@ -108,6 +109,7 @@ controls:\n> > > > > >\n> > > > > > - AwbState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AWB algorithm state. Currently identical\n> > > > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > > > @@ -129,6 +131,7 @@ controls:\n> > > > > >\n> > > > > > - SensorRollingShutterSkew:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the time between the start of exposure of the first\n> > > > > > row and the start of exposure of the last row. Currently identical to\n> > > > > > @@ -262,6 +265,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceRectangles:\n> > > > > > type: Rectangle\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Boundary rectangles of the detected faces. The number of values is\n> > > > > > the number of detected faces.\n> > > > > > @@ -273,6 +277,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceScores:\n> > > > > > type: uint8_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Confidence score of each of the detected faces. The range of score is\n> > > > > > [0, 100]. The number of values should be the number of faces reported\n> > > > > > @@ -285,6 +290,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceLandmarks:\n> > > > > > type: Point\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > > > @@ -298,6 +304,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceIds:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > > > face is visible to the camera device. A face that leaves the field of\n> > > > >\n> > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > > > as well ?\n> > > >\n> > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > > > only.\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DA781C3200\n\tfor ;\n\tTue, 26 Nov 2024 16:07:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87E8E66077;\n\tTue, 26 Nov 2024 17:07:52 +0100 (CET)","from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com\n\t[IPv6:2607:f8b0:4864:20::112b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB83E65FA0\n\tfor ;\n\tTue, 26 Nov 2024 17:07:49 +0100 (CET)","by mail-yw1-x112b.google.com with SMTP id\n\t00721157ae682-6ee91fe8550so1586597b3.2\n\tfor ;\n\tTue, 26 Nov 2024 08:07:49 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"M1xbRV9v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1732637269; x=1733242069;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=z9OoK87lJHVkoM4gFZ6V4uR98DfMebVRzjAQSrcC7OM=;\n\tb=M1xbRV9vU5YO4qKh7CMLNT0VW6Hl/HuQAPQB+PURFp0ID/lJPXDzMxvZfdkDcbkb+V\n\t35GT4GnfwHD01XiLQS6RQPf7tNNuOme2udyqVCQGZjxxzFf1pD5fV/3e0wDVvUTDYjJT\n\tC2Qsp7pdr+JZOKgQ1Qtun/RA3NSW6iiZsSsFTHF/i2dL7Y6pU+vV9LcrLii8ojyE1mOy\n\tJG0LSrsaubSa1egBASBtr2oQ3byhp8I9Q16mMGwFDOpFw5SgCb1BImgfSEg8HxatBG8A\n\t8CfqT6g/vMZuUpuNvHnHb7UlIt83597SB/+tRfuMg91qEcByL8+53UK42mNOz6Ohe+J1\n\t/LlQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732637269; x=1733242069;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=z9OoK87lJHVkoM4gFZ6V4uR98DfMebVRzjAQSrcC7OM=;\n\tb=aNIogKs5Vp5xb95IRq8h8vMMJzzSPgxTaPJkifnh8eE/4Uee4KeYnsLuc5rhOReblT\n\tCh9f+YBSmLu3cjJpD7YgMswt5KMIjX4x/sGVvaUrIUewkYgu3POWlj6G3+8jlEZQNCT2\n\tvx0KrAx1ouv4wcOlLUbWN3XA0l/Q7Xp/Q5fftVJu8XcO3iNiUrOl3qZsbL/YjYe3yEAO\n\t4JwO+5hkL7EATrZzhKO6Qhh49DPjxzZMfRLi8n5qEb65mz1XmNCGpiRKd+h0IGkfM0Tz\n\tKNqebkK1ieZFL+ZKsQWEcY33hhZTJ0DInUW1s+2fuNamJSPYMcGHyOkPwPBErE084Qug\n\tUvBQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVFotZJ4rmX28HpLXUsBX7dr25F8/vjOSPMI3jJy+7EIbv4zmtUk+dmNWFWeTsTI38/Q6Q9b3xQ9TE3xk298/o=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwlZhkDPmkDeGRoxjewBTsnK/ocxqa0eQ2/8nO68Z5GX46vk8vx\n\tRG+E+EaLczesbPVywFZRFJedcYmuuWDg1OR0N0MaUajcDxdrMEqlI2dDMq5LZCKlvrkPV0pvYtv\n\tGmOSok58dpHcieQj9F+TIX4HZhGl7CR/nWUe8Vg==","X-Gm-Gg":"ASbGncuhH3UF8+ORWRuSGcNsvEMDTdVbEkJ6BrfkAuEfP1tvLYuSv89Pac+5x0xMmNI\n\t5QcKw2RKgRaC7XQD037gAHqAw3up+","X-Google-Smtp-Source":"AGHT+IHbFutWOFW9R1qK/RcT6l5GT6vP9Ncgo5ZbrsJfXgMLvyP9oaOfHiGqPvBuDPOzbr5heCr1KOahgE9mPPUmEFY=","X-Received":"by 2002:a05:690c:4b11:b0:6ea:8cfa:52a with SMTP id\n\t00721157ae682-6ef2284e548mr19387497b3.9.1732637268866;\n\tTue, 26 Nov 2024 08:07:48 -0800 (PST)","MIME-Version":"1.0","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 26 Nov 2024 16:07:31 +0000","Message-ID":"","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","To":"Jacopo Mondi ","Cc":"Laurent Pinchart , \n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32394,"web_url":"https://patchwork.libcamera.org/comment/32394/","msgid":"<20241126171008.GJ5461@pendragon.ideasonboard.com>","date":"2024-11-26T17:10:08","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 05:04:47PM +0100, Jacopo Mondi wrote:\n> On Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote:\n> > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart wrote:\n> > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > > > In preparation for adding support for querying direction information\n> > > > > > from controls, populate the corresponding field in the control ID\n> > > > > > defintions.\n> > > > >\n> > > > > I wish I could save you the usual bikeshedding on names, but...\n> > > > >\n> > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > > > it (and I think most people in the industry is also accustomed to\n> > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > > > 'out' and 'inout' makes sense. How do we document that ?\n> > > >\n> > > > I personally thought that in and out were less confusing than controls\n> > > > and metadata, mainly because both controls and metadata go in a\n> > > > ControlList, and then we talk about setting controls in metadata that is\n> > > > returned in a ControlList, while we can also set controls in controls.\n> > > >\n> > > > Also I got the idea from doxygen \\params[inout]\n> > >\n> > > If our Control* classes were named using a generic term other than\n> > > control, then we could use \"control\" and \"metadata\". Or if\n> > > Request::controls() was named something other than \"controls\". But for\n> > > now we're using these names, so I think it would just add to the\n> > > confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> > > they can do for now.\n> > >\n> > > > > Also, the absence of the field indicates 'inout'. This should be\n> > > > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > > > implicitly the default.\n> > > >\n> > > > If you want to be explicit you could write it? I thought it was good\n> > > > just to leave as an option.\n> > >\n> > > The advantage of being explicit is that we can catch missing information\n> > > easily.\n> >\n> > To be honest, I am not sure about \"in\" and \"out\" either. I have only\n> > a slight preference of using \"read\" and \"write\" instead, but I'm not\n> > convinced it's much better.\n> \n> What matter the most, and I forgot to mention it in the reply, is that\n> we document the direction.\n> \n> In/write means \"from applications to the camera\"\n> Out/read means \"from the camera to the application\"\n> \n> (if I got your suggested usage of read/write correctly)\n> \n> If I'm not mistaken I haven't seen this being clearly specified, and I\n> think it's worth it as for application developer 'in' could potentialy\n> mean \"it comes from the camera\"\n\nI've never thought it could be misconstrued like that. I may of course\nbe biased. At least we haven't gone the V4L2 way with output/capture :-)\nJokes aside, we constantly and consistently talk about camera outputting\nstreams and taking requests as inputs, so I think the in/out vocabulary\nis well established. Read/write carries a similar possibility of\nmisinterpretation: a camera writes frames, but the metadata would be\nconsidered as being \"read\".\n\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > > > 2 files changed, 19 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > >\n> > > > > At the beginning of the file we have\n> > > > >\n> > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > > > # set through Request::controls() and returned out through Request::metadata().\n> > > > >\n> > > > > Could you expand it by mentioning 'direction' ?\n> > > >\n> > > > It's already mentioned: \"bi-*direction*al\" :)\n> > > >\n> > > > No but seriously, that was what I used to validate my uncertainty about\n> > > > using the word \"direction\".\n> > > >\n> > > > >\n> > > > > > @@ -17,6 +17,7 @@ controls:\n> > > > > >\n> > > > > > - AeLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AE algorithm.\n> > > > > >\n> > > > > > @@ -236,6 +237,7 @@ controls:\n> > > > > >\n> > > > > > - AeFlickerDetected:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Flicker period detected in microseconds.\n> > > > > >\n> > > > > > @@ -274,6 +276,7 @@ controls:\n> > > > > >\n> > > > > > - Lux:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report an estimate of the current illuminance level in lux.\n> > > > > >\n> > > > > > @@ -324,6 +327,7 @@ controls:\n> > > > > >\n> > > > > > - AwbLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AWB algorithm.\n> > > > > >\n> > > > >\n> > > > > ColourTemperature is missing\n> > > > >\n> > > > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > > > >\n> > > > > The ColourTemperature control can only be returned in metadata.\n> > > >\n> > > > What do you mean, I thought this was settable as of...\n> > > >\n> > > > https://patchwork.libcamera.org/patch/20894/\n> > > >\n> > > > Oh, it's not merged yet. Ok.\n> > > >\n> > > > > > @@ -361,6 +365,7 @@ controls:\n> > > > > >\n> > > > > > - SensorBlackLevels:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports the sensor black levels used for processing a frame.\n> > > > > >\n> > > > > > @@ -385,6 +390,7 @@ controls:\n> > > > > >\n> > > > > > - FocusFoM:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > > > >\n> > > > > > @@ -442,6 +448,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDuration:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The instantaneous frame duration from start of frame exposure to start\n> > > > > > of next exposure, expressed in microseconds.\n> > > > > > @@ -450,6 +457,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDurationLimits:\n> > > > > > type: int64_t\n> > > > > > + direction: in\n> > > > >\n> > > > > I read\n> > > > >\n> > > > > When reported in\n> > > > > metadata, the control expresses the minimum and maximum frame\n> > > > > durations used after being clipped to the sensor provided frame\n> > > > > duration limits.\n> > > >\n> > > > And I missed that. Nice catch.\n> > > >\n> > > > > > description: |\n> > > > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > > > microseconds.\n> > > > > > @@ -487,6 +495,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTemperature:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Temperature measure from the camera sensor in Celsius.\n> > > > > >\n> > > > > > @@ -499,6 +508,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTimestamp:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The time when the first row of the image sensor active array is exposed.\n> > > > > >\n> > > > > > @@ -667,6 +677,7 @@ controls:\n> > > > > >\n> > > > > > - AfTrigger:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Start an autofocus scan.\n> > > > > >\n> > > > > > @@ -692,6 +703,7 @@ controls:\n> > > > > >\n> > > > > > - AfPause:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Pause lens movements when in continuous autofocus mode.\n> > > > >\n> > > > > Should AfState be out only ?\n> > > > > Also AfPauseState seems to be a metadata only.\n> > > >\n> > > > They indeed should, yes.\n> > > >\n> > > > >\n> > > > > HdrChannel I read\n> > > > >\n> > > > > This value is reported back to the application so that it can discover\n> > > > > whether this capture corresponds to the short or long exposure image\n> > > > > (or any other image used by the HDR procedure). An application can\n> > > > > monitor the HDR channel to discover when the differently exposed images\n> > > > > have arrived.\n> > > > >\n> > > > > which suggests me it's a metadata only\n> > > >\n> > > > I think you're right.\n> > > >\n> > > > >\n> > > > > and DebugMetadataEnable is possibily input only ?\n> > > >\n> > > > My thought process was that all enable-type controls should be both, as\n> > > > you'd probably want to know if what you've set has actually been set.\n> > > >\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > > > index 1b284257f601..59aa6141c25c 100644\n> > > > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > > > @@ -79,6 +79,7 @@ controls:\n> > > > > >\n> > > > > > - AeState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AE algorithm state. Currently identical to\n> > > > > > ANDROID_CONTROL_AE_STATE.\n> > > > > > @@ -108,6 +109,7 @@ controls:\n> > > > > >\n> > > > > > - AwbState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AWB algorithm state. Currently identical\n> > > > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > > > @@ -129,6 +131,7 @@ controls:\n> > > > > >\n> > > > > > - SensorRollingShutterSkew:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the time between the start of exposure of the first\n> > > > > > row and the start of exposure of the last row. Currently identical to\n> > > > > > @@ -262,6 +265,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceRectangles:\n> > > > > > type: Rectangle\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Boundary rectangles of the detected faces. The number of values is\n> > > > > > the number of detected faces.\n> > > > > > @@ -273,6 +277,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceScores:\n> > > > > > type: uint8_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Confidence score of each of the detected faces. The range of score is\n> > > > > > [0, 100]. The number of values should be the number of faces reported\n> > > > > > @@ -285,6 +290,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceLandmarks:\n> > > > > > type: Point\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > > > @@ -298,6 +304,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceIds:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > > > face is visible to the camera device. A face that leaves the field of\n> > > > >\n> > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > > > as well ?\n> > > >\n> > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > > > only.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C4F4EBD16B\n\tfor ;\n\tTue, 26 Nov 2024 17:10:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADE046607E;\n\tTue, 26 Nov 2024 18:10:19 +0100 (CET)","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 1752065FA0\n\tfor ;\n\tTue, 26 Nov 2024 18:10:18 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 48208526;\n\tTue, 26 Nov 2024 18:09:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Gr2yYKKo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732640995;\n\tbh=9I9HkoiLEHzRgJWL2u87K0MafzkxUpFv+kWTTyKWPSA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gr2yYKKof2tdDOkD4T3dYbhMW+h8NsPqpjSEh/2RWZrREtulPvrLRw80DHxNhSuT4\n\tCcJWconoCW69PLuxiVfTSIwp/wj+3acvvIlywd1htzMifBuuLhsSo4CmjQttn0dsMS\n\tPm7FQdaJImKNRAojo+2z5hzFQWzYhPXPis5CSIJo=","Date":"Tue, 26 Nov 2024 19:10:08 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Naushir Patuck ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"<20241126171008.GJ5461@pendragon.ideasonboard.com>","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32405,"web_url":"https://patchwork.libcamera.org/comment/32405/","msgid":"","date":"2024-11-27T08:12:28","subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Nov 26, 2024 at 04:06:09PM +0000, Kieran Bingham wrote:\n> Quoting Naushir Patuck (2024-11-26 15:58:03)\n> > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart\n> > wrote:\n> > >\n> > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote:\n> > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote:\n> > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote:\n> > > > > > In preparation for adding support for querying direction information\n> > > > > > from controls, populate the corresponding field in the control ID\n> > > > > > defintions.\n> > > > >\n> > > > > I wish I could save you the usual bikeshedding on names, but...\n> > > > >\n> > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from\n> > > > > app to camera) from 'metadata' (from camera to app) and I quite like\n> > > > > it (and I think most people in the industry is also accustomed to\n> > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in'\n> > > > > 'out' and 'inout' makes sense. How do we document that ?\n> > > >\n> > > > I personally thought that in and out were less confusing than controls\n> > > > and metadata, mainly because both controls and metadata go in a\n> > > > ControlList, and then we talk about setting controls in metadata that is\n> > > > returned in a ControlList, while we can also set controls in controls.\n> > > >\n> > > > Also I got the idea from doxygen \\params[inout]\n> > >\n> > > If our Control* classes were named using a generic term other than\n> > > control, then we could use \"control\" and \"metadata\". Or if\n> > > Request::controls() was named something other than \"controls\". But for\n> > > now we're using these names, so I think it would just add to the\n> > > confusion. \"in\" and \"out\" are probably not the absolute best but I think\n> > > they can do for now.\n> > >\n> > > > > Also, the absence of the field indicates 'inout'. This should be\n> > > > > documented, and I wonder if 'inout' is valid as a value if it's\n> > > > > implicitly the default.\n> > > >\n> > > > If you want to be explicit you could write it? I thought it was good\n> > > > just to leave as an option.\n> > >\n> > > The advantage of being explicit is that we can catch missing information\n> > > easily.\n> > \n> > To be honest, I am not sure about \"in\" and \"out\" either. I have only\n> > a slight preference of using \"read\" and \"write\" instead, but I'm not\n> > convinced it's much better.\n> \n> As there's a shed to be painted...\n> \n> I think I would be in the R/W camp here too\n> \n> Properties and Metadata are R only\n> Controls are W or potentially RW ...?\n> \n> Otherwise direction feels odd to me. Which direction is it framed from ?\n> Are metadata 'in' to the applciation or 'out' from libcamera ? (Yes, the\n> documentation would say that), while a Read/Write 'action' conveys the\n> direction is from the actor...\n\nimo input and output are unambiguous. I'm having a hard time imagining,\nanalogously, a function caller and callee having an argument on what is\nconsidered the input and output. The application passes input controls\nto the camera, and the camera receives input controls. Reading and\nwriting on the other hand depend on your frame of reference (pun\nintended).\n\nI suppose that regardless it should indeed be documented for clarity...\nI'm having trouble figuring out where though, since application\ndevelopers aren't going to be checking the comments in\ncontrol_ids_core.yaml. I found this really nice comment in\napplication-developer.rst:\n\n.. TODO: A request can also have controls or parameters that you can\napply to the image.\n\nSo I think we're missing guide-level documentation completely on how to\nuse controls and metadata...? git grep ControlList | grep Documentation\nonly returns ipa and pipeline handler developer guides.\n\n\nPaul\n\n> > \n> > >\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++\n> > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++\n> > > > > > 2 files changed, 19 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > index d34a2d068b60..c0e91d1852cd 100644\n> > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > >\n> > > > > At the beginning of the file we have\n> > > > >\n> > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > > > # set through Request::controls() and returned out through Request::metadata().\n> > > > >\n> > > > > Could you expand it by mentioning 'direction' ?\n> > > >\n> > > > It's already mentioned: \"bi-*direction*al\" :)\n> > > >\n> > > > No but seriously, that was what I used to validate my uncertainty about\n> > > > using the word \"direction\".\n> > > >\n> > > > >\n> > > > > > @@ -17,6 +17,7 @@ controls:\n> > > > > >\n> > > > > > - AeLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AE algorithm.\n> > > > > >\n> > > > > > @@ -236,6 +237,7 @@ controls:\n> > > > > >\n> > > > > > - AeFlickerDetected:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Flicker period detected in microseconds.\n> > > > > >\n> > > > > > @@ -274,6 +276,7 @@ controls:\n> > > > > >\n> > > > > > - Lux:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report an estimate of the current illuminance level in lux.\n> > > > > >\n> > > > > > @@ -324,6 +327,7 @@ controls:\n> > > > > >\n> > > > > > - AwbLocked:\n> > > > > > type: bool\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Report the lock status of a running AWB algorithm.\n> > > > > >\n> > > > >\n> > > > > ColourTemperature is missing\n> > > > >\n> > > > > Report the estimate of the colour temperature for the frame, in kelvin.\n> > > > >\n> > > > > The ColourTemperature control can only be returned in metadata.\n> > > >\n> > > > What do you mean, I thought this was settable as of...\n> > > >\n> > > > https://patchwork.libcamera.org/patch/20894/\n> > > >\n> > > > Oh, it's not merged yet. Ok.\n> > > >\n> > > > > > @@ -361,6 +365,7 @@ controls:\n> > > > > >\n> > > > > > - SensorBlackLevels:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports the sensor black levels used for processing a frame.\n> > > > > >\n> > > > > > @@ -385,6 +390,7 @@ controls:\n> > > > > >\n> > > > > > - FocusFoM:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is.\n> > > > > >\n> > > > > > @@ -442,6 +448,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDuration:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The instantaneous frame duration from start of frame exposure to start\n> > > > > > of next exposure, expressed in microseconds.\n> > > > > > @@ -450,6 +457,7 @@ controls:\n> > > > > >\n> > > > > > - FrameDurationLimits:\n> > > > > > type: int64_t\n> > > > > > + direction: in\n> > > > >\n> > > > > I read\n> > > > >\n> > > > > When reported in\n> > > > > metadata, the control expresses the minimum and maximum frame\n> > > > > durations used after being clipped to the sensor provided frame\n> > > > > duration limits.\n> > > >\n> > > > And I missed that. Nice catch.\n> > > >\n> > > > > > description: |\n> > > > > > The minimum and maximum (in that order) frame duration, expressed in\n> > > > > > microseconds.\n> > > > > > @@ -487,6 +495,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTemperature:\n> > > > > > type: float\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Temperature measure from the camera sensor in Celsius.\n> > > > > >\n> > > > > > @@ -499,6 +508,7 @@ controls:\n> > > > > >\n> > > > > > - SensorTimestamp:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > The time when the first row of the image sensor active array is exposed.\n> > > > > >\n> > > > > > @@ -667,6 +677,7 @@ controls:\n> > > > > >\n> > > > > > - AfTrigger:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Start an autofocus scan.\n> > > > > >\n> > > > > > @@ -692,6 +703,7 @@ controls:\n> > > > > >\n> > > > > > - AfPause:\n> > > > > > type: int32_t\n> > > > > > + direction: in\n> > > > > > description: |\n> > > > > > Pause lens movements when in continuous autofocus mode.\n> > > > >\n> > > > > Should AfState be out only ?\n> > > > > Also AfPauseState seems to be a metadata only.\n> > > >\n> > > > They indeed should, yes.\n> > > >\n> > > > >\n> > > > > HdrChannel I read\n> > > > >\n> > > > > This value is reported back to the application so that it can discover\n> > > > > whether this capture corresponds to the short or long exposure image\n> > > > > (or any other image used by the HDR procedure). An application can\n> > > > > monitor the HDR channel to discover when the differently exposed images\n> > > > > have arrived.\n> > > > >\n> > > > > which suggests me it's a metadata only\n> > > >\n> > > > I think you're right.\n> > > >\n> > > > >\n> > > > > and DebugMetadataEnable is possibily input only ?\n> > > >\n> > > > My thought process was that all enable-type controls should be both, as\n> > > > you'd probably want to know if what you've set has actually been set.\n> > > >\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml\n> > > > > > index 1b284257f601..59aa6141c25c 100644\n> > > > > > --- a/src/libcamera/control_ids_draft.yaml\n> > > > > > +++ b/src/libcamera/control_ids_draft.yaml\n> > > > > > @@ -79,6 +79,7 @@ controls:\n> > > > > >\n> > > > > > - AeState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AE algorithm state. Currently identical to\n> > > > > > ANDROID_CONTROL_AE_STATE.\n> > > > > > @@ -108,6 +109,7 @@ controls:\n> > > > > >\n> > > > > > - AwbState:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the current AWB algorithm state. Currently identical\n> > > > > > to ANDROID_CONTROL_AWB_STATE.\n> > > > > > @@ -129,6 +131,7 @@ controls:\n> > > > > >\n> > > > > > - SensorRollingShutterSkew:\n> > > > > > type: int64_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Control to report the time between the start of exposure of the first\n> > > > > > row and the start of exposure of the last row. Currently identical to\n> > > > > > @@ -262,6 +265,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceRectangles:\n> > > > > > type: Rectangle\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Boundary rectangles of the detected faces. The number of values is\n> > > > > > the number of detected faces.\n> > > > > > @@ -273,6 +277,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceScores:\n> > > > > > type: uint8_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Confidence score of each of the detected faces. The range of score is\n> > > > > > [0, 100]. The number of values should be the number of faces reported\n> > > > > > @@ -285,6 +290,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceLandmarks:\n> > > > > > type: Point\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Array of human face landmark coordinates in format [..., left_eye_i,\n> > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The\n> > > > > > @@ -298,6 +304,7 @@ controls:\n> > > > > >\n> > > > > > - FaceDetectFaceIds:\n> > > > > > type: int32_t\n> > > > > > + direction: out\n> > > > > > description: |\n> > > > > > Each detected face is given a unique ID that is valid for as long as the\n> > > > > > face is visible to the camera device. A face that leaves the field of\n> > > > >\n> > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml\n> > > > > as well ?\n> > > >\n> > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out\n> > > > only.\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7F73EC3213\n\tfor ;\n\tWed, 27 Nov 2024 08:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B76F6609F;\n\tWed, 27 Nov 2024 09:12:37 +0100 (CET)","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 EBBE466098\n\tfor ;\n\tWed, 27 Nov 2024 09:12:35 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:7dcc:a4ea:e361:d355])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50FD578C;\n\tWed, 27 Nov 2024 09:12:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bgYza0xi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732695133;\n\tbh=D1MCt0RrURFjHceJ2BdZn2MaIIx8oiZNSWHQhKCT81I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bgYza0xiydFtlzSFDIg6u2TVntUfEbuqk5XAvWZesMLzSMt8Q7CeSlYVzJwF4ONHe\n\trs6XkRKS8Ag1lH1/7nQUf/1K+QWNvyO3onVtk8snNsfNbZbWqDsg/5DaD9tm4PxmHu\n\trfNkhUBd4s4R4cTYstI3mIxBa5ioK7Omb/HqozC0=","Date":"Wed, 27 Nov 2024 17:12:28 +0900","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tNaushir Patuck ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/4] libcamera: controls: Populate direction field in\n\tcontrol definitions","Message-ID":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241125230417.GY19381@pendragon.ideasonboard.com>\n\t\n\t<173263716933.1605529.16246026126566571780@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<173263716933.1605529.16246026126566571780@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]