[libcamera-devel] [PATCH v4 5/8] libcamera: v4l2_videodevice: Better tracking of the device state

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, Mar 22, 2022 at 09:22:54AM +0000, Naushir Patuck via libcamera-devel wrote:
> Replace the existing streaming_ state variable with an enum to track the
> following three state: Streaming, Stopping, and Stopped. The alternate states
> will be used in a subsequent commit.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  3 ++-
>  src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 2d2ccc477c91..32e022543385 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -258,7 +258,8 @@ private:
>  
>  	EventNotifier *fdBufferNotifier_;
>  
> -	bool streaming_;
> +	enum class State { Streaming, Stopping, Stopped };

	enum class State {
		Streaming,
		Stopping,
		Stopped,
	};

and this should go right after LIBCAMERA_DISABLE_COPY() as we declare
types before functions and variables.

> +	State state_;
>  };
>  
>  class V4L2M2MDevice
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 5f36ee20710d..9cea6a608660 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
>  	: V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> -	  fdBufferNotifier_(nullptr), streaming_(false)
> +	  fdBufferNotifier_(nullptr), state_(State::Stopped)
>  {
>  	/*
>  	 * We default to an MMAP based CAPTURE video device, however this will
> @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
>  		return ret;
>  	}
>  
> -	streaming_ = true;
> +	state_ = State::Streaming;
>  
>  	return 0;
>  }
> @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> -	if (!streaming_ && queuedBuffers_.empty())
> +	if (state_ != State::Streaming && queuedBuffers_.empty())
>  		return 0;
>  
>  	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
>  		return ret;
>  	}
>  
> +	state_ = State::Stopping;
> +
>  	/* Send back all queued buffers. */
>  	for (auto it : queuedBuffers_) {
>  		FrameBuffer *buffer = it.second;
> @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()
>  
>  	queuedBuffers_.clear();
>  	fdBufferNotifier_->setEnabled(false);
> -	streaming_ = false;
> +	state_ = State::Stopped;

This looks fine, but may depend on how it's then used by the next patch.
Provided that's fine too, with the above change,

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

>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart