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

[Jacopo Mondi]

Hi Kieran,

On Tue, Feb 26, 2019 at 11:28:09PM +0000, Kieran Bingham wrote:
>
>
> 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()

In prevision of introducing support for META_OUTPUT, I have now named
this 'isMetaCapture()'.

I'm not so concerned about the if matrix, it will look like:

	if (caps_.isCapture()) {
		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
		bufferType_ = caps_.isMultiplanar()
			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
	} else if (caps_.isOutput()) {
		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
		bufferType_ = caps_.isMultiplanar()
			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
	} else if (caps_.isMetaCapture()) {
		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
	} else if (caps_.isMetaOutput()) {
		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
	} else {
		LOG(V4L2, Debug) << "Device is not a supported type";
		return -EINVAL;
	}

We could rename isCapture/isOutput to isVideoCapture/isVideoOutput for
simmetry, but I don't think that's a big deal, isn't it?

Thanks
  j

>
> 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
-------------- 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/20190227/0436cab8/attachment.sig>