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

[Kieran Bingham]

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); }


>>> 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 :)

> 		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>

>>>> +/**
>>>> + * \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

<snip>
-- 
Regards
--
Kieran