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

[Umang Jain]

Hi Kieran,

On 11/4/22 4:34 PM, Kieran Bingham wrote:
> 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.

Precisely. validate() doesn't give the info such as FrameDurationLimits 
(which are I suppose, static information) until we configure the camera 
and read the camera->controls().
> 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
>>