[1/3] gstreamer: Remove auto-focus-mode property
diff mbox series

Message ID 20240805100038.11972-2-jaslo@ziska.de
State Superseded
Headers show
Series
  • gstreamer: Generate controls from control_ids_*.yaml files
Related show

Commit Message

Jaslo Ziska Aug. 5, 2024, 9:28 a.m. UTC 
In preparation of the next commit remove the auto-focus-mode property
from the libcamera element.

Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
---
 src/gstreamer/gstlibcamerasrc.cpp | 28 ----------------------------
 1 file changed, 28 deletions(-)

Comments

Laurent Pinchart Aug. 7, 2024, 8:13 p.m. UTC | #1 
Hi Jaslo,

Thank you for the patch.

On Mon, Aug 05, 2024 at 11:28:36AM +0200, Jaslo Ziska wrote:
> In preparation of the next commit remove the auto-focus-mode property
> from the libcamera element.

I would write

    In preparation for generic support of all libcaemra controls, remove the
    manual handling of the auto-focus-mode property from the libcamerasrc
    element.

Could you explain here why you're not also removing the manual
auto-focus handling from src/gstreamer/gstlibcameraprovider.cpp ?

> Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 28 ----------------------------
>  1 file changed, 28 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index e1bb6b4c..5a3e2989 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -142,7 +142,6 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> -	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
>  
>  	std::atomic<GstEvent *> pending_eos;
>  
> @@ -154,7 +153,6 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> -	PROP_AUTO_FOCUS_MODE,
>  };
>  
>  static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
> @@ -663,18 +661,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  		gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
>  	}
>  
> -	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 {
> -			GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> -					  ("Failed to enable auto focus"),
> -					  ("AfMode not supported by this camera, "
> -					   "please retry with 'auto-focus-mode=AfModeManual'"));
> -		}
> -	}
> -
>  	ret = state->cam_->start(&state->initControls_);
>  	if (ret) {
>  		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> @@ -742,9 +728,6 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
>  		g_free(self->camera_name);
>  		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));
> -		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>  		break;
> @@ -762,9 +745,6 @@ 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));
> -		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>  		break;
> @@ -967,14 +947,6 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  							     | G_PARAM_STATIC_STRINGS));
>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
>  
> -	spec = g_param_spec_enum("auto-focus-mode",
> -				 "Set auto-focus mode",
> -				 "Available options: AfModeManual, "
> -				 "AfModeAuto or AfModeContinuous.",
> -				 gst_libcamera_auto_focus_get_type(),
> -				 static_cast<gint>(controls::AfModeManual),
> -				 G_PARAM_WRITABLE);
> -	g_object_class_install_property(object_class, PROP_AUTO_FOCUS_MODE, spec);
>  }
>  
>  /* GstChildProxy implementation */
Nicolas Dufresne Aug. 7, 2024, 8:51 p.m. UTC | #2 
Hi Laurent,

Le mercredi 07 août 2024 à 23:13 +0300, Laurent Pinchart a écrit :
> Hi Jaslo,
> 
> Thank you for the patch.
> 
> On Mon, Aug 05, 2024 at 11:28:36AM +0200, Jaslo Ziska wrote:
> > In preparation of the next commit remove the auto-focus-mode property
> > from the libcamera element.
> 
> I would write
> 
>     In preparation for generic support of all libcaemra controls, remove the
>     manual handling of the auto-focus-mode property from the libcamerasrc
>     element.
> 
> Could you explain here why you're not also removing the manual
> auto-focus handling from src/gstreamer/gstlibcameraprovider.cpp ?

That escaped my review, and should have never been introduce in the first place.
Can be removed independently from this patchset. There should be no property
proxying between the provider and the generated elements.

Nicolas

