| Message ID | 20231130155323.13259-2-jaslo@ziska.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Le jeudi 30 novembre 2023 à 16:43 +0100, Jaslo Ziska via libcamera-devel a écrit : > Move the code which negotiates all the source pad caps into a separate > function called gst_libcamera_src_negotiate. When the negotiation fails > this function will return FALSE and TRUE otherwise. true and false Since it has been ported, but just a nit, I suppose maintainers can deal with such edit. > > Use this function instead of doing the negotiation manually in > gst_libcamera_src_task_enter and remove the now redundant error handling > code accordingly. > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 178 +++++++++++++++--------------- > 1 file changed, 89 insertions(+), 89 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index a6f240f5..e57ba52f 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -379,6 +379,88 @@ gst_libcamera_src_open(GstLibcameraSrc *self) > return true; > } > > +/* Must be called with stream_lock held. */ > +static bool > +gst_libcamera_src_negotiate(GstLibcameraSrc *self) > +{ > + GstLibcameraSrcState *state = self->state; > + > + g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > + > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + StreamConfiguration &stream_cfg = state->config_->at(i); > + > + /* 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)) > + return false; > + > + /* Fixate caps and configure the stream. */ > + caps = gst_caps_make_writable(caps); > + gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > + gst_libcamera_get_framerate_from_caps(caps, element_caps); > + } > + > + /* Validate the configuration. */ > + if (state->config_->validate() == CameraConfiguration::Invalid) > + return false; > + > + int ret = state->cam_->configure(state->config_.get()); > + if (ret) { > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > + ("Failed to configure camera: %s", g_strerror(-ret)), > + ("Camera::configure() failed with error code %i", ret)); > + return false; > + } > + > + /* Check frame duration bounds within controls::FrameDurationLimits */ > + gst_libcamera_clamp_and_set_frameduration(state->initControls_, > + state->cam_->controls(), element_caps); > + > + /* > + * Regardless if it has been modified, create clean caps and push the > + * caps event. Downstream will decide if the caps are acceptable. > + */ > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + const StreamConfiguration &stream_cfg = state->config_->at(i); > + > + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > + gst_libcamera_framerate_to_caps(caps, element_caps); > + > + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) > + return false; > + > + } > + > + if (self->allocator) > + g_clear_object(&self->allocator); > + > + self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); > + if (!self->allocator) { > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > + ("Failed to allocate memory"), > + ("gst_libcamera_allocator_new() failed.")); > + return false; > + } > + > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + const StreamConfiguration &stream_cfg = state->config_->at(i); > + > + GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, > + stream_cfg.stream()); > + g_signal_connect_swapped(pool, "buffer-notify", > + G_CALLBACK(gst_task_resume), self->task); > + > + gst_libcamera_pad_set_pool(srcpad, pool); > + } > + > + return true; > +} > + > static void > gst_libcamera_src_task_run(gpointer user_data) > { > @@ -468,11 +550,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > GLibRecLocker lock(&self->stream_lock); > GstLibcameraSrcState *state = self->state; > - GstFlowReturn flow_ret = GST_FLOW_OK; > gint ret; > > - g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > - > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > gint stream_id_num = 0; > @@ -500,62 +579,16 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > } > g_assert(state->config_->size() == state->srcpads_.size()); > > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > - GstPad *srcpad = state->srcpads_[i]; > - StreamConfiguration &stream_cfg = state->config_->at(i); > - > - /* 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; > - } > - > - /* Fixate caps and configure the stream. */ > - caps = gst_caps_make_writable(caps); > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > - gst_libcamera_get_framerate_from_caps(caps, element_caps); > - } > - > - if (flow_ret != GST_FLOW_OK) > - goto done; > - > - /* Validate the configuration. */ > - if (state->config_->validate() == CameraConfiguration::Invalid) { > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > - goto done; > - } > - > - ret = state->cam_->configure(state->config_.get()); > - if (ret) { > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > - ("Failed to configure camera: %s", g_strerror(-ret)), > - ("Camera::configure() failed with error code %i", ret)); > + if (!gst_libcamera_src_negotiate(self)) { > + state->initControls_.clear(); > + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); > gst_task_stop(task); > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > - goto done; > + return; > } > > - /* Check frame duration bounds within controls::FrameDurationLimits */ > - gst_libcamera_clamp_and_set_frameduration(state->initControls_, > - state->cam_->controls(), element_caps); > - > - /* > - * Regardless if it has been modified, create clean caps and push the > - * caps event. Downstream will decide if the caps are acceptable. > - */ > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > - GstPad *srcpad = state->srcpads_[i]; > - const StreamConfiguration &stream_cfg = state->config_->at(i); > - > - g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > - gst_libcamera_framerate_to_caps(caps, element_caps); > - > - if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > - break; > - } > + self->flow_combiner = gst_flow_combiner_new(); > + for (GstPad *srcpad : state->srcpads_) { > + gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > > /* Send an open segment event with time format. */ > GstSegment segment; > @@ -563,28 +596,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > gst_pad_push_event(srcpad, gst_event_new_segment(&segment)); > } > > - self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); > - if (!self->allocator) { > - GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > - ("Failed to allocate memory"), > - ("gst_libcamera_allocator_new() failed.")); > - gst_task_stop(task); > - return; > - } > - > - self->flow_combiner = gst_flow_combiner_new(); > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > - GstPad *srcpad = state->srcpads_[i]; > - const StreamConfiguration &stream_cfg = state->config_->at(i); > - GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, > - stream_cfg.stream()); > - g_signal_connect_swapped(pool, "buffer-notify", > - G_CALLBACK(gst_task_resume), task); > - > - gst_libcamera_pad_set_pool(srcpad, pool); > - gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > - } > - > if (self->auto_focus_mode != controls::AfModeManual) { > const ControlInfoMap &infoMap = state->cam_->controls(); > if (infoMap.find(&controls::AfMode) != infoMap.end()) { > @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > gst_task_stop(task); > return; > } > - > -done: > - state->initControls_.clear(); > - switch (flow_ret) { > - case GST_FLOW_NOT_NEGOTIATED: > - GST_ELEMENT_FLOW_ERROR(self, flow_ret); > - gst_task_stop(task); > - break; > - default: > - break; > - } > } > > static void
On Tue, Dec 05, 2023 at 11:10:42AM -0500, Nicolas Dufresne via libcamera-devel wrote: > Le jeudi 30 novembre 2023 à 16:43 +0100, Jaslo Ziska via libcamera-devel a écrit : > > Move the code which negotiates all the source pad caps into a separate > > function called gst_libcamera_src_negotiate. When the negotiation fails > > this function will return FALSE and TRUE otherwise. > > true and false > > Since it has been ported, but just a nit, I suppose maintainers can deal with > such edit. Sure, I'll update the commit message. I'll also add () after function names, as customary. > > Use this function instead of doing the negotiation manually in > > gst_libcamera_src_task_enter and remove the now redundant error handling > > code accordingly. > > > > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 178 +++++++++++++++--------------- > > 1 file changed, 89 insertions(+), 89 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index a6f240f5..e57ba52f 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -379,6 +379,88 @@ gst_libcamera_src_open(GstLibcameraSrc *self) > > return true; > > } > > > > +/* Must be called with stream_lock held. */ > > +static bool > > +gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > +{ > > + GstLibcameraSrcState *state = self->state; > > + > > + g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > > + > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + StreamConfiguration &stream_cfg = state->config_->at(i); > > + > > + /* 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)) > > + return false; > > + > > + /* Fixate caps and configure the stream. */ > > + caps = gst_caps_make_writable(caps); > > + gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > > + gst_libcamera_get_framerate_from_caps(caps, element_caps); > > + } > > + > > + /* Validate the configuration. */ > > + if (state->config_->validate() == CameraConfiguration::Invalid) > > + return false; > > + > > + int ret = state->cam_->configure(state->config_.get()); > > + if (ret) { > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > + ("Failed to configure camera: %s", g_strerror(-ret)), > > + ("Camera::configure() failed with error code %i", ret)); > > + return false; > > + } > > + > > + /* Check frame duration bounds within controls::FrameDurationLimits */ > > + gst_libcamera_clamp_and_set_frameduration(state->initControls_, > > + state->cam_->controls(), element_caps); > > + > > + /* > > + * Regardless if it has been modified, create clean caps and push the > > + * caps event. Downstream will decide if the caps are acceptable. > > + */ > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + const StreamConfiguration &stream_cfg = state->config_->at(i); > > + > > + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > > + gst_libcamera_framerate_to_caps(caps, element_caps); > > + > > + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) > > + return false; > > + Extra blank line, I'll remove it when applying the series if no other changes require a new version to be posted. By the way, Jaslo, we have a tool called checkstyle.py to detect coding style issues. You can enable it as a git commit hook as explained at the end of Documentation/coding-style.rst. I've tried to review the patch to the best of my knowledge of the GStreamer element. The result looks cleaner. There are some behavioural changes that I can't really judge, but nothing appears wrong. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + } > > + > > + if (self->allocator) > > + g_clear_object(&self->allocator); > > + > > + self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); > > + if (!self->allocator) { > > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > + ("Failed to allocate memory"), > > + ("gst_libcamera_allocator_new() failed.")); > > + return false; > > + } > > + > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + const StreamConfiguration &stream_cfg = state->config_->at(i); > > + > > + GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, > > + stream_cfg.stream()); > > + g_signal_connect_swapped(pool, "buffer-notify", > > + G_CALLBACK(gst_task_resume), self->task); > > + > > + gst_libcamera_pad_set_pool(srcpad, pool); > > + } > > + > > + return true; > > +} > > + > > static void > > gst_libcamera_src_task_run(gpointer user_data) > > { > > @@ -468,11 +550,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > GLibRecLocker lock(&self->stream_lock); > > GstLibcameraSrcState *state = self->state; > > - GstFlowReturn flow_ret = GST_FLOW_OK; > > gint ret; > > > > - g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); > > - > > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > > > gint stream_id_num = 0; > > @@ -500,62 +579,16 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > } > > g_assert(state->config_->size() == state->srcpads_.size()); > > > > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > > - GstPad *srcpad = state->srcpads_[i]; > > - StreamConfiguration &stream_cfg = state->config_->at(i); > > - > > - /* 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; > > - } > > - > > - /* Fixate caps and configure the stream. */ > > - caps = gst_caps_make_writable(caps); > > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > > - gst_libcamera_get_framerate_from_caps(caps, element_caps); > > - } > > - > > - if (flow_ret != GST_FLOW_OK) > > - goto done; > > - > > - /* Validate the configuration. */ > > - if (state->config_->validate() == CameraConfiguration::Invalid) { > > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > > - goto done; > > - } > > - > > - ret = state->cam_->configure(state->config_.get()); > > - if (ret) { > > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > - ("Failed to configure camera: %s", g_strerror(-ret)), > > - ("Camera::configure() failed with error code %i", ret)); > > + if (!gst_libcamera_src_negotiate(self)) { > > + state->initControls_.clear(); > > + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); > > gst_task_stop(task); > > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > > - goto done; > > + return; > > } > > > > - /* Check frame duration bounds within controls::FrameDurationLimits */ > > - gst_libcamera_clamp_and_set_frameduration(state->initControls_, > > - state->cam_->controls(), element_caps); > > - > > - /* > > - * Regardless if it has been modified, create clean caps and push the > > - * caps event. Downstream will decide if the caps are acceptable. > > - */ > > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > > - GstPad *srcpad = state->srcpads_[i]; > > - const StreamConfiguration &stream_cfg = state->config_->at(i); > > - > > - g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > > - gst_libcamera_framerate_to_caps(caps, element_caps); > > - > > - if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { > > - flow_ret = GST_FLOW_NOT_NEGOTIATED; > > - break; > > - } > > + self->flow_combiner = gst_flow_combiner_new(); > > + for (GstPad *srcpad : state->srcpads_) { > > + gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > > > > /* Send an open segment event with time format. */ > > GstSegment segment; > > @@ -563,28 +596,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > gst_pad_push_event(srcpad, gst_event_new_segment(&segment)); > > } > > > > - self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); > > - if (!self->allocator) { > > - GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > - ("Failed to allocate memory"), > > - ("gst_libcamera_allocator_new() failed.")); > > - gst_task_stop(task); > > - return; > > - } > > - > > - self->flow_combiner = gst_flow_combiner_new(); > > - for (gsize i = 0; i < state->srcpads_.size(); i++) { > > - GstPad *srcpad = state->srcpads_[i]; > > - const StreamConfiguration &stream_cfg = state->config_->at(i); > > - GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, > > - stream_cfg.stream()); > > - g_signal_connect_swapped(pool, "buffer-notify", > > - G_CALLBACK(gst_task_resume), task); > > - > > - gst_libcamera_pad_set_pool(srcpad, pool); > > - gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > > - } > > - > > if (self->auto_focus_mode != controls::AfModeManual) { > > const ControlInfoMap &infoMap = state->cam_->controls(); > > if (infoMap.find(&controls::AfMode) != infoMap.end()) { > > @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > gst_task_stop(task); > > return; > > } > > - > > -done: > > - state->initControls_.clear(); > > - switch (flow_ret) { > > - case GST_FLOW_NOT_NEGOTIATED: > > - GST_ELEMENT_FLOW_ERROR(self, flow_ret); > > - gst_task_stop(task); > > - break; > > - default: > > - break; > > - } > > } > > > > static void
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Tue, Dec 05, 2023 at 11:10:42AM -0500, Nicolas Dufresne via > libcamera-devel wrote: >> Le jeudi 30 novembre 2023 à 16:43 +0100, Jaslo Ziska via >> libcamera-devel a écrit : >> > Move the code which negotiates all the source pad caps into a >> > separate >> > function called gst_libcamera_src_negotiate. When the >> > negotiation fails >> > this function will return FALSE and TRUE otherwise. >> >> true and false >> >> Since it has been ported, but just a nit, I suppose maintainers >> can deal with >> such edit. > > Sure, I'll update the commit message. I'll also add () after > function > names, as customary. > >> > Use this function instead of doing the negotiation manually >> > in >> > gst_libcamera_src_task_enter and remove the now redundant >> > error handling >> > code accordingly. >> > >> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de> >> >> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> >> >> > --- >> > src/gstreamer/gstlibcamerasrc.cpp | 178 >> > +++++++++++++++--------------- >> > 1 file changed, 89 insertions(+), 89 deletions(-) >> > >> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp >> > b/src/gstreamer/gstlibcamerasrc.cpp >> > index a6f240f5..e57ba52f 100644 >> > --- a/src/gstreamer/gstlibcamerasrc.cpp >> > +++ b/src/gstreamer/gstlibcamerasrc.cpp >> > @@ -379,6 +379,88 @@ gst_libcamera_src_open(GstLibcameraSrc >> > *self) >> > return true; >> > } >> > >> > +/* Must be called with stream_lock held. */ >> > +static bool >> > +gst_libcamera_src_negotiate(GstLibcameraSrc *self) >> > +{ >> > + GstLibcameraSrcState *state = self->state; >> > + >> > + g_autoptr(GstStructure) element_caps = >> > gst_structure_new_empty("caps"); >> > + >> > + for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > + GstPad *srcpad = state->srcpads_[i]; >> > + StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > + >> > + /* 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)) >> > + return false; >> > + >> > + /* Fixate caps and configure the stream. */ >> > + caps = gst_caps_make_writable(caps); >> > + gst_libcamera_configure_stream_from_caps(stream_cfg, >> > caps); >> > + gst_libcamera_get_framerate_from_caps(caps, >> > element_caps); >> > + } >> > + >> > + /* Validate the configuration. */ >> > + if (state->config_->validate() == >> > CameraConfiguration::Invalid) >> > + return false; >> > + >> > + int ret = state->cam_->configure(state->config_.get()); >> > + if (ret) { >> > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, >> > + ("Failed to configure camera: %s", >> > g_strerror(-ret)), >> > + ("Camera::configure() failed with error code >> > %i", ret)); >> > + return false; >> > + } >> > + >> > + /* Check frame duration bounds within >> > controls::FrameDurationLimits */ >> > + >> > gst_libcamera_clamp_and_set_frameduration(state->initControls_, >> > + state->cam_->controls(), >> > element_caps); >> > + >> > + /* >> > + * Regardless if it has been modified, create clean caps >> > and push the >> > + * caps event. Downstream will decide if the caps are >> > acceptable. >> > + */ >> > + for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > + GstPad *srcpad = state->srcpads_[i]; >> > + const StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > + >> > + g_autoptr(GstCaps) caps = >> > gst_libcamera_stream_configuration_to_caps(stream_cfg); >> > + gst_libcamera_framerate_to_caps(caps, element_caps); >> > + >> > + if (!gst_pad_push_event(srcpad, >> > gst_event_new_caps(caps))) >> > + return false; >> > + > > Extra blank line, I'll remove it when applying the series if no > other > changes require a new version to be posted. > > By the way, Jaslo, we have a tool called checkstyle.py to detect > coding > style issues. You can enable it as a git commit hook as > explained at the > end of Documentation/coding-style.rst. > > I've tried to review the patch to the best of my knowledge of > the > GStreamer element. The result looks cleaner. There are some > behavioural > changes that I can't really judge, but nothing appears wrong. > > Reviewed-by: Laurent Pinchart > <laurent.pinchart@ideasonboard.com> > Thanks for the reviews and taking for care of this. I did not notice the checkstyle.py tool before, thanks for the tip. Regards, Jaslo >> > + } >> > + >> > + if (self->allocator) >> > + g_clear_object(&self->allocator); >> > + >> > + self->allocator = gst_libcamera_allocator_new(state->cam_, >> > state->config_.get()); >> > + if (!self->allocator) { >> > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, >> > + ("Failed to allocate memory"), >> > + ("gst_libcamera_allocator_new() failed.")); >> > + return false; >> > + } >> > + >> > + for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > + GstPad *srcpad = state->srcpads_[i]; >> > + const StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > + >> > + GstLibcameraPool *pool = >> > gst_libcamera_pool_new(self->allocator, >> > + stream_cfg.stream()); >> > + g_signal_connect_swapped(pool, "buffer-notify", >> > + G_CALLBACK(gst_task_resume), self->task); >> > + >> > + gst_libcamera_pad_set_pool(srcpad, pool); >> > + } >> > + >> > + return true; >> > +} >> > + >> > static void >> > gst_libcamera_src_task_run(gpointer user_data) >> > { >> > @@ -468,11 +550,8 @@ gst_libcamera_src_task_enter(GstTask >> > *task, [[maybe_unused]] GThread *thread, >> > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); >> > GLibRecLocker lock(&self->stream_lock); >> > GstLibcameraSrcState *state = self->state; >> > - GstFlowReturn flow_ret = GST_FLOW_OK; >> > gint ret; >> > >> > - g_autoptr(GstStructure) element_caps = >> > gst_structure_new_empty("caps"); >> > - >> > GST_DEBUG_OBJECT(self, "Streaming thread has started"); >> > >> > gint stream_id_num = 0; >> > @@ -500,62 +579,16 @@ gst_libcamera_src_task_enter(GstTask >> > *task, [[maybe_unused]] GThread *thread, >> > } >> > g_assert(state->config_->size() == >> > state->srcpads_.size()); >> > >> > - for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > - GstPad *srcpad = state->srcpads_[i]; >> > - StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > - >> > - /* 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; >> > - } >> > - >> > - /* Fixate caps and configure the stream. */ >> > - caps = gst_caps_make_writable(caps); >> > - gst_libcamera_configure_stream_from_caps(stream_cfg, >> > caps); >> > - gst_libcamera_get_framerate_from_caps(caps, >> > element_caps); >> > - } >> > - >> > - if (flow_ret != GST_FLOW_OK) >> > - goto done; >> > - >> > - /* Validate the configuration. */ >> > - if (state->config_->validate() == >> > CameraConfiguration::Invalid) { >> > - flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > - goto done; >> > - } >> > - >> > - ret = state->cam_->configure(state->config_.get()); >> > - if (ret) { >> > - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, >> > - ("Failed to configure camera: %s", >> > g_strerror(-ret)), >> > - ("Camera::configure() failed with error code >> > %i", ret)); >> > + if (!gst_libcamera_src_negotiate(self)) { >> > + state->initControls_.clear(); >> > + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); >> > gst_task_stop(task); >> > - flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > - goto done; >> > + return; >> > } >> > >> > - /* Check frame duration bounds within >> > controls::FrameDurationLimits */ >> > - >> > gst_libcamera_clamp_and_set_frameduration(state->initControls_, >> > - state->cam_->controls(), >> > element_caps); >> > - >> > - /* >> > - * Regardless if it has been modified, create clean caps >> > and push the >> > - * caps event. Downstream will decide if the caps are >> > acceptable. >> > - */ >> > - for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > - GstPad *srcpad = state->srcpads_[i]; >> > - const StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > - >> > - g_autoptr(GstCaps) caps = >> > gst_libcamera_stream_configuration_to_caps(stream_cfg); >> > - gst_libcamera_framerate_to_caps(caps, element_caps); >> > - >> > - if (!gst_pad_push_event(srcpad, >> > gst_event_new_caps(caps))) { >> > - flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > - break; >> > - } >> > + self->flow_combiner = gst_flow_combiner_new(); >> > + for (GstPad *srcpad : state->srcpads_) { >> > + gst_flow_combiner_add_pad(self->flow_combiner, >> > srcpad); >> > >> > /* Send an open segment event with time format. */ >> > GstSegment segment; >> > @@ -563,28 +596,6 @@ gst_libcamera_src_task_enter(GstTask >> > *task, [[maybe_unused]] GThread *thread, >> > gst_pad_push_event(srcpad, >> > gst_event_new_segment(&segment)); >> > } >> > >> > - self->allocator = gst_libcamera_allocator_new(state->cam_, >> > state->config_.get()); >> > - if (!self->allocator) { >> > - GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, >> > - ("Failed to allocate memory"), >> > - ("gst_libcamera_allocator_new() failed.")); >> > - gst_task_stop(task); >> > - return; >> > - } >> > - >> > - self->flow_combiner = gst_flow_combiner_new(); >> > - for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > - GstPad *srcpad = state->srcpads_[i]; >> > - const StreamConfiguration &stream_cfg = >> > state->config_->at(i); >> > - GstLibcameraPool *pool = >> > gst_libcamera_pool_new(self->allocator, >> > - stream_cfg.stream()); >> > - g_signal_connect_swapped(pool, "buffer-notify", >> > - G_CALLBACK(gst_task_resume), task); >> > - >> > - gst_libcamera_pad_set_pool(srcpad, pool); >> > - gst_flow_combiner_add_pad(self->flow_combiner, >> > srcpad); >> > - } >> > - >> > if (self->auto_focus_mode != controls::AfModeManual) { >> > const ControlInfoMap &infoMap = >> > state->cam_->controls(); >> > if (infoMap.find(&controls::AfMode) != infoMap.end()) >> > { >> > @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask >> > *task, [[maybe_unused]] GThread *thread, >> > gst_task_stop(task); >> > return; >> > } >> > - >> > -done: >> > - state->initControls_.clear(); >> > - switch (flow_ret) { >> > - case GST_FLOW_NOT_NEGOTIATED: >> > - GST_ELEMENT_FLOW_ERROR(self, flow_ret); >> > - gst_task_stop(task); >> > - break; >> > - default: >> > - break; >> > - } >> > } >> > >> > static void
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index a6f240f5..e57ba52f 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -379,6 +379,88 @@ gst_libcamera_src_open(GstLibcameraSrc *self) return true; } +/* Must be called with stream_lock held. */ +static bool +gst_libcamera_src_negotiate(GstLibcameraSrc *self) +{ + GstLibcameraSrcState *state = self->state; + + g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); + + for (gsize i = 0; i < state->srcpads_.size(); i++) { + GstPad *srcpad = state->srcpads_[i]; + StreamConfiguration &stream_cfg = state->config_->at(i); + + /* 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)) + return false; + + /* Fixate caps and configure the stream. */ + caps = gst_caps_make_writable(caps); + gst_libcamera_configure_stream_from_caps(stream_cfg, caps); + gst_libcamera_get_framerate_from_caps(caps, element_caps); + } + + /* Validate the configuration. */ + if (state->config_->validate() == CameraConfiguration::Invalid) + return false; + + int ret = state->cam_->configure(state->config_.get()); + if (ret) { + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, + ("Failed to configure camera: %s", g_strerror(-ret)), + ("Camera::configure() failed with error code %i", ret)); + return false; + } + + /* Check frame duration bounds within controls::FrameDurationLimits */ + gst_libcamera_clamp_and_set_frameduration(state->initControls_, + state->cam_->controls(), element_caps); + + /* + * Regardless if it has been modified, create clean caps and push the + * caps event. Downstream will decide if the caps are acceptable. + */ + for (gsize i = 0; i < state->srcpads_.size(); i++) { + GstPad *srcpad = state->srcpads_[i]; + const StreamConfiguration &stream_cfg = state->config_->at(i); + + g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); + gst_libcamera_framerate_to_caps(caps, element_caps); + + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) + return false; + + } + + if (self->allocator) + g_clear_object(&self->allocator); + + self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); + if (!self->allocator) { + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, + ("Failed to allocate memory"), + ("gst_libcamera_allocator_new() failed.")); + return false; + } + + for (gsize i = 0; i < state->srcpads_.size(); i++) { + GstPad *srcpad = state->srcpads_[i]; + const StreamConfiguration &stream_cfg = state->config_->at(i); + + GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, + stream_cfg.stream()); + g_signal_connect_swapped(pool, "buffer-notify", + G_CALLBACK(gst_task_resume), self->task); + + gst_libcamera_pad_set_pool(srcpad, pool); + } + + return true; +} + static void gst_libcamera_src_task_run(gpointer user_data) { @@ -468,11 +550,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); GLibRecLocker lock(&self->stream_lock); GstLibcameraSrcState *state = self->state; - GstFlowReturn flow_ret = GST_FLOW_OK; gint ret; - g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps"); - GST_DEBUG_OBJECT(self, "Streaming thread has started"); gint stream_id_num = 0; @@ -500,62 +579,16 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, } g_assert(state->config_->size() == state->srcpads_.size()); - for (gsize i = 0; i < state->srcpads_.size(); i++) { - GstPad *srcpad = state->srcpads_[i]; - StreamConfiguration &stream_cfg = state->config_->at(i); - - /* 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; - } - - /* Fixate caps and configure the stream. */ - caps = gst_caps_make_writable(caps); - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); - gst_libcamera_get_framerate_from_caps(caps, element_caps); - } - - if (flow_ret != GST_FLOW_OK) - goto done; - - /* Validate the configuration. */ - if (state->config_->validate() == CameraConfiguration::Invalid) { - flow_ret = GST_FLOW_NOT_NEGOTIATED; - goto done; - } - - ret = state->cam_->configure(state->config_.get()); - if (ret) { - GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, - ("Failed to configure camera: %s", g_strerror(-ret)), - ("Camera::configure() failed with error code %i", ret)); + if (!gst_libcamera_src_negotiate(self)) { + state->initControls_.clear(); + GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED); gst_task_stop(task); - flow_ret = GST_FLOW_NOT_NEGOTIATED; - goto done; + return; } - /* Check frame duration bounds within controls::FrameDurationLimits */ - gst_libcamera_clamp_and_set_frameduration(state->initControls_, - state->cam_->controls(), element_caps); - - /* - * Regardless if it has been modified, create clean caps and push the - * caps event. Downstream will decide if the caps are acceptable. - */ - for (gsize i = 0; i < state->srcpads_.size(); i++) { - GstPad *srcpad = state->srcpads_[i]; - const StreamConfiguration &stream_cfg = state->config_->at(i); - - g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); - gst_libcamera_framerate_to_caps(caps, element_caps); - - if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { - flow_ret = GST_FLOW_NOT_NEGOTIATED; - break; - } + self->flow_combiner = gst_flow_combiner_new(); + for (GstPad *srcpad : state->srcpads_) { + gst_flow_combiner_add_pad(self->flow_combiner, srcpad); /* Send an open segment event with time format. */ GstSegment segment; @@ -563,28 +596,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, gst_pad_push_event(srcpad, gst_event_new_segment(&segment)); } - self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); - if (!self->allocator) { - GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, - ("Failed to allocate memory"), - ("gst_libcamera_allocator_new() failed.")); - gst_task_stop(task); - return; - } - - self->flow_combiner = gst_flow_combiner_new(); - for (gsize i = 0; i < state->srcpads_.size(); i++) { - GstPad *srcpad = state->srcpads_[i]; - const StreamConfiguration &stream_cfg = state->config_->at(i); - GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, - stream_cfg.stream()); - g_signal_connect_swapped(pool, "buffer-notify", - G_CALLBACK(gst_task_resume), task); - - gst_libcamera_pad_set_pool(srcpad, pool); - gst_flow_combiner_add_pad(self->flow_combiner, srcpad); - } - if (self->auto_focus_mode != controls::AfModeManual) { const ControlInfoMap &infoMap = state->cam_->controls(); if (infoMap.find(&controls::AfMode) != infoMap.end()) { @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, gst_task_stop(task); return; } - -done: - state->initControls_.clear(); - switch (flow_ret) { - case GST_FLOW_NOT_NEGOTIATED: - GST_ELEMENT_FLOW_ERROR(self, flow_ret); - gst_task_stop(task); - break; - default: - break; - } } static void
Move the code which negotiates all the source pad caps into a separate function called gst_libcamera_src_negotiate. When the negotiation fails this function will return FALSE and TRUE otherwise. Use this function instead of doing the negotiation manually in gst_libcamera_src_task_enter and remove the now redundant error handling code accordingly. Signed-off-by: Jaslo Ziska <jaslo@ziska.de> --- src/gstreamer/gstlibcamerasrc.cpp | 178 +++++++++++++++--------------- 1 file changed, 89 insertions(+), 89 deletions(-)