[PATCH 1/4] libcamera: controls: Populate direction field in control definitions

Paul Elder paul.elder at ideasonboard.com
Mon Nov 25 21:11:17 CET 2024


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 at 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
> >


More information about the libcamera-devel mailing list