[libcamera-devel] [PATCH v4 1/2] gstreamer: Configure the camera before exposing the caps.

[Rishikesh Donadkar]

> As we introduce framerate only in 2/2, hence, we should _not_ construct
> a framerate centric commit message.
>
> You can however mention it briefly with stating that it makes sense to
> configure the camera first(before exposing) as  the stream configuration
> and controls values are available, required to expose the new caps.
> Framerate is one such example.
>
> Would you be able to re-frame the commit message again based on above ?
Yes
>
> I think what you need here is:
>
>      flow_ret = GST_FLOW_NOT_NEGOTIATED;
>      goto done;
>
> instead of return; similar to what happens when stream configuration is
> not found to be valid (just above)
Noted.


On Mon, Sep 12, 2022 at 3:54 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Rishi,
>
> On 9/12/22 3:26 PM, Rishikesh Donadkar wrote:
> > In libcamerasrc capabilities are exposed on the src pad before
> > configuring the camera. To add support to control and
> > negotiate the framerate, the controls::FrameDuration needs to be
> > bound checked between the min/max values that camera can support
> > and later the applied framerate needs to be exposed along with
> > resolutions and colorimetry through caps. Valid bounds of the
> > controls::FrameDuration cannot be known before the
> > configuration of the camera.
> >
> > Shift the camera::configure() before the for loop that is exposing
> > resolutions, colorimetry to the GStreamer through a new CAPS event.
> > Through this we can know the valid bounds of the FrameDuration and
> > clam the frame_duration accordingly before applying to the camera.
> > Through this caps can be exposed without a need of additional new
> > CAPS event for framerate.
>
> As we introduce framerate only in 2/2, hence, we should _not_ construct
> a framerate centric commit message.
>
> You can however mention it briefly with stating that it makes sense to
> configure the camera first(before exposing) as  the stream configuration
> and controls values are available, required to expose the new caps.
> Framerate is one such example.
>
> Would you be able to re-frame the commit message again based on above ?
>
> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> > ---
> >   src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 16d70fea..1ee1d08a 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -515,6 +515,15 @@ 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);
> > +             return;
>
> I think what you need here is:
>
>      flow_ret = GST_FLOW_NOT_NEGOTIATED;
>      goto done;
>
> instead of return; similar to what happens when stream configuration is
> not found to be valid (just above)
>
> > +     }
> > +
> >       /*
> >        * Regardless if it has been modified, create clean caps and push the
> >        * caps event. Downstream will decide if the caps are acceptable.
> > @@ -535,15 +544,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,
>