> 
> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 28 ----------------------------
> >  1 file changed, 28 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index e1bb6b4c..5a3e2989 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -142,7 +142,6 @@ struct _GstLibcameraSrc {
> >  	GstTask *task;
> >  
> >  	gchar *camera_name;
> > -	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> >  
> >  	std::atomic<GstEvent *> pending_eos;
> >  
> > @@ -154,7 +153,6 @@ struct _GstLibcameraSrc {
> >  enum {
> >  	PROP_0,
> >  	PROP_CAMERA_NAME,
> > -	PROP_AUTO_FOCUS_MODE,
> >  };
> >  
> >  static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
> > @@ -663,18 +661,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> >  		gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
> >  	}
> >  
> > -	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 {
> > -			GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > -					  ("Failed to enable auto focus"),
> > -					  ("AfMode not supported by this camera, "
> > -					   "please retry with 'auto-focus-mode=AfModeManual'"));
> > -		}
> > -	}
> > -
> >  	ret = state->cam_->start(&state->initControls_);
> >  	if (ret) {
> >  		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > @@ -742,9 +728,6 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
> >  		g_free(self->camera_name);
> >  		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));
> > -		break;
> >  	default:
> >  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >  		break;
> > @@ -762,9 +745,6 @@ 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));
> > -		break;
> >  	default:
> >  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >  		break;
> > @@ -967,14 +947,6 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >  							     | G_PARAM_STATIC_STRINGS));
> >  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> >  
> > -	spec = g_param_spec_enum("auto-focus-mode",
> > -				 "Set auto-focus mode",
> > -				 "Available options: AfModeManual, "
> > -				 "AfModeAuto or AfModeContinuous.",
> > -				 gst_libcamera_auto_focus_get_type(),
> > -				 static_cast<gint>(controls::AfModeManual),
> > -				 G_PARAM_WRITABLE);
> > -	g_object_class_install_property(object_class, PROP_AUTO_FOCUS_MODE, spec);
> >  }
> >  
> >  /* GstChildProxy implementation */
>
Jaslo Ziska Aug. 8, 2024, 11:32 a.m. UTC | #3 
Hi Nicolas and Laurent,

Nicolas Dufresne <nicolas@ndufresne.ca> writes:
> Hi Laurent,
>
> Le mercredi 07 août 2024 à 23:13 +0300, Laurent Pinchart a 
> écrit :
>> Hi Jaslo,
>> 
>> Thank you for the patch.
>> 
>> On Mon, Aug 05, 2024 at 11:28:36AM +0200, Jaslo Ziska wrote:
>> > In preparation of the next commit remove the auto-focus-mode 
>> > property
>> > from the libcamera element.
>> 
>> I would write
>> 
>>     In preparation for generic support of all libcaemra 
>>     controls, remove the
>>     manual handling of the auto-focus-mode property from the 
>>     libcamerasrc
>>     element.
>> 
>> Could you explain here why you're not also removing the manual
>> auto-focus handling from src/gstreamer/gstlibcameraprovider.cpp 
>> ?

Oops, I didn't even know that was there.

> That escaped my review, and should have never been introduce in 
> the first place.
> Can be removed independently from this patchset. There should be 
> no property
> proxying between the provider and the generated elements.

If you want to I can also add a patch removing this to the set (or 
add it to the existing patch removing auto-focus-mode support).

Best regards,

Jaslo

