[libcamera-devel] [PATCH v6 1/2] gstreamer: Do not expose the caps before configuring the camera

[Kieran Bingham]

Quoting Umang Jain via libcamera-devel (2022-11-02 13:56:13)
> From: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> 
> Configure the camera before exposing the caps valid controls values
> (and bounds) are available. These control values might be of interest
> to be exposed on the capabilites, which otherwise, would not be
> available if the camera is configured after the update caps event.
> 
> For instance, the FrameDurationLimits are computed by RPi's IPA in
> its configure(). Hence, we need to Camera::configure() to happen in

Argh, validate() is supposed to be able to give you everything you need,
but clearly it doesn't. Maybe we need to look at making calls into the
IPA for validation too.

But that's an aside to this patch,

> order to know the FrameDurationLimits, that can be exposed in the caps.
> This ties into the framerate support for libcamerasrc which will happen
> in a follow-up commit.
> 
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Tested-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 16d70fea..60032236 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -515,6 +515,16 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>                 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));
> +               gst_task_stop(task);
> +               flow_ret = GST_FLOW_NOT_NEGOTIATED;
> +               goto done;
> +       }
> +

If you weren't already taking ownership of the camera, I would have
worreid here - because you shouldn't change the state of the camera just
to be able to get it's properties - however, in this instance I beleive
we're ok - as it's the start streaming thread - so the camera is
acquired anyway.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>         /*
>          * Regardless if it has been modified, create clean caps and push the
>          * caps event. Downstream will decide if the caps are acceptable.
> @@ -535,15 +545,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>                 gst_pad_push_event(srcpad, gst_event_new_segment(&segment));
>         }
>  
> -       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;
> -       }
> -
>         self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());
>         if (!self->allocator) {
>                 GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> -- 
> 2.37.3
>