[PATCH 1/4] libcamera: controls: Populate direction field in control definitions
Paul Elder
paul.elder at ideasonboard.com
Wed Nov 27 09:12:28 CET 2024
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 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.
>
> 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 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