>
> Nicolas
>
>> 
>> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
>> > ---
>> >  src/gstreamer/gstlibcamerasrc.cpp | 28 
>> >  ----------------------------
>> >  1 file changed, 28 deletions(-)
>> > 
>> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp 
>> > b/src/gstreamer/gstlibcamerasrc.cpp
>> > index e1bb6b4c..5a3e2989 100644
>> > --- a/src/gstreamer/gstlibcamerasrc.cpp
>> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> > @@ -142,7 +142,6 @@ struct _GstLibcameraSrc {
>> >  	GstTask *task;
>> >  
>> >  	gchar *camera_name;
>> > -	controls::AfModeEnum auto_focus_mode = 
>> > controls::AfModeManual;
>> >  
>> >  	std::atomic<GstEvent *> pending_eos;
>> >  
>> > @@ -154,7 +153,6 @@ struct _GstLibcameraSrc {
>> >  enum {
>> >  	PROP_0,
>> >  	PROP_CAMERA_NAME,
>> > -	PROP_AUTO_FOCUS_MODE,
>> >  };
>> >  
>> >  static void gst_libcamera_src_child_proxy_init(gpointer 
>> >  g_iface,
>> > @@ -663,18 +661,6 @@ gst_libcamera_src_task_enter(GstTask 
>> > *task, [[maybe_unused]] GThread *thread,
>> >  		gst_pad_push_event(srcpad, 
>> >  gst_event_new_segment(&segment));
>> >  	}
>> >  
>> > -	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 {
>> > -			GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> > -					  ("Failed to enable auto focus"),
>> > -					  ("AfMode not supported by this camera, "
>> > -					   "please retry with 
>> > 'auto-focus-mode=AfModeManual'"));
>> > -		}
>> > -	}
>> > -
>> >  	ret = state->cam_->start(&state->initControls_);
>> >  	if (ret) {
>> >  		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>> > @@ -742,9 +728,6 @@ gst_libcamera_src_set_property(GObject 
>> > *object, guint prop_id,
>> >  		g_free(self->camera_name);
>> >  		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));
>> > -		break;
>> >  	default:
>> >  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, 
>> >  pspec);
>> >  		break;
>> > @@ -762,9 +745,6 @@ 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));
>> > -		break;
>> >  	default:
>> >  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, 
>> >  pspec);
>> >  		break;
>> > @@ -967,14 +947,6 @@ 
>> > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>> >  							     | G_PARAM_STATIC_STRINGS));
>> >  	g_object_class_install_property(object_class, 
>> >  PROP_CAMERA_NAME, spec);
>> >  
>> > -	spec = g_param_spec_enum("auto-focus-mode",
>> > -				 "Set auto-focus mode",
>> > -				 "Available options: AfModeManual, "
>> > -				 "AfModeAuto or AfModeContinuous.",
>> > -				 gst_libcamera_auto_focus_get_type(),
>> > -				 static_cast<gint>(controls::AfModeManual),
>> > -				 G_PARAM_WRITABLE);
>> > -	g_object_class_install_property(object_class, 
>> > PROP_AUTO_FOCUS_MODE, spec);
>> >  }
>> >  
>> >  /* GstChildProxy implementation */
>>

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index e1bb6b4c..5a3e2989 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -142,7 +142,6 @@  struct _GstLibcameraSrc {
 	GstTask *task;
 
 	gchar *camera_name;
-	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
 
 	std::atomic<GstEvent *> pending_eos;
 
@@ -154,7 +153,6 @@  struct _GstLibcameraSrc {
 enum {
 	PROP_0,
 	PROP_CAMERA_NAME,
-	PROP_AUTO_FOCUS_MODE,
 };
 
 static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
@@ -663,18 +661,6 @@  gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
 		gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
 	}
 
-	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 {
-			GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
-					  ("Failed to enable auto focus"),
-					  ("AfMode not supported by this camera, "
-					   "please retry with 'auto-focus-mode=AfModeManual'"));
-		}
-	}
-
 	ret = state->cam_->start(&state->initControls_);
 	if (ret) {
 		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
@@ -742,9 +728,6 @@  gst_libcamera_src_set_property(GObject *object, guint prop_id,
 		g_free(self->camera_name);
 		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));
-		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
 		break;
@@ -762,9 +745,6 @@  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));
-		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
 		break;
@@ -967,14 +947,6 @@  gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
 							     | G_PARAM_STATIC_STRINGS));
 	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
 
-	spec = g_param_spec_enum("auto-focus-mode",
-				 "Set auto-focus mode",
-				 "Available options: AfModeManual, "
-				 "AfModeAuto or AfModeContinuous.",
-				 gst_libcamera_auto_focus_get_type(),
-				 static_cast<gint>(controls::AfModeManual),
-				 G_PARAM_WRITABLE);
-	g_object_class_install_property(object_class, PROP_AUTO_FOCUS_MODE, spec);
 }
 
 /* GstChildProxy implementation */