[libcamera-devel] [PATCH v2 15/27] gst: libcamerasrc: Implement minimal caps negotiation
Nicolas Dufresne
nicolas.dufresne at collabora.com
Fri Mar 6 19:18:10 CET 2020
Le samedi 29 février 2020 à 16:14 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
>
> Thank you for the patch.
>
> On Thu, Feb 27, 2020 at 03:03:55PM -0500, Nicolas Dufresne wrote:
> > This is not expected to work in every possible cases, but should be
> > sufficient as
> > an initial implementation. What is does is that it turns the StreamFormats
> > into
>
> s/is does is/it does is/
>
> > caps and queries downstream caps with that as a filter.
> >
> > The result is the subset of caps that can be used. We then keep the first
> > structure in that result and fixate using the default values found in
> > StreamConfiguration as a default in case a range is available.
> >
> > We then validate this configuration and turn the potentially modified
> > configuration into caps that we push downstream. Note that we trust the
> > order
> > in StreamFormats as being sorted best first, but this is not currently in
> > libcamera. A todo has been added in the head of this file as a reminder to
> > fix
> > that in the core.
> >
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> > src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++++++++++++++-
> > 1 file changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > b/src/gstreamer/gstlibcamerasrc.cpp
> > index e2c63d1..2c5c34c 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -6,6 +6,12 @@
> > * gstlibcamerasrc.cpp - GStreamer Capture Element
> > */
> >
> > +/**
> > + * \todo libcamera UVC drivers picks the lowest possible resolution first,
> > this
> > + * should be fixed so that we get a decent resolution and framerate for the
> > + * role by default.
> > + */
> > +
> > #include "gstlibcamera-utils.h"
> > #include "gstlibcamerapad.h"
> > #include "gstlibcamerasrc.h"
> > @@ -23,6 +29,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);
> > struct GstLibcameraSrcState {
> > std::unique_ptr<CameraManager> cm;
> > std::shared_ptr<Camera> cam;
> > + std::unique_ptr<CameraConfiguration> config;
> > std::vector<GstPad *> srcpads;
> > };
> >
> > @@ -131,16 +138,89 @@ gst_libcamera_src_task_enter(GstTask *task, GThread
> > *thread, gpointer user_data)
> > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > GLibRecLocker(&self->stream_lock);
> > GstLibcameraSrcState *state = self->state;
> > + GstFlowReturn flow_ret = GST_FLOW_OK;
> >
> > GST_DEBUG_OBJECT(self, "Streaming thread has started");
> >
> > guint group_id = gst_util_group_id_next();
> > + StreamRoles roles;
> > for (GstPad *srcpad : state->srcpads) {
> > - /* Create stream-id and push stream-start */
> > + /* Create stream-id and push stream-start .*/
>
> This probably belongs to a previous patch (and the space should go after
> the period, not before).
>
> > g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad,
> > GST_ELEMENT(self), nullptr);
> > GstEvent *event = gst_event_new_stream_start(stream_id);
> > gst_event_set_group_id(event, group_id);
> > gst_pad_push_event(srcpad, event);
> > +
> > + /* Collect the streams roles for the next iteration .*/
>
> s/ \./. /
>
> Search and replace went wrong ? :-)
>
> > + roles.push_back(gst_libcamera_pad_get_role(srcpad));
> > + }
> > +
> > + /* Generate the stream configurations, there should be one per pad .*/
>
> Same here.
>
> > + state->config = state->cam->generateConfiguration(roles);
> > + /* \todo Check if camera may increase or decrease the number of streams
> > + * regardless of the number of roles.*/
>
> /*
> * ...
> */
>
> And to answer your question, it may decrease it, but not increase it.
> This should be better documented in libcamera itself, let's keep the
> todo here as a reminder.
>
> > + 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);
> > + }
> > +
> > + 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;
> > + }
> > +
> > + /*
> > + * 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);
> > + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {
> > + flow_ret = GST_FLOW_NOT_NEGOTIATED;
> > + break;
> > + }
> > + }
> > +
> > + {
> > + gint 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));
> > + gst_task_stop(task);
> > + return;
> > + }
> > + }
>
> If the braces are here for the sole purpose of declaring ret, you can
> move gint ret; to the beginning of the function and remove the braces.
Ok, it can only be at the top (or at least befor the goto) as the compiler won't
let a goto cross a declaration.
>
> > +
> > +done:
> > + switch (flow_ret) {
> > + case GST_FLOW_NOT_NEGOTIATED:
> > + GST_ELEMENT_FLOW_ERROR(self, flow_ret);
> > + gst_task_stop(task);
> > + return;
>
> Maybe s/return/break/ for symmetry wit the default case ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + default:
> > + break;
> > }
> > }
> >
More information about the libcamera-devel
mailing list