[PATCH 2/3] gstreamer: Generate controls from control_ids_*.yaml files
Jaslo Ziska
jaslo at ziska.de
Thu Aug 8 14:36:16 CEST 2024
Hi Kieran,
thanks for the review.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> Hi Nicolas, Jaslo,
>
> Adding comments where I think I can add helpful (I hope)
> feedback.
>
> Jaslo - Thanks again for starting this work - It's certainly
> worthwhile
> - even if there might be a few challenges below!
It seems that *few* might be an understatement ;)
> And I've already seen one person on IRC who was able to solve
> their
> issues by using this series! \o/
I'm very happy to hear that!
>
> Quoting Nicolas Dufresne (2024-08-07 20:13:08)
>> Hi,
>>
>> I started inspecting the generated documentation. It is
>> generally hard to get a
>> perfectly clean results, but let me comment few things. We can
>> at least try some
>> improvement.
>>
>
> <snip>
>
>> > ae-flicker-period : Manual flicker period in microseconds.
>> > This value sets the current flicker period to avoid. It
>> > is used when AeFlickerMode is set to FlickerManual. To cancel
>> > 50Hz mains flicker, this should be set to 10000
>> > (corresponding to 100Hz), or 8333 (120Hz) for 60Hz mains.
>> > Setting the mode to FlickerManual when no AeFlickerPeriod
>> > has ever been set means that no flicker cancellation occurs
>> > (until the value of this control is updated). Switching
>> > to modes other than FlickerManual has no effect on the value
>> > of the AeFlickerPeriod control. See also:
>> > ae-flicker-mode.
>> > flags: readable, writable,
>> > controllable
>> > Integer. Range: -2147483648 -
>> > 2147483647 Default: 0
>> >
>> > ae-locked : Report the lock status of a running AE
>> > algorithm. If the AE algorithm is locked the value shall be
>> > set
>> > to true, if it's converging it shall be set to false. If the
>> > AE algorithm is not running the control shall not be
>> > present in the metadata control list. See also: ae-enable.
>> > flags: readable, writable,
>> > controllable
>> > Boolean. Default: false
>>
>> As this one is a status, it should only be readable, without
>> the writable/controllable. If I'm not mistaken, you get this
>> value withing the finalized request, would be nice enhancement
>> to notify our users of the change using g_notify() and/or bus
>> messages.
>
>
> This comes down to the fact that readable controls / properties
> are only
> reported in metadata and not exposed as a control by the
> camera...
>
>> >
>> > ae-metering-mode : Specify a metering mode for the AE
>> > algorithm to use. The metering modes determine which parts
>> > of the image are used to determine the scene brightness.
>> > Metering modes may be platform specific and not all
>> > metering modes may be supported.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AeMeteringMode" Default: 0,
>> > "centre-weighted"
>> > (0): centre-weighted -
>> > Centre-weighted metering mode.
>> > (1): spot - Spot
>> > metering mode.
>> > (2): matrix - Matrix
>> > metering mode.
>> > (3): custom - Custom
>> > metering mode.
>>
>> From the doc, this seems pretty obscure. Shouldn't "custom"
>> comes with another control ? Perhaps we introduce a mask to
>> hide the controls that are not yet usable or that the usage
>> isn't obvious and may need special case or rework ?
>>
>> >
>> > af-metering : Instruct the AF algorithm how it
>> > should decide which parts of the image should be used to
>> > measure focus.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AfMetering" Default: 0, "auto"
>> > (0): auto - The AF
>> > algorithm should decide for itself
>> > where it will measure focus.
>> > (1): windows - The AF
>> > algorithm should use the
>> > rectangles defined by the
>> > AfWindows control to measure
>> > focus. If no windows are specified
>> > the behaviour is platform
>> > dependent.
>> >
>> > af-mode : Control to set the mode of the AF
>> > (autofocus) algorithm. An implementation may choose not to
>> > implement all the modes.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AfMode" Default: 0, "manual"
>>
>> Is that appropriate default ? Of course does not matter if
>> there is no focus on the target. I wonder how you get to know
>> if focus is there or not ...
>
> Cameras without autofocus should not expose the controls, as
> they do not
> 'exist' and they will not be handled...
>
> However - that doesn't fit in this gstreamer model where all
> controls
> are going to be 'always' exposed. (Even if they are actually
> 'metadata
> only' as well). It's up to the Camera at runtime to report what
> the
> pipeline handler will accept based on the underlying hardware
> capabilities or configuration.
>
> In fact setting them would likely / potentially cause an
> assertion? (or
> at least a failed request - potentially causing a frame drop...
> ?).
At the moment the control gets set anyway, whether it is supported
or not, and if it isn't there is an error printed on every
request. Whether a control is supported should probably already be
checked and handled in the gstreamer element.
Best regards,
Jaslo
>
> <snip>
>
>
>> > Enum "AfState" Default: 0, "idle"
>> > (0): idle - The AF
>> > algorithm is in manual mode
>> > (AfModeManual) or in auto mode
>> > (AfModeAuto) and a scan has not
>> > yet been triggered, or an
>> > in-progress scan was cancelled.
>> > (1): scanning - The AF
>> > algorithm is in auto mode
>> > (AfModeAuto), and a scan has been
>> > started using the AfTrigger
>> > control. The scan can be cancelled
>> > by sending AfTriggerCancel at
>> > which point the algorithm will
>> > either move back to AfStateIdle
>> > or, if the scan actually completes
>> > before the cancel request is
>> > processed, to one of
>> > AfStateFocused or AfStateFailed.
>> > Alternatively the AF algorithm
>> > could be in continuous mode
>> > (AfModeContinuous) at which point
>> > it may enter this state
>> > spontaneously whenever it
>> > determines that a rescan is
>> > needed.
>> > (2): focused - The AF
>> > algorithm is in auto (AfModeAuto)
>> > or continuous (AfModeContinuous)
>> > mode and a scan has completed with
>> > the result that the algorithm
>> > believes the image is now in
>> > focus.
>> > (3): failed - The AF
>> > algorithm is in auto (AfModeAuto)
>> > or continuous (AfModeContinuous)
>> > mode and a scan has completed with
>> > the result that the algorithm did
>> > not find a good focus position.
>> >
>> > af-trigger : This control starts an autofocus scan
>> > when AfMode is set to AfModeAuto, and can also be used to
>> > terminate a scan early. It is ignored if AfMode is set to
>> > AfModeManual or AfModeContinuous.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AfTrigger" Default: 0, "start"
>> > (0): start - Start an
>> > AF scan. Ignored if a scan is in
>> > progress.
>> > (1): cancel - Cancel an
>> > AF scan. This does not cause the
>> > lens to move anywhere else.
>> > Ignored if no scan is in progress.
>>
>> This one is missing something to make sense, at least from
>> GStreamer point of view. When you are not starting or
>> cancelling, what value this control should change to ? That
>> question might be relevant for libcamera globally.
>>
>> Without this third state, I would say this one should not be
>> there, and replace with action signals.
>
>
> In libcamera - controls only 'exist' in a single request. What
> is an
> 'action signal' in Gstreamer?
>
>
> So - AfTrigger for start and cancel are, in this case 'events'
>
> - "Start an AFscan at this request"
> - "Cancel any active scan"
>
>
> There are no 'events' in between. Controls are only 'set' in
> Requests.
> Any 'response' is only available in the metadata from a
> completed
> Request.
>
> You can not 'read' the state of a control in the same sense of a
> V4L2
> control.
>
> (Maybe this should change for some use cases, or for an abilty
> to set
> controls directly on a camera, which I believe Stefan has
> explored, and
> not through a request, but it's the current model).
>
>
>
>> > analogue-gain : Analogue gain value applied in the sensor
>> > device. The value of the control specifies the gain
>> > multiplier applied to all colour channels. This value cannot
>> > be lower than 1.0. Setting this value means that it is
>> > now fixed and the AE algorithm may not change it. Setting it
>> > back to zero returns it to the control of the AE
>> > algorithm. See also: exposure-time, ae-enable. Todo: Document
>> > the interactions between AeEnable and setting a fixed
>> > value for this control. Consider interactions with other AE
>> > features, such as aperture and aperture/shutter priority
>> > mode, and decide if control of which features should be
>> > automatically adjusted shouldn't better be handled through a
>> > separate AE mode control.
>> > flags: readable, writable,
>> > controllable
>> > Float. Range: -3.402823e+38 -
>> > 3.402823e+38 Default: 0
>>
>> I'm not confident there is no range, would be nice if we can
>> align to something sensible (might need yaml changes of
>> course).
>
> Different cameras will have different ranges ... only
> determinable at
> runtime. We can't specify this in YAML. Though - it should
> always be
> positive at least! (and as you've mentioned elsewhere, if it was
> machine readable to say always above 1.0 would be beneficial
> too).
>
>
>
>> > awb-enable : Enable or disable the AWB. See also:
>> > colour-gains.
>> > flags: readable, writable,
>> > controllable
>> > Boolean. Default: false
>>
>> Should be on by default (if not, we should do that in gst ;-P).
>
>
> Maybe that's a GST decision. It is likely use case dependent.
> But the same could be said for either default ... (and 'on'
> seems more
> likely to be useful than 'off' in the majority case?)
>
>
>> > awb-locked : Report the lock status of a running AWB
>> > algorithm. If the AWB algorithm is locked the value shall
>> > be
>> > set to true, if it's converging it shall be set to false. If
>> > the AWB algorithm is not running the control shall not
>> > be present in the metadata control list. See also:
>> > awb-enable.
>> > flags: readable, writable,
>> > controllable
>> > Boolean. Default: false
>>
>> Read-only.
>
> Reported through metadata() same for all the other Read-only
> 'control
> ids' I suspect.
>
>
>
>>
>> >
>> > awb-mode : Specify the range of illuminants to
>> > use for the AWB algorithm. The modes supported are platform
>> > specific, and not all modes may be supported.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AwbMode" Default: 0, "auto"
>> > (0): auto - Search
>> > over the whole colour temperature
>> > range.
>> > (1): incandescent -
>> > Incandescent AWB lamp mode.
>> > (2): tungsten - Tungsten
>> > AWB lamp mode.
>> > (3): fluorescent -
>> > Fluorescent AWB lamp mode.
>> > (4): indoor - Indoor AWB
>> > lighting mode.
>> > (5): daylight - Daylight
>> > AWB lighting mode.
>> > (6): cloudy - Cloudy AWB
>> > lighting mode.
>> > (7): custom - Custom AWB
>> > mode.
>>
>> Again, unclear how custom can be used.
>
> I think/suspect this was added by Raspberry Pi. We should
> certainly
> revisit this I think.
>
>> > brightness : Specify a fixed brightness parameter.
>> > Positive values (up to 1.0) produce brighter images;
>> > negative values (up to -1.0) produce darker images and 0.0
>> > leaves pixels unchanged.
>> > flags: readable, writable,
>> > controllable
>> > Float. Range: -3.402823e+38 -
>> > 3.402823e+38 Default: 0
>>
>> So this time the range is documented, we need a machine
>> readable form.
>
> Indeed.
>
>
>> >
>> > camera-name : Select by name which camera to use.
>> > flags: readable, writable, changeable
>> > only in NULL or READY state
>> > String. Default: null
>> >
>> > colour-correction-matrix: The 3x3 matrix that converts
>> > camera RGB to sRGB within the imaging pipeline. This should
>> > describe the matrix that is used after pixels have been
>> > white-balanced, but before any gamma transformation. The 3x3
>> > matrix is stored in conventional reading order in an array of
>> > 9 floating point values.
>> > flags: readable, writable,
>> > controllable
>> > Default: "< >"
>> > GstValueArray of GValues of type
>> > "gfloat"
>>
>> The type only allow flat list, but as its a matrix I was
>> expecting
>>
>> "< < 1, 2, 3 >,
>> < 4, 5, 6 >,
>> < 7, 8, 9 > >"
>>
>> That being said I would be fine with flat raster data:
>>
>> "< 1, 2, 3,
>> 4, 5, 6,
>> 7, 8, 9 >"
>>
>> The down side is for bindings. Consider Python, the first would
>> create a collection that you can access with matrix[Y][X] and
>> more importantly can store into numpy.matrix.
>>
>> A comment for all list "< >" syntax. The usage and
>> serialization syntax is not easy to guess. For matrix, if we
>> had the identity matrix as default instead of an empty list,
>> that would help. Otherwise we need a way to extend the doc for
>> GStreamer usage.
>>
>
> I recall various bits of work on that recently...
>
> But I think this is already handled through python. Stefan/Tomi
> ?
>
>
>
> <snip>
>
>> > draft-ae-precapture-trigger: Control for AE metering
>> > trigger. Currently identical to
>> > ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER. Whether the camera
>> > device will trigger a precapture metering sequence when it
>> > processes this request.
>> > flags: readable, writable,
>> > controllable
>> > Enum "AePrecaptureTrigger" Default:
>> > 0, "idle"
>> > (0): idle - The
>> > trigger is idle.
>> > (1): start - The
>> > pre-capture AE metering is started
>> > by the camera.
>> > (2): cancel - The camera
>> > will cancel any active or
>> > completed metering sequence. The
>> > AE algorithm is reset to its
>> > initial state.
>>
>> Perhaps we can skip over draft- as this could later break our
>> interface. We can also add an env to include them ?
>
>
> Skipping them is an option. I think most of the 'draft' were
> only added
> to match the Android interface. But I wonder how we'll handle
> other
> namespaced controls like 'RPi' which I guess will be below...
>
>
> <snip>
>
>> I think my comments begins to be repetitive, and if we solve
>> the earlier issues
>> we will solve them all. The main thing is that the yaml should
>> explicitly define
>> more stuff rather then relying to text. And using that, we can
>> easily adjust
>
> Agreed, I think at least the yaml files could specify if a
> control can
> only be accessed as metadata which would simplify all of the
> 'read-only'
> issues.
>
>> GStreamer properties. A final note, not all of these will be
>> supported. I was
>> wondering if we should add an extra property that tells use
>> which one are
>> actually supported ? We don't need anything fency, a "/"
>> seperate list in a
>> string or similar could do. We can also go for GstStructure.
>
> I don't understand this part fully. Do you mean a new
> control/property
> that gstreamer libcamerasrc will read at runtime from libcamera?
> It
> already has a Camera->controls() that it can read that from.
>
> But you mention GstStructure - do you mean that the libcamerasrc
> will
> generate this ? (Perhaps from the Camera->controls()?)
>
>> Nicolas
>
> Kieran
More information about the libcamera-devel
mailing list