gstreamer: Merge initControls_ with request controls
diff mbox series

Message ID 20241101172220.5765-1-umang.jain@ideasonboard.com
State New
Headers show
Series
  • gstreamer: Merge initControls_ with request controls
Related show

Commit Message

Umang Jain Nov. 1, 2024, 5:22 p.m. UTC
It turns out there a few pipeline-handler/IPA that do not honor
the controls passed during start() phase. For instance, he gstreamer
libcamerasrc element, passes the target framerate during start() to
libcamera. As rkisp1 pipeline-handler/IPA doesn't do anything with
controls passed during start(), the target fps is unable to take
effect on rkisp1 platform.

Hence, merge the initControls_ with request controls with
ControlList::MergePolicy::KeepExisting. This way, the controls passed
by libcamerasrc during start(), can atleast get honoured on the
platform through requests' control.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 1 +
 1 file changed, 1 insertion(+)

Comments

Nicolas Dufresne Nov. 1, 2024, 7:02 p.m. UTC | #1
Hi Umang,

Le vendredi 01 novembre 2024 à 22:52 +0530, Umang Jain a écrit :
> It turns out there a few pipeline-handler/IPA that do not honor
> the controls passed during start() phase. For instance, he gstreamer
> libcamerasrc element, passes the target framerate during start() to
> libcamera. As rkisp1 pipeline-handler/IPA doesn't do anything with
> controls passed during start(), the target fps is unable to take
> effect on rkisp1 platform.
> 
> Hence, merge the initControls_ with request controls with
> ControlList::MergePolicy::KeepExisting. This way, the controls passed
> by libcamerasrc during start(), can atleast get honoured on the
> platform through requests' control.

I'm not sure what to think of this, I'm literally hesitant if that should just
be merged or given a hard NAK (no joke). This is in my opinion an severe
interoperability issue if one method completely fail with a different ISP
implementation. Have we discussed the implication of letting the IPA do whatever
they want before patching the applications ?

regards,
Nicolas

> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 912a8d55..0d615f4a 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -208,6 +208,7 @@ int GstLibcameraSrcState::queueRequest()
>  	}
>  
>  	GST_TRACE_OBJECT(src_, "Requesting buffers");
> +	wrap->request_.get()->controls().merge(initControls_, ControlList::MergePolicy::KeepExisting);
>  	cam_->queueRequest(wrap->request_.get());
>  
>  	{
Laurent Pinchart Nov. 2, 2024, 12:20 a.m. UTC | #2
On Fri, Nov 01, 2024 at 03:02:06PM -0400, Nicolas Dufresne wrote:
> Hi Umang,
> 
> Le vendredi 01 novembre 2024 à 22:52 +0530, Umang Jain a écrit :
> > It turns out there a few pipeline-handler/IPA that do not honor
> > the controls passed during start() phase. For instance, he gstreamer
> > libcamerasrc element, passes the target framerate during start() to
> > libcamera. As rkisp1 pipeline-handler/IPA doesn't do anything with
> > controls passed during start(), the target fps is unable to take
> > effect on rkisp1 platform.
> > 
> > Hence, merge the initControls_ with request controls with
> > ControlList::MergePolicy::KeepExisting. This way, the controls passed
> > by libcamerasrc during start(), can atleast get honoured on the
> > platform through requests' control.
> 
> I'm not sure what to think of this, I'm literally hesitant if that should just
> be merged or given a hard NAK (no joke). This is in my opinion an severe
> interoperability issue if one method completely fail with a different ISP
> implementation. Have we discussed the implication of letting the IPA do whatever
> they want before patching the applications ?

I agree, this should be fixed in the pipeline handlers.

> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 912a8d55..0d615f4a 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -208,6 +208,7 @@ int GstLibcameraSrcState::queueRequest()
> >  	}
> >  
> >  	GST_TRACE_OBJECT(src_, "Requesting buffers");
> > +	wrap->request_.get()->controls().merge(initControls_, ControlList::MergePolicy::KeepExisting);
> >  	cam_->queueRequest(wrap->request_.get());
> >  
> >  	{

Patch
diff mbox series

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 912a8d55..0d615f4a 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -208,6 +208,7 @@  int GstLibcameraSrcState::queueRequest()
 	}
 
 	GST_TRACE_OBJECT(src_, "Requesting buffers");
+	wrap->request_.get()->controls().merge(initControls_, ControlList::MergePolicy::KeepExisting);
 	cam_->queueRequest(wrap->request_.get());
 
 	{