[libcamera-devel] [PATCH] gstreamer: Maintain a control list to track libcamerasrc control properties
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 26 18:15:37 CEST 2023
HI Kieran,
On 6/26/23 9:24 PM, Kieran Bingham wrote:
> Hi Umang,
>
> Quoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)
>> As more and more libcamera controls get plumbed, more member variables
>> get introduced in struct GstLibcameraSrc. Instead of doing that, now
>> maintain a single ControlList which is more appropriate to keep track
>> of controls that get sets on libcamerasrc.
>>
>> This also brings easy validation of controls set on libcamerasrc. If
>> the controls are not supported by camera, abort early and report what is
> I would change 'abort' to 'fail negotiation' as I thought from reading
> this there was going to be a call to abort().
ack
>
>> not supported.
>>
>> The current patch migrates previously introduced control,
>> auto-focus-mode, to be set directly in control list.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> Cedric, Can you please help test this?
>>
>> (I'm travelling so have tested this on my RPi and inferring from
>> libcamera/gstreamer logs, it seems to have not broken anything)
>>
>> An additional test from your side would be appreciated.
>> ---
>> src/gstreamer/gstlibcamerasrc.cpp | 45 ++++++++++++++++++++++---------
>> src/gstreamer/gstlibcamerasrc.h | 6 ++---
>> 2 files changed, 36 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
>> index 47d8ff43..a2266310 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {
>> GstTask *task;
>>
>> gchar *camera_name;
>> - controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
>> + ControlList *controls;
>>
>> GstLibcameraSrcState *state;
>> GstLibcameraAllocator *allocator;
>> @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>> GLibRecLocker lock(&self->stream_lock);
>> GstLibcameraSrcState *state = self->state;
>> + const ControlInfoMap &info_map = state->cam_->controls();
>> GstFlowReturn flow_ret = GST_FLOW_OK;
>> + gboolean supported_ctrl = true;
>> gint ret;
>>
>> g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
>> @@ -579,18 +581,31 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>> gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>> }
>>
>> - if (self->auto_focus_mode != controls::AfModeManual) {
>> - const ControlInfoMap &infoMap = state->cam_->controls();
>> - if (infoMap.find(&controls::AfMode) != infoMap.end()) {
>> - state->initControls_.set(controls::AfMode, self->auto_focus_mode);
>> - } else {
>> + /*
>> + * Check the controls set on libcamerasrc is supported by the camera.
>> + * This can be easily done by CameraControlValidator but it is an internal
>> + * libcamera class. Hence avoid adding a internal header even though
>> + * gstreamer links with libcamera-private.
>> + */
>> + for (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {
>> + if (info_map.find(iter->first) == info_map.end()) {
>> + const ControlIdMap *id_map = self->controls->idMap();
>> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> - ("Failed to enable auto focus"),
>> - ("AfMode not supported by this camera, "
>> - "please retry with 'auto-focus-mode=AfModeManual'"));
>> + ("Failed to set %s", id_map->at(iter->first)->name().c_str()),
>> + ("%s not supported by this camera, ", id_map->at(iter->first)->name().c_str()));
> Isn't this printing the name twice? Maybe this could be a single output
> line.
Ah probably, I 'll try to take a look.
>
>> + supported_ctrl = false;
>> + break;
> I would probably not break here - as if there are multiple controls that
> fail to validate - it might make sense to report all of them at once.
Good idea!
>
>
>> }
>> }
>>
>> + if (!supported_ctrl) {
>> + gst_task_stop(task);
>> + flow_ret = GST_FLOW_NOT_NEGOTIATED;
>> + goto done;
>> + }
>> +
>> + state->initControls_.merge(*self->controls);
>> +
>> ret = state->cam_->start(&state->initControls_);
>> if (ret) {
>> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> @@ -674,7 +689,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
>> self->camera_name = g_value_dup_string(value);
>> break;
>> case PROP_AUTO_FOCUS_MODE:
>> - self->auto_focus_mode = static_cast<controls::AfModeEnum>(g_value_get_enum(value));
>> + self->controls->set(controls::AfMode,
>> + static_cast<controls::AfModeEnum>(g_value_get_enum(value)));
>> break;
>> default:
>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>> @@ -693,9 +709,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
>> case PROP_CAMERA_NAME:
>> g_value_set_string(value, self->camera_name);
>> break;
>> - case PROP_AUTO_FOCUS_MODE:
>> - g_value_set_enum(value, static_cast<gint>(self->auto_focus_mode));
>> + case PROP_AUTO_FOCUS_MODE: {
>> + auto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);
>> + g_value_set_enum(value, auto_focus_mode);
>> break;
>> + }
>> default:
>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>> break;
>> @@ -760,6 +778,7 @@ gst_libcamera_src_finalize(GObject *object)
>> g_rec_mutex_clear(&self->stream_lock);
>> g_clear_object(&self->task);
>> g_free(self->camera_name);
>> + delete self->controls;
> I would have said can we use a unique_ptr, but it doesn't look like that
> makes sense without reworking the rest of the structure...
I think you can't do unique_ptr of ControlList right now.
>
>> delete self->state;
>>
>> return klass->finalize(object);
>> @@ -783,6 +802,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
>> /* C-style friend. */
>> state->src_ = self;
>> self->state = state;
>> +
>> + self->controls = new ControlList(controls::controls, nullptr);
>> }
>>
>> static GstPad *
>> diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h
>> index 0a88ba02..2e6d74cb 100644
>> --- a/src/gstreamer/gstlibcamerasrc.h
>> +++ b/src/gstreamer/gstlibcamerasrc.h
>> @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()
>> static GType type = 0;
>> static const GEnumValue values[] = {
>> {
>> - static_cast<gint>(libcamera::controls::AfModeManual),
>> + libcamera::controls::AfModeManual,
> Nice to see the cast is no longer needed. Is that a result of a change
> here, or was it already possible. ?
Not sure, I didn't test this isolated change with the earlier AF mode
patch, but AFAIR i started from here developing this entire patch.
>
>> "AfModeManual",
>> "manual-focus",
>> },
>> {
>> - static_cast<gint>(libcamera::controls::AfModeAuto),
>> + libcamera::controls::AfModeAuto,
>> "AfModeAuto",
>> "automatic-auto-focus",
>> },
>> {
>> - static_cast<gint>(libcamera::controls::AfModeContinuous),
>> + libcamera::controls::AfModeContinuous,
>> "AfModeContinuous",
>> "continuous-auto-focus",
>> },
>> --
>> 2.39.1
>>
More information about the libcamera-devel
mailing list