| Message ID | 20220912095656.19013-2-rishikeshdonadkar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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,
> 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@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@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, >
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; + } + /* * 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,
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. Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> --- src/gstreamer/gstlibcamerasrc.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)