[libcamera-devel] [PATCH v3 3/8] libcamera: v4l2_device: Add support for META_CAPTURE devices

[Kieran Bingham]

On 26/02/2019 23:24, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 26/02/2019 16:26, Jacopo Mondi wrote:
>> Add support for devices that provide video meta-data to v4l2_device.cpp
>> and re-arrange bufferType handling in open() method.
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/libcamera/include/v4l2_device.h |  4 +++
>>  src/libcamera/v4l2_device.cpp       | 38 +++++++++++++++++------------
>>  2 files changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> index 1d31d1b403bc..52eb6785cc15 100644
>> --- a/src/libcamera/include/v4l2_device.h
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability {
>>  		return device_caps() & (V4L2_CAP_VIDEO_CAPTURE |
>>  					V4L2_CAP_VIDEO_CAPTURE_MPLANE);
>>  	}
>> +	bool isMeta() const

One last thought here,

I'm a bit concerned that if we have isMeta and isOutput handling this -
we might end up with an ugly 2 x 2 matrix in if statements.

Any pretty way to make this better?

Or would it be cleaner to move towards:

bool isMetaCapture()
bool isMetaOutput()

This bit feels like a pain point - but I don't think that should stop
this patch right now.




>> +	{
>> +		return device_caps() & V4L2_CAP_META_CAPTURE;
>> +	}
>>  	bool isOutput() const
>>  	{
>>  		return device_caps() & (V4L2_CAP_VIDEO_OUTPUT |
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 24e115554a99..8be8af7a2893 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2)
>>   * \return True if the device can output video frames
>>   */
>>  
>> +/**
>> + * \fn bool V4L2Capability::isMeta()
>> + * \brief Identify if the device is capable of providing video meta-data
>> + *
>> + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20
> 
> I think I saw Niklas suggest to make this a Todo: rather than fixme -
> but I don't think we need to worry too much over terminology that should
> be removed within a couple of weeks :D as We'll update this after the
> 5.0 is released.
> 
> 
> Likewise, I think the conditionals below might get some more thought put
> into them when we add in META_OUTPUT - but that's all future work.
> 
> This patch gets us closer.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
>> + *
>> + * \return True if the device can provide video meta-data
>> + */
>> +
>>  /**
>>   * \fn bool V4L2Capability::hasStreaming()
>>   * \brief Determine if the device can perform Streaming I/O
>> @@ -280,33 +289,32 @@ int V4L2Device::open()
>>  		<< "Opened device " << caps_.bus_info() << ": "
>>  		<< caps_.driver() << ": " << caps_.card();
>>  
>> -	if (!caps_.isCapture() && !caps_.isOutput()) {
>> -		LOG(V4L2, Debug) << "Device is not a supported type";
>> -		return -EINVAL;
>> -	}
>> -
>>  	if (!caps_.hasStreaming()) {
>>  		LOG(V4L2, Error) << "Device does not support streaming I/O";
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (caps_.isCapture())
>> +	/*
>> +	 * Set buffer type and wait for read notifications on CAPTURE devices
>> +	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
>> +	 */
>> +	if (caps_.isCapture()) {
>> +		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
>>  		bufferType_ = caps_.isMultiplanar()
>>  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>>  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -	else
>> +	} else if (caps_.isOutput()) {
>> +		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
>>  		bufferType_ = caps_.isMultiplanar()
>>  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>>  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> -
>> -	/*
>> -	 *  We wait for Read notifications on CAPTURE devices (POLLIN), and
>> -	 *  Write notifications for OUTPUT devices (POLLOUT).
>> -	 */
>> -	if (caps_.isCapture())
>> +	} else if (caps_.isMeta()) {
>>  		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
>> -	else
>> -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
>> +		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>> +	} else {
>> +		LOG(V4L2, Debug) << "Device is not a supported type";
>> +		return -EINVAL;
>> +	}
>>  
>>  	fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable);
>>  	fdEvent_->setEnabled(false);
>>
> 

-- 
Regards
--
Kieran