gstreamer: Use G_TYPE_UINT type for libcamera::Size
diff mbox series

Message ID 20250729154954.161715-1-uajain@igalia.com
State Rejected
Headers show
Series
  • gstreamer: Use G_TYPE_UINT type for libcamera::Size
Related show

Commit Message

Umang Jain July 29, 2025, 3:49 p.m. UTC
libcamera::Size has two components - width and height, which are of
unsigned integer type. Hence, use the G_TYPE_UINT for initialising
and setting the GValue for libcamera::Size in
gst_libcamera_gvalue_set_size().

Signed-off-by: Umang Jain <uajain@igalia.com>
---
Based on [PATCH v5 0/3] gstreamer: Report camera properties as device properties
---
 src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Nicolas Dufresne July 29, 2025, 4:12 p.m. UTC | #1
Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> libcamera::Size has two components - width and height, which are of
> unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> and setting the GValue for libcamera::Size in
> gst_libcamera_gvalue_set_size().
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> 

Note that in GStreamer, GstVideoInfo.width/height is gint, which have two
benefits, it allows having an invalid value for uninitialized structure, but the
second has to do with GStreamer deserializer, which require no cast for int.

I'm fine with the change, but I wanted to add some context, and the fact that
this will imply using cast when string from + deserializer is used. For video
resolution, the provided range of MAX_INT was way enough.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
> Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> properties
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> b/src/gstreamer/gstlibcamera-utils.cpp
> index ebc43f6c..e13ed43a 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue *value, const
> Point &point)
>  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
>  {
>  	GValue width = G_VALUE_INIT;
> -	g_value_init(&width, G_TYPE_INT);
> -	g_value_set_int(&width, size.width);
> +	g_value_init(&width, G_TYPE_UINT);
> +	g_value_set_uint(&width, size.width);
>  	gst_value_array_append_and_take_value(value, &width);
>  
>  	GValue height = G_VALUE_INIT;
> -	g_value_init(&height, G_TYPE_INT);
> -	g_value_set_int(&height, size.height);
> +	g_value_init(&height, G_TYPE_UINT);
> +	g_value_set_uint(&height, size.height);
>  	gst_value_array_append_and_take_value(value, &height);
>  }
>
Umang Jain July 30, 2025, 4:57 a.m. UTC | #2
Hi Nicolas,

Thanks for the review.

On Tue, Jul 29, 2025 at 12:12:39PM -0400, Nicolas Dufresne wrote:
> Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> > libcamera::Size has two components - width and height, which are of
> > unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> > and setting the GValue for libcamera::Size in
> > gst_libcamera_gvalue_set_size().
> > 
> > Signed-off-by: Umang Jain <uajain@igalia.com>
> > 
> 
> Note that in GStreamer, GstVideoInfo.width/height is gint, which have two
> benefits, it allows having an invalid value for uninitialized structure, but the
> second has to do with GStreamer deserializer, which require no cast for int.

I was not aware of that, thanks for this context.

> 
> I'm fine with the change, but I wanted to add some context, and the fact that
> this will imply using cast when string from + deserializer is used. For video
> resolution, the provided range of MAX_INT was way enough.

I see that signed int usage has defined purpose in gstreamer, should we
align to that  here and probably consider dropping this?

> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> > Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> > properties
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > b/src/gstreamer/gstlibcamera-utils.cpp
> > index ebc43f6c..e13ed43a 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue *value, const
> > Point &point)
> >  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
> >  {
> >  	GValue width = G_VALUE_INIT;
> > -	g_value_init(&width, G_TYPE_INT);
> > -	g_value_set_int(&width, size.width);
> > +	g_value_init(&width, G_TYPE_UINT);
> > +	g_value_set_uint(&width, size.width);
> >  	gst_value_array_append_and_take_value(value, &width);
> >  
> >  	GValue height = G_VALUE_INIT;
> > -	g_value_init(&height, G_TYPE_INT);
> > -	g_value_set_int(&height, size.height);
> > +	g_value_init(&height, G_TYPE_UINT);
> > +	g_value_set_uint(&height, size.height);
> >  	gst_value_array_append_and_take_value(value, &height);
> >  }
> >
Nicolas Dufresne July 30, 2025, 12:45 p.m. UTC | #3
Le mercredi 30 juillet 2025 à 10:27 +0530, Umang Jain a écrit :
> Hi Nicolas,
> 
> Thanks for the review.
> 
> On Tue, Jul 29, 2025 at 12:12:39PM -0400, Nicolas Dufresne wrote:
> > Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> > > libcamera::Size has two components - width and height, which are of
> > > unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> > > and setting the GValue for libcamera::Size in
> > > gst_libcamera_gvalue_set_size().
> > > 
> > > Signed-off-by: Umang Jain <uajain@igalia.com>
> > > 
> > 
> > Note that in GStreamer, GstVideoInfo.width/height is gint, which have two
> > benefits, it allows having an invalid value for uninitialized structure, but
> > the
> > second has to do with GStreamer deserializer, which require no cast for int.
> 
> I was not aware of that, thanks for this context.
> 
> > 
> > I'm fine with the change, but I wanted to add some context, and the fact
> > that
> > this will imply using cast when string from + deserializer is used. For
> > video
> > resolution, the provided range of MAX_INT was way enough.
> 
> I see that signed int usage has defined purpose in gstreamer, should we
> align to that  here and probably consider dropping this?

Its up to you, that helper is only used for the device properties, which are
device specific, so we are free to choose.

> 
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > > ---
> > > Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> > > properties
> > > ---
> > >  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > index ebc43f6c..e13ed43a 100644
> > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue *value,
> > > const
> > > Point &point)
> > >  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
> > >  {
> > >  	GValue width = G_VALUE_INIT;
> > > -	g_value_init(&width, G_TYPE_INT);
> > > -	g_value_set_int(&width, size.width);
> > > +	g_value_init(&width, G_TYPE_UINT);
> > > +	g_value_set_uint(&width, size.width);
> > >  	gst_value_array_append_and_take_value(value, &width);
> > >  
> > >  	GValue height = G_VALUE_INIT;
> > > -	g_value_init(&height, G_TYPE_INT);
> > > -	g_value_set_int(&height, size.height);
> > > +	g_value_init(&height, G_TYPE_UINT);
> > > +	g_value_set_uint(&height, size.height);
> > >  	gst_value_array_append_and_take_value(value, &height);
> > >  }
> > >  
>
Umang Jain July 31, 2025, 1:55 p.m. UTC | #4
On Wed, Jul 30, 2025 at 08:45:55AM -0400, Nicolas Dufresne wrote:
> Le mercredi 30 juillet 2025 à 10:27 +0530, Umang Jain a écrit :
> > Hi Nicolas,
> > 
> > Thanks for the review.
> > 
> > On Tue, Jul 29, 2025 at 12:12:39PM -0400, Nicolas Dufresne wrote:
> > > Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> > > > libcamera::Size has two components - width and height, which are of
> > > > unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> > > > and setting the GValue for libcamera::Size in
> > > > gst_libcamera_gvalue_set_size().
> > > > 
> > > > Signed-off-by: Umang Jain <uajain@igalia.com>
> > > > 
> > > 
> > > Note that in GStreamer, GstVideoInfo.width/height is gint, which have two
> > > benefits, it allows having an invalid value for uninitialized structure, but
> > > the
> > > second has to do with GStreamer deserializer, which require no cast for int.
> > 
> > I was not aware of that, thanks for this context.
> > 
> > > 
> > > I'm fine with the change, but I wanted to add some context, and the fact
> > > that
> > > this will imply using cast when string from + deserializer is used. For
> > > video
> > > resolution, the provided range of MAX_INT was way enough.
> > 
> > I see that signed int usage has defined purpose in gstreamer, should we
> > align to that  here and probably consider dropping this?
> 
> Its up to you, that helper is only used for the device properties, which are
> device specific, so we are free to choose.

It's used for elements properties(i.e. libcamera controls) as well -
where we have to set rectangles (e.g. scaler crops).

