| Message ID | 20241125153003.3309066-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > In preparation for adding support for querying direction information > from controls, populate the corresponding field in the control ID > defintions. I wish I could save you the usual bikeshedding on names, but... Are 'out' and 'in' the best terms ? Android divides 'controls' (from app to camera) from 'metadata' (from camera to app) and I quite like it (and I think most people in the industry is also accustomed to them, but I agree it makes hard to express 'both' easily, so yeah 'in' 'out' and 'inout' makes sense. How do we document that ? Also, the absence of the field indicates 'inout'. This should be documented, and I wonder if 'inout' is valid as a value if it's implicitly the default. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > src/libcamera/control_ids_draft.yaml | 7 +++++++ > 2 files changed, 19 insertions(+) > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > index d34a2d068b60..c0e91d1852cd 100644 > --- a/src/libcamera/control_ids_core.yaml > +++ b/src/libcamera/control_ids_core.yaml At the beginning of the file we have # Unless otherwise stated, all controls are bi-directional, i.e. they can be # set through Request::controls() and returned out through Request::metadata(). Could you expand it by mentioning 'direction' ? > @@ -17,6 +17,7 @@ controls: > > - AeLocked: > type: bool > + direction: out > description: | > Report the lock status of a running AE algorithm. > > @@ -236,6 +237,7 @@ controls: > > - AeFlickerDetected: > type: int32_t > + direction: out > description: | > Flicker period detected in microseconds. > > @@ -274,6 +276,7 @@ controls: > > - Lux: > type: float > + direction: out > description: | > Report an estimate of the current illuminance level in lux. > > @@ -324,6 +327,7 @@ controls: > > - AwbLocked: > type: bool > + direction: out > description: | > Report the lock status of a running AWB algorithm. > ColourTemperature is missing Report the estimate of the colour temperature for the frame, in kelvin. The ColourTemperature control can only be returned in metadata. > @@ -361,6 +365,7 @@ controls: > > - SensorBlackLevels: > type: int32_t > + direction: out > description: | > Reports the sensor black levels used for processing a frame. > > @@ -385,6 +390,7 @@ controls: > > - FocusFoM: > type: int32_t > + direction: out > description: | > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > @@ -442,6 +448,7 @@ controls: > > - FrameDuration: > type: int64_t > + direction: out > description: | > The instantaneous frame duration from start of frame exposure to start > of next exposure, expressed in microseconds. > @@ -450,6 +457,7 @@ controls: > > - FrameDurationLimits: > type: int64_t > + direction: in I read When reported in metadata, the control expresses the minimum and maximum frame durations used after being clipped to the sensor provided frame duration limits. > description: | > The minimum and maximum (in that order) frame duration, expressed in > microseconds. > @@ -487,6 +495,7 @@ controls: > > - SensorTemperature: > type: float > + direction: out > description: | > Temperature measure from the camera sensor in Celsius. > > @@ -499,6 +508,7 @@ controls: > > - SensorTimestamp: > type: int64_t > + direction: out > description: | > The time when the first row of the image sensor active array is exposed. > > @@ -667,6 +677,7 @@ controls: > > - AfTrigger: > type: int32_t > + direction: in > description: | > Start an autofocus scan. > > @@ -692,6 +703,7 @@ controls: > > - AfPause: > type: int32_t > + direction: in > description: | > Pause lens movements when in continuous autofocus mode. Should AfState be out only ? Also AfPauseState seems to be a metadata only. HdrChannel I read This value is reported back to the application so that it can discover whether this capture corresponds to the short or long exposure image (or any other image used by the HDR procedure). An application can monitor the HDR channel to discover when the differently exposed images have arrived. which suggests me it's a metadata only and DebugMetadataEnable is possibily input only ? > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > index 1b284257f601..59aa6141c25c 100644 > --- a/src/libcamera/control_ids_draft.yaml > +++ b/src/libcamera/control_ids_draft.yaml > @@ -79,6 +79,7 @@ controls: > > - AeState: > type: int32_t > + direction: out > description: | > Control to report the current AE algorithm state. Currently identical to > ANDROID_CONTROL_AE_STATE. > @@ -108,6 +109,7 @@ controls: > > - AwbState: > type: int32_t > + direction: out > description: | > Control to report the current AWB algorithm state. Currently identical > to ANDROID_CONTROL_AWB_STATE. > @@ -129,6 +131,7 @@ controls: > > - SensorRollingShutterSkew: > type: int64_t > + direction: out > description: | > Control to report the time between the start of exposure of the first > row and the start of exposure of the last row. Currently identical to > @@ -262,6 +265,7 @@ controls: > > - FaceDetectFaceRectangles: > type: Rectangle > + direction: out > description: | > Boundary rectangles of the detected faces. The number of values is > the number of detected faces. > @@ -273,6 +277,7 @@ controls: > > - FaceDetectFaceScores: > type: uint8_t > + direction: out > description: | > Confidence score of each of the detected faces. The range of score is > [0, 100]. The number of values should be the number of faces reported > @@ -285,6 +290,7 @@ controls: > > - FaceDetectFaceLandmarks: > type: Point > + direction: out > description: | > Array of human face landmark coordinates in format [..., left_eye_i, > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > @@ -298,6 +304,7 @@ controls: > > - FaceDetectFaceIds: > type: int32_t > + direction: out > description: | > Each detected face is given a unique ID that is valid for as long as the > face is visible to the camera device. A face that leaves the field of Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml as well ? > -- > 2.39.2 >
Hi Jacopo, Thanks for the review. On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > Hi Paul > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > In preparation for adding support for querying direction information > > from controls, populate the corresponding field in the control ID > > defintions. > > I wish I could save you the usual bikeshedding on names, but... > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > app to camera) from 'metadata' (from camera to app) and I quite like > it (and I think most people in the industry is also accustomed to > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > 'out' and 'inout' makes sense. How do we document that ? I personally thought that in and out were less confusing than controls and metadata, mainly because both controls and metadata go in a ControlList, and then we talk about setting controls in metadata that is returned in a ControlList, while we can also set controls in controls. Also I got the idea from doxygen \params[inout] > Also, the absence of the field indicates 'inout'. This should be > documented, and I wonder if 'inout' is valid as a value if it's > implicitly the default. If you want to be explicit you could write it? I thought it was good just to leave as an option. > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > index d34a2d068b60..c0e91d1852cd 100644 > > --- a/src/libcamera/control_ids_core.yaml > > +++ b/src/libcamera/control_ids_core.yaml > > At the beginning of the file we have > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > # set through Request::controls() and returned out through Request::metadata(). > > Could you expand it by mentioning 'direction' ? It's already mentioned: "bi-*direction*al" :) No but seriously, that was what I used to validate my uncertainty about using the word "direction". > > > @@ -17,6 +17,7 @@ controls: > > > > - AeLocked: > > type: bool > > + direction: out > > description: | > > Report the lock status of a running AE algorithm. > > > > @@ -236,6 +237,7 @@ controls: > > > > - AeFlickerDetected: > > type: int32_t > > + direction: out > > description: | > > Flicker period detected in microseconds. > > > > @@ -274,6 +276,7 @@ controls: > > > > - Lux: > > type: float > > + direction: out > > description: | > > Report an estimate of the current illuminance level in lux. > > > > @@ -324,6 +327,7 @@ controls: > > > > - AwbLocked: > > type: bool > > + direction: out > > description: | > > Report the lock status of a running AWB algorithm. > > > > ColourTemperature is missing > > Report the estimate of the colour temperature for the frame, in kelvin. > > The ColourTemperature control can only be returned in metadata. What do you mean, I thought this was settable as of... https://patchwork.libcamera.org/patch/20894/ Oh, it's not merged yet. Ok. > > > > @@ -361,6 +365,7 @@ controls: > > > > - SensorBlackLevels: > > type: int32_t > > + direction: out > > description: | > > Reports the sensor black levels used for processing a frame. > > > > @@ -385,6 +390,7 @@ controls: > > > > - FocusFoM: > > type: int32_t > > + direction: out > > description: | > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > @@ -442,6 +448,7 @@ controls: > > > > - FrameDuration: > > type: int64_t > > + direction: out > > description: | > > The instantaneous frame duration from start of frame exposure to start > > of next exposure, expressed in microseconds. > > @@ -450,6 +457,7 @@ controls: > > > > - FrameDurationLimits: > > type: int64_t > > + direction: in > > I read > > When reported in > metadata, the control expresses the minimum and maximum frame > durations used after being clipped to the sensor provided frame > duration limits. And I missed that. Nice catch. > > > description: | > > The minimum and maximum (in that order) frame duration, expressed in > > microseconds. > > @@ -487,6 +495,7 @@ controls: > > > > - SensorTemperature: > > type: float > > + direction: out > > description: | > > Temperature measure from the camera sensor in Celsius. > > > > @@ -499,6 +508,7 @@ controls: > > > > - SensorTimestamp: > > type: int64_t > > + direction: out > > description: | > > The time when the first row of the image sensor active array is exposed. > > > > @@ -667,6 +677,7 @@ controls: > > > > - AfTrigger: > > type: int32_t > > + direction: in > > description: | > > Start an autofocus scan. > > > > @@ -692,6 +703,7 @@ controls: > > > > - AfPause: > > type: int32_t > > + direction: in > > description: | > > Pause lens movements when in continuous autofocus mode. > > Should AfState be out only ? > Also AfPauseState seems to be a metadata only. They indeed should, yes. > > HdrChannel I read > > This value is reported back to the application so that it can discover > whether this capture corresponds to the short or long exposure image > (or any other image used by the HDR procedure). An application can > monitor the HDR channel to discover when the differently exposed images > have arrived. > > which suggests me it's a metadata only I think you're right. > > and DebugMetadataEnable is possibily input only ? My thought process was that all enable-type controls should be both, as you'd probably want to know if what you've set has actually been set. > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > index 1b284257f601..59aa6141c25c 100644 > > --- a/src/libcamera/control_ids_draft.yaml > > +++ b/src/libcamera/control_ids_draft.yaml > > @@ -79,6 +79,7 @@ controls: > > > > - AeState: > > type: int32_t > > + direction: out > > description: | > > Control to report the current AE algorithm state. Currently identical to > > ANDROID_CONTROL_AE_STATE. > > @@ -108,6 +109,7 @@ controls: > > > > - AwbState: > > type: int32_t > > + direction: out > > description: | > > Control to report the current AWB algorithm state. Currently identical > > to ANDROID_CONTROL_AWB_STATE. > > @@ -129,6 +131,7 @@ controls: > > > > - SensorRollingShutterSkew: > > type: int64_t > > + direction: out > > description: | > > Control to report the time between the start of exposure of the first > > row and the start of exposure of the last row. Currently identical to > > @@ -262,6 +265,7 @@ controls: > > > > - FaceDetectFaceRectangles: > > type: Rectangle > > + direction: out > > description: | > > Boundary rectangles of the detected faces. The number of values is > > the number of detected faces. > > @@ -273,6 +277,7 @@ controls: > > > > - FaceDetectFaceScores: > > type: uint8_t > > + direction: out > > description: | > > Confidence score of each of the detected faces. The range of score is > > [0, 100]. The number of values should be the number of faces reported > > @@ -285,6 +290,7 @@ controls: > > > > - FaceDetectFaceLandmarks: > > type: Point > > + direction: out > > description: | > > Array of human face landmark coordinates in format [..., left_eye_i, > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > @@ -298,6 +304,7 @@ controls: > > > > - FaceDetectFaceIds: > > type: int32_t > > + direction: out > > description: | > > Each detected face is given a unique ID that is valid for as long as the > > face is visible to the camera device. A face that leaves the field of > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > as well ? debug is empty and rpi... I missed Bcm2835StatsOutput should be out only. Thanks, Paul > > > -- > > 2.39.2 > >
On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > In preparation for adding support for querying direction information > > > from controls, populate the corresponding field in the control ID > > > defintions. > > > > I wish I could save you the usual bikeshedding on names, but... > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > app to camera) from 'metadata' (from camera to app) and I quite like > > it (and I think most people in the industry is also accustomed to > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > 'out' and 'inout' makes sense. How do we document that ? > > I personally thought that in and out were less confusing than controls > and metadata, mainly because both controls and metadata go in a > ControlList, and then we talk about setting controls in metadata that is > returned in a ControlList, while we can also set controls in controls. > > Also I got the idea from doxygen \params[inout] If our Control* classes were named using a generic term other than control, then we could use "control" and "metadata". Or if Request::controls() was named something other than "controls". But for now we're using these names, so I think it would just add to the confusion. "in" and "out" are probably not the absolute best but I think they can do for now. > > Also, the absence of the field indicates 'inout'. This should be > > documented, and I wonder if 'inout' is valid as a value if it's > > implicitly the default. > > If you want to be explicit you could write it? I thought it was good > just to leave as an option. The advantage of being explicit is that we can catch missing information easily. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > 2 files changed, 19 insertions(+) > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > index d34a2d068b60..c0e91d1852cd 100644 > > > --- a/src/libcamera/control_ids_core.yaml > > > +++ b/src/libcamera/control_ids_core.yaml > > > > At the beginning of the file we have > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > # set through Request::controls() and returned out through Request::metadata(). > > > > Could you expand it by mentioning 'direction' ? > > It's already mentioned: "bi-*direction*al" :) > > No but seriously, that was what I used to validate my uncertainty about > using the word "direction". > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > - AeLocked: > > > type: bool > > > + direction: out > > > description: | > > > Report the lock status of a running AE algorithm. > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > - AeFlickerDetected: > > > type: int32_t > > > + direction: out > > > description: | > > > Flicker period detected in microseconds. > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > - Lux: > > > type: float > > > + direction: out > > > description: | > > > Report an estimate of the current illuminance level in lux. > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > - AwbLocked: > > > type: bool > > > + direction: out > > > description: | > > > Report the lock status of a running AWB algorithm. > > > > > > > ColourTemperature is missing > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > The ColourTemperature control can only be returned in metadata. > > What do you mean, I thought this was settable as of... > > https://patchwork.libcamera.org/patch/20894/ > > Oh, it's not merged yet. Ok. > > > > @@ -361,6 +365,7 @@ controls: > > > > > > - SensorBlackLevels: > > > type: int32_t > > > + direction: out > > > description: | > > > Reports the sensor black levels used for processing a frame. > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > - FocusFoM: > > > type: int32_t > > > + direction: out > > > description: | > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > - FrameDuration: > > > type: int64_t > > > + direction: out > > > description: | > > > The instantaneous frame duration from start of frame exposure to start > > > of next exposure, expressed in microseconds. > > > @@ -450,6 +457,7 @@ controls: > > > > > > - FrameDurationLimits: > > > type: int64_t > > > + direction: in > > > > I read > > > > When reported in > > metadata, the control expresses the minimum and maximum frame > > durations used after being clipped to the sensor provided frame > > duration limits. > > And I missed that. Nice catch. > > > > description: | > > > The minimum and maximum (in that order) frame duration, expressed in > > > microseconds. > > > @@ -487,6 +495,7 @@ controls: > > > > > > - SensorTemperature: > > > type: float > > > + direction: out > > > description: | > > > Temperature measure from the camera sensor in Celsius. > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > - SensorTimestamp: > > > type: int64_t > > > + direction: out > > > description: | > > > The time when the first row of the image sensor active array is exposed. > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > - AfTrigger: > > > type: int32_t > > > + direction: in > > > description: | > > > Start an autofocus scan. > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > - AfPause: > > > type: int32_t > > > + direction: in > > > description: | > > > Pause lens movements when in continuous autofocus mode. > > > > Should AfState be out only ? > > Also AfPauseState seems to be a metadata only. > > They indeed should, yes. > > > > > HdrChannel I read > > > > This value is reported back to the application so that it can discover > > whether this capture corresponds to the short or long exposure image > > (or any other image used by the HDR procedure). An application can > > monitor the HDR channel to discover when the differently exposed images > > have arrived. > > > > which suggests me it's a metadata only > > I think you're right. > > > > > and DebugMetadataEnable is possibily input only ? > > My thought process was that all enable-type controls should be both, as > you'd probably want to know if what you've set has actually been set. > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > index 1b284257f601..59aa6141c25c 100644 > > > --- a/src/libcamera/control_ids_draft.yaml > > > +++ b/src/libcamera/control_ids_draft.yaml > > > @@ -79,6 +79,7 @@ controls: > > > > > > - AeState: > > > type: int32_t > > > + direction: out > > > description: | > > > Control to report the current AE algorithm state. Currently identical to > > > ANDROID_CONTROL_AE_STATE. > > > @@ -108,6 +109,7 @@ controls: > > > > > > - AwbState: > > > type: int32_t > > > + direction: out > > > description: | > > > Control to report the current AWB algorithm state. Currently identical > > > to ANDROID_CONTROL_AWB_STATE. > > > @@ -129,6 +131,7 @@ controls: > > > > > > - SensorRollingShutterSkew: > > > type: int64_t > > > + direction: out > > > description: | > > > Control to report the time between the start of exposure of the first > > > row and the start of exposure of the last row. Currently identical to > > > @@ -262,6 +265,7 @@ controls: > > > > > > - FaceDetectFaceRectangles: > > > type: Rectangle > > > + direction: out > > > description: | > > > Boundary rectangles of the detected faces. The number of values is > > > the number of detected faces. > > > @@ -273,6 +277,7 @@ controls: > > > > > > - FaceDetectFaceScores: > > > type: uint8_t > > > + direction: out > > > description: | > > > Confidence score of each of the detected faces. The range of score is > > > [0, 100]. The number of values should be the number of faces reported > > > @@ -285,6 +290,7 @@ controls: > > > > > > - FaceDetectFaceLandmarks: > > > type: Point > > > + direction: out > > > description: | > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > @@ -298,6 +304,7 @@ controls: > > > > > > - FaceDetectFaceIds: > > > type: int32_t > > > + direction: out > > > description: | > > > Each detected face is given a unique ID that is valid for as long as the > > > face is visible to the camera device. A face that leaves the field of > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > as well ? > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > only.
On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > In preparation for adding support for querying direction information > > > > from controls, populate the corresponding field in the control ID > > > > defintions. > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > it (and I think most people in the industry is also accustomed to > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > 'out' and 'inout' makes sense. How do we document that ? > > > > I personally thought that in and out were less confusing than controls > > and metadata, mainly because both controls and metadata go in a > > ControlList, and then we talk about setting controls in metadata that is > > returned in a ControlList, while we can also set controls in controls. > > > > Also I got the idea from doxygen \params[inout] > > If our Control* classes were named using a generic term other than > control, then we could use "control" and "metadata". Or if > Request::controls() was named something other than "controls". But for > now we're using these names, so I think it would just add to the > confusion. "in" and "out" are probably not the absolute best but I think > they can do for now. > > > > Also, the absence of the field indicates 'inout'. This should be > > > documented, and I wonder if 'inout' is valid as a value if it's > > > implicitly the default. > > > > If you want to be explicit you could write it? I thought it was good > > just to leave as an option. > > The advantage of being explicit is that we can catch missing information > easily. To be honest, I am not sure about "in" and "out" either. I have only a slight preference of using "read" and "write" instead, but I'm not convinced it's much better. Naush > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > 2 files changed, 19 insertions(+) > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > --- a/src/libcamera/control_ids_core.yaml > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > At the beginning of the file we have > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > Could you expand it by mentioning 'direction' ? > > > > It's already mentioned: "bi-*direction*al" :) > > > > No but seriously, that was what I used to validate my uncertainty about > > using the word "direction". > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > - AeLocked: > > > > type: bool > > > > + direction: out > > > > description: | > > > > Report the lock status of a running AE algorithm. > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > - AeFlickerDetected: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Flicker period detected in microseconds. > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > - Lux: > > > > type: float > > > > + direction: out > > > > description: | > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > - AwbLocked: > > > > type: bool > > > > + direction: out > > > > description: | > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > ColourTemperature is missing > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > The ColourTemperature control can only be returned in metadata. > > > > What do you mean, I thought this was settable as of... > > > > https://patchwork.libcamera.org/patch/20894/ > > > > Oh, it's not merged yet. Ok. > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > - SensorBlackLevels: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > - FocusFoM: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > - FrameDuration: > > > > type: int64_t > > > > + direction: out > > > > description: | > > > > The instantaneous frame duration from start of frame exposure to start > > > > of next exposure, expressed in microseconds. > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > - FrameDurationLimits: > > > > type: int64_t > > > > + direction: in > > > > > > I read > > > > > > When reported in > > > metadata, the control expresses the minimum and maximum frame > > > durations used after being clipped to the sensor provided frame > > > duration limits. > > > > And I missed that. Nice catch. > > > > > > description: | > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > microseconds. > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > - SensorTemperature: > > > > type: float > > > > + direction: out > > > > description: | > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > - SensorTimestamp: > > > > type: int64_t > > > > + direction: out > > > > description: | > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > - AfTrigger: > > > > type: int32_t > > > > + direction: in > > > > description: | > > > > Start an autofocus scan. > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > - AfPause: > > > > type: int32_t > > > > + direction: in > > > > description: | > > > > Pause lens movements when in continuous autofocus mode. > > > > > > Should AfState be out only ? > > > Also AfPauseState seems to be a metadata only. > > > > They indeed should, yes. > > > > > > > > HdrChannel I read > > > > > > This value is reported back to the application so that it can discover > > > whether this capture corresponds to the short or long exposure image > > > (or any other image used by the HDR procedure). An application can > > > monitor the HDR channel to discover when the differently exposed images > > > have arrived. > > > > > > which suggests me it's a metadata only > > > > I think you're right. > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > My thought process was that all enable-type controls should be both, as > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > index 1b284257f601..59aa6141c25c 100644 > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > - AeState: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Control to report the current AE algorithm state. Currently identical to > > > > ANDROID_CONTROL_AE_STATE. > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > - AwbState: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Control to report the current AWB algorithm state. Currently identical > > > > to ANDROID_CONTROL_AWB_STATE. > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > - SensorRollingShutterSkew: > > > > type: int64_t > > > > + direction: out > > > > description: | > > > > Control to report the time between the start of exposure of the first > > > > row and the start of exposure of the last row. Currently identical to > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > - FaceDetectFaceRectangles: > > > > type: Rectangle > > > > + direction: out > > > > description: | > > > > Boundary rectangles of the detected faces. The number of values is > > > > the number of detected faces. > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > - FaceDetectFaceScores: > > > > type: uint8_t > > > > + direction: out > > > > description: | > > > > Confidence score of each of the detected faces. The range of score is > > > > [0, 100]. The number of values should be the number of faces reported > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > - FaceDetectFaceLandmarks: > > > > type: Point > > > > + direction: out > > > > description: | > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > - FaceDetectFaceIds: > > > > type: int32_t > > > > + direction: out > > > > description: | > > > > Each detected face is given a unique ID that is valid for as long as the > > > > face is visible to the camera device. A face that leaves the field of > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > as well ? > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > only. > > -- > Regards, > > Laurent Pinchart
Hi Naush On Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote: > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > > In preparation for adding support for querying direction information > > > > > from controls, populate the corresponding field in the control ID > > > > > defintions. > > > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > > it (and I think most people in the industry is also accustomed to > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > > 'out' and 'inout' makes sense. How do we document that ? > > > > > > I personally thought that in and out were less confusing than controls > > > and metadata, mainly because both controls and metadata go in a > > > ControlList, and then we talk about setting controls in metadata that is > > > returned in a ControlList, while we can also set controls in controls. > > > > > > Also I got the idea from doxygen \params[inout] > > > > If our Control* classes were named using a generic term other than > > control, then we could use "control" and "metadata". Or if > > Request::controls() was named something other than "controls". But for > > now we're using these names, so I think it would just add to the > > confusion. "in" and "out" are probably not the absolute best but I think > > they can do for now. > > > > > > Also, the absence of the field indicates 'inout'. This should be > > > > documented, and I wonder if 'inout' is valid as a value if it's > > > > implicitly the default. > > > > > > If you want to be explicit you could write it? I thought it was good > > > just to leave as an option. > > > > The advantage of being explicit is that we can catch missing information > > easily. > > To be honest, I am not sure about "in" and "out" either. I have only > a slight preference of using "read" and "write" instead, but I'm not > convinced it's much better. What matter the most, and I forgot to mention it in the reply, is that we document the direction. In/write means "from applications to the camera" Out/read means "from the camera to the application" (if I got your suggested usage of read/write correctly) If I'm not mistaken I haven't seen this being clearly specified, and I think it's worth it as for application developer 'in' could potentialy mean "it comes from the camera" Thanks j > > Naush > > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > --- > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > > --- a/src/libcamera/control_ids_core.yaml > > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > > > At the beginning of the file we have > > > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > > > Could you expand it by mentioning 'direction' ? > > > > > > It's already mentioned: "bi-*direction*al" :) > > > > > > No but seriously, that was what I used to validate my uncertainty about > > > using the word "direction". > > > > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > > > - AeLocked: > > > > > type: bool > > > > > + direction: out > > > > > description: | > > > > > Report the lock status of a running AE algorithm. > > > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > > > - AeFlickerDetected: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Flicker period detected in microseconds. > > > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > > > - Lux: > > > > > type: float > > > > > + direction: out > > > > > description: | > > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > > > - AwbLocked: > > > > > type: bool > > > > > + direction: out > > > > > description: | > > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > > > > ColourTemperature is missing > > > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > > > The ColourTemperature control can only be returned in metadata. > > > > > > What do you mean, I thought this was settable as of... > > > > > > https://patchwork.libcamera.org/patch/20894/ > > > > > > Oh, it's not merged yet. Ok. > > > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > > > - SensorBlackLevels: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > > > - FocusFoM: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > > > - FrameDuration: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > The instantaneous frame duration from start of frame exposure to start > > > > > of next exposure, expressed in microseconds. > > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > > > - FrameDurationLimits: > > > > > type: int64_t > > > > > + direction: in > > > > > > > > I read > > > > > > > > When reported in > > > > metadata, the control expresses the minimum and maximum frame > > > > durations used after being clipped to the sensor provided frame > > > > duration limits. > > > > > > And I missed that. Nice catch. > > > > > > > > description: | > > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > > microseconds. > > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > > > - SensorTemperature: > > > > > type: float > > > > > + direction: out > > > > > description: | > > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > > > - SensorTimestamp: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > > > - AfTrigger: > > > > > type: int32_t > > > > > + direction: in > > > > > description: | > > > > > Start an autofocus scan. > > > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > > > - AfPause: > > > > > type: int32_t > > > > > + direction: in > > > > > description: | > > > > > Pause lens movements when in continuous autofocus mode. > > > > > > > > Should AfState be out only ? > > > > Also AfPauseState seems to be a metadata only. > > > > > > They indeed should, yes. > > > > > > > > > > > HdrChannel I read > > > > > > > > This value is reported back to the application so that it can discover > > > > whether this capture corresponds to the short or long exposure image > > > > (or any other image used by the HDR procedure). An application can > > > > monitor the HDR channel to discover when the differently exposed images > > > > have arrived. > > > > > > > > which suggests me it's a metadata only > > > > > > I think you're right. > > > > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > > > My thought process was that all enable-type controls should be both, as > > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > > index 1b284257f601..59aa6141c25c 100644 > > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > > > - AeState: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the current AE algorithm state. Currently identical to > > > > > ANDROID_CONTROL_AE_STATE. > > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > > > - AwbState: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the current AWB algorithm state. Currently identical > > > > > to ANDROID_CONTROL_AWB_STATE. > > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > > > - SensorRollingShutterSkew: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the time between the start of exposure of the first > > > > > row and the start of exposure of the last row. Currently identical to > > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > > > - FaceDetectFaceRectangles: > > > > > type: Rectangle > > > > > + direction: out > > > > > description: | > > > > > Boundary rectangles of the detected faces. The number of values is > > > > > the number of detected faces. > > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > > > - FaceDetectFaceScores: > > > > > type: uint8_t > > > > > + direction: out > > > > > description: | > > > > > Confidence score of each of the detected faces. The range of score is > > > > > [0, 100]. The number of values should be the number of faces reported > > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > > > - FaceDetectFaceLandmarks: > > > > > type: Point > > > > > + direction: out > > > > > description: | > > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > > > - FaceDetectFaceIds: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Each detected face is given a unique ID that is valid for as long as the > > > > > face is visible to the camera device. A face that leaves the field of > > > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > > as well ? > > > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > > only. > > > > -- > > Regards, > > > > Laurent Pinchart
Quoting Naushir Patuck (2024-11-26 15:58:03) > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > > In preparation for adding support for querying direction information > > > > > from controls, populate the corresponding field in the control ID > > > > > defintions. > > > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > > it (and I think most people in the industry is also accustomed to > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > > 'out' and 'inout' makes sense. How do we document that ? > > > > > > I personally thought that in and out were less confusing than controls > > > and metadata, mainly because both controls and metadata go in a > > > ControlList, and then we talk about setting controls in metadata that is > > > returned in a ControlList, while we can also set controls in controls. > > > > > > Also I got the idea from doxygen \params[inout] > > > > If our Control* classes were named using a generic term other than > > control, then we could use "control" and "metadata". Or if > > Request::controls() was named something other than "controls". But for > > now we're using these names, so I think it would just add to the > > confusion. "in" and "out" are probably not the absolute best but I think > > they can do for now. > > > > > > Also, the absence of the field indicates 'inout'. This should be > > > > documented, and I wonder if 'inout' is valid as a value if it's > > > > implicitly the default. > > > > > > If you want to be explicit you could write it? I thought it was good > > > just to leave as an option. > > > > The advantage of being explicit is that we can catch missing information > > easily. > > To be honest, I am not sure about "in" and "out" either. I have only > a slight preference of using "read" and "write" instead, but I'm not > convinced it's much better. As there's a shed to be painted... I think I would be in the R/W camp here too Properties and Metadata are R only Controls are W or potentially RW ...? Otherwise direction feels odd to me. Which direction is it framed from ? Are metadata 'in' to the applciation or 'out' from libcamera ? (Yes, the documentation would say that), while a Read/Write 'action' conveys the direction is from the actor... -- Kieran > > Naush > > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > --- > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > > --- a/src/libcamera/control_ids_core.yaml > > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > > > At the beginning of the file we have > > > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > > > Could you expand it by mentioning 'direction' ? > > > > > > It's already mentioned: "bi-*direction*al" :) > > > > > > No but seriously, that was what I used to validate my uncertainty about > > > using the word "direction". > > > > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > > > - AeLocked: > > > > > type: bool > > > > > + direction: out > > > > > description: | > > > > > Report the lock status of a running AE algorithm. > > > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > > > - AeFlickerDetected: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Flicker period detected in microseconds. > > > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > > > - Lux: > > > > > type: float > > > > > + direction: out > > > > > description: | > > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > > > - AwbLocked: > > > > > type: bool > > > > > + direction: out > > > > > description: | > > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > > > > ColourTemperature is missing > > > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > > > The ColourTemperature control can only be returned in metadata. > > > > > > What do you mean, I thought this was settable as of... > > > > > > https://patchwork.libcamera.org/patch/20894/ > > > > > > Oh, it's not merged yet. Ok. > > > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > > > - SensorBlackLevels: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > > > - FocusFoM: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > > > - FrameDuration: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > The instantaneous frame duration from start of frame exposure to start > > > > > of next exposure, expressed in microseconds. > > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > > > - FrameDurationLimits: > > > > > type: int64_t > > > > > + direction: in > > > > > > > > I read > > > > > > > > When reported in > > > > metadata, the control expresses the minimum and maximum frame > > > > durations used after being clipped to the sensor provided frame > > > > duration limits. > > > > > > And I missed that. Nice catch. > > > > > > > > description: | > > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > > microseconds. > > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > > > - SensorTemperature: > > > > > type: float > > > > > + direction: out > > > > > description: | > > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > > > - SensorTimestamp: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > > > - AfTrigger: > > > > > type: int32_t > > > > > + direction: in > > > > > description: | > > > > > Start an autofocus scan. > > > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > > > - AfPause: > > > > > type: int32_t > > > > > + direction: in > > > > > description: | > > > > > Pause lens movements when in continuous autofocus mode. > > > > > > > > Should AfState be out only ? > > > > Also AfPauseState seems to be a metadata only. > > > > > > They indeed should, yes. > > > > > > > > > > > HdrChannel I read > > > > > > > > This value is reported back to the application so that it can discover > > > > whether this capture corresponds to the short or long exposure image > > > > (or any other image used by the HDR procedure). An application can > > > > monitor the HDR channel to discover when the differently exposed images > > > > have arrived. > > > > > > > > which suggests me it's a metadata only > > > > > > I think you're right. > > > > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > > > My thought process was that all enable-type controls should be both, as > > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > > index 1b284257f601..59aa6141c25c 100644 > > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > > > - AeState: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the current AE algorithm state. Currently identical to > > > > > ANDROID_CONTROL_AE_STATE. > > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > > > - AwbState: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the current AWB algorithm state. Currently identical > > > > > to ANDROID_CONTROL_AWB_STATE. > > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > > > - SensorRollingShutterSkew: > > > > > type: int64_t > > > > > + direction: out > > > > > description: | > > > > > Control to report the time between the start of exposure of the first > > > > > row and the start of exposure of the last row. Currently identical to > > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > > > - FaceDetectFaceRectangles: > > > > > type: Rectangle > > > > > + direction: out > > > > > description: | > > > > > Boundary rectangles of the detected faces. The number of values is > > > > > the number of detected faces. > > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > > > - FaceDetectFaceScores: > > > > > type: uint8_t > > > > > + direction: out > > > > > description: | > > > > > Confidence score of each of the detected faces. The range of score is > > > > > [0, 100]. The number of values should be the number of faces reported > > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > > > - FaceDetectFaceLandmarks: > > > > > type: Point > > > > > + direction: out > > > > > description: | > > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > > > - FaceDetectFaceIds: > > > > > type: int32_t > > > > > + direction: out > > > > > description: | > > > > > Each detected face is given a unique ID that is valid for as long as the > > > > > face is visible to the camera device. A face that leaves the field of > > > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > > as well ? > > > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > > only. > > > > -- > > Regards, > > > > Laurent Pinchart
Hi Jacopo, On Tue, 26 Nov 2024 at 16:04, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote: > > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > > > In preparation for adding support for querying direction information > > > > > > from controls, populate the corresponding field in the control ID > > > > > > defintions. > > > > > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > > > it (and I think most people in the industry is also accustomed to > > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > > > 'out' and 'inout' makes sense. How do we document that ? > > > > > > > > I personally thought that in and out were less confusing than controls > > > > and metadata, mainly because both controls and metadata go in a > > > > ControlList, and then we talk about setting controls in metadata that is > > > > returned in a ControlList, while we can also set controls in controls. > > > > > > > > Also I got the idea from doxygen \params[inout] > > > > > > If our Control* classes were named using a generic term other than > > > control, then we could use "control" and "metadata". Or if > > > Request::controls() was named something other than "controls". But for > > > now we're using these names, so I think it would just add to the > > > confusion. "in" and "out" are probably not the absolute best but I think > > > they can do for now. > > > > > > > > Also, the absence of the field indicates 'inout'. This should be > > > > > documented, and I wonder if 'inout' is valid as a value if it's > > > > > implicitly the default. > > > > > > > > If you want to be explicit you could write it? I thought it was good > > > > just to leave as an option. > > > > > > The advantage of being explicit is that we can catch missing information > > > easily. > > > > To be honest, I am not sure about "in" and "out" either. I have only > > a slight preference of using "read" and "write" instead, but I'm not > > convinced it's much better. > > What matter the most, and I forgot to mention it in the reply, is that > we document the direction. > > In/write means "from applications to the camera" > Out/read means "from the camera to the application" > > (if I got your suggested usage of read/write correctly) That's correct, my read/write is from the reference of the application developer. Naush > > If I'm not mistaken I haven't seen this being clearly specified, and I > think it's worth it as for application developer 'in' could potentialy > mean "it comes from the camera" > > Thanks > j > > > > > Naush > > > > > > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > > > --- a/src/libcamera/control_ids_core.yaml > > > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > > > > > At the beginning of the file we have > > > > > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > > > > > Could you expand it by mentioning 'direction' ? > > > > > > > > It's already mentioned: "bi-*direction*al" :) > > > > > > > > No but seriously, that was what I used to validate my uncertainty about > > > > using the word "direction". > > > > > > > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > > > > > - AeLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AE algorithm. > > > > > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > > > > > - AeFlickerDetected: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Flicker period detected in microseconds. > > > > > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > > > > > - Lux: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > > > > > - AwbLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > > > > > > > ColourTemperature is missing > > > > > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > > > > > The ColourTemperature control can only be returned in metadata. > > > > > > > > What do you mean, I thought this was settable as of... > > > > > > > > https://patchwork.libcamera.org/patch/20894/ > > > > > > > > Oh, it's not merged yet. Ok. > > > > > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > > > > > - SensorBlackLevels: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > > > > > - FocusFoM: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > > > > > - FrameDuration: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The instantaneous frame duration from start of frame exposure to start > > > > > > of next exposure, expressed in microseconds. > > > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > > > > > - FrameDurationLimits: > > > > > > type: int64_t > > > > > > + direction: in > > > > > > > > > > I read > > > > > > > > > > When reported in > > > > > metadata, the control expresses the minimum and maximum frame > > > > > durations used after being clipped to the sensor provided frame > > > > > duration limits. > > > > > > > > And I missed that. Nice catch. > > > > > > > > > > description: | > > > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > > > microseconds. > > > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > > > > > - SensorTemperature: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > > > > > - SensorTimestamp: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > > > > > - AfTrigger: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Start an autofocus scan. > > > > > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > > > > > - AfPause: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Pause lens movements when in continuous autofocus mode. > > > > > > > > > > Should AfState be out only ? > > > > > Also AfPauseState seems to be a metadata only. > > > > > > > > They indeed should, yes. > > > > > > > > > > > > > > HdrChannel I read > > > > > > > > > > This value is reported back to the application so that it can discover > > > > > whether this capture corresponds to the short or long exposure image > > > > > (or any other image used by the HDR procedure). An application can > > > > > monitor the HDR channel to discover when the differently exposed images > > > > > have arrived. > > > > > > > > > > which suggests me it's a metadata only > > > > > > > > I think you're right. > > > > > > > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > > > > > My thought process was that all enable-type controls should be both, as > > > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > > > index 1b284257f601..59aa6141c25c 100644 > > > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > > > > > - AeState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AE algorithm state. Currently identical to > > > > > > ANDROID_CONTROL_AE_STATE. > > > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > > > > > - AwbState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AWB algorithm state. Currently identical > > > > > > to ANDROID_CONTROL_AWB_STATE. > > > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > > > > > - SensorRollingShutterSkew: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the time between the start of exposure of the first > > > > > > row and the start of exposure of the last row. Currently identical to > > > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceRectangles: > > > > > > type: Rectangle > > > > > > + direction: out > > > > > > description: | > > > > > > Boundary rectangles of the detected faces. The number of values is > > > > > > the number of detected faces. > > > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceScores: > > > > > > type: uint8_t > > > > > > + direction: out > > > > > > description: | > > > > > > Confidence score of each of the detected faces. The range of score is > > > > > > [0, 100]. The number of values should be the number of faces reported > > > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceLandmarks: > > > > > > type: Point > > > > > > + direction: out > > > > > > description: | > > > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceIds: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Each detected face is given a unique ID that is valid for as long as the > > > > > > face is visible to the camera device. A face that leaves the field of > > > > > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > > > as well ? > > > > > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > > > only. > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
On Tue, Nov 26, 2024 at 05:04:47PM +0100, Jacopo Mondi wrote: > On Tue, Nov 26, 2024 at 03:58:03PM +0000, Naushir Patuck wrote: > > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart wrote: > > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > > > In preparation for adding support for querying direction information > > > > > > from controls, populate the corresponding field in the control ID > > > > > > defintions. > > > > > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > > > it (and I think most people in the industry is also accustomed to > > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > > > 'out' and 'inout' makes sense. How do we document that ? > > > > > > > > I personally thought that in and out were less confusing than controls > > > > and metadata, mainly because both controls and metadata go in a > > > > ControlList, and then we talk about setting controls in metadata that is > > > > returned in a ControlList, while we can also set controls in controls. > > > > > > > > Also I got the idea from doxygen \params[inout] > > > > > > If our Control* classes were named using a generic term other than > > > control, then we could use "control" and "metadata". Or if > > > Request::controls() was named something other than "controls". But for > > > now we're using these names, so I think it would just add to the > > > confusion. "in" and "out" are probably not the absolute best but I think > > > they can do for now. > > > > > > > > Also, the absence of the field indicates 'inout'. This should be > > > > > documented, and I wonder if 'inout' is valid as a value if it's > > > > > implicitly the default. > > > > > > > > If you want to be explicit you could write it? I thought it was good > > > > just to leave as an option. > > > > > > The advantage of being explicit is that we can catch missing information > > > easily. > > > > To be honest, I am not sure about "in" and "out" either. I have only > > a slight preference of using "read" and "write" instead, but I'm not > > convinced it's much better. > > What matter the most, and I forgot to mention it in the reply, is that > we document the direction. > > In/write means "from applications to the camera" > Out/read means "from the camera to the application" > > (if I got your suggested usage of read/write correctly) > > If I'm not mistaken I haven't seen this being clearly specified, and I > think it's worth it as for application developer 'in' could potentialy > mean "it comes from the camera" I've never thought it could be misconstrued like that. I may of course be biased. At least we haven't gone the V4L2 way with output/capture :-) Jokes aside, we constantly and consistently talk about camera outputting streams and taking requests as inputs, so I think the in/out vocabulary is well established. Read/write carries a similar possibility of misinterpretation: a camera writes frames, but the metadata would be considered as being "read". > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > > > --- a/src/libcamera/control_ids_core.yaml > > > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > > > > > At the beginning of the file we have > > > > > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > > > > > Could you expand it by mentioning 'direction' ? > > > > > > > > It's already mentioned: "bi-*direction*al" :) > > > > > > > > No but seriously, that was what I used to validate my uncertainty about > > > > using the word "direction". > > > > > > > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > > > > > - AeLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AE algorithm. > > > > > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > > > > > - AeFlickerDetected: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Flicker period detected in microseconds. > > > > > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > > > > > - Lux: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > > > > > - AwbLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > > > > > > > ColourTemperature is missing > > > > > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > > > > > The ColourTemperature control can only be returned in metadata. > > > > > > > > What do you mean, I thought this was settable as of... > > > > > > > > https://patchwork.libcamera.org/patch/20894/ > > > > > > > > Oh, it's not merged yet. Ok. > > > > > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > > > > > - SensorBlackLevels: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > > > > > - FocusFoM: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > > > > > - FrameDuration: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The instantaneous frame duration from start of frame exposure to start > > > > > > of next exposure, expressed in microseconds. > > > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > > > > > - FrameDurationLimits: > > > > > > type: int64_t > > > > > > + direction: in > > > > > > > > > > I read > > > > > > > > > > When reported in > > > > > metadata, the control expresses the minimum and maximum frame > > > > > durations used after being clipped to the sensor provided frame > > > > > duration limits. > > > > > > > > And I missed that. Nice catch. > > > > > > > > > > description: | > > > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > > > microseconds. > > > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > > > > > - SensorTemperature: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > > > > > - SensorTimestamp: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > > > > > - AfTrigger: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Start an autofocus scan. > > > > > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > > > > > - AfPause: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Pause lens movements when in continuous autofocus mode. > > > > > > > > > > Should AfState be out only ? > > > > > Also AfPauseState seems to be a metadata only. > > > > > > > > They indeed should, yes. > > > > > > > > > > > > > > HdrChannel I read > > > > > > > > > > This value is reported back to the application so that it can discover > > > > > whether this capture corresponds to the short or long exposure image > > > > > (or any other image used by the HDR procedure). An application can > > > > > monitor the HDR channel to discover when the differently exposed images > > > > > have arrived. > > > > > > > > > > which suggests me it's a metadata only > > > > > > > > I think you're right. > > > > > > > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > > > > > My thought process was that all enable-type controls should be both, as > > > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > > > index 1b284257f601..59aa6141c25c 100644 > > > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > > > > > - AeState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AE algorithm state. Currently identical to > > > > > > ANDROID_CONTROL_AE_STATE. > > > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > > > > > - AwbState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AWB algorithm state. Currently identical > > > > > > to ANDROID_CONTROL_AWB_STATE. > > > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > > > > > - SensorRollingShutterSkew: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the time between the start of exposure of the first > > > > > > row and the start of exposure of the last row. Currently identical to > > > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceRectangles: > > > > > > type: Rectangle > > > > > > + direction: out > > > > > > description: | > > > > > > Boundary rectangles of the detected faces. The number of values is > > > > > > the number of detected faces. > > > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceScores: > > > > > > type: uint8_t > > > > > > + direction: out > > > > > > description: | > > > > > > Confidence score of each of the detected faces. The range of score is > > > > > > [0, 100]. The number of values should be the number of faces reported > > > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceLandmarks: > > > > > > type: Point > > > > > > + direction: out > > > > > > description: | > > > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceIds: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Each detected face is given a unique ID that is valid for as long as the > > > > > > face is visible to the camera device. A face that leaves the field of > > > > > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > > > as well ? > > > > > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > > > only.
On Tue, Nov 26, 2024 at 04:06:09PM +0000, Kieran Bingham wrote: > Quoting Naushir Patuck (2024-11-26 15:58:03) > > On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > On Tue, Nov 26, 2024 at 05:11:17AM +0900, Paul Elder wrote: > > > > On Mon, Nov 25, 2024 at 08:57:55PM +0100, Jacopo Mondi wrote: > > > > > On Tue, Nov 26, 2024 at 12:30:00AM +0900, Paul Elder wrote: > > > > > > In preparation for adding support for querying direction information > > > > > > from controls, populate the corresponding field in the control ID > > > > > > defintions. > > > > > > > > > > I wish I could save you the usual bikeshedding on names, but... > > > > > > > > > > Are 'out' and 'in' the best terms ? Android divides 'controls' (from > > > > > app to camera) from 'metadata' (from camera to app) and I quite like > > > > > it (and I think most people in the industry is also accustomed to > > > > > them, but I agree it makes hard to express 'both' easily, so yeah 'in' > > > > > 'out' and 'inout' makes sense. How do we document that ? > > > > > > > > I personally thought that in and out were less confusing than controls > > > > and metadata, mainly because both controls and metadata go in a > > > > ControlList, and then we talk about setting controls in metadata that is > > > > returned in a ControlList, while we can also set controls in controls. > > > > > > > > Also I got the idea from doxygen \params[inout] > > > > > > If our Control* classes were named using a generic term other than > > > control, then we could use "control" and "metadata". Or if > > > Request::controls() was named something other than "controls". But for > > > now we're using these names, so I think it would just add to the > > > confusion. "in" and "out" are probably not the absolute best but I think > > > they can do for now. > > > > > > > > Also, the absence of the field indicates 'inout'. This should be > > > > > documented, and I wonder if 'inout' is valid as a value if it's > > > > > implicitly the default. > > > > > > > > If you want to be explicit you could write it? I thought it was good > > > > just to leave as an option. > > > > > > The advantage of being explicit is that we can catch missing information > > > easily. > > > > To be honest, I am not sure about "in" and "out" either. I have only > > a slight preference of using "read" and "write" instead, but I'm not > > convinced it's much better. > > As there's a shed to be painted... > > I think I would be in the R/W camp here too > > Properties and Metadata are R only > Controls are W or potentially RW ...? > > Otherwise direction feels odd to me. Which direction is it framed from ? > Are metadata 'in' to the applciation or 'out' from libcamera ? (Yes, the > documentation would say that), while a Read/Write 'action' conveys the > direction is from the actor... imo input and output are unambiguous. I'm having a hard time imagining, analogously, a function caller and callee having an argument on what is considered the input and output. The application passes input controls to the camera, and the camera receives input controls. Reading and writing on the other hand depend on your frame of reference (pun intended). I suppose that regardless it should indeed be documented for clarity... I'm having trouble figuring out where though, since application developers aren't going to be checking the comments in control_ids_core.yaml. I found this really nice comment in application-developer.rst: .. TODO: A request can also have controls or parameters that you can apply to the image. So I think we're missing guide-level documentation completely on how to use controls and metadata...? git grep ControlList | grep Documentation only returns ipa and pipeline handler developer guides. Paul > > > > > > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > > > > src/libcamera/control_ids_core.yaml | 12 ++++++++++++ > > > > > > src/libcamera/control_ids_draft.yaml | 7 +++++++ > > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml > > > > > > index d34a2d068b60..c0e91d1852cd 100644 > > > > > > --- a/src/libcamera/control_ids_core.yaml > > > > > > +++ b/src/libcamera/control_ids_core.yaml > > > > > > > > > > At the beginning of the file we have > > > > > > > > > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > > > > # set through Request::controls() and returned out through Request::metadata(). > > > > > > > > > > Could you expand it by mentioning 'direction' ? > > > > > > > > It's already mentioned: "bi-*direction*al" :) > > > > > > > > No but seriously, that was what I used to validate my uncertainty about > > > > using the word "direction". > > > > > > > > > > > > > > > @@ -17,6 +17,7 @@ controls: > > > > > > > > > > > > - AeLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AE algorithm. > > > > > > > > > > > > @@ -236,6 +237,7 @@ controls: > > > > > > > > > > > > - AeFlickerDetected: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Flicker period detected in microseconds. > > > > > > > > > > > > @@ -274,6 +276,7 @@ controls: > > > > > > > > > > > > - Lux: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Report an estimate of the current illuminance level in lux. > > > > > > > > > > > > @@ -324,6 +327,7 @@ controls: > > > > > > > > > > > > - AwbLocked: > > > > > > type: bool > > > > > > + direction: out > > > > > > description: | > > > > > > Report the lock status of a running AWB algorithm. > > > > > > > > > > > > > > > > ColourTemperature is missing > > > > > > > > > > Report the estimate of the colour temperature for the frame, in kelvin. > > > > > > > > > > The ColourTemperature control can only be returned in metadata. > > > > > > > > What do you mean, I thought this was settable as of... > > > > > > > > https://patchwork.libcamera.org/patch/20894/ > > > > > > > > Oh, it's not merged yet. Ok. > > > > > > > > > > @@ -361,6 +365,7 @@ controls: > > > > > > > > > > > > - SensorBlackLevels: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports the sensor black levels used for processing a frame. > > > > > > > > > > > > @@ -385,6 +390,7 @@ controls: > > > > > > > > > > > > - FocusFoM: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. > > > > > > > > > > > > @@ -442,6 +448,7 @@ controls: > > > > > > > > > > > > - FrameDuration: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The instantaneous frame duration from start of frame exposure to start > > > > > > of next exposure, expressed in microseconds. > > > > > > @@ -450,6 +457,7 @@ controls: > > > > > > > > > > > > - FrameDurationLimits: > > > > > > type: int64_t > > > > > > + direction: in > > > > > > > > > > I read > > > > > > > > > > When reported in > > > > > metadata, the control expresses the minimum and maximum frame > > > > > durations used after being clipped to the sensor provided frame > > > > > duration limits. > > > > > > > > And I missed that. Nice catch. > > > > > > > > > > description: | > > > > > > The minimum and maximum (in that order) frame duration, expressed in > > > > > > microseconds. > > > > > > @@ -487,6 +495,7 @@ controls: > > > > > > > > > > > > - SensorTemperature: > > > > > > type: float > > > > > > + direction: out > > > > > > description: | > > > > > > Temperature measure from the camera sensor in Celsius. > > > > > > > > > > > > @@ -499,6 +508,7 @@ controls: > > > > > > > > > > > > - SensorTimestamp: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > The time when the first row of the image sensor active array is exposed. > > > > > > > > > > > > @@ -667,6 +677,7 @@ controls: > > > > > > > > > > > > - AfTrigger: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Start an autofocus scan. > > > > > > > > > > > > @@ -692,6 +703,7 @@ controls: > > > > > > > > > > > > - AfPause: > > > > > > type: int32_t > > > > > > + direction: in > > > > > > description: | > > > > > > Pause lens movements when in continuous autofocus mode. > > > > > > > > > > Should AfState be out only ? > > > > > Also AfPauseState seems to be a metadata only. > > > > > > > > They indeed should, yes. > > > > > > > > > > > > > > HdrChannel I read > > > > > > > > > > This value is reported back to the application so that it can discover > > > > > whether this capture corresponds to the short or long exposure image > > > > > (or any other image used by the HDR procedure). An application can > > > > > monitor the HDR channel to discover when the differently exposed images > > > > > have arrived. > > > > > > > > > > which suggests me it's a metadata only > > > > > > > > I think you're right. > > > > > > > > > > > > > > and DebugMetadataEnable is possibily input only ? > > > > > > > > My thought process was that all enable-type controls should be both, as > > > > you'd probably want to know if what you've set has actually been set. > > > > > > > > > > > > > > > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml > > > > > > index 1b284257f601..59aa6141c25c 100644 > > > > > > --- a/src/libcamera/control_ids_draft.yaml > > > > > > +++ b/src/libcamera/control_ids_draft.yaml > > > > > > @@ -79,6 +79,7 @@ controls: > > > > > > > > > > > > - AeState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AE algorithm state. Currently identical to > > > > > > ANDROID_CONTROL_AE_STATE. > > > > > > @@ -108,6 +109,7 @@ controls: > > > > > > > > > > > > - AwbState: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the current AWB algorithm state. Currently identical > > > > > > to ANDROID_CONTROL_AWB_STATE. > > > > > > @@ -129,6 +131,7 @@ controls: > > > > > > > > > > > > - SensorRollingShutterSkew: > > > > > > type: int64_t > > > > > > + direction: out > > > > > > description: | > > > > > > Control to report the time between the start of exposure of the first > > > > > > row and the start of exposure of the last row. Currently identical to > > > > > > @@ -262,6 +265,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceRectangles: > > > > > > type: Rectangle > > > > > > + direction: out > > > > > > description: | > > > > > > Boundary rectangles of the detected faces. The number of values is > > > > > > the number of detected faces. > > > > > > @@ -273,6 +277,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceScores: > > > > > > type: uint8_t > > > > > > + direction: out > > > > > > description: | > > > > > > Confidence score of each of the detected faces. The range of score is > > > > > > [0, 100]. The number of values should be the number of faces reported > > > > > > @@ -285,6 +290,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceLandmarks: > > > > > > type: Point > > > > > > + direction: out > > > > > > description: | > > > > > > Array of human face landmark coordinates in format [..., left_eye_i, > > > > > > right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The > > > > > > @@ -298,6 +304,7 @@ controls: > > > > > > > > > > > > - FaceDetectFaceIds: > > > > > > type: int32_t > > > > > > + direction: out > > > > > > description: | > > > > > > Each detected face is given a unique ID that is valid for as long as the > > > > > > face is visible to the camera device. A face that leaves the field of > > > > > > > > > > Have you gone through control_ids_rpi.yaml and control_ids_debug.yaml > > > > > as well ? > > > > > > > > debug is empty and rpi... I missed Bcm2835StatsOutput should be out > > > > only. > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml index d34a2d068b60..c0e91d1852cd 100644 --- a/src/libcamera/control_ids_core.yaml +++ b/src/libcamera/control_ids_core.yaml @@ -17,6 +17,7 @@ controls: - AeLocked: type: bool + direction: out description: | Report the lock status of a running AE algorithm. @@ -236,6 +237,7 @@ controls: - AeFlickerDetected: type: int32_t + direction: out description: | Flicker period detected in microseconds. @@ -274,6 +276,7 @@ controls: - Lux: type: float + direction: out description: | Report an estimate of the current illuminance level in lux. @@ -324,6 +327,7 @@ controls: - AwbLocked: type: bool + direction: out description: | Report the lock status of a running AWB algorithm. @@ -361,6 +365,7 @@ controls: - SensorBlackLevels: type: int32_t + direction: out description: | Reports the sensor black levels used for processing a frame. @@ -385,6 +390,7 @@ controls: - FocusFoM: type: int32_t + direction: out description: | Reports a Figure of Merit (FoM) to indicate how in-focus the frame is. @@ -442,6 +448,7 @@ controls: - FrameDuration: type: int64_t + direction: out description: | The instantaneous frame duration from start of frame exposure to start of next exposure, expressed in microseconds. @@ -450,6 +457,7 @@ controls: - FrameDurationLimits: type: int64_t + direction: in description: | The minimum and maximum (in that order) frame duration, expressed in microseconds. @@ -487,6 +495,7 @@ controls: - SensorTemperature: type: float + direction: out description: | Temperature measure from the camera sensor in Celsius. @@ -499,6 +508,7 @@ controls: - SensorTimestamp: type: int64_t + direction: out description: | The time when the first row of the image sensor active array is exposed. @@ -667,6 +677,7 @@ controls: - AfTrigger: type: int32_t + direction: in description: | Start an autofocus scan. @@ -692,6 +703,7 @@ controls: - AfPause: type: int32_t + direction: in description: | Pause lens movements when in continuous autofocus mode. diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml index 1b284257f601..59aa6141c25c 100644 --- a/src/libcamera/control_ids_draft.yaml +++ b/src/libcamera/control_ids_draft.yaml @@ -79,6 +79,7 @@ controls: - AeState: type: int32_t + direction: out description: | Control to report the current AE algorithm state. Currently identical to ANDROID_CONTROL_AE_STATE. @@ -108,6 +109,7 @@ controls: - AwbState: type: int32_t + direction: out description: | Control to report the current AWB algorithm state. Currently identical to ANDROID_CONTROL_AWB_STATE. @@ -129,6 +131,7 @@ controls: - SensorRollingShutterSkew: type: int64_t + direction: out description: | Control to report the time between the start of exposure of the first row and the start of exposure of the last row. Currently identical to @@ -262,6 +265,7 @@ controls: - FaceDetectFaceRectangles: type: Rectangle + direction: out description: | Boundary rectangles of the detected faces. The number of values is the number of detected faces. @@ -273,6 +277,7 @@ controls: - FaceDetectFaceScores: type: uint8_t + direction: out description: | Confidence score of each of the detected faces. The range of score is [0, 100]. The number of values should be the number of faces reported @@ -285,6 +290,7 @@ controls: - FaceDetectFaceLandmarks: type: Point + direction: out description: | Array of human face landmark coordinates in format [..., left_eye_i, right_eye_i, mouth_i, left_eye_i+1, ...], with i = index of face. The @@ -298,6 +304,7 @@ controls: - FaceDetectFaceIds: type: int32_t + direction: out description: | Each detected face is given a unique ID that is valid for as long as the face is visible to the camera device. A face that leaves the field of
In preparation for adding support for querying direction information from controls, populate the corresponding field in the control ID defintions. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/libcamera/control_ids_core.yaml | 12 ++++++++++++ src/libcamera/control_ids_draft.yaml | 7 +++++++ 2 files changed, 19 insertions(+)