[PATCH 1/4] libcamera: controls: Populate direction field in control definitions
Naushir Patuck
naush at raspberrypi.com
Tue Nov 26 16:58:03 CET 2024
On Mon, 25 Nov 2024 at 23:04, Laurent Pinchart
<laurent.pinchart at 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 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