| Message ID | 20250729154954.161715-1-uajain@igalia.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
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); > } >
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); > > } > >
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); > > > } > > > >
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); > > > > } > > > > > >
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); > > > > > } > > > > > > > > >
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); > > > > > > } > > > > > > > > > > > >
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); }
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(-)