libcamera: v4l2_videodevice: Use ScopeExitActions
diff mbox series

Message ID 20250725235311.7280-1-laurent.pinchart@ideasonboard.com
State Rejected
Headers show
Series
  • libcamera: v4l2_videodevice: Use ScopeExitActions
Related show

Commit Message

Laurent Pinchart July 25, 2025, 11:53 p.m. UTC 
The V4L2VideoDevice::queueBuffer() function performs the same cleanup
action in many error paths. Use ScopeExitActions to simplify it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/v4l2_videodevice.cpp | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


base-commit: b65df7e7554b45e2d3d7fdb5b37c2ab7df3db4fe
--
Regards,

Laurent Pinchart

Comments

Umang Jain July 28, 2025, 4:24 a.m. UTC | #1 
On Sat, Jul 26, 2025 at 02:53:11AM +0300, Laurent Pinchart wrote:
> The V4L2VideoDevice::queueBuffer() function performs the same cleanup
> action in many error paths. Use ScopeExitActions to simplify it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Umang Jain <uajain@igalia.com>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 71cc7e895d8c..5f57e546e80b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1646,6 +1646,7 @@ int V4L2VideoDevice::releaseBuffers()
>  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  {
>  	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> +	utils::ScopeExitActions actions;
>  	struct v4l2_buffer buf = {};
>  	int ret;
> 
> @@ -1668,6 +1669,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  	if (ret < 0)
>  		return ret;
> 
> +	actions += [&]() { cache_->put(buf.index); };
> +
>  	buf.index = ret;
>  	buf.type = bufferType_;
>  	buf.memory = memoryType_;
> @@ -1683,15 +1686,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  	 */
>  	if (planes.size() < numV4l2Planes) {
>  		LOG(V4L2, Error) << "Frame buffer has too few planes";
> -		cache_->put(buf.index);
> -
>  		return -EINVAL;
>  	}
> 
>  	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
>  		LOG(V4L2, Error) << "Device format requires contiguous buffer";
> -		cache_->put(buf.index);
> -
>  		return -EINVAL;
>  	}
> 
> @@ -1734,8 +1733,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  				if (i != planes.size() - 1 && bytesused != length) {
>  					LOG(V4L2, Error)
>  						<< "Holes in multi-planar buffer not supported";
> -					cache_->put(buf.index);
> -
>  					return -EINVAL;
>  				}
>  			}
> @@ -1785,8 +1782,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  		LOG(V4L2, Error)
>  			<< "Failed to queue buffer " << buf.index << ": "
>  			<< strerror(-ret);
> -		cache_->put(buf.index);
> -
>  		return ret;
>  	}
> 
> @@ -1798,6 +1793,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> 
>  	queuedBuffers_[buf.index] = buffer;
> 
> +	actions.release();
>  	return 0;
>  }
> 
> 
> base-commit: b65df7e7554b45e2d3d7fdb5b37c2ab7df3db4fe
> --
> Regards,
> 
> Laurent Pinchart
>
Pőcze Barnabás July 28, 2025, 7:12 a.m. UTC | #2 
Hi