> 
> > 
> > > 
> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > 
> > > > ---
> > > > Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> > > > properties
> > > > ---
> > > >  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > index ebc43f6c..e13ed43a 100644
> > > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > > @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue *value,
> > > > const
> > > > Point &point)
> > > >  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
> > > >  {
> > > >  	GValue width = G_VALUE_INIT;
> > > > -	g_value_init(&width, G_TYPE_INT);
> > > > -	g_value_set_int(&width, size.width);
> > > > +	g_value_init(&width, G_TYPE_UINT);
> > > > +	g_value_set_uint(&width, size.width);
> > > >  	gst_value_array_append_and_take_value(value, &width);
> > > >  
> > > >  	GValue height = G_VALUE_INIT;
> > > > -	g_value_init(&height, G_TYPE_INT);
> > > > -	g_value_set_int(&height, size.height);
> > > > +	g_value_init(&height, G_TYPE_UINT);
> > > > +	g_value_set_uint(&height, size.height);
> > > >  	gst_value_array_append_and_take_value(value, &height);
> > > >  }
> > > >  
> >
Nicolas Dufresne July 31, 2025, 2:19 p.m. UTC | #5
Le jeudi 31 juillet 2025 à 19:25 +0530, Umang Jain a écrit :
> On Wed, Jul 30, 2025 at 08:45:55AM -0400, Nicolas Dufresne wrote:
> > Le mercredi 30 juillet 2025 à 10:27 +0530, Umang Jain a écrit :
> > > Hi Nicolas,
> > > 
> > > Thanks for the review.
> > > 
> > > On Tue, Jul 29, 2025 at 12:12:39PM -0400, Nicolas Dufresne wrote:
> > > > Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> > > > > libcamera::Size has two components - width and height, which are of
> > > > > unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> > > > > and setting the GValue for libcamera::Size in
> > > > > gst_libcamera_gvalue_set_size().
> > > > > 
> > > > > Signed-off-by: Umang Jain <uajain@igalia.com>
> > > > > 
> > > > 
> > > > Note that in GStreamer, GstVideoInfo.width/height is gint, which have
> > > > two
> > > > benefits, it allows having an invalid value for uninitialized structure,
> > > > but
> > > > the
> > > > second has to do with GStreamer deserializer, which require no cast for
> > > > int.
> > > 
> > > I was not aware of that, thanks for this context.
> > > 
> > > > 
> > > > I'm fine with the change, but I wanted to add some context, and the fact
> > > > that
> > > > this will imply using cast when string from + deserializer is used. For
> > > > video
> > > > resolution, the provided range of MAX_INT was way enough.
> > > 
> > > I see that signed int usage has defined purpose in gstreamer, should we
> > > align to that  here and probably consider dropping this?
> > 
> > Its up to you, that helper is only used for the device properties, which are
> > device specific, so we are free to choose.
> 
> It's used for elements properties(i.e. libcamera controls) as well -
> where we have to set rectangles (e.g. scaler crops).

Then I'd drop this, since users will be very confused why they have to cast now.

Nicolas

> 
> > 
> > > 
> > > > 
> > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > 
> > > > > ---
> > > > > Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> > > > > properties
> > > > > ---
> > > > >  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
> > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > index ebc43f6c..e13ed43a 100644
> > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue
> > > > > *value,
> > > > > const
> > > > > Point &point)
> > > > >  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
> > > > >  {
> > > > >  	GValue width = G_VALUE_INIT;
> > > > > -	g_value_init(&width, G_TYPE_INT);
> > > > > -	g_value_set_int(&width, size.width);
> > > > > +	g_value_init(&width, G_TYPE_UINT);
> > > > > +	g_value_set_uint(&width, size.width);
> > > > >  	gst_value_array_append_and_take_value(value, &width);
> > > > >  
> > > > >  	GValue height = G_VALUE_INIT;
> > > > > -	g_value_init(&height, G_TYPE_INT);
> > > > > -	g_value_set_int(&height, size.height);
> > > > > +	g_value_init(&height, G_TYPE_UINT);
> > > > > +	g_value_set_uint(&height, size.height);
> > > > >  	gst_value_array_append_and_take_value(value, &height);
> > > > >  }
> > > > >  
> > > 
>
Umang Jain July 31, 2025, 3 p.m. UTC | #6
On Thu, Jul 31, 2025 at 10:19:08AM -0400, Nicolas Dufresne wrote:
> Le jeudi 31 juillet 2025 à 19:25 +0530, Umang Jain a écrit :
> > On Wed, Jul 30, 2025 at 08:45:55AM -0400, Nicolas Dufresne wrote:
> > > Le mercredi 30 juillet 2025 à 10:27 +0530, Umang Jain a écrit :
> > > > Hi Nicolas,
> > > > 
> > > > Thanks for the review.
> > > > 
> > > > On Tue, Jul 29, 2025 at 12:12:39PM -0400, Nicolas Dufresne wrote:
> > > > > Le mardi 29 juillet 2025 à 21:19 +0530, Umang Jain a écrit :
> > > > > > libcamera::Size has two components - width and height, which are of
> > > > > > unsigned integer type. Hence, use the G_TYPE_UINT for initialising
> > > > > > and setting the GValue for libcamera::Size in
> > > > > > gst_libcamera_gvalue_set_size().
> > > > > > 
> > > > > > Signed-off-by: Umang Jain <uajain@igalia.com>
> > > > > > 
> > > > > 
> > > > > Note that in GStreamer, GstVideoInfo.width/height is gint, which have
> > > > > two
> > > > > benefits, it allows having an invalid value for uninitialized structure,
> > > > > but
> > > > > the
> > > > > second has to do with GStreamer deserializer, which require no cast for
> > > > > int.
> > > > 
> > > > I was not aware of that, thanks for this context.
> > > > 
> > > > > 
> > > > > I'm fine with the change, but I wanted to add some context, and the fact
> > > > > that
> > > > > this will imply using cast when string from + deserializer is used. For
> > > > > video
> > > > > resolution, the provided range of MAX_INT was way enough.
> > > > 
> > > > I see that signed int usage has defined purpose in gstreamer, should we
> > > > align to that  here and probably consider dropping this?
> > > 
> > > Its up to you, that helper is only used for the device properties, which are
> > > device specific, so we are free to choose.
> > 
> > It's used for elements properties(i.e. libcamera controls) as well -
> > where we have to set rectangles (e.g. scaler crops).
> 
> Then I'd drop this, since users will be very confused why they have to cast now.
> 

