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

[Jacopo Mondi]

Hi Kieran,

On Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/01/2019 11:11, Jacopo Mondi wrote:
>
> <snip>
> >>
> >> The getDriver() and getCard() wraps the casting required again into the
> >> class definition so that it doesn't have to be done in arbitrary places
> >> in the code.
> >>
> >> I expect to add next:
> >>
> >>   V4L2Device::driverName() { return caps_.getDriver(); }
> >>   V4L2Device::deviceName() { return caps_.getCard(); }
> >> 	// Or ? getDeviceName()?
> >>   V4L2Device::busName() { return caps_.getBus(); }
> >> 	// (Yes, I'll add this one in to the V4L2Capabiltiy)
> >>
> >> I anticipate that these strings will be useful to you in the pipeline
> >> classes.
> >>
> >> The UVC pipeline manager should certainly use them.
> >>
> >
> > One note I missed in both my previous comments: getter methods are named as
> > the member field they access, without the "get" prefix.
>
> I can't do that here.
> If I name the method the same as the base struct member I get:
>
> ../src/libcamera/include/v4l2_device.h: In member function
> ‘std::__cxx11::string libcamera::V4L2Capability::driver() const’:
> ../src/libcamera/include/v4l2_device.h:23:58: error: invalid use of
> member function ‘std::__cxx11::string
> libcamera::V4L2Capability::driver() const’ (did you forget the ‘()’ ?)
>   std::string driver() const { return std::string((char *)driver); }
>

Ah! that's because of the v4l2_capability 'driver' field.
That would be called driver_ in a standalone V4L2Capabilities class...
>
> >>> I won't push anymore on this though.
> >>
> >> I don't mind the push ... I knew this 'wrapping' would be potentially
> >> controversial it's partly me trying to experiment with good ways to
> >> interact with the Kernel API.
> >>
> >> At the moment, I must say I really like it though ...
> >> 	 <waits for the tumbleweed to pass if I'm the only one>
> >>
> >> Especially with the ease and cleanliness for exposing the capability
> >> strings.
> >>
> >
> > I'll let other express concerns, if any. Otherwise that's fine with
> > me.
>
> Ok thanks,
>
>
> >>>> +
> >>>> +	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> >>>
> >>> You pointed out in your self-review this was meant to be changed,
> >>> right?
> >>
> >> Half-right.
> >>
> >> Then based on your other comments regarding MPLANE/Single Plane I
> >> decided to drop MPLANE support.
> >>
> >> We don't need it yet, the IPU3 doesn't support MPLANE, the RCAR-VIN
> >> doesn't support MPLANE, UVC doesn't support MPLANE ...
> >>
> >
> > Are you sure?
> >
> > - entity 4: ipu3-cio2 0 (1 pad, 1 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >
> >
> > # v4l2-ctl -D -d /dev/video0
> > Driver Info (not using libv4l2):
> > 	Driver name   : ipu3-cio2
> > 	Card type     : Intel IPU3 CIO2
> > 	Bus info      : PCI:0000:00:14.3
> > 	Driver version: 4.20.0
> > 	Capabilities  : 0x84201000
> > 		Video Capture Multiplanar       <---------------
>
> Hrm ... my grep foo failed me then ...
>
> OK - so MPLANE /is/ back on the requirements list :)
>

Yes please. MPLANE support should be there from the very beginning
imho.

> > 		Streaming
> > 		Extended Pix Format
> > 		Device Capabilities
> > 	Device Caps   : 0x04201000
> > 		Video Capture Multiplanar
> > 		Streaming
> > 		Extended Pix Format
> >
> >>
> >>>> +	bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
> >>>> +};
> >>>> +
> >>>> +class V4L2Device
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Device() = delete;
> >>>
> >>> Should this be private?
> >>
> >> I wondered that - but I figured - it doesn't matter if it's public or
> >> private. It's declaring that it's gone. And I thought it was better to
> >> keep the constructors (or lack of) and destructors together.
> >>
> >> If this is something we want to standardize (keeping deletions in
> >> private:) I can move it.
> >>
> >
> > No big deal, right...
>
> I also realise I should add a deletion for the copy constructor...
>
> <added for next spin>
>
> <snip>

Good point. Thanks.

>
> >>>> +/**
> >>>> + * \brief Open a V4L2 device and query properties from the device.
> >>>> + * \return 0 on success, or a negative error code otherwise
> >>>> + */
> >>>> +int V4L2Device::open()
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	if (isOpen()) {
> >>>> +		LOG(Error) << "Device already open";
> >>>> +		return -EBUSY;
> >>>> +	}
> >>>> +
> >>>> +	ret = ::open(devnode_.c_str(), O_RDWR);
> >>>> +	if (ret < 0) {
> >>>> +		ret = -errno;
>
> This assignment feels a little redundant.
>
> I agree on only setting the fd_ = ret if success is good - but I'm
> feeling like I can save a line (think of the bits) and just return -errno?
>
>
> >>>> +		LOG(Error) << "Failed to open V4L2 device " << devnode_
> >>>> +			   << " : " << strerror(ret);
> >>>
> >>> strerror(-ret)
> >>>
> >>> Even if in this case you could have used errno directly. Sorry, my
> >>> comment might has mis-lead you
> >>
> >> Ahh oops - and good spot.
> >>
> >> But yes - I'd much rather reference the errno directly for strerror().
> >>
> >>
> >>>
> >>>> +		return ret;
> >>>> +	}
> >>>> +	fd_ = ret;
> >>>> +
> >>>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> >>>
>
> <snip>
>
> >>>> +/**
> >>>> + * \brief Check if device is successfully opened
> >>>> + */
> >>>> +bool V4L2Device::isOpen() const
> >>>> +{
> >>>> +	return (fd_ != -1);
> >>>
> >>> Ah, I see what you've done here (return with no () )
> >>>
> >>> 	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> >>
> >> Ah I've been caught! (ok well it wasn't intentional hehe)
> >>
> >> I /almost/ want to add () to the isCapture() but I don't want to
> >> increase that line length...
> >>
> >> /me cries a little and does:
> >>
> >>    s/(fd_ != -1)/fd != -1/
> >>
> >
> > As you like the most!
>
> I've removed the brackets to simplify.

Thanks
   j

>
>
> >
> > Thanks
> >   j
>
> <snip>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190116/d977541c/attachment.sig>