2025. 07. 26. 1:53 keltezéssel, Laurent Pinchart írta:
> The V4L2VideoDevice::queueBuffer() function performs the same cleanup
> action in many error paths. Use ScopeExitActions to simplify it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/libcamera/v4l2_videodevice.cpp | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 71cc7e895d8c..5f57e546e80b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1646,6 +1646,7 @@ int V4L2VideoDevice::releaseBuffers()
>   int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>   {
>   	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> +	utils::ScopeExitActions actions;
>   	struct v4l2_buffer buf = {};
>   	int ret;
> 
> @@ -1668,6 +1669,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>   	if (ret < 0)
>   		return ret;
> 
> +	actions += [&]() { cache_->put(buf.index); };

I think it can be argued that `V4L2VideoDevice::queueBuffer` should be considered
a "hot path" in libcamera. And for that reason I think avoiding the constant memory
allocations of `utils::ScopeExitActions` would be preferable.

I think adding https://www.en.cppreference.com/w/cpp/experimental/scope_exit.html
should be simple to do, and it would address the above.


Regards,
Barnabás Pőcze


> +
>   	buf.index = ret;
>   	buf.type = bufferType_;
>   	buf.memory = memoryType_;
> @@ -1683,15 +1686,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>   	 */
>   	if (planes.size() < numV4l2Planes) {
>   		LOG(V4L2, Error) << "Frame buffer has too few planes";
> -		cache_->put(buf.index);
> -
>   		return -EINVAL;
>   	}
> 
>   	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
>   		LOG(V4L2, Error) << "Device format requires contiguous buffer";
> -		cache_->put(buf.index);
> -
>   		return -EINVAL;
>   	}
> 
> @@ -1734,8 +1733,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>   				if (i != planes.size() - 1 && bytesused != length) {
>   					LOG(V4L2, Error)
>   						<< "Holes in multi-planar buffer not supported";
> -					cache_->put(buf.index);
> -
>   					return -EINVAL;
>   				}
>   			}
> @@ -1785,8 +1782,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>   		LOG(V4L2, Error)
>   			<< "Failed to queue buffer " << buf.index << ": "
>   			<< strerror(-ret);
> -		cache_->put(buf.index);
> -
>   		return ret;
>   	}
> 
> @@ -1798,6 +1793,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> 
>   	queuedBuffers_[buf.index] = buffer;
> 
> +	actions.release();
>   	return 0;
>   }
> 
> 
> base-commit: b65df7e7554b45e2d3d7fdb5b37c2ab7df3db4fe
> --
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart July 28, 2025, 10:47 a.m. UTC | #3 
On Mon, Jul 28, 2025 at 07:12:24AM +0000, Barnabás Pőcze wrote:
> 2025. 07. 26. 1:53 keltezéssel, Laurent Pinchart írta:
> > The V4L2VideoDevice::queueBuffer() function performs the same cleanup
> > action in many error paths. Use ScopeExitActions to simplify it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   src/libcamera/v4l2_videodevice.cpp | 12 ++++--------
> >   1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 71cc7e895d8c..5f57e546e80b 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1646,6 +1646,7 @@ int V4L2VideoDevice::releaseBuffers()
> >   int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >   {
> >   	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> > +	utils::ScopeExitActions actions;
> >   	struct v4l2_buffer buf = {};
> >   	int ret;
> > 
> > @@ -1668,6 +1669,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >   	if (ret < 0)
> >   		return ret;
> > 
> > +	actions += [&]() { cache_->put(buf.index); };
> 
> I think it can be argued that `V4L2VideoDevice::queueBuffer` should be considered
> a "hot path" in libcamera. And for that reason I think avoiding the constant memory
> allocations of `utils::ScopeExitActions` would be preferable.

Good point. I agree with you.

> I think adding https://www.en.cppreference.com/w/cpp/experimental/scope_exit.html
> should be simple to do, and it would address the above.

I'll give that a try.

> > +
> >   	buf.index = ret;
> >   	buf.type = bufferType_;
> >   	buf.memory = memoryType_;
> > @@ -1683,15 +1686,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >   	 */
> >   	if (planes.size() < numV4l2Planes) {
> >   		LOG(V4L2, Error) << "Frame buffer has too few planes";
> > -		cache_->put(buf.index);
> > -
> >   		return -EINVAL;
> >   	}
> > 
> >   	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
> >   		LOG(V4L2, Error) << "Device format requires contiguous buffer";
> > -		cache_->put(buf.index);
> > -
> >   		return -EINVAL;
> >   	}
> > 
> > @@ -1734,8 +1733,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >   				if (i != planes.size() - 1 && bytesused != length) {
> >   					LOG(V4L2, Error)
> >   						<< "Holes in multi-planar buffer not supported";
> > -					cache_->put(buf.index);
> > -
> >   					return -EINVAL;
> >   				}
> >   			}
> > @@ -1785,8 +1782,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >   		LOG(V4L2, Error)
> >   			<< "Failed to queue buffer " << buf.index << ": "
> >   			<< strerror(-ret);
> > -		cache_->put(buf.index);
> > -
> >   		return ret;
> >   	}
> > 
> > @@ -1798,6 +1793,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> > 
> >   	queuedBuffers_[buf.index] = buffer;
> > 
> > +	actions.release();
> >   	return 0;
> >   }
> > 
> > 
> > base-commit: b65df7e7554b45e2d3d7fdb5b37c2ab7df3db4fe

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 71cc7e895d8c..5f57e546e80b 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1646,6 +1646,7 @@  int V4L2VideoDevice::releaseBuffers()
 int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 {
 	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
+	utils::ScopeExitActions actions;
 	struct v4l2_buffer buf = {};
 	int ret;

@@ -1668,6 +1669,8 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 	if (ret < 0)
 		return ret;

+	actions += [&]() { cache_->put(buf.index); };
+
 	buf.index = ret;
 	buf.type = bufferType_;
 	buf.memory = memoryType_;
@@ -1683,15 +1686,11 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 	 */
 	if (planes.size() < numV4l2Planes) {
 		LOG(V4L2, Error) << "Frame buffer has too few planes";
-		cache_->put(buf.index);
-
 		return -EINVAL;
 	}

 	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
 		LOG(V4L2, Error) << "Device format requires contiguous buffer";
-		cache_->put(buf.index);
-
 		return -EINVAL;
 	}

@@ -1734,8 +1733,6 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 				if (i != planes.size() - 1 && bytesused != length) {
 					LOG(V4L2, Error)
 						<< "Holes in multi-planar buffer not supported";
-					cache_->put(buf.index);
-
 					return -EINVAL;
 				}
 			}
@@ -1785,8 +1782,6 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 		LOG(V4L2, Error)
 			<< "Failed to queue buffer " << buf.index << ": "
 			<< strerror(-ret);
-		cache_->put(buf.index);
-
 		return ret;
 	}

@@ -1798,6 +1793,7 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)

 	queuedBuffers_[buf.index] = buffer;

+	actions.release();
 	return 0;
 }