[libcamera-devel] [PATCH v2] gstreamer: Maintain a control list to track libcamerasrc control properties

[Umang Jain]

Hi Nicolas,

On 6/26/23 12:50 AM, Nicolas Dufresne wrote:
> Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :
>> 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, fail negotiation and report
>> what is 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>
>> ---
>> Changes in v2:
>> - Edit commit message
>> - Drop a break; so that all unsupported user-provided controls are
>>    reported at once.
>> ---
>>   src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------
>>   src/gstreamer/gstlibcamerasrc.h   |  6 ++---
>>   2 files changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
>> index 47d8ff43..11f15068 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,30 @@ 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.
> I don't remember why GStreamer links to that, and why we should accept this as a
> fact. The longer term plan, when libcamera is stable, was to migrate this
> element into GStreamer project, but such thing will prevent it.

It's one of the mutex lock used in struct GstLibcameraSrcState because 
<libcamera/base/mutex.h> is part of private [1]

I can  replace it with GMutex and GLibLocker right away but we will 
probably lose clang's thread annotation there itself.

[1] 
https://git.libcamera.org/libcamera/libcamera.git/commit/?id=777b0e0a655cce258a2b11e98546c3fc5a5be031
>
>> +	 */
>> +	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()));
>> +			supported_ctrl = false;
>>   		}
>>   	}
>>   
>> +	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 +688,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 +708,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 +777,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;
>>   	delete self->state;
>>   
>>   	return klass->finalize(object);
>> @@ -783,6 +801,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,
>>   			"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",
>>   		},