ack, thanks for the discussion!

> Nicolas
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > 
> > > > > > ---
> > > > > > Based on [PATCH v5 0/3] gstreamer: Report camera properties as device
> > > > > > properties
> > > > > > ---
> > > > > >  src/gstreamer/gstlibcamera-utils.cpp | 8 ++++----
> > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > index ebc43f6c..e13ed43a 100644
> > > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > @@ -629,13 +629,13 @@ void gst_libcamera_gvalue_set_point(GValue
> > > > > > *value,
> > > > > > const
> > > > > > Point &point)
> > > > > >  void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
> > > > > >  {
> > > > > >  	GValue width = G_VALUE_INIT;
> > > > > > -	g_value_init(&width, G_TYPE_INT);
> > > > > > -	g_value_set_int(&width, size.width);
> > > > > > +	g_value_init(&width, G_TYPE_UINT);
> > > > > > +	g_value_set_uint(&width, size.width);
> > > > > >  	gst_value_array_append_and_take_value(value, &width);
> > > > > >  
> > > > > >  	GValue height = G_VALUE_INIT;
> > > > > > -	g_value_init(&height, G_TYPE_INT);
> > > > > > -	g_value_set_int(&height, size.height);
> > > > > > +	g_value_init(&height, G_TYPE_UINT);
> > > > > > +	g_value_set_uint(&height, size.height);
> > > > > >  	gst_value_array_append_and_take_value(value, &height);
> > > > > >  }
> > > > > >  
> > > > 
> >

Patch
diff mbox series

diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
index ebc43f6c..e13ed43a 100644
--- a/src/gstreamer/gstlibcamera-utils.cpp
+++ b/src/gstreamer/gstlibcamera-utils.cpp
@@ -629,13 +629,13 @@  void gst_libcamera_gvalue_set_point(GValue *value, const Point &point)
 void gst_libcamera_gvalue_set_size(GValue *value, const Size &size)
 {
 	GValue width = G_VALUE_INIT;
-	g_value_init(&width, G_TYPE_INT);
-	g_value_set_int(&width, size.width);
+	g_value_init(&width, G_TYPE_UINT);
+	g_value_set_uint(&width, size.width);
 	gst_value_array_append_and_take_value(value, &width);
 
 	GValue height = G_VALUE_INIT;
-	g_value_init(&height, G_TYPE_INT);
-	g_value_set_int(&height, size.height);
+	g_value_init(&height, G_TYPE_UINT);
+	g_value_set_uint(&height, size.height);
 	gst_value_array_append_and_take_value(value, &height);
 }