[PATCH 1/4] libcamera: controls: Populate direction field in control definitions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 26 00:04:17 CET 2024
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 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list