[libcamera-devel] [PATCH v2 1/3] gstreamer: Move negotiation logic to separate function
Jaslo Ziska
jaslo at ziska.de
Thu Dec 14 17:02:55 CET 2023
Hi Laurent,
Laurent Pinchart <laurent.pinchart at 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 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>
>
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
More information about the libcamera-devel
mailing list