[libcamera-devel,RFC] libcamera: v4l2_videodevice: Track streaming state

Message ID 20191022160110.19419-1-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,RFC] libcamera: v4l2_videodevice: Track streaming state
Related show

Commit Message

Kieran Bingham Oct. 22, 2019, 4:01 p.m. UTC
Track the state of streamon/streamoff calls to simplify error paths.

Ensuring that streamOff() can be called on non-streaming streams facilitates simpler
error code paths, where a set of devices can all call streamOff regardless of their
initialisation state.

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

This is a patch I have while developing the RPi ISP handler.
It means I can do the following for error paths:

	data->isp_.stats_->streamOff();
	data->isp_.capture1_->streamOff();
 	data->isp_.capture0_->streamOff();
 	data->isp_.output_->streamOff();
 	data->unicam_->streamOff();

which essentially means I can just call stop(camera) for cleanup, without
having to track which ones were successfully started or not.

So - Reasonable or not ?

Patch will not apply directly to master, as it's currently based on my dev
branch, hence RFC

 src/libcamera/include/v4l2_videodevice.h | 2 ++
 src/libcamera/v4l2_videodevice.cpp       | 9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Oct. 23, 2019, 3:08 p.m. UTC | #1
Hi Kieran,

Thank you for the patch.

On Tue, Oct 22, 2019 at 05:01:10PM +0100, Kieran Bingham wrote:
> Track the state of streamon/streamoff calls to simplify error paths.
> 
> Ensuring that streamOff() can be called on non-streaming streams facilitates simpler
> error code paths, where a set of devices can all call streamOff regardless of their
> initialisation state.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> 
> This is a patch I have while developing the RPi ISP handler.
> It means I can do the following for error paths:
> 
> 	data->isp_.stats_->streamOff();
> 	data->isp_.capture1_->streamOff();
>  	data->isp_.capture0_->streamOff();
>  	data->isp_.output_->streamOff();
>  	data->unicam_->streamOff();
> 
> which essentially means I can just call stop(camera) for cleanup, without
> having to track which ones were successfully started or not.
> 
> So - Reasonable or not ?

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I hope this answers your question :-)

> Patch will not apply directly to master, as it's currently based on my dev
> branch, hence RFC
> 
>  src/libcamera/include/v4l2_videodevice.h | 2 ++
>  src/libcamera/v4l2_videodevice.cpp       | 9 ++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 60fca01670c6..d6e0c0e7bf02 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -188,6 +188,8 @@ private:
>  	std::map<unsigned int, Buffer *> queuedBuffers_;
>  
>  	EventNotifier *fdEvent_;
> +
> +	bool streaming_;
>  };
>  
>  class V4L2M2MDevice
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 71c32766fc2d..01e11597cfb0 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -273,7 +273,7 @@ const std::string V4L2DeviceFormat::toString() const
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
>  	: V4L2Device(deviceNode), multiPlanar_(false), bufferPool_(nullptr),
> -	  fdEvent_(nullptr)
> +	  fdEvent_(nullptr), streaming_(false)
>  {
>  	/*
>  	 * We default to an MMAP based CAPTURE video device, however this will
> @@ -1197,6 +1197,8 @@ int V4L2VideoDevice::streamOn()
>  		return ret;
>  	}
>  
> +	streaming_ = true;
> +
>  	return 0;
>  }
>  
> @@ -1214,6 +1216,9 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> +	if (!streaming_)
> +		return 0;
> +
>  	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
> @@ -1234,6 +1239,8 @@ int V4L2VideoDevice::streamOff()
>  	queuedBuffers_.clear();
>  	fdEvent_->setEnabled(false);
>  
> +	streaming_ = false;
> +
>  	return 0;
>  }
>

Patch

diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
index 60fca01670c6..d6e0c0e7bf02 100644
--- a/src/libcamera/include/v4l2_videodevice.h
+++ b/src/libcamera/include/v4l2_videodevice.h
@@ -188,6 +188,8 @@  private:
 	std::map<unsigned int, Buffer *> queuedBuffers_;
 
 	EventNotifier *fdEvent_;
+
+	bool streaming_;
 };
 
 class V4L2M2MDevice
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 71c32766fc2d..01e11597cfb0 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -273,7 +273,7 @@  const std::string V4L2DeviceFormat::toString() const
  */
 V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
 	: V4L2Device(deviceNode), multiPlanar_(false), bufferPool_(nullptr),
-	  fdEvent_(nullptr)
+	  fdEvent_(nullptr), streaming_(false)
 {
 	/*
 	 * We default to an MMAP based CAPTURE video device, however this will
@@ -1197,6 +1197,8 @@  int V4L2VideoDevice::streamOn()
 		return ret;
 	}
 
+	streaming_ = true;
+
 	return 0;
 }
 
@@ -1214,6 +1216,9 @@  int V4L2VideoDevice::streamOff()
 {
 	int ret;
 
+	if (!streaming_)
+		return 0;
+
 	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
 	if (ret < 0) {
 		LOG(V4L2, Error)
@@ -1234,6 +1239,8 @@  int V4L2VideoDevice::streamOff()
 	queuedBuffers_.clear();
 	fdEvent_->setEnabled(false);
 
+	streaming_ = false;
+
 	return 0;
 }