| Message ID | 20190124153135.378-1-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Thu, Jan 24, 2019 at 04:31:35PM +0100, Jacopo Mondi wrote: > Group operations commont to V4L2Device and the forthcoming V4L2Subdevice > in a common V4L2Base class. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > As I started implementing V4L2Subdevice support, I realized I was going to > re-implement what is now part of V4L2Base. As I mentioned before, I think the amount of code currently in your V4L2Base class isn't worth it. It would be a different story if we had more code to share, it would then be easier to convince me :-) Furthemore, the code in V4L2Base isn't V4L2-specific, so V4L2Base isn't a proper name in my opinion and I don't think we want to create a custom File class just to encapsulate open and close. > 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 > > --- > 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; > + > + ::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
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; + + ::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
Group operations commont to V4L2Device and the forthcoming V4L2Subdevice in a common V4L2Base class. Signed-off-by: Jacopo Mondi <jacopo@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(-) -- 2.20.1