| Message ID | 20230613103519.91370-3-robert.mader@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Robert Mader via libcamera-devel (2023-06-13 11:34:50) > This allows users to request a transform using the Gstreamer > equivalent. If the combined transform of the requested one and a > possible rotation from the camera properties is not fully supported by > the sensor, the remaining transform will be passed down to downstream > elements as tag. 'as a tag' ? (minor could be fixed when applying) > > The later is common for 90/270 degree rotations. Thus, a side effect of > this feature is that libcamerasrc now behaves similar to pipewiresrc in > regards to rotated cameras in e.g. phones, allowing apps to compensate > accordingly. Sounds helpful! > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 72 ++++++++++++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 5 ++ > src/gstreamer/gstlibcamerasrc.cpp | 41 +++++++++++++++- > 3 files changed, 117 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 750ec351..43ce75cb 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -553,3 +553,75 @@ gst_libcamera_get_camera_manager(int &ret) > > return cm; > } > + > +libcamera::Transform > +gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation) > +{ > + switch (orientation) { > + case GST_VIDEO_ORIENTATION_90R: > + return Transform::Rot90; > + case GST_VIDEO_ORIENTATION_180: > + return Transform::Rot180; > + case GST_VIDEO_ORIENTATION_90L: > + return Transform::Rot270; > + case GST_VIDEO_ORIENTATION_HORIZ: > + return Transform::HFlip; > + case GST_VIDEO_ORIENTATION_VERT: > + return Transform::VFlip; > + case GST_VIDEO_ORIENTATION_UL_LR: > + return Transform::Transpose; > + case GST_VIDEO_ORIENTATION_UR_LL: > + return Transform::Rot180Transpose; > + case GST_VIDEO_ORIENTATION_IDENTITY: > + default: > + return Transform::Identity; > + } > +} > + > +GstVideoOrientationMethod > +gst_libcamera_transform_to_orientation(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return GST_VIDEO_ORIENTATION_90R; > + case Transform::Rot180: > + return GST_VIDEO_ORIENTATION_180; > + case Transform::Rot270: > + return GST_VIDEO_ORIENTATION_90L; > + case Transform::HFlip: > + return GST_VIDEO_ORIENTATION_HORIZ; > + case Transform::VFlip: > + return GST_VIDEO_ORIENTATION_VERT; > + case Transform::Transpose: > + return GST_VIDEO_ORIENTATION_UL_LR; > + case Transform::Rot180Transpose: > + return GST_VIDEO_ORIENTATION_UR_LL; > + case Transform::Identity: > + default: > + return GST_VIDEO_ORIENTATION_IDENTITY; > + } > +} > + > +const char * > +gst_libcamera_transform_to_tag_string(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return "rotate-90"; > + case Transform::Rot180: > + return "rotate-180"; > + case Transform::Rot270: > + return "rotate-270"; > + case Transform::HFlip: > + return "flip-rotate-0"; > + case Transform::VFlip: > + return "flip-rotate-180"; > + case Transform::Transpose: > + return "flip-rotate-270"; > + case Transform::Rot180Transpose: > + return "flip-rotate-90"; > + case Transform::Identity: > + default: > + return "rotate-0"; > + } > +} > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index fd304a8b..84d26c47 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -11,6 +11,7 @@ > #include <libcamera/camera_manager.h> > #include <libcamera/controls.h> > #include <libcamera/stream.h> > +#include <libcamera/transform.h> > > #include <gst/gst.h> > #include <gst/video/video.h> > @@ -30,6 +31,10 @@ gboolean gst_task_resume(GstTask *task); > #endif > std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret); > > +libcamera::Transform gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation); > +GstVideoOrientationMethod gst_libcamera_transform_to_orientation(libcamera::Transform transform); > +const char *gst_libcamera_transform_to_tag_string(libcamera::Transform transform); > + > /** > * \class GLibLocker > * \brief A simple scoped mutex locker for GMutex > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 721b35c2..9d9437d0 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -147,6 +147,8 @@ struct _GstLibcameraSrc { > > gchar *camera_name; > > + GstVideoOrientationMethod transform; > + > GstLibcameraSrcState *state; > GstLibcameraAllocator *allocator; > GstFlowCombiner *flow_combiner; > @@ -154,7 +156,9 @@ struct _GstLibcameraSrc { > > enum { > PROP_0, > - PROP_CAMERA_NAME > + PROP_CAMERA_NAME, > + PROP_TRANSFORM, > + PROP_LAST > }; > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT, > @@ -461,6 +465,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GLibRecLocker lock(&self->stream_lock); > GstLibcameraSrcState *state = self->state; > GstFlowReturn flow_ret = GST_FLOW_OK; > + libcamera::Transform tag_transform; > + const char* tag_string; > gint ret; > > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > @@ -513,12 +519,27 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > if (flow_ret != GST_FLOW_OK) > goto done; > > + state->config_->transform = > + gst_libcamera_orientation_to_transform (self->transform); > + Ooof I misred the function as 'gst (libcamera_orientation) to transform' instead of ' gst libcamera "orientation" to "transform" ' So I think this sounds right. I just need to remember that the gst_libcamera is the whole prefix. > /* Validate the configuration. */ > if (state->config_->validate() == CameraConfiguration::Invalid) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > goto done; > } > Given the use of the ^ operator here, which is a little bit of C++ magic, it's probably worth a comment to future readers to say that this block identifies the delta between what was requested and what was possible to expose the actions that still need to be taken. (As I understand it). Is it clear that the 'tag' is an action still needed to take rather than a description of what the image contains in normal gstreamer elements? (Unless I've got it backwards of course, which is entirely possible). > + tag_transform = (gst_libcamera_orientation_to_transform (self->transform) ^ > + state->config_->transform); > + tag_string = gst_libcamera_transform_to_tag_string(tag_transform); > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + GstEvent *tag_event; > + > + tag_event = gst_event_new_tag(gst_tag_list_new(GST_TAG_IMAGE_ORIENTATION, > + tag_string, NULL)); > + gst_pad_push_event (srcpad, tag_event); > + } > + > ret = state->cam_->configure(state->config_.get()); > if (ret) { > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > @@ -659,6 +680,11 @@ 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_TRANSFORM: > + self->transform = > + static_cast<GstVideoOrientationMethod>( > + g_value_get_enum(value)); > + break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -676,6 +702,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_TRANSFORM: > + g_value_set_enum(value, self->transform); > + break; Indentation issues in this one ^. > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -845,4 +874,14 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > | G_PARAM_CONSTRUCT > | G_PARAM_READWRITE > | G_PARAM_STATIC_STRINGS))); > + I know the commit describes that transforms that couldn't be applied will be passed in a tag, but it is probably worth documenting that here too where the property is registered for future developers/readers.. (Unless that's just standard, expected and documented gstreamer behaviour?) > + g_object_class_install_property (object_class, PROP_TRANSFORM, > + g_param_spec_enum ("transform", "Transform", > + "Request a transform (rotation and/or flip).", > + GST_TYPE_VIDEO_ORIENTATION_METHOD, > + GST_VIDEO_ORIENTATION_IDENTITY, > + (GParamFlags)(GST_PARAM_MUTABLE_READY > + | G_PARAM_CONSTRUCT > + | G_PARAM_READWRITE > + | G_PARAM_STATIC_STRINGS))); > } > -- > 2.41.0 >
Le mardi 13 juin 2023 à 12:34 +0200, Robert Mader via libcamera-devel a écrit : > This allows users to request a transform using the Gstreamer > equivalent. If the combined transform of the requested one and a > possible rotation from the camera properties is not fully supported by > the sensor, the remaining transform will be passed down to downstream > elements as tag. > > The later is common for 90/270 degree rotations. Thus, a side effect of > this feature is that libcamerasrc now behaves similar to pipewiresrc in > regards to rotated cameras in e.g. phones, allowing apps to compensate > accordingly. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 72 ++++++++++++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 5 ++ > src/gstreamer/gstlibcamerasrc.cpp | 41 +++++++++++++++- > 3 files changed, 117 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 750ec351..43ce75cb 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -553,3 +553,75 @@ gst_libcamera_get_camera_manager(int &ret) > > return cm; > } > + > +libcamera::Transform > +gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation) > +{ > + switch (orientation) { > + case GST_VIDEO_ORIENTATION_90R: > + return Transform::Rot90; > + case GST_VIDEO_ORIENTATION_180: > + return Transform::Rot180; > + case GST_VIDEO_ORIENTATION_90L: > + return Transform::Rot270; > + case GST_VIDEO_ORIENTATION_HORIZ: > + return Transform::HFlip; > + case GST_VIDEO_ORIENTATION_VERT: > + return Transform::VFlip; > + case GST_VIDEO_ORIENTATION_UL_LR: > + return Transform::Transpose; > + case GST_VIDEO_ORIENTATION_UR_LL: > + return Transform::Rot180Transpose; > + case GST_VIDEO_ORIENTATION_IDENTITY: > + default: > + return Transform::Identity; > + } > +} > + > +GstVideoOrientationMethod > +gst_libcamera_transform_to_orientation(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return GST_VIDEO_ORIENTATION_90R; > + case Transform::Rot180: > + return GST_VIDEO_ORIENTATION_180; > + case Transform::Rot270: > + return GST_VIDEO_ORIENTATION_90L; > + case Transform::HFlip: > + return GST_VIDEO_ORIENTATION_HORIZ; > + case Transform::VFlip: > + return GST_VIDEO_ORIENTATION_VERT; > + case Transform::Transpose: > + return GST_VIDEO_ORIENTATION_UL_LR; > + case Transform::Rot180Transpose: > + return GST_VIDEO_ORIENTATION_UR_LL; > + case Transform::Identity: > + default: > + return GST_VIDEO_ORIENTATION_IDENTITY; > + } > +} > + > +const char * > +gst_libcamera_transform_to_tag_string(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return "rotate-90"; > + case Transform::Rot180: > + return "rotate-180"; > + case Transform::Rot270: > + return "rotate-270"; > + case Transform::HFlip: > + return "flip-rotate-0"; > + case Transform::VFlip: > + return "flip-rotate-180"; > + case Transform::Transpose: > + return "flip-rotate-270"; > + case Transform::Rot180Transpose: > + return "flip-rotate-90"; > + case Transform::Identity: > + default: > + return "rotate-0"; > + } > +} > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index fd304a8b..84d26c47 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -11,6 +11,7 @@ > #include <libcamera/camera_manager.h> > #include <libcamera/controls.h> > #include <libcamera/stream.h> > +#include <libcamera/transform.h> > > #include <gst/gst.h> > #include <gst/video/video.h> > @@ -30,6 +31,10 @@ gboolean gst_task_resume(GstTask *task); > #endif > std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret); > > +libcamera::Transform gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation); > +GstVideoOrientationMethod gst_libcamera_transform_to_orientation(libcamera::Transform transform); > +const char *gst_libcamera_transform_to_tag_string(libcamera::Transform transform); nit: Just to follow the style, I'd been adding empty line between each declaration to ease the readability. The style checker didn't allow to indent it further though. > + > /** > * \class GLibLocker > * \brief A simple scoped mutex locker for GMutex > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 721b35c2..9d9437d0 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -147,6 +147,8 @@ struct _GstLibcameraSrc { > > gchar *camera_name; > > + GstVideoOrientationMethod transform; > + > GstLibcameraSrcState *state; > GstLibcameraAllocator *allocator; > GstFlowCombiner *flow_combiner; > @@ -154,7 +156,9 @@ struct _GstLibcameraSrc { > > enum { > PROP_0, > - PROP_CAMERA_NAME > + PROP_CAMERA_NAME, > + PROP_TRANSFORM, > + PROP_LAST > }; > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT, > @@ -461,6 +465,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GLibRecLocker lock(&self->stream_lock); > GstLibcameraSrcState *state = self->state; > GstFlowReturn flow_ret = GST_FLOW_OK; > + libcamera::Transform tag_transform; > + const char* tag_string; > gint ret; > > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > @@ -513,12 +519,27 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > if (flow_ret != GST_FLOW_OK) > goto done; > > + state->config_->transform = > + gst_libcamera_orientation_to_transform (self->transform); > + > /* Validate the configuration. */ > if (state->config_->validate() == CameraConfiguration::Invalid) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > goto done; > } > > + tag_transform = (gst_libcamera_orientation_to_transform (self->transform) ^ > + state->config_->transform); > + tag_string = gst_libcamera_transform_to_tag_string(tag_transform); > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + GstEvent *tag_event; > + > + tag_event = gst_event_new_tag(gst_tag_list_new(GST_TAG_IMAGE_ORIENTATION, > + tag_string, NULL)); > + gst_pad_push_event (srcpad, tag_event); > + } > + > ret = state->cam_->configure(state->config_.get()); > if (ret) { > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > @@ -659,6 +680,11 @@ 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_TRANSFORM: > + self->transform = > + static_cast<GstVideoOrientationMethod>( > + g_value_get_enum(value)); > + break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -676,6 +702,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_TRANSFORM: > + g_value_set_enum(value, self->transform); > + break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -845,4 +874,14 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > | G_PARAM_CONSTRUCT > | G_PARAM_READWRITE > | G_PARAM_STATIC_STRINGS))); > + > + g_object_class_install_property (object_class, PROP_TRANSFORM, > + g_param_spec_enum ("transform", "Transform", Shouldn't this be "video-direction" and this patch should implement the GstVideoDirection interface ? Every other element in gstreamer do that. > + "Request a transform (rotation and/or flip).", > + GST_TYPE_VIDEO_ORIENTATION_METHOD, > + GST_VIDEO_ORIENTATION_IDENTITY, > + (GParamFlags)(GST_PARAM_MUTABLE_READY > + | G_PARAM_CONSTRUCT > + | G_PARAM_READWRITE > + | G_PARAM_STATIC_STRINGS))); > }
Le mardi 13 juin 2023 à 12:52 +0100, Kieran Bingham via libcamera-devel a écrit : > Quoting Robert Mader via libcamera-devel (2023-06-13 11:34:50) > > This allows users to request a transform using the Gstreamer > > equivalent. If the combined transform of the requested one and a > > possible rotation from the camera properties is not fully supported by > > the sensor, the remaining transform will be passed down to downstream > > elements as tag. > > 'as a tag' ? (minor could be fixed when applying) > > > > > The later is common for 90/270 degree rotations. Thus, a side effect of > > this feature is that libcamerasrc now behaves similar to pipewiresrc in > > regards to rotated cameras in e.g. phones, allowing apps to compensate > > accordingly. > > Sounds helpful! > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 72 ++++++++++++++++++++++++++++ > > src/gstreamer/gstlibcamera-utils.h | 5 ++ > > src/gstreamer/gstlibcamerasrc.cpp | 41 +++++++++++++++- > > 3 files changed, 117 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > index 750ec351..43ce75cb 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -553,3 +553,75 @@ gst_libcamera_get_camera_manager(int &ret) > > > > return cm; > > } > > + > > +libcamera::Transform > > +gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation) > > +{ > > + switch (orientation) { > > + case GST_VIDEO_ORIENTATION_90R: > > + return Transform::Rot90; > > + case GST_VIDEO_ORIENTATION_180: > > + return Transform::Rot180; > > + case GST_VIDEO_ORIENTATION_90L: > > + return Transform::Rot270; > > + case GST_VIDEO_ORIENTATION_HORIZ: > > + return Transform::HFlip; > > + case GST_VIDEO_ORIENTATION_VERT: > > + return Transform::VFlip; > > + case GST_VIDEO_ORIENTATION_UL_LR: > > + return Transform::Transpose; > > + case GST_VIDEO_ORIENTATION_UR_LL: > > + return Transform::Rot180Transpose; > > + case GST_VIDEO_ORIENTATION_IDENTITY: > > + default: > > + return Transform::Identity; > > + } > > +} > > + > > +GstVideoOrientationMethod > > +gst_libcamera_transform_to_orientation(libcamera::Transform transform) > > +{ > > + switch (transform) { > > + case Transform::Rot90: > > + return GST_VIDEO_ORIENTATION_90R; > > + case Transform::Rot180: > > + return GST_VIDEO_ORIENTATION_180; > > + case Transform::Rot270: > > + return GST_VIDEO_ORIENTATION_90L; > > + case Transform::HFlip: > > + return GST_VIDEO_ORIENTATION_HORIZ; > > + case Transform::VFlip: > > + return GST_VIDEO_ORIENTATION_VERT; > > + case Transform::Transpose: > > + return GST_VIDEO_ORIENTATION_UL_LR; > > + case Transform::Rot180Transpose: > > + return GST_VIDEO_ORIENTATION_UR_LL; > > + case Transform::Identity: > > + default: > > + return GST_VIDEO_ORIENTATION_IDENTITY; > > + } > > +} > > + > > +const char * > > +gst_libcamera_transform_to_tag_string(libcamera::Transform transform) > > +{ > > + switch (transform) { > > + case Transform::Rot90: > > + return "rotate-90"; > > + case Transform::Rot180: > > + return "rotate-180"; > > + case Transform::Rot270: > > + return "rotate-270"; > > + case Transform::HFlip: > > + return "flip-rotate-0"; > > + case Transform::VFlip: > > + return "flip-rotate-180"; > > + case Transform::Transpose: > > + return "flip-rotate-270"; > > + case Transform::Rot180Transpose: > > + return "flip-rotate-90"; > > + case Transform::Identity: > > + default: > > + return "rotate-0"; > > + } > > +} > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > index fd304a8b..84d26c47 100644 > > --- a/src/gstreamer/gstlibcamera-utils.h > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -11,6 +11,7 @@ > > #include <libcamera/camera_manager.h> > > #include <libcamera/controls.h> > > #include <libcamera/stream.h> > > +#include <libcamera/transform.h> > > > > #include <gst/gst.h> > > #include <gst/video/video.h> > > @@ -30,6 +31,10 @@ gboolean gst_task_resume(GstTask *task); > > #endif > > std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret); > > > > +libcamera::Transform gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation); > > +GstVideoOrientationMethod gst_libcamera_transform_to_orientation(libcamera::Transform transform); > > +const char *gst_libcamera_transform_to_tag_string(libcamera::Transform transform); > > + > > /** > > * \class GLibLocker > > * \brief A simple scoped mutex locker for GMutex > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 721b35c2..9d9437d0 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -147,6 +147,8 @@ struct _GstLibcameraSrc { > > > > gchar *camera_name; > > > > + GstVideoOrientationMethod transform; > > + > > GstLibcameraSrcState *state; > > GstLibcameraAllocator *allocator; > > GstFlowCombiner *flow_combiner; > > @@ -154,7 +156,9 @@ struct _GstLibcameraSrc { > > > > enum { > > PROP_0, > > - PROP_CAMERA_NAME > > + PROP_CAMERA_NAME, > > + PROP_TRANSFORM, > > + PROP_LAST > > }; > > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT, > > @@ -461,6 +465,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > GLibRecLocker lock(&self->stream_lock); > > GstLibcameraSrcState *state = self->state; > > GstFlowReturn flow_ret = GST_FLOW_OK; > > + libcamera::Transform tag_transform; > > + const char* tag_string; > > gint ret; > > > > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > > @@ -513,12 +519,27 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > if (flow_ret != GST_FLOW_OK) > > goto done; > > > > + state->config_->transform = > > + gst_libcamera_orientation_to_transform (self->transform); > > + > > Ooof I misred the function as 'gst (libcamera_orientation) to transform' > instead of ' gst libcamera "orientation" to "transform" ' > > So I think this sounds right. I just need to remember that the > gst_libcamera is the whole prefix. s/prefix/namespace > > > > /* Validate the configuration. */ > > if (state->config_->validate() == CameraConfiguration::Invalid) { > > flow_ret = GST_FLOW_NOT_NEGOTIATED; > > goto done; > > } > > > > Given the use of the ^ operator here, which is a little bit of C++ > magic, it's probably worth a comment to future readers to say that this > block identifies the delta between what was requested and what was > possible to expose the actions that still need to be taken. (As I > understand it). > > Is it clear that the 'tag' is an action still needed to take rather than a > description of what the image contains in normal gstreamer elements? > > (Unless I've got it backwards of course, which is entirely possible). Its seems a little complicated, I guess it worth digging further. We have: 1. libcamera transform: no idea, is that the transform needed to make the image straight ? 2. Gst orientation-tag: This is what the video will look like if you render it as-is without any transformation. 3. video-direction property: This is a transform the element will apply to the image before delivering it My thinking is that in normal condition you expect the image to come out straight. So if the IPA can correct it, we should by default (video- orientation=auto) just make it happen. No tags, nothing, just a up straight image. If we can't transform in the IPA, then having video-direction=default should probably lead to having one of image-orientation tag or GstVideoAffineTransformationMeta attached to buffers (can't use both at the same time). This allows a downstream element to correct it. Now, if we explicitly ask for a video-direction, that's our call. It cold be that a) its not supported. But if its supported, we need to make a choice. If we ask for video-direction=90r as an example, and the video from the IPA is already that way, maybe it would be logical to just leave it like this. But in this case, there should be no image-orientation tag, since this is the deliberate direction the user wanted to display. With the logic, case a) can be compensated by calculating the oriention the video will look like if 90r was wanted but could be be applied. That also brings me to, maybe you should make two patches. First patch to introduce image-orientation, and second patch to add the video-direction property (using the interface I mention of course) ? > > > > + tag_transform = (gst_libcamera_orientation_to_transform (self->transform) ^ > > + state->config_->transform); > > + tag_string = gst_libcamera_transform_to_tag_string(tag_transform); > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + GstEvent *tag_event; > > + > > + tag_event = gst_event_new_tag(gst_tag_list_new(GST_TAG_IMAGE_ORIENTATION, > > + tag_string, NULL)); > > + gst_pad_push_event (srcpad, tag_event); > > + } > > + > > ret = state->cam_->configure(state->config_.get()); > > if (ret) { > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > @@ -659,6 +680,11 @@ 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_TRANSFORM: > > + self->transform = > > + static_cast<GstVideoOrientationMethod>( > > + g_value_get_enum(value)); > > + break; > > default: > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > > break; > > @@ -676,6 +702,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_TRANSFORM: > > + g_value_set_enum(value, self->transform); > > + break; > > Indentation issues in this one ^. > > > default: > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > > break; > > @@ -845,4 +874,14 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > | G_PARAM_CONSTRUCT > > | G_PARAM_READWRITE > > | G_PARAM_STATIC_STRINGS))); > > + > > I know the commit describes that transforms that couldn't be applied > will be passed in a tag, but it is probably worth documenting that here > too where the property is registered for future developers/readers.. > (Unless that's just standard, expected and documented gstreamer > behaviour?) > > > > > + g_object_class_install_property (object_class, PROP_TRANSFORM, > > + g_param_spec_enum ("transform", "Transform", > > + "Request a transform (rotation and/or flip).", > > + GST_TYPE_VIDEO_ORIENTATION_METHOD, > > + GST_VIDEO_ORIENTATION_IDENTITY, > > + (GParamFlags)(GST_PARAM_MUTABLE_READY > > + | G_PARAM_CONSTRUCT > > + | G_PARAM_READWRITE > > + | G_PARAM_STATIC_STRINGS))); > > } > > -- > > 2.41.0 > >
Hi Robert, On 6/13/23 4:04 PM, Robert Mader via libcamera-devel wrote: > This allows users to request a transform using the Gstreamer > equivalent. If the combined transform of the requested one and a > possible rotation from the camera properties is not fully supported by > the sensor, the remaining transform will be passed down to downstream > elements as tag. > > The later is common for 90/270 degree rotations. Thus, a side effect of > this feature is that libcamerasrc now behaves similar to pipewiresrc in > regards to rotated cameras in e.g. phones, allowing apps to compensate > accordingly. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 72 ++++++++++++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 5 ++ > src/gstreamer/gstlibcamerasrc.cpp | 41 +++++++++++++++- > 3 files changed, 117 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 750ec351..43ce75cb 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -553,3 +553,75 @@ gst_libcamera_get_camera_manager(int &ret) > > return cm; > } > + > +libcamera::Transform > +gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation) > +{ > + switch (orientation) { > + case GST_VIDEO_ORIENTATION_90R: > + return Transform::Rot90; > + case GST_VIDEO_ORIENTATION_180: > + return Transform::Rot180; > + case GST_VIDEO_ORIENTATION_90L: > + return Transform::Rot270; > + case GST_VIDEO_ORIENTATION_HORIZ: > + return Transform::HFlip; > + case GST_VIDEO_ORIENTATION_VERT: > + return Transform::VFlip; > + case GST_VIDEO_ORIENTATION_UL_LR: > + return Transform::Transpose; > + case GST_VIDEO_ORIENTATION_UR_LL: > + return Transform::Rot180Transpose; > + case GST_VIDEO_ORIENTATION_IDENTITY: > + default: > + return Transform::Identity; > + } > +} > + > +GstVideoOrientationMethod > +gst_libcamera_transform_to_orientation(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return GST_VIDEO_ORIENTATION_90R; > + case Transform::Rot180: > + return GST_VIDEO_ORIENTATION_180; > + case Transform::Rot270: > + return GST_VIDEO_ORIENTATION_90L; > + case Transform::HFlip: > + return GST_VIDEO_ORIENTATION_HORIZ; > + case Transform::VFlip: > + return GST_VIDEO_ORIENTATION_VERT; > + case Transform::Transpose: > + return GST_VIDEO_ORIENTATION_UL_LR; > + case Transform::Rot180Transpose: > + return GST_VIDEO_ORIENTATION_UR_LL; > + case Transform::Identity: > + default: > + return GST_VIDEO_ORIENTATION_IDENTITY; > + } > +} > + > +const char * > +gst_libcamera_transform_to_tag_string(libcamera::Transform transform) > +{ > + switch (transform) { > + case Transform::Rot90: > + return "rotate-90"; > + case Transform::Rot180: > + return "rotate-180"; > + case Transform::Rot270: > + return "rotate-270"; > + case Transform::HFlip: > + return "flip-rotate-0"; > + case Transform::VFlip: > + return "flip-rotate-180"; > + case Transform::Transpose: > + return "flip-rotate-270"; > + case Transform::Rot180Transpose: > + return "flip-rotate-90"; > + case Transform::Identity: > + default: > + return "rotate-0"; > + } > +} > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index fd304a8b..84d26c47 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -11,6 +11,7 @@ > #include <libcamera/camera_manager.h> > #include <libcamera/controls.h> > #include <libcamera/stream.h> > +#include <libcamera/transform.h> > > #include <gst/gst.h> > #include <gst/video/video.h> > @@ -30,6 +31,10 @@ gboolean gst_task_resume(GstTask *task); > #endif > std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret); > > +libcamera::Transform gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation); > +GstVideoOrientationMethod gst_libcamera_transform_to_orientation(libcamera::Transform transform); > +const char *gst_libcamera_transform_to_tag_string(libcamera::Transform transform); > + > /** > * \class GLibLocker > * \brief A simple scoped mutex locker for GMutex > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 721b35c2..9d9437d0 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -147,6 +147,8 @@ struct _GstLibcameraSrc { > > gchar *camera_name; > > + GstVideoOrientationMethod transform; > + > GstLibcameraSrcState *state; > GstLibcameraAllocator *allocator; > GstFlowCombiner *flow_combiner; > @@ -154,7 +156,9 @@ struct _GstLibcameraSrc { > > enum { > PROP_0, > - PROP_CAMERA_NAME > + PROP_CAMERA_NAME, > + PROP_TRANSFORM, > + PROP_LAST > }; > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT, > @@ -461,6 +465,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GLibRecLocker lock(&self->stream_lock); > GstLibcameraSrcState *state = self->state; > GstFlowReturn flow_ret = GST_FLOW_OK; > + libcamera::Transform tag_transform; > + const char* tag_string; > gint ret; > > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > @@ -513,12 +519,27 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > if (flow_ret != GST_FLOW_OK) > goto done; > > + state->config_->transform = > + gst_libcamera_orientation_to_transform (self->transform); > + > /* Validate the configuration. */ > if (state->config_->validate() == CameraConfiguration::Invalid) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > goto done; > } > > + tag_transform = (gst_libcamera_orientation_to_transform (self->transform) ^ > + state->config_->transform); > + tag_string = gst_libcamera_transform_to_tag_string(tag_transform); > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + GstEvent *tag_event; > + > + tag_event = gst_event_new_tag(gst_tag_list_new(GST_TAG_IMAGE_ORIENTATION, > + tag_string, NULL)); How about separating this to a dedicated function with ability to add more new tags easily for the downstream pipeline? > + gst_pad_push_event (srcpad, tag_event); > + } > + > ret = state->cam_->configure(state->config_.get()); > if (ret) { > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > @@ -659,6 +680,11 @@ 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_TRANSFORM: > + self->transform = > + static_cast<GstVideoOrientationMethod>( > + g_value_get_enum(value)); > + break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -676,6 +702,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_TRANSFORM: > + g_value_set_enum(value, self->transform); > + break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); > break; > @@ -845,4 +874,14 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > | G_PARAM_CONSTRUCT > | G_PARAM_READWRITE > | G_PARAM_STATIC_STRINGS))); > + > + g_object_class_install_property (object_class, PROP_TRANSFORM, > + g_param_spec_enum ("transform", "Transform", > + "Request a transform (rotation and/or flip).", > + GST_TYPE_VIDEO_ORIENTATION_METHOD, > + GST_VIDEO_ORIENTATION_IDENTITY, > + (GParamFlags)(GST_PARAM_MUTABLE_READY > + | G_PARAM_CONSTRUCT > + | G_PARAM_READWRITE > + | G_PARAM_STATIC_STRINGS))); > }
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 750ec351..43ce75cb 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -553,3 +553,75 @@ gst_libcamera_get_camera_manager(int &ret) return cm; } + +libcamera::Transform +gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation) +{ + switch (orientation) { + case GST_VIDEO_ORIENTATION_90R: + return Transform::Rot90; + case GST_VIDEO_ORIENTATION_180: + return Transform::Rot180; + case GST_VIDEO_ORIENTATION_90L: + return Transform::Rot270; + case GST_VIDEO_ORIENTATION_HORIZ: + return Transform::HFlip; + case GST_VIDEO_ORIENTATION_VERT: + return Transform::VFlip; + case GST_VIDEO_ORIENTATION_UL_LR: + return Transform::Transpose; + case GST_VIDEO_ORIENTATION_UR_LL: + return Transform::Rot180Transpose; + case GST_VIDEO_ORIENTATION_IDENTITY: + default: + return Transform::Identity; + } +} + +GstVideoOrientationMethod +gst_libcamera_transform_to_orientation(libcamera::Transform transform) +{ + switch (transform) { + case Transform::Rot90: + return GST_VIDEO_ORIENTATION_90R; + case Transform::Rot180: + return GST_VIDEO_ORIENTATION_180; + case Transform::Rot270: + return GST_VIDEO_ORIENTATION_90L; + case Transform::HFlip: + return GST_VIDEO_ORIENTATION_HORIZ; + case Transform::VFlip: + return GST_VIDEO_ORIENTATION_VERT; + case Transform::Transpose: + return GST_VIDEO_ORIENTATION_UL_LR; + case Transform::Rot180Transpose: + return GST_VIDEO_ORIENTATION_UR_LL; + case Transform::Identity: + default: + return GST_VIDEO_ORIENTATION_IDENTITY; + } +} + +const char * +gst_libcamera_transform_to_tag_string(libcamera::Transform transform) +{ + switch (transform) { + case Transform::Rot90: + return "rotate-90"; + case Transform::Rot180: + return "rotate-180"; + case Transform::Rot270: + return "rotate-270"; + case Transform::HFlip: + return "flip-rotate-0"; + case Transform::VFlip: + return "flip-rotate-180"; + case Transform::Transpose: + return "flip-rotate-270"; + case Transform::Rot180Transpose: + return "flip-rotate-90"; + case Transform::Identity: + default: + return "rotate-0"; + } +} diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index fd304a8b..84d26c47 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -11,6 +11,7 @@ #include <libcamera/camera_manager.h> #include <libcamera/controls.h> #include <libcamera/stream.h> +#include <libcamera/transform.h> #include <gst/gst.h> #include <gst/video/video.h> @@ -30,6 +31,10 @@ gboolean gst_task_resume(GstTask *task); #endif std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret); +libcamera::Transform gst_libcamera_orientation_to_transform(GstVideoOrientationMethod orientation); +GstVideoOrientationMethod gst_libcamera_transform_to_orientation(libcamera::Transform transform); +const char *gst_libcamera_transform_to_tag_string(libcamera::Transform transform); + /** * \class GLibLocker * \brief A simple scoped mutex locker for GMutex diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 721b35c2..9d9437d0 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -147,6 +147,8 @@ struct _GstLibcameraSrc { gchar *camera_name; + GstVideoOrientationMethod transform; + GstLibcameraSrcState *state; GstLibcameraAllocator *allocator; GstFlowCombiner *flow_combiner; @@ -154,7 +156,9 @@ struct _GstLibcameraSrc { enum { PROP_0, - PROP_CAMERA_NAME + PROP_CAMERA_NAME, + PROP_TRANSFORM, + PROP_LAST }; G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT, @@ -461,6 +465,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, GLibRecLocker lock(&self->stream_lock); GstLibcameraSrcState *state = self->state; GstFlowReturn flow_ret = GST_FLOW_OK; + libcamera::Transform tag_transform; + const char* tag_string; gint ret; g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); @@ -513,12 +519,27 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, if (flow_ret != GST_FLOW_OK) goto done; + state->config_->transform = + gst_libcamera_orientation_to_transform (self->transform); + /* Validate the configuration. */ if (state->config_->validate() == CameraConfiguration::Invalid) { flow_ret = GST_FLOW_NOT_NEGOTIATED; goto done; } + tag_transform = (gst_libcamera_orientation_to_transform (self->transform) ^ + state->config_->transform); + tag_string = gst_libcamera_transform_to_tag_string(tag_transform); + for (gsize i = 0; i < state->srcpads_.size(); i++) { + GstPad *srcpad = state->srcpads_[i]; + GstEvent *tag_event; + + tag_event = gst_event_new_tag(gst_tag_list_new(GST_TAG_IMAGE_ORIENTATION, + tag_string, NULL)); + gst_pad_push_event (srcpad, tag_event); + } + ret = state->cam_->configure(state->config_.get()); if (ret) { GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, @@ -659,6 +680,11 @@ 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_TRANSFORM: + self->transform = + static_cast<GstVideoOrientationMethod>( + g_value_get_enum(value)); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -676,6 +702,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_TRANSFORM: + g_value_set_enum(value, self->transform); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -845,4 +874,14 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS))); + + g_object_class_install_property (object_class, PROP_TRANSFORM, + g_param_spec_enum ("transform", "Transform", + "Request a transform (rotation and/or flip).", + GST_TYPE_VIDEO_ORIENTATION_METHOD, + GST_VIDEO_ORIENTATION_IDENTITY, + (GParamFlags)(GST_PARAM_MUTABLE_READY + | G_PARAM_CONSTRUCT + | G_PARAM_READWRITE + | G_PARAM_STATIC_STRINGS))); }
This allows users to request a transform using the Gstreamer equivalent. If the combined transform of the requested one and a possible rotation from the camera properties is not fully supported by the sensor, the remaining transform will be passed down to downstream elements as tag. The later is common for 90/270 degree rotations. Thus, a side effect of this feature is that libcamerasrc now behaves similar to pipewiresrc in regards to rotated cameras in e.g. phones, allowing apps to compensate accordingly. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/gstreamer/gstlibcamera-utils.cpp | 72 ++++++++++++++++++++++++++++ src/gstreamer/gstlibcamera-utils.h | 5 ++ src/gstreamer/gstlibcamerasrc.cpp | 41 +++++++++++++++- 3 files changed, 117 insertions(+), 1 deletion(-)