| Message ID | 20220828145630.51618-1-rishikeshdonadkar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Rishikesh, Thank you for the patch. Regarding the subject, you should have mentioend [RFC] as you mentioned you are still working on it and could've mention the missing pieces below. On 8/28/22 8:26 PM, Rishikesh Donadkar wrote: > This patch aims to add framerate support to libcamerasrc in the > direction gstreamer->libcamera. > > Add a field of the type ControlList to GstLibcameraSrcState. > Get the framerate from the caps and convert it to FrameDuration. > Set the FrameDurationLimits control in the initControls_ and pass > the initControls_ at the time of starting camera. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > > --- > Tested the patch with 3 different framerates with the help of the > fpsdisplaysink element. > > https://paste.debian.net/1251951/ > > https://paste.debian.net/1251952/ > > https://paste.debian.net/1251953/ Great results, happy to see. Also you could've mentioned the platform on which you tested. In this case RPi 4 + OV5647 > --- > src/gstreamer/gstlibcamera-utils.cpp | 18 ++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 3 +++ > src/gstreamer/gstlibcamerasrc.cpp | 7 ++++++- > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 5a21a391..7d8519da 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -8,6 +8,7 @@ > > #include "gstlibcamera-utils.h" > > +#include <libcamera/control_ids.h> > #include <libcamera/formats.h> > > using namespace libcamera; > @@ -236,6 +237,23 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > stream_cfg.size.height = height; > } > > +void > +gst_libcamera_configure_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps) > +{ > + /* read framerate from caps - convert to integer and set to frame_time. */ > + GstStructure *s = gst_caps_get_structure(caps, 0); > + gint fps_n = -1, fps_d = -1; > + if (gst_structure_has_field(s, "framerate")) > + gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d); I think if the user input is malformed, get_fraction() will return a FALSE. We should check that for invalid input and log errors. > + > + if (fps_n < 0 || fps_d < 0) > + return; Can it happen that only the numerator is mentioned? Have you tested something like : "framerate=30" instead of "framerate=30/1" > + > + gdouble frame_duration = static_cast<double>(fps_d) / static_cast<double>(fps_n) * 1000000.0; Since frame_duration is int64_t, and fps_d and fps_n are both integers, can we do: int64_t frame_duration = (fps_d * 1000000) / fps_n; ? > + controls.set(controls::FrameDurationLimits, > + Span<const int64_t, 2>({ static_cast<int64_t>(frame_duration), static_cast<int64_t>(frame_duration) })); you could then drop the cast from here I believe > +} > + > #if !GST_CHECK_VERSION(1, 17, 1) > gboolean > gst_task_resume(GstTask *task) > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index 164189a2..1f737e84 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -9,6 +9,7 @@ > #pragma once > > #include <libcamera/camera_manager.h> > +#include <libcamera/controls.h> > #include <libcamera/stream.h> > > #include <gst/gst.h> > @@ -18,6 +19,8 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); > void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > GstCaps *caps); > +void gst_libcamera_configure_controls_from_caps(libcamera::ControlList &controls, GstCaps *caps); nit: To be pedantic, we are only parsing controls needed at camera::StarT(); so maybe gst_libcamera_get_init_ctrls_from_caps(...); ? > + > #if !GST_CHECK_VERSION(1, 17, 1) > gboolean gst_task_resume(GstTask *task); > #endif > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 16d70fea..d1080271 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { > std::queue<std::unique_ptr<RequestWrap>> completedRequests_ > LIBCAMERA_TSA_GUARDED_BY(lock_); > > + ControlList initControls_; > guint group_id_; > > int queueRequest(); > @@ -496,6 +497,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > /* Retrieve the supported caps. */ > g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); > g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter); > + > if (gst_caps_is_empty(caps)) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > break; > @@ -504,6 +506,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > /* Fixate caps and configure the stream. */ > caps = gst_caps_make_writable(caps); > gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > + gst_libcamera_configure_controls_from_caps(state->initControls_, caps); > } > > if (flow_ret != GST_FLOW_OK) > @@ -524,6 +527,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > const StreamConfiguration &stream_cfg = state->config_->at(i); > > g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > + > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > break; > @@ -566,7 +570,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > } > > - ret = state->cam_->start(); > + ret = state->cam_->start(&state->initControls_); > if (ret) { > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > ("Failed to start the camera: %s", g_strerror(-ret)), > @@ -576,6 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > } > > done: > + state->initControls_.clear(); > switch (flow_ret) { > case GST_FLOW_NOT_NEGOTIATED: > GST_ELEMENT_FLOW_ERROR(self, flow_ret);
> > Regarding the subject, you should have mentioend [RFC] as you mentioned > you are still working on it and could've mention the missing pieces below. Noted. > Great results, happy to see. Also you could've mentioned the platform on > which you tested. > In this case RPi 4 + OV5647 > Noted. > > +void > > +gst_libcamera_configure_controls_from_caps(ControlList &controls, > [[maybe_unused]] GstCaps *caps) > > +{ > > + /* read framerate from caps - convert to integer and set to > frame_time. */ > > + GstStructure *s = gst_caps_get_structure(caps, 0); > > + gint fps_n = -1, fps_d = -1; > > + if (gst_structure_has_field(s, "framerate")) > > + gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d); > > I think if the user input is malformed, get_fraction() will return a > FALSE. We should check that for invalid input and log errors. > Okay. > > + > > + if (fps_n < 0 || fps_d < 0) > > + return; > > Can it happen that only the numerator is mentioned? Have you tested > something like : "framerate=30" instead of "framerate=30/1" > I tested for the case "framerate=30". We cannot accept (int)30 for the framerate field that is supposed to be a fraction. Passing 30 fails the negotiation as gst_pad_peer_query_caps() returns empty caps here. g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter); if (gst_caps_is_empty(caps)) { flow_ret = GST_FLOW_NOT_NEGOTIATED; break; } > Since frame_duration is int64_t, and fps_d and fps_n are both integers, > can we do: > > int64_t frame_duration = (fps_d * 1000000) / fps_n; ? > > + controls.set(controls::FrameDurationLimits, > > + Span<const int64_t, 2>({ > static_cast<int64_t>(frame_duration), static_cast<int64_t>(frame_duration) > })); > > you could then drop the cast from here I believe > Right. > nit: To be pedantic, we are only parsing controls needed at > camera::StarT(); so maybe > gst_libcamera_get_init_ctrls_from_caps(...); > ? Yes, this will make more sense.
Hi Rishikesh, one more comment, On 8/29/22 12:17 PM, Umang Jain via libcamera-devel wrote: > Hi Rishikesh, > > Thank you for the patch. > > Regarding the subject, you should have mentioend [RFC] as you > mentioned you are still working on it and could've mention the missing > pieces below. > > On 8/28/22 8:26 PM, Rishikesh Donadkar wrote: >> This patch aims to add framerate support to libcamerasrc in the >> direction gstreamer->libcamera. >> >> Add a field of the type ControlList to GstLibcameraSrcState. >> Get the framerate from the caps and convert it to FrameDuration. >> Set the FrameDurationLimits control in the initControls_ and pass >> the initControls_ at the time of starting camera. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> >> >> --- >> Tested the patch with 3 different framerates with the help of the >> fpsdisplaysink element. >> >> https://paste.debian.net/1251951/ >> >> https://paste.debian.net/1251952/ >> >> https://paste.debian.net/1251953/ > > Great results, happy to see. Also you could've mentioned the platform > on which you tested. > In this case RPi 4 + OV5647 > >> --- >> src/gstreamer/gstlibcamera-utils.cpp | 18 ++++++++++++++++++ >> src/gstreamer/gstlibcamera-utils.h | 3 +++ >> src/gstreamer/gstlibcamerasrc.cpp | 7 ++++++- >> 3 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp >> b/src/gstreamer/gstlibcamera-utils.cpp >> index 5a21a391..7d8519da 100644 >> --- a/src/gstreamer/gstlibcamera-utils.cpp >> +++ b/src/gstreamer/gstlibcamera-utils.cpp >> @@ -8,6 +8,7 @@ >> #include "gstlibcamera-utils.h" >> +#include <libcamera/control_ids.h> >> #include <libcamera/formats.h> >> using namespace libcamera; >> @@ -236,6 +237,23 @@ >> gst_libcamera_configure_stream_from_caps(StreamConfiguration >> &stream_cfg, >> stream_cfg.size.height = height; >> } >> +void >> +gst_libcamera_configure_controls_from_caps(ControlList &controls, >> [[maybe_unused]] GstCaps *caps) >> +{ >> + /* read framerate from caps - convert to integer and set to >> frame_time. */ >> + GstStructure *s = gst_caps_get_structure(caps, 0); >> + gint fps_n = -1, fps_d = -1; >> + if (gst_structure_has_field(s, "framerate")) >> + gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d); > > I think if the user input is malformed, get_fraction() will return a > FALSE. We should check that for invalid input and log errors. >> + >> + if (fps_n < 0 || fps_d < 0) >> + return; > > Can it happen that only the numerator is mentioned? Have you tested > something like : "framerate=30" instead of "framerate=30/1" >> + >> + gdouble frame_duration = static_cast<double>(fps_d) / >> static_cast<double>(fps_n) * 1000000.0; > > Since frame_duration is int64_t, and fps_d and fps_n are both > integers, can we do: > > int64_t frame_duration = (fps_d * 1000000) / fps_n; ? we also need to check the frame_duration whether it is in-bound to the max / min frame-duration supported by the camera itself. If it's lower than the minimum, we should set the frame_duration to minimum If it's larger than the maximum, we should set the frame duration to maximum. We need to do this on the libcamerasrc side, since passing initCtrls_ at camera::start() doesn't tell us if the controls have been applied to the camera successfully or not (or they have adjusted in some fashion). Until that is introduced in camera::start() API, we need to carry out such handling on the application side. >> + controls.set(controls::FrameDurationLimits, >> + Span<const int64_t, 2>({ >> static_cast<int64_t>(frame_duration), >> static_cast<int64_t>(frame_duration) })); > > you could then drop the cast from here I believe >> +} >> + >> #if !GST_CHECK_VERSION(1, 17, 1) >> gboolean >> gst_task_resume(GstTask *task) >> diff --git a/src/gstreamer/gstlibcamera-utils.h >> b/src/gstreamer/gstlibcamera-utils.h >> index 164189a2..1f737e84 100644 >> --- a/src/gstreamer/gstlibcamera-utils.h >> +++ b/src/gstreamer/gstlibcamera-utils.h >> @@ -9,6 +9,7 @@ >> #pragma once >> #include <libcamera/camera_manager.h> >> +#include <libcamera/controls.h> >> #include <libcamera/stream.h> >> #include <gst/gst.h> >> @@ -18,6 +19,8 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const >> libcamera::StreamFormats &fo >> GstCaps *gst_libcamera_stream_configuration_to_caps(const >> libcamera::StreamConfiguration &stream_cfg); >> void >> gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration >> &stream_cfg, >> GstCaps *caps); >> +void >> gst_libcamera_configure_controls_from_caps(libcamera::ControlList >> &controls, GstCaps *caps); > > nit: To be pedantic, we are only parsing controls needed at > camera::StarT(); so maybe > gst_libcamera_get_init_ctrls_from_caps(...); > ? >> + >> #if !GST_CHECK_VERSION(1, 17, 1) >> gboolean gst_task_resume(GstTask *task); >> #endif >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >> b/src/gstreamer/gstlibcamerasrc.cpp >> index 16d70fea..d1080271 100644 >> --- a/src/gstreamer/gstlibcamerasrc.cpp >> +++ b/src/gstreamer/gstlibcamerasrc.cpp >> @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { >> std::queue<std::unique_ptr<RequestWrap>> completedRequests_ >> LIBCAMERA_TSA_GUARDED_BY(lock_); >> + ControlList initControls_; >> guint group_id_; >> int queueRequest(); >> @@ -496,6 +497,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> /* Retrieve the supported caps. */ >> g_autoptr(GstCaps) filter = >> gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); >> g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, >> filter); >> + >> if (gst_caps_is_empty(caps)) { >> flow_ret = GST_FLOW_NOT_NEGOTIATED; >> break; >> @@ -504,6 +506,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> /* Fixate caps and configure the stream. */ >> caps = gst_caps_make_writable(caps); >> gst_libcamera_configure_stream_from_caps(stream_cfg, caps); >> + gst_libcamera_configure_controls_from_caps(state->initControls_, >> caps); >> } >> if (flow_ret != GST_FLOW_OK) >> @@ -524,6 +527,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> const StreamConfiguration &stream_cfg = state->config_->at(i); >> g_autoptr(GstCaps) caps = >> gst_libcamera_stream_configuration_to_caps(stream_cfg); >> + >> if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { >> flow_ret = GST_FLOW_NOT_NEGOTIATED; >> break; >> @@ -566,7 +570,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> gst_flow_combiner_add_pad(self->flow_combiner, srcpad); >> } >> - ret = state->cam_->start(); >> + ret = state->cam_->start(&state->initControls_); >> if (ret) { >> GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, >> ("Failed to start the camera: %s", >> g_strerror(-ret)), >> @@ -576,6 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> } >> done: >> + state->initControls_.clear(); >> switch (flow_ret) { >> case GST_FLOW_NOT_NEGOTIATED: >> GST_ELEMENT_FLOW_ERROR(self, flow_ret); >
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 5a21a391..7d8519da 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -8,6 +8,7 @@ #include "gstlibcamera-utils.h" +#include <libcamera/control_ids.h> #include <libcamera/formats.h> using namespace libcamera; @@ -236,6 +237,23 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, stream_cfg.size.height = height; } +void +gst_libcamera_configure_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps) +{ + /* read framerate from caps - convert to integer and set to frame_time. */ + GstStructure *s = gst_caps_get_structure(caps, 0); + gint fps_n = -1, fps_d = -1; + if (gst_structure_has_field(s, "framerate")) + gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d); + + if (fps_n < 0 || fps_d < 0) + return; + + gdouble frame_duration = static_cast<double>(fps_d) / static_cast<double>(fps_n) * 1000000.0; + controls.set(controls::FrameDurationLimits, + Span<const int64_t, 2>({ static_cast<int64_t>(frame_duration), static_cast<int64_t>(frame_duration) })); +} + #if !GST_CHECK_VERSION(1, 17, 1) gboolean gst_task_resume(GstTask *task) diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index 164189a2..1f737e84 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -9,6 +9,7 @@ #pragma once #include <libcamera/camera_manager.h> +#include <libcamera/controls.h> #include <libcamera/stream.h> #include <gst/gst.h> @@ -18,6 +19,8 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, GstCaps *caps); +void gst_libcamera_configure_controls_from_caps(libcamera::ControlList &controls, GstCaps *caps); + #if !GST_CHECK_VERSION(1, 17, 1) gboolean gst_task_resume(GstTask *task); #endif diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 16d70fea..d1080271 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { std::queue<std::unique_ptr<RequestWrap>> completedRequests_ LIBCAMERA_TSA_GUARDED_BY(lock_); + ControlList initControls_; guint group_id_; int queueRequest(); @@ -496,6 +497,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, /* Retrieve the supported caps. */ g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter); + if (gst_caps_is_empty(caps)) { flow_ret = GST_FLOW_NOT_NEGOTIATED; break; @@ -504,6 +506,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, /* Fixate caps and configure the stream. */ caps = gst_caps_make_writable(caps); gst_libcamera_configure_stream_from_caps(stream_cfg, caps); + gst_libcamera_configure_controls_from_caps(state->initControls_, caps); } if (flow_ret != GST_FLOW_OK) @@ -524,6 +527,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, const StreamConfiguration &stream_cfg = state->config_->at(i); g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { flow_ret = GST_FLOW_NOT_NEGOTIATED; break; @@ -566,7 +570,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, gst_flow_combiner_add_pad(self->flow_combiner, srcpad); } - ret = state->cam_->start(); + ret = state->cam_->start(&state->initControls_); if (ret) { GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, ("Failed to start the camera: %s", g_strerror(-ret)), @@ -576,6 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, } done: + state->initControls_.clear(); switch (flow_ret) { case GST_FLOW_NOT_NEGOTIATED: GST_ELEMENT_FLOW_ERROR(self, flow_ret);