| Message ID | 20231122135406.14921-2-jaslo@ziska.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi thanks for the patch. Le mercredi 22 novembre 2023 à 14: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. > > 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 | 176 +++++++++++++++--------------- > 1 file changed, 88 insertions(+), 88 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 767017db..e7a49fef 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -379,6 +379,87 @@ gst_libcamera_src_open(GstLibcameraSrc *self) > return true; > } > > +static gboolean > +gst_libcamera_src_negotiate(GstLibcameraSrc *self) Try and document the locking in a comment above, this is very helpful for future work. nit: Just a general rule we have followed, if the function is a GObject virtual funtion, callback, etc. then we use gboolean, otherwise we always use the C++ bool. With these minor changes, the refactoring looks good. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > +{ > + 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 +549,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,89 +578,22 @@ 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; > gst_segment_init(&segment, GST_FORMAT_TIME); > 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) { > @@ -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 767017db..e7a49fef 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -379,6 +379,87 @@ gst_libcamera_src_open(GstLibcameraSrc *self) return true; } +static gboolean +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 +549,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,89 +578,22 @@ 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; gst_segment_init(&segment, GST_FORMAT_TIME); 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) { @@ -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 | 176 +++++++++++++++--------------- 1 file changed, 88 insertions(+), 88 deletions(-)