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

2221 diff mbox series

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.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---

Since RFC:
 - Rebased to master.

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

Hi Kieran,

Thank you for the patch.

On Fri, Oct 25, 2019 at 03:13:27PM +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.
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> 
> Since RFC:
>  - Rebased to master.
> 
>  src/libcamera/include/v4l2_videodevice.h |  2 ++
>  src/libcamera/v4l2_videodevice.cpp       | 10 +++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 4b8cf9394eb9..caf36bf9c1aa 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -187,6 +187,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 208ab54199b1..c0b3a79f76da 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -272,7 +272,8 @@ const std::string V4L2DeviceFormat::toString() const
>   * \param[in] deviceNode The file-system path to the video device node
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> -	: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)
> +	: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr),
> +	  streaming_(false)
>  {
>  	/*
>  	 * We default to an MMAP based CAPTURE video device, however this will
> @@ -1173,6 +1174,8 @@ int V4L2VideoDevice::streamOn()
>  		return ret;
>  	}
>  
> +	streaming_ = true;
> +
>  	return 0;
>  }
>  
> @@ -1190,6 +1193,9 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> +	if (!streaming_)
> +		return 0;
> +

I forgot to mention that previously, I think the documentation should be
updated to state that stopping an already stopped device is a no-op.
With that,

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

>  	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
> @@ -1210,6 +1216,8 @@ int V4L2VideoDevice::streamOff()
>  	queuedBuffers_.clear();
>  	fdEvent_->setEnabled(false);
>  
> +	streaming_ = false;
> +
>  	return 0;
>  }
>

Hi Kieran,

Thanks for your work.

On 2019-10-25 15:13:27 +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.
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

With Laurent's comment about documenting the new behavior of streamOff() 
addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> 
> Since RFC:
>  - Rebased to master.
> 
>  src/libcamera/include/v4l2_videodevice.h |  2 ++
>  src/libcamera/v4l2_videodevice.cpp       | 10 +++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 4b8cf9394eb9..caf36bf9c1aa 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -187,6 +187,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 208ab54199b1..c0b3a79f76da 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -272,7 +272,8 @@ const std::string V4L2DeviceFormat::toString() const
>   * \param[in] deviceNode The file-system path to the video device node
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> -	: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)
> +	: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr),
> +	  streaming_(false)
>  {
>  	/*
>  	 * We default to an MMAP based CAPTURE video device, however this will
> @@ -1173,6 +1174,8 @@ int V4L2VideoDevice::streamOn()
>  		return ret;
>  	}
>  
> +	streaming_ = true;
> +
>  	return 0;
>  }
>  
> @@ -1190,6 +1193,9 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> +	if (!streaming_)
> +		return 0;
> +
>  	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
> @@ -1210,6 +1216,8 @@ int V4L2VideoDevice::streamOff()
>  	queuedBuffers_.clear();
>  	fdEvent_->setEnabled(false);
>  
> +	streaming_ = false;
> +
>  	return 0;
>  }
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel