[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object

[Laurent Pinchart]

Hi Kieran,

On Wed, Jan 16, 2019 at 06:00:55PM +0000, Kieran Bingham wrote:
> On 16/01/2019 14:59, Niklas Söderlund wrote:
> > On 2019-01-15 23:19:08 +0000, Kieran Bingham wrote:
> >> Provide a helper V4L2 device object capable of interacting with the
> >> V4L2 Linux Kernel APIs.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/include/v4l2_device.h |  45 +++++++++
> >>  src/libcamera/meson.build           |   2 +
> >>  src/libcamera/v4l2_device.cpp       | 150 ++++++++++++++++++++++++++++
> >>  3 files changed, 197 insertions(+)
> >>  create mode 100644 src/libcamera/include/v4l2_device.h
> >>  create mode 100644 src/libcamera/v4l2_device.cpp
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> new file mode 100644
> >> index 000000000000..b0f5e9689654
> >> --- /dev/null
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -0,0 +1,45 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * v4l2_device.h - V4L2 Device
> >> + */
> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >> +#define __LIBCAMERA_V4L2_DEVICE_H__
> >> +
> >> +#include <string>
> >> +
> >> +#include <linux/videodev2.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class V4L2Capability : public v4l2_capability
> >> +{
> >> +public:
> >> +	std::string getDriver() const { return std::string((char *)driver); }
> >> +	std::string getCard() const { return std::string((char *)card); }
> > 
> > I'm not loving this. I think this is a deal breaker in this case to 
> > inherit from struct v4l2_capability. I would make the struct a private 
> > member of the class and make this drivre() and card().
> 
> Can you explain /why/ you think it's a deal breaker please?
> 
> 
> Is your objection to the 'get' prefix (as in getDriver())?
> I now have a solution for that with appropriate casting:
> 
>  std::string driver() const {
> 	return reinterpret_cast<const char *>(v4l2_capability::driver);
>  }
> 
> 
> Otherwise, I believe it does inherit from v4l2_capability.
> 
> If I could I would write write:
> 
> struct v4l2_capability : struct v4l2_capability {
> public:
> 	std::string driver() const;
> 	std::string card() const;
> 	bool isCapture() const;
> };
> 
> 
> The point is if I could add methods to the struct v4l2_capability, I'd
> be doing that ... but I can't. And I certainly don't want to duplicate
> the structure...
> 
> The V4L2Capability is simply providing an interface at the C++ level and
> defining how to work with the data in the structure in an object
> orientated way.
> 
> This means that any methods which need to interact with struct
> v4l2_capability are contained in a common location - and those methods
> can only operate on the structure ...
> 
> Putting a v4l2_capability as a private member in a class V4L2Capability
> produces: [2] which I quite dislike. I've updated V4L2Capability to mark
> it final to express that this class should not be further extended or
> overridden.
> 
> [1] https://paste.debian.net/1060853/ {inheritance}
> [2] https://paste.debian.net/1060852/ {composition}
> [3] https://paste.debian.net/1060839/ {flat in V4L2Device}
> 
> 
> My biggest reason for continuing to push this - is because I feel this
> pattern will be beneficial for the many complex structure types in V4L2.
> I'm thinking controls, pix_format, fmtdesc, requestbuffers...
> 
> (Not everything, just ones where it makes sense to define helpers or
> bring the kernel API to a C++ interface)
> 
> They are all 'objects' which are managed by the kernel. It's just that
> the data for them is in a c-struct because that's what the kernel has.
> 
> In my opinion - handling those types as objects in the C++ will keep the
> code where it counts. On the object it deals with.
> 
> If we go with [3] (which I don't want), then the V4L2Device class is
> going to be polluted with all sorts of helpers (helpful as they may be)
> which operate on the various structures. It's also no good in the case
> where there are multiple objects (like v4l2_buffers).
> 
> 
> I don't think [2] is appropriate here. It doesn't protect us from
> anything, and we are not extending any types or fields in the struct
> v4l2_capability.
> 
> 
> (I'd also envisage re-using this V4L2Device class externally too, so
> having nice wrappings around things can provide further benefits)

FOr what it's worth, I think wrapping v4l2_capabilities is useful in
this case, and I like the proposed implementation, especially with the
solution that creates getters with a get prefix.

> >> +
> >> +	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> >> +	bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
> > 
> > Not saying it's wrong but it confuses me a bit. Why is one capability 
> > check prefixed with 'is' and the other 'has'?
> 
> Grammar :)
> 
> The device 'is' a capture device.
> 	(_CAPTURE | _CAPTURE_MPLANE) vs (_OUTPUT / _M2M)
> 
> The device 'has' streaming IO ioctls
> The device 'has' async io ioctls

And the device 'has' capture capability ;-) Both prefixes are valid, I'm
slightly tempted to standardize on has in this case, but I won't push
for it.

> ...
> 
> (Now technically an M2M 'is' both a _CAPTURE and an _OUTPUT but that's
> differentiated by being an M2M...)
> 
> > 
> >> +};

[snip]

-- 
Regards,

Laurent Pinchart