[libcamera-devel,v1,3/5] gstreamer:Install the colorimetry property
diff mbox series

Message ID 20220703073358.76643-3-rishikeshdonadkar@gmail.com
State Superseded
Headers show
Series
  • [libcamera-devel,v1,1/5] gstreamer: Convert form colorspace to colorimetry.
Related show

Commit Message

Rishikesh Donadkar July 3, 2022, 7:33 a.m. UTC 
Add the colorimetry field in the _GstLibcameraSrc. Add switch cases in
gst_libcamera_src_set_property() and gst_libcamera_src_get_property() to
access and modify the property.

Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Nicolas Dufresne July 4, 2022, 6:13 p.m. UTC | #1 
Le dimanche 03 juillet 2022 à 13:03 +0530, Rishikesh Donadkar a écrit :
> Add the colorimetry field in the _GstLibcameraSrc. Add switch cases in
> gst_libcamera_src_set_property() and gst_libcamera_src_get_property() to
> access and modify the property.
> 
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 46fd02d2..120319b3 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -121,6 +121,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> +	gchar *colorimetry;
>  
>  	GstLibcameraSrcState *state;
>  	GstLibcameraAllocator *allocator;
> @@ -129,7 +130,8 @@ struct _GstLibcameraSrc {
>  
>  enum {
>  	PROP_0,
> -	PROP_CAMERA_NAME
> +	PROP_CAMERA_NAME,
> +	PROP_COLORIMETRY,
>  };
>  
>  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> @@ -532,6 +534,10 @@ 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_COLORIMETRY:
> +		g_free(self->colorimetry);
> +		self->colorimetry = g_value_dup_string(value);
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>  		break;
> @@ -549,6 +555,9 @@ 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_COLORIMETRY:
> +		g_value_set_string(value, self->colorimetry);
> +		break;
>  	default:
>  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>  		break;
> @@ -717,4 +726,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  							     | G_PARAM_READWRITE
>  							     | G_PARAM_STATIC_STRINGS));
>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> +
> +	GParamSpec *spec2 = g_param_spec_string("colorimetry", "Coloirmetry",
> +						"Colorimetry that will be applied to the StreamConfiguration", nullptr,
> +					       (GParamFlags)(GST_PARAM_MUTABLE_READY
> +							     | G_PARAM_READWRITE
> +							     | G_PARAM_STATIC_STRINGS));

As this is a field in caps, the appropriate way to interact in userland is to
place such feild in downstream caps filter. Properties are not acceptable.

  libcamerasrc ! video/x-raw,colorimetry=bt709 ! ...

What would be a use case though for forcing a specific colorimetry ? Do the V4L2
stack we have supports this ?

> +	g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
>  }
Rishikesh Donadkar July 6, 2022, 7:58 a.m. UTC | #2 
> What would be a use case though for forcing a specific colorimetry ? Do the V4L2
> stack we have supports this ?
>
> > +     g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
> >  }

a use case would be to set a colorspace requested by user with something like
g_object_set(source, "colorimetry", "bt2020", NULL);
I have tested that the requested colorimetry gets applied to the
camera if it passes the validation.



On Mon, Jul 4, 2022 at 11:43 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le dimanche 03 juillet 2022 à 13:03 +0530, Rishikesh Donadkar a écrit :
> > Add the colorimetry field in the _GstLibcameraSrc. Add switch cases in
> > gst_libcamera_src_set_property() and gst_libcamera_src_get_property() to
> > access and modify the property.
> >
> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 46fd02d2..120319b3 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -121,6 +121,7 @@ struct _GstLibcameraSrc {
> >       GstTask *task;
> >
> >       gchar *camera_name;
> > +     gchar *colorimetry;
> >
> >       GstLibcameraSrcState *state;
> >       GstLibcameraAllocator *allocator;
> > @@ -129,7 +130,8 @@ struct _GstLibcameraSrc {
> >
> >  enum {
> >       PROP_0,
> > -     PROP_CAMERA_NAME
> > +     PROP_CAMERA_NAME,
> > +     PROP_COLORIMETRY,
> >  };
> >
> >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> > @@ -532,6 +534,10 @@ 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_COLORIMETRY:
> > +             g_free(self->colorimetry);
> > +             self->colorimetry = g_value_dup_string(value);
> > +             break;
> >       default:
> >               G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >               break;
> > @@ -549,6 +555,9 @@ 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_COLORIMETRY:
> > +             g_value_set_string(value, self->colorimetry);
> > +             break;
> >       default:
> >               G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >               break;
> > @@ -717,4 +726,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >                                                            | G_PARAM_READWRITE
> >                                                            | G_PARAM_STATIC_STRINGS));
> >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> > +
> > +     GParamSpec *spec2 = g_param_spec_string("colorimetry", "Coloirmetry",
> > +                                             "Colorimetry that will be applied to the StreamConfiguration", nullptr,
> > +                                            (GParamFlags)(GST_PARAM_MUTABLE_READY
> > +                                                          | G_PARAM_READWRITE
> > +                                                          | G_PARAM_STATIC_STRINGS));
>
> As this is a field in caps, the appropriate way to interact in userland is to
> place such feild in downstream caps filter. Properties are not acceptable.
>
>   libcamerasrc ! video/x-raw,colorimetry=bt709 ! ...
>
> What would be a use case though for forcing a specific colorimetry ? Do the V4L2
> stack we have supports this ?
>
> > +     g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
> >  }
>
Nicolas Dufresne July 6, 2022, 2:08 p.m. UTC | #3 
Le mercredi 06 juillet 2022 à 13:28 +0530, Rishikesh Donadkar a écrit :
> > What would be a use case though for forcing a specific colorimetry ? Do the
> > V4L2
> > stack we have supports this ?
> > 
> > > +    
> > > g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
> > >  }
> 
> a use case would be to set a colorspace requested by user with something like
> g_object_set(source, "colorimetry", "bt2020", NULL);
> I have tested that the requested colorimetry gets applied to the
> camera if it passes the validation.

I might have miss-explained, caps fields in GStreamer shall never be replaced by
a property. The caps negotiation process should let you interact with element
(per stream) to select a colorimetry. Something like:

  libcamerasrc ! video/x-raw,colorimetry=bt709 ! ...

Would force bt709 or fail. Something like:

  libcamerasrc ! "video/x-raw,colorimetry={bt709,bt601}" ! ...

Should try bt709 or bt601, or fail. And something like:

  libcamerasrc ! "video/x-raw,colorimetry=bt709;video/x-raw" ! ...

Will ensure bt709 is preffered, but accept anything if that is not possible. So
instead of the properly, the caps negotiation needs to be enhance to try and
match requested colorimetry signalled by downstream. With this method, you can
have different colorimetry per stream/pad if this is supported by the ISP.

Nicolas
> 
> 
> 
> On Mon, Jul 4, 2022 at 11:43 PM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > Le dimanche 03 juillet 2022 à 13:03 +0530, Rishikesh Donadkar a écrit :
> > > Add the colorimetry field in the _GstLibcameraSrc. Add switch cases in
> > > gst_libcamera_src_set_property() and gst_libcamera_src_get_property() to
> > > access and modify the property.
> > > 
> > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> > > ---
> > >  src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > > b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 46fd02d2..120319b3 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -121,6 +121,7 @@ struct _GstLibcameraSrc {
> > >       GstTask *task;
> > > 
> > >       gchar *camera_name;
> > > +     gchar *colorimetry;
> > > 
> > >       GstLibcameraSrcState *state;
> > >       GstLibcameraAllocator *allocator;
> > > @@ -129,7 +130,8 @@ struct _GstLibcameraSrc {
> > > 
> > >  enum {
> > >       PROP_0,
> > > -     PROP_CAMERA_NAME
> > > +     PROP_CAMERA_NAME,
> > > +     PROP_COLORIMETRY,
> > >  };
> > > 
> > >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,
> > > GST_TYPE_ELEMENT,
> > > @@ -532,6 +534,10 @@ 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_COLORIMETRY:
> > > +             g_free(self->colorimetry);
> > > +             self->colorimetry = g_value_dup_string(value);
> > > +             break;
> > >       default:
> > >               G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > >               break;
> > > @@ -549,6 +555,9 @@ 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_COLORIMETRY:
> > > +             g_value_set_string(value, self->colorimetry);
> > > +             break;
> > >       default:
> > >               G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > >               break;
> > > @@ -717,4 +726,11 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass
> > > *klass)
> > >                                                            |
> > > G_PARAM_READWRITE
> > >                                                            |
> > > G_PARAM_STATIC_STRINGS));
> > >       g_object_class_install_property(object_class, PROP_CAMERA_NAME,
> > > spec);
> > > +
> > > +     GParamSpec *spec2 = g_param_spec_string("colorimetry",
> > > "Coloirmetry",
> > > +                                             "Colorimetry that will be
> > > applied to the StreamConfiguration", nullptr,
> > > +                                           
> > > (GParamFlags)(GST_PARAM_MUTABLE_READY
> > > +                                                          |
> > > G_PARAM_READWRITE
> > > +                                                          |
> > > G_PARAM_STATIC_STRINGS));
> > 
> > As this is a field in caps, the appropriate way to interact in userland is
> > to
> > place such feild in downstream caps filter. Properties are not acceptable.
> > 
> >   libcamerasrc ! video/x-raw,colorimetry=bt709 ! ...
> > 
> > What would be a use case though for forcing a specific colorimetry ? Do the
> > V4L2
> > stack we have supports this ?
> > 
> > > +    
> > > g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
> > >  }
> >

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 46fd02d2..120319b3 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -121,6 +121,7 @@  struct _GstLibcameraSrc {
 	GstTask *task;
 
 	gchar *camera_name;
+	gchar *colorimetry;
 
 	GstLibcameraSrcState *state;
 	GstLibcameraAllocator *allocator;
@@ -129,7 +130,8 @@  struct _GstLibcameraSrc {
 
 enum {
 	PROP_0,
-	PROP_CAMERA_NAME
+	PROP_CAMERA_NAME,
+	PROP_COLORIMETRY,
 };
 
 G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
@@ -532,6 +534,10 @@  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_COLORIMETRY:
+		g_free(self->colorimetry);
+		self->colorimetry = g_value_dup_string(value);
+		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
 		break;
@@ -549,6 +555,9 @@  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_COLORIMETRY:
+		g_value_set_string(value, self->colorimetry);
+		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
 		break;
@@ -717,4 +726,11 @@  gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
 							     | G_PARAM_READWRITE
 							     | G_PARAM_STATIC_STRINGS));
 	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
+
+	GParamSpec *spec2 = g_param_spec_string("colorimetry", "Coloirmetry",
+						"Colorimetry that will be applied to the StreamConfiguration", nullptr,
+					       (GParamFlags)(GST_PARAM_MUTABLE_READY
+							     | G_PARAM_READWRITE
+							     | G_PARAM_STATIC_STRINGS));
+	g_object_class_install_property(object_class,PROP_COLORIMETRY,spec2);
 }