[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