[libcamera-devel] [PATCH v1 15/23] gst: libcamerasrc: Implement minimal caps negotiation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Feb 12 00:47:42 CET 2020
Hi Nicolas,
Thank you for the patch.
On Tue, Jan 28, 2020 at 10:32:02PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>
> This is not expected to work in every possible cases, but should be sufficient as
> an initial implementation. What is does it that it turns the StreamFormats into
s/is does it/it does is/
> caps and query downstream caps with that as a filter.
s/query/queries/
> 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 strust the order
s/strust/trust/
> in StreamFormats as being sorted best first, but this is not currently in
> libcamera.
Could you add a \todo comment in the code to address this ?
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
> src/gstreamer/gstlibcamerasrc.cpp | 71 +++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index b1a21dc..7b478fa 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -24,6 +24,7 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);
> struct GstLibcameraSrcState {
> std::shared_ptr<CameraManager> cm;
> std::shared_ptr<Camera> cam;
> + std::unique_ptr<CameraConfiguration> config;
> std::vector<GstPad *> srcpads;
> };
>
> @@ -132,16 +133,86 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)
> STREAM_LOCKER(user_data);
> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> 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 */
> 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 */
> + roles.push_back(gst_libcamera_pad_get_role(srcpad));
> + }
> +
> + /* Generate the stream configurations, there should be one per pad */
> + state->config = state->cam->generateConfiguration(roles);
> + g_assert(state->config->size() == state->srcpads.size());
What if more pads have been created than the camera can handle ? Is
there a way to fail more gracefully than with a g_assert() ? It doesn't
have to be implement now, but would be nice in the future. Maybe a \todo
comment with a brief description of how to fix it ?
> +
> + for (gsize i = 0; i < state->srcpads.size(); i++) {
> + GstPad *srcpad = state->srcpads[i];
> + StreamConfiguration &stream_cfg = state->config->at(i);
> +
> + /* Retreive the supported caps */
s/Retreive/Retrieve/
> + 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 hte caps are acceptable */
s/hte/the/
Comment style:
/*
* ...
*/
> + 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;
> + }
> + }
> +
> + 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));
s/.configure/::configure/
> + gst_task_stop(task);
> + return;
Just for my information, why is there no need to push an EOS events on
pads here ? And what if the previous loop fails and sets flow_ret to
GST_FLOW_NOT_NEGOTIATED and this then fails, is it expected that the
done: label won't be reached in that case ?
> + }
> +
> +done:
> + switch (flow_ret) {
> + case GST_FLOW_NOT_NEGOTIATED:
> + for (GstPad *srcpad : state->srcpads) {
> + gst_pad_push_event(srcpad, gst_event_new_eos());
> + }
No need for braces.
> + GST_ELEMENT_FLOW_ERROR(self, flow_ret);
> + gst_task_stop(task);
> + return;
> + default:
> + break;
> }
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list