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

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

Commit Message

Barnabás Pőcze July 1, 2025, 9:06 a.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, 18 insertions(+), 9 deletions(-)

Comments

Barnabás Pőcze July 1, 2025, 9:07 a.m. UTC | #1
2025. 07. 01. 11:06 keltezéssel, Barnabás Pőcze írta:
> 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>
> ---

changes in v2:
   * process controls before streamOn()

v1: https://patchwork.libcamera.org/patch/23543/


>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 +++++++++++++-------
>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index d52b88042..4b5816dfd 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;
> @@ -296,13 +296,22 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = data->video_->streamOn();
> -	if (ret < 0) {
> -		data->video_->releaseBuffers();
> -		return ret;
> +	if (controls) {
> +		ret = processControls(data, *controls);
> +		if (ret < 0)
> +			goto err_release_buffers;
>   	}
>   
> +	ret = data->video_->streamOn();
> +	if (ret < 0)
> +		goto err_release_buffers;
> +
>   	return 0;
> +
> +err_release_buffers:
> +	data->video_->releaseBuffers();
> +
> +	return ret;
>   }
>   
>   void PipelineHandlerUVC::stopDevice(Camera *camera)
> @@ -412,11 +421,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 +453,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;
>
Kieran Bingham July 1, 2025, 9:12 a.m. UTC | #2
Quoting Barnabás Pőcze (2025-07-01 10:06:30)
> 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, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index d52b88042..4b5816dfd 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;
> @@ -296,13 +296,22 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>         if (ret < 0)
>                 return ret;
>  
> -       ret = data->video_->streamOn();
> -       if (ret < 0) {
> -               data->video_->releaseBuffers();
> -               return ret;
> +       if (controls) {
> +               ret = processControls(data, *controls);
> +               if (ret < 0)
> +                       goto err_release_buffers;
>         }
>  

Thanks, that ordering is better I think.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       ret = data->video_->streamOn();
> +       if (ret < 0)
> +               goto err_release_buffers;
> +
>         return 0;
> +
> +err_release_buffers:
> +       data->video_->releaseBuffers();
> +
> +       return ret;
>  }
>  
>  void PipelineHandlerUVC::stopDevice(Camera *camera)
> @@ -412,11 +421,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 +453,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.50.0
>
Umang Jain July 1, 2025, 10:49 a.m. UTC | #3
Hi,

On Tue, Jul 01, 2025 at 11:07:35AM +0200, Barnabás Pőcze wrote:
> 2025. 07. 01. 11:06 keltezéssel, Barnabás Pőcze írta:
> > 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>

LGTM

> > ---
> 
> changes in v2:
>   * process controls before streamOn()
> 
> v1: https://patchwork.libcamera.org/patch/23543/

Paul's R-b tag was not collected on v2.

Reviewed-by: Umang Jain <uajain@igalia.com>
> 
> 
> >   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 +++++++++++++-------
> >   1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index d52b88042..4b5816dfd 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;
> > @@ -296,13 +296,22 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> >   	if (ret < 0)
> >   		return ret;
> > -	ret = data->video_->streamOn();
> > -	if (ret < 0) {
> > -		data->video_->releaseBuffers();
> > -		return ret;
> > +	if (controls) {
> > +		ret = processControls(data, *controls);
> > +		if (ret < 0)
> > +			goto err_release_buffers;
> >   	}
> > +	ret = data->video_->streamOn();
> > +	if (ret < 0)
> > +		goto err_release_buffers;
> > +
> >   	return 0;
> > +
> > +err_release_buffers:
> > +	data->video_->releaseBuffers();
> > +
> > +	return ret;
> >   }
> >   void PipelineHandlerUVC::stopDevice(Camera *camera)
> > @@ -412,11 +421,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 +453,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;
>
Barnabás Pőcze July 1, 2025, 11:57 a.m. UTC | #4
2025. 07. 01. 12:49 keltezéssel, Umang Jain írta:
> Hi,
> 
> On Tue, Jul 01, 2025 at 11:07:35AM +0200, Barnabás Pőcze wrote:
>> 2025. 07. 01. 11:06 keltezéssel, Barnabás Pőcze írta:
>>> 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>
> 
> LGTM
> 
>>> ---
>>
>> changes in v2:
>>    * process controls before streamOn()
>>
>> v1: https://patchwork.libcamera.org/patch/23543/
> 
> Paul's R-b tag was not collected on v2.

Hmm... I thought the changes were significant enough to omit it;
I like to err on the side of caution, but maybe they aren't as
significant as I had first thought.


> 
> Reviewed-by: Umang Jain <uajain@igalia.com>
>>
>>
>>>    src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 27 +++++++++++++-------
>>>    1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> index d52b88042..4b5816dfd 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;
>>> @@ -296,13 +296,22 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>>>    	if (ret < 0)
>>>    		return ret;
>>> -	ret = data->video_->streamOn();
>>> -	if (ret < 0) {
>>> -		data->video_->releaseBuffers();
>>> -		return ret;
>>> +	if (controls) {
>>> +		ret = processControls(data, *controls);
>>> +		if (ret < 0)
>>> +			goto err_release_buffers;
>>>    	}
>>> +	ret = data->video_->streamOn();
>>> +	if (ret < 0)
>>> +		goto err_release_buffers;
>>> +
>>>    	return 0;
>>> +
>>> +err_release_buffers:
>>> +	data->video_->releaseBuffers();
>>> +
>>> +	return ret;
>>>    }
>>>    void PipelineHandlerUVC::stopDevice(Camera *camera)
>>> @@ -412,11 +421,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 +453,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..4b5816dfd 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;
@@ -296,13 +296,22 @@  int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
 	if (ret < 0)
 		return ret;
 
-	ret = data->video_->streamOn();
-	if (ret < 0) {
-		data->video_->releaseBuffers();
-		return ret;
+	if (controls) {
+		ret = processControls(data, *controls);
+		if (ret < 0)
+			goto err_release_buffers;
 	}
 
+	ret = data->video_->streamOn();
+	if (ret < 0)
+		goto err_release_buffers;
+
 	return 0;
+
+err_release_buffers:
+	data->video_->releaseBuffers();
+
+	return ret;
 }
 
 void PipelineHandlerUVC::stopDevice(Camera *camera)
@@ -412,11 +421,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 +453,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;