[libcamera-devel] [PATCH v2 1/3] gstreamer: Move negotiation logic to separate function
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Dec 7 00:20:10 CET 2023
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 at ziska.de>
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at 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 at 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list