[v1] libcamera: pipeline: uvcvideo: Handle controls during startup
diff mbox series

Message ID 20250611140451.592358-1-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • [v1] libcamera: pipeline: uvcvideo: Handle controls during startup
Related show

Commit Message

Barnabás Pőcze June 11, 2025, 2:04 p.m. UTC 
Process the control list passed to `Camera::start()`, and set
the V4L2 controls accordingly.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 ++++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Paul Elder June 27, 2025, 6:17 a.m. UTC | #1 
Quoting Barnabás Pőcze (2025-06-11 23:04:51)
> Process the control list passed to `Camera::start()`, and set
> the V4L2 controls accordingly.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 ++++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index d52b88042..202420ea3 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -100,7 +100,7 @@ public:
>  private:
>         int processControl(const UVCCameraData *data, ControlList *controls,
>                            unsigned int id, const ControlValue &value);
> -       int processControls(UVCCameraData *data, Request *request);
> +       int processControls(UVCCameraData *data, const ControlList &reqControls);
>  
>         bool acquireDevice(Camera *camera) override;
>         void releaseDevice(Camera *camera) override;
> @@ -287,7 +287,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
>         return data->video_->exportBuffers(count, buffers);
>  }
>  
> -int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> +int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
>  {
>         UVCCameraData *data = cameraData(camera);
>         unsigned int count = data->stream_.configuration().bufferCount;
> @@ -297,12 +297,23 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>                 return ret;
>  
>         ret = data->video_->streamOn();
> -       if (ret < 0) {
> -               data->video_->releaseBuffers();
> -               return ret;
> +       if (ret < 0)
> +               goto err_release_buffers;
> +
> +       if (controls) {
> +               ret = processControls(data, *controls);
> +               if (ret < 0)
> +                       goto err_stream_off;
>         }
>  
>         return 0;
> +
> +err_stream_off:
> +       data->video_->streamOff();
> +err_release_buffers:
> +       data->video_->releaseBuffers();
> +
> +       return ret;
>  }
>  
>  void PipelineHandlerUVC::stopDevice(Camera *camera)
> @@ -412,11 +423,11 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>         return 0;
>  }
>  
> -int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> +int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &reqControls)
>  {
>         ControlList controls(data->video_->controls());
>  
> -       for (const auto &[id, value] : request->controls())
> +       for (const auto &[id, value] : reqControls)
>                 processControl(data, &controls, id, value);
>  
>         for (const auto &ctrl : controls)
> @@ -444,7 +455,7 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
>                 return -ENOENT;
>         }
>  
> -       int ret = processControls(data, request);
> +       int ret = processControls(data, request->controls());
>         if (ret < 0)
>                 return ret;
>  
> -- 
> 2.49.0
>
Kieran Bingham June 27, 2025, 7:51 a.m. UTC | #2 
Quoting Paul Elder (2025-06-27 07:17:47)
> Quoting Barnabás Pőcze (2025-06-11 23:04:51)
> > Process the control list passed to `Camera::start()`, and set
> > the V4L2 controls accordingly.
> > 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> Looks good to me.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 ++++++++++++++------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index d52b88042..202420ea3 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -100,7 +100,7 @@ public:
> >  private:
> >         int processControl(const UVCCameraData *data, ControlList *controls,
> >                            unsigned int id, const ControlValue &value);
> > -       int processControls(UVCCameraData *data, Request *request);
> > +       int processControls(UVCCameraData *data, const ControlList &reqControls);
> >  
> >         bool acquireDevice(Camera *camera) override;
> >         void releaseDevice(Camera *camera) override;
> > @@ -287,7 +287,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> >         return data->video_->exportBuffers(count, buffers);
> >  }
> >  
> > -int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > +int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
> >  {
> >         UVCCameraData *data = cameraData(camera);
> >         unsigned int count = data->stream_.configuration().bufferCount;
> > @@ -297,12 +297,23 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> >                 return ret;
> >  
> >         ret = data->video_->streamOn();
> > -       if (ret < 0) {
> > -               data->video_->releaseBuffers();
> > -               return ret;
> > +       if (ret < 0)
> > +               goto err_release_buffers;
> > +

Does anyone know if there's a preferred order on startup here? I feel
like we should set the controls before the first frame ...  will that
ensure they all take effect? or are there any issues setting controls
before calling streamOn ?

I expect the controls should persist correctly in V4L2 so that they are
prepared when we call stream on then.

> > +       if (controls) {
> > +               ret = processControls(data, *controls);
> > +               if (ret < 0)
> > +                       goto err_stream_off;
> >         }
> >  
> >         return 0;
> > +
> > +err_stream_off:
> > +       data->video_->streamOff();
> > +err_release_buffers:
> > +       data->video_->releaseBuffers();
> > +
> > +       return ret;
> >  }
> >  
> >  void PipelineHandlerUVC::stopDevice(Camera *camera)
> > @@ -412,11 +423,11 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> >         return 0;
> >  }
> >  
> > -int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > +int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &reqControls)
> >  {
> >         ControlList controls(data->video_->controls());
> >  
> > -       for (const auto &[id, value] : request->controls())
> > +       for (const auto &[id, value] : reqControls)
> >                 processControl(data, &controls, id, value);
> >  
> >         for (const auto &ctrl : controls)
> > @@ -444,7 +455,7 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> >                 return -ENOENT;
> >         }
> >  
> > -       int ret = processControls(data, request);
> > +       int ret = processControls(data, request->controls());
> >         if (ret < 0)
> >                 return ret;
> >  
> > -- 
> > 2.49.0
> >
Laurent Pinchart June 30, 2025, 11:13 p.m. UTC | #3 
On Fri, Jun 27, 2025 at 08:51:30AM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2025-06-27 07:17:47)
> > Quoting Barnabás Pőcze (2025-06-11 23:04:51)
> > > Process the control list passed to `Camera::start()`, and set
> > > the V4L2 controls accordingly.
> > > 
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 ++++++++++++++------
> > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index d52b88042..202420ea3 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -100,7 +100,7 @@ public:
> > >  private:
> > >         int processControl(const UVCCameraData *data, ControlList *controls,
> > >                            unsigned int id, const ControlValue &value);
> > > -       int processControls(UVCCameraData *data, Request *request);
> > > +       int processControls(UVCCameraData *data, const ControlList &reqControls);
> > >  
> > >         bool acquireDevice(Camera *camera) override;
> > >         void releaseDevice(Camera *camera) override;
> > > @@ -287,7 +287,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> > >         return data->video_->exportBuffers(count, buffers);
> > >  }
> > >  
> > > -int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > +int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
> > >  {
> > >         UVCCameraData *data = cameraData(camera);
> > >         unsigned int count = data->stream_.configuration().bufferCount;
> > > @@ -297,12 +297,23 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> > >                 return ret;
> > >  
> > >         ret = data->video_->streamOn();
> > > -       if (ret < 0) {
> > > -               data->video_->releaseBuffers();
> > > -               return ret;
> > > +       if (ret < 0)
> > > +               goto err_release_buffers;
> > > +
> 
> Does anyone know if there's a preferred order on startup here? I feel
> like we should set the controls before the first frame ...  will that
> ensure they all take effect? or are there any issues setting controls
> before calling streamOn ?
> 
> I expect the controls should persist correctly in V4L2 so that they are
> prepared when we call stream on then.

Setting them before starting streaming should ensure they take effect
from the very first frame, yes. I think that would be better.

> > > +       if (controls) {
> > > +               ret = processControls(data, *controls);
> > > +               if (ret < 0)
> > > +                       goto err_stream_off;
> > >         }
> > >  
> > >         return 0;
> > > +
> > > +err_stream_off:
> > > +       data->video_->streamOff();
> > > +err_release_buffers:
> > > +       data->video_->releaseBuffers();
> > > +
> > > +       return ret;
> > >  }
> > >  
> > >  void PipelineHandlerUVC::stopDevice(Camera *camera)
> > > @@ -412,11 +423,11 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > >         return 0;
> > >  }
> > >  
> > > -int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > > +int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &reqControls)
> > >  {
> > >         ControlList controls(data->video_->controls());
> > >  
> > > -       for (const auto &[id, value] : request->controls())
> > > +       for (const auto &[id, value] : reqControls)
> > >                 processControl(data, &controls, id, value);
> > >  
> > >         for (const auto &ctrl : controls)
> > > @@ -444,7 +455,7 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> > >                 return -ENOENT;
> > >         }
> > >  
> > > -       int ret = processControls(data, request);
> > > +       int ret = processControls(data, request->controls());
> > >         if (ret < 0)
> > >                 return ret;
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index d52b88042..202420ea3 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -100,7 +100,7 @@  public:
 private:
 	int processControl(const UVCCameraData *data, ControlList *controls,
 			   unsigned int id, const ControlValue &value);
-	int processControls(UVCCameraData *data, Request *request);
+	int processControls(UVCCameraData *data, const ControlList &reqControls);
 
 	bool acquireDevice(Camera *camera) override;
 	void releaseDevice(Camera *camera) override;
@@ -287,7 +287,7 @@  int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
 	return data->video_->exportBuffers(count, buffers);
 }
 
-int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
+int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
 {
 	UVCCameraData *data = cameraData(camera);
 	unsigned int count = data->stream_.configuration().bufferCount;
@@ -297,12 +297,23 @@  int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
 		return ret;
 
 	ret = data->video_->streamOn();
-	if (ret < 0) {
-		data->video_->releaseBuffers();
-		return ret;
+	if (ret < 0)
+		goto err_release_buffers;
+
+	if (controls) {
+		ret = processControls(data, *controls);
+		if (ret < 0)
+			goto err_stream_off;
 	}
 
 	return 0;
+
+err_stream_off:
+	data->video_->streamOff();
+err_release_buffers:
+	data->video_->releaseBuffers();
+
+	return ret;
 }
 
 void PipelineHandlerUVC::stopDevice(Camera *camera)
@@ -412,11 +423,11 @@  int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
 	return 0;
 }
 
-int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
+int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &reqControls)
 {
 	ControlList controls(data->video_->controls());
 
-	for (const auto &[id, value] : request->controls())
+	for (const auto &[id, value] : reqControls)
 		processControl(data, &controls, id, value);
 
 	for (const auto &ctrl : controls)
@@ -444,7 +455,7 @@  int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
-	int ret = processControls(data, request);
+	int ret = processControls(data, request->controls());
 	if (ret < 0)
 		return ret;