| Message ID | 20250704053424.23718-1-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Fri, Jul 04, 2025 at 11:04:24AM +0530, Umang Jain wrote: > Iterate over all libcamera camera properties and report them as > device properties. The values are reported as strings, including the ones > which enum types, for better readability. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > Changes in v3: > - Iterate over all camera properties and report everything. > --- > src/gstreamer/gstlibcameraprovider.cpp | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp > index 5da96ea3..237baeb2 100644 > --- a/src/gstreamer/gstlibcameraprovider.cpp > +++ b/src/gstreamer/gstlibcameraprovider.cpp > @@ -12,6 +12,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > +#include <libcamera/property_ids.h> > > #include "gstlibcamerasrc.h" > #include "gstlibcamera-utils.h" > @@ -130,6 +131,7 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) > { > static const std::array roles{ StreamRole::VideoRecording }; > g_autoptr(GstCaps) caps = gst_caps_new_empty(); > + g_autoptr(GstStructure) props = gst_structure_new_empty("camera-properties"); You can move this just before the for loop below. > const gchar *name = camera->id().c_str(); > > std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles); > @@ -144,12 +146,34 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) > gst_caps_append(caps, sub_caps); > } > > + for (const auto &[key, value] : camera->properties()) { > + const ControlId *id = properties::properties.at(key); > + > + g_autoptr(GString) prop_str = g_string_new("api.libcamera."); > + g_string_append(prop_str, id->name().c_str()); > + > + /* Use string names for enum values for better readability */ s/readability/readability./ > + if (value.type() == ControlTypeInteger32) { > + int32_t val = value.get<int32_t>(); > + const auto &it = id->enumerators().find(val); > + if (it != id->enumerators().end()) { > + gst_structure_set(props, prop_str->str, G_TYPE_STRING, > + it->second.c_str(), nullptr); > + continue; > + } > + } > + > + gst_structure_set(props, prop_str->str, G_TYPE_STRING, > + value.toString().c_str(), nullptr); Instead of using a libcamera-specific internal string representation of the value, it should be converted to native GStreamer types. Numerical properties should use a numerical type, rectangles should be converted to the native GStreamer representation, ... Nicolas, any opinion on this ? > + } > + > return GST_DEVICE(g_object_new(GST_TYPE_LIBCAMERA_DEVICE, > /* \todo Use a unique identifier instead of camera name. */ > "name", name, > "display-name", name, > "caps", caps, > "device-class", "Source/Video", > + "properties", props, > nullptr)); > } >
Le vendredi 04 juillet 2025 à 12:46 +0300, Laurent Pinchart a écrit : > Hi Umang, > > Thank you for the patch. > > On Fri, Jul 04, 2025 at 11:04:24AM +0530, Umang Jain wrote: > > Iterate over all libcamera camera properties and report them as > > device properties. The values are reported as strings, including the ones > > which enum types, for better readability. > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > Changes in v3: > > - Iterate over all camera properties and report everything. Here, outside of commit message, you could pate the output of gst-device- monitor-1.0 Video/Source, that helps to see what kind of output we get. > > --- > > src/gstreamer/gstlibcameraprovider.cpp | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp > > b/src/gstreamer/gstlibcameraprovider.cpp > > index 5da96ea3..237baeb2 100644 > > --- a/src/gstreamer/gstlibcameraprovider.cpp > > +++ b/src/gstreamer/gstlibcameraprovider.cpp > > @@ -12,6 +12,7 @@ > > > > #include <libcamera/camera.h> > > #include <libcamera/camera_manager.h> > > +#include <libcamera/property_ids.h> > > > > #include "gstlibcamerasrc.h" > > #include "gstlibcamera-utils.h" > > @@ -130,6 +131,7 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> > > &camera) > > { > > static const std::array roles{ StreamRole::VideoRecording }; > > g_autoptr(GstCaps) caps = gst_caps_new_empty(); > > + g_autoptr(GstStructure) props = gst_structure_new_empty("camera- > > properties"); > > You can move this just before the for loop below. > > > const gchar *name = camera->id().c_str(); > > > > std::unique_ptr<CameraConfiguration> config = camera- > > >generateConfiguration(roles); > > @@ -144,12 +146,34 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> > > &camera) > > gst_caps_append(caps, sub_caps); > > } > > > > + for (const auto &[key, value] : camera->properties()) { > > + const ControlId *id = properties::properties.at(key); > > + > > + g_autoptr(GString) prop_str = > > g_string_new("api.libcamera."); > > + g_string_append(prop_str, id->name().c_str()); > > + > > + /* Use string names for enum values for better readability > > */ > > s/readability/readability./ > > > + if (value.type() == ControlTypeInteger32) { > > + int32_t val = value.get<int32_t>(); > > + const auto &it = id->enumerators().find(val); > > + if (it != id->enumerators().end()) { > > + gst_structure_set(props, prop_str->str, > > G_TYPE_STRING, > > + it->second.c_str(), > > nullptr); > > + continue; > > + } > > + } > > + > > + gst_structure_set(props, prop_str->str, G_TYPE_STRING, > > + value.toString().c_str(), nullptr); > > Instead of using a libcamera-specific internal string representation of > the value, it should be converted to native GStreamer types. Numerical > properties should use a numerical type, rectangles should be converted > to the native GStreamer representation, ... > > Nicolas, any opinion on this ? This is the first time we have an API we don't have to fully map manually. Looking around, only G_TYPE_UINT and G_TYPE_STRING is currently used by other providers. Its likely a good idea for simplicity. Though, when I look at the vulkan providers, what they did for flags and enum is to have both, e.g. flags + flags_str (no compromise). If you link to Vulkan API, you will find handy the value, and otherwise you have a string version which is human readable. Now, I don't see an "enum" type in the list of libcamera control types. So can this really be handled generically ? Here's that list I found: ControlTypeBool -> UINT ControlTypeByte --> BASE64 STRING ControlTypeUnsigned16 -> UINT ControlTypeUnsigned32 --> UINT ControlTypeInteger32 --> UINT ControlTypeInteger64 --> UINT64 ControlTypeFloat --> DOUBLE ? ControlTypeString --> STRING ControlTypeRectangle --> No match, break in 4 _x, _y, _width, _height? ControlTypeSize --> UINT64 ControlTypePoint --> No match, break in 2 _x, _y ? GST_TYPE_LIST/ARRAY can be used if these comes as an array, you can define your own types + serializers also if you want to have rec/point in a single field. This is not a GStreamer but a GLib thing, GstStructure holds GValue, and offer helper for the most common. Stepping back, if we consider we need manual mapping, why not go back to just exposing the rotation for now, and make it so its easy to map more later ? > > > + } > > + > > return GST_DEVICE(g_object_new(GST_TYPE_LIBCAMERA_DEVICE, > > /* \todo Use a unique identifier > > instead of camera name. */ > > "name", name, > > "display-name", name, > > "caps", caps, > > "device-class", "Source/Video", > > + "properties", props, > > nullptr)); > > } > >
diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp index 5da96ea3..237baeb2 100644 --- a/src/gstreamer/gstlibcameraprovider.cpp +++ b/src/gstreamer/gstlibcameraprovider.cpp @@ -12,6 +12,7 @@ #include <libcamera/camera.h> #include <libcamera/camera_manager.h> +#include <libcamera/property_ids.h> #include "gstlibcamerasrc.h" #include "gstlibcamera-utils.h" @@ -130,6 +131,7 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) { static const std::array roles{ StreamRole::VideoRecording }; g_autoptr(GstCaps) caps = gst_caps_new_empty(); + g_autoptr(GstStructure) props = gst_structure_new_empty("camera-properties"); const gchar *name = camera->id().c_str(); std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles); @@ -144,12 +146,34 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) gst_caps_append(caps, sub_caps); } + for (const auto &[key, value] : camera->properties()) { + const ControlId *id = properties::properties.at(key); + + g_autoptr(GString) prop_str = g_string_new("api.libcamera."); + g_string_append(prop_str, id->name().c_str()); + + /* Use string names for enum values for better readability */ + if (value.type() == ControlTypeInteger32) { + int32_t val = value.get<int32_t>(); + const auto &it = id->enumerators().find(val); + if (it != id->enumerators().end()) { + gst_structure_set(props, prop_str->str, G_TYPE_STRING, + it->second.c_str(), nullptr); + continue; + } + } + + gst_structure_set(props, prop_str->str, G_TYPE_STRING, + value.toString().c_str(), nullptr); + } + return GST_DEVICE(g_object_new(GST_TYPE_LIBCAMERA_DEVICE, /* \todo Use a unique identifier instead of camera name. */ "name", name, "display-name", name, "caps", caps, "device-class", "Source/Video", + "properties", props, nullptr)); }
Iterate over all libcamera camera properties and report them as device properties. The values are reported as strings, including the ones which enum types, for better readability. Signed-off-by: Umang Jain <uajain@igalia.com> --- Changes in v3: - Iterate over all camera properties and report everything. --- src/gstreamer/gstlibcameraprovider.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)