[libcamera-devel] [PATCH] libcamera: v4l2_device: Group base operations in V4L2Base

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-01-24 16:31:35 +0100, Jacopo Mondi wrote:
> Group operations commont to V4L2Device and the forthcoming V4L2Subdevice
> in a common V4L2Base class.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> As I started implementing V4L2Subdevice support, I realized I was going to
> re-implement what is now part of V4L2Base.
> 
> I would like to develop V4L2Subdevice on top of this.
> Patch to be applied on top of my latest:
> [PATCH v2 0/2] Add mplane support to V4L2 device
> 
> Thanks
>    j
> ---
>  src/libcamera/include/v4l2_device.h |  38 ++++++---
>  src/libcamera/v4l2_device.cpp       | 127 ++++++++++++++++++----------
>  2 files changed, 112 insertions(+), 53 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 2da30d1..08bfe9f 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -54,30 +54,48 @@ struct V4L2Capability final : v4l2_capability {
>  };
> 
>  class MediaEntity;
> -class V4L2Device
> +class V4L2Base
>  {
>  public:
> -	explicit V4L2Device(const std::string &deviceNode);
> -	explicit V4L2Device(const MediaEntity &entity);
> +	virtual ~V4L2Base() { close(); }
> +
> +	virtual int open();
> +	bool isOpen() const { return fd_ != -1; }
> +	virtual void close();
> +
> +protected:
> +	V4L2Base(const std::string &deviceNode);
> +	V4L2Base(const MediaEntity &entity);
> +	V4L2Base(const V4L2Base &) = delete;
> +	bool operator=(const V4L2Base &) = delete;
> +
> +	std::string deviceNode_;
> +	int fd_;
> +};
> +
> +class V4L2Device : public V4L2Base
> +{
> +public:
> +	explicit V4L2Device(const std::string &deviceNode)
> +		: V4L2Base(deviceNode) {}
> +	explicit V4L2Device(const MediaEntity &entity)
> +		: V4L2Base(entity) {}
>  	V4L2Device(const V4L2Device &) = delete;
> -	~V4L2Device();
> 
> -	void operator=(const V4L2Device &) = delete;
> +	~V4L2Device() {}
> 
> -	int open();
> -	bool isOpen() const;
> -	void close();
> +	void operator=(const V4L2Device &) = delete;
> 
>  	const char *driverName() const { return caps_.driver(); }
>  	const char *deviceName() const { return caps_.card(); }
>  	const char *busName() const { return caps_.bus_info(); }
> 
> +	int open();
> +
>  	int format();
>  	int setFormat();
> 
>  private:
> -	std::string deviceNode_;
> -	int fd_;
>  	V4L2Capability caps_;
> 
>  	int getFormatSingleplane();
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2ddb800..f505240 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -81,16 +81,15 @@ LOG_DEFINE_CATEGORY(V4L2)
>   */
> 
>  /**
> - * \class V4L2Device
> - * \brief V4L2Device object and API
> + * \class V4L2Base
> + * \brief Group methods and field common to V4L2 devices and subdevices
>   *
> - * The V4L2 Device API class models an instance of a V4L2 device node.
> - * It is constructed with the path to a V4L2 video device node. The device node
> - * is only opened upon a call to open() which must be checked for success.
> + * Represents the base class for V4L2 devices and sub-devices.
> + * It is constructed with the path to a V4L2 video (sub-)device node or
> + * with a MediaEntity, provided its deviceNode() path has been set.
>   *
> - * The device capabilities are validated when the device is opened and the
> - * device is rejected if it is not a suitable V4L2 capture or output device, or
> - * if the device does not support streaming I/O.
> + * The device node is only opened upon a call to open() which must be
> + * checked for success.
>   *
>   * No API call other than open(), isOpen() and close() shall be called on an
>   * unopened device instance.
> @@ -100,35 +99,32 @@ LOG_DEFINE_CATEGORY(V4L2)
>   */
> 
>  /**
> - * \brief Construct a V4L2Device
> - * \param deviceNode The file-system path to the video device node
> + * \fn V4L2Base::V4L2Base(const std::string &deviceNode)
> + * \brief Construct a V4L2 base instance
> + * \param deviceNode The file-system path to the video (sub-)device node
>   */
> -V4L2Device::V4L2Device(const std::string &deviceNode)
> +V4L2Base::V4L2Base(const std::string &deviceNode)
>  	: deviceNode_(deviceNode), fd_(-1)
>  {
>  }
> 
>  /**
> - * \brief Construct a V4L2Device from a MediaEntity
> + * \fn V4L2Base::V4L2Base(const MediaEntity &entity)
> + * \brief Construct a V4L2 base instance from a MediaEntity
>   * \param entity The MediaEntity to build the device from
>   *
> - * Construct a V4L2Device from a MediaEntity's device node path.
> + * Construct a V4L2 base instance from a MediaEntity's device node path.
>   */
> -V4L2Device::V4L2Device(const MediaEntity &entity)
> -	: V4L2Device(entity.deviceNode())
> -{
> -}
> -
> -V4L2Device::~V4L2Device()
> +V4L2Base::V4L2Base(const MediaEntity &entity)
> +	: V4L2Base(entity.deviceNode())
>  {
> -	close();
>  }
> 
>  /**
> - * \brief Open a V4L2 device and query its capabilities
> + * \brief Open a device node associated with a V4L2 device or subdevice
>   * \return 0 on success, or a negative error code otherwise
>   */
> -int V4L2Device::open()
> +int V4L2Base::open()
>  {
>  	int ret;
> 
> @@ -147,6 +143,72 @@ int V4L2Device::open()
>  	}
>  	fd_ = ret;
> 
> +	return 0;
> +}
> +
> +/**
> + * \fn V4L2Base::isOpen()
> + * \brief Check if device is successfully opened
> + * \return True if the device is open, false otherwise
> + */
> +
> +/**
> + * \brief Close the device node, releasing any resources acquired by open()
> + */
> +void V4L2Base::close()
> +{
> +	if (fd_ < 0)
> +		return;

I would use isOpen() here to make it consistent.

With this fixed,

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

> +
> +	::close(fd_);
> +	fd_ = -1;
> +}
> +
> +/**
> + * \var V4L2Base::deviceNode_
> + * \brief The device node path associated with the V4L2 instance.
> + */
> +
> +/**
> + * \var V4L2Base::fd_
> + * \brief The file descriptor associated with an open device node
> + */
> +
> +/**
> + * \class V4L2Device
> + * \brief V4L2Device object and API
> + *
> + * The V4L2 Device API class models an instance of a V4L2 device node.
> + *
> + * The device capabilities are validated when the device is opened and the
> + * device is rejected if it is not a suitable V4L2 capture or output device, or
> + * if the device does not support streaming I/O.
> + */
> +
> +/**
> + * \fn V4L2Device::V4L2Device(const std::string &deviceNode)
> + * \brief Construct a V4L2Device
> + * \param deviceNode The file-system path to the video (sub)device node
> + */
> +
> +/**
> + * \fn V4L2Device::V4L2Device(const MediaEntity &entity)
> + * \brief Construct a V4L2Device from a MediaEntity
> + * \param entity The MediaEntity to build the device from
> + *
> + * Construct a V4L2Device from a MediaEntity's device node path.
> + */
> +
> +/**
> + * \brief Open a V4L2 device and query its capabilities
> + * \return 0 on success, or a negative error code otherwise
> + */
> +int V4L2Device::open()
> +{
> +	int ret = V4L2Base::open();
> +	if (ret)
> +		return ret;
> +
>  	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
>  	if (ret < 0) {
>  		ret = -errno;
> @@ -174,27 +236,6 @@ int V4L2Device::open()
>  	return 0;
>  }
> 
> -/**
> - * \brief Check if device is successfully opened
> - * \return True if the device is open, false otherwise
> - */
> -bool V4L2Device::isOpen() const
> -{
> -	return fd_ != -1;
> -}
> -
> -/**
> - * \brief Close the device, releasing any resources acquired by open()
> - */
> -void V4L2Device::close()
> -{
> -	if (fd_ < 0)
> -		return;
> -
> -	::close(fd_);
> -	fd_ = -1;
> -}
> -
>  /**
>   * \fn const char *V4L2Device::driverName()
>   * \brief Retrieve the name of the V4L2 device driver
> --
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund