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

[Jacopo Mondi]

Hi Laurent,

On Thu, Feb 28, 2019 at 11:45:37PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Feb 28, 2019 at 08:15:57PM +0100, Jacopo Mondi wrote:
> > On Thu, Feb 28, 2019 at 07:25:49PM +0200, Laurent Pinchart wrote:
> > > On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote:
> > >> On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote:
> > >>> On Tue, Feb 26, 2019 at 05:26:36PM +0100, 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);
> >
> > [snip]
> >
> > >> As done in v4, I introduced an isMetaCapture() method, preparing for
> > >> an isMetaOutput() one. In this way isCapture() and isOutput() would
> > >> refer to video streams only and the bufferType intialization looks
> > >> like:
> > >
> > > That's confusing. Let's have
> > >
> > > isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE |
> > > 	       V4L2_CAP_VIDEO_CAPTURE_MPLANE
> > > isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT |
> > > 	      V4L2_CAP_VIDEO_OUTPUT_MPLANE
> > > isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT
> > > isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> > > 	     V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE
> > >
> > > and you can then also add, if desired,
> > >
> > > isMetaCapture() -> isMeta() && isCapture()
> > > ...
> > >
> >
> > I think the code below here, which is the only place where those
> > functions are used would become less nice if this case.
> >
> > Those functions are only used to assign bufferType_ and fdEvent_,
> > let's keep it straightforward as the below series of if/else
>
> The code below is fine, my point is that isCapture() should not mean
> isVideoCapture().
>
> 	bool isCapture() const
> 	{
> 		return device_caps() & (V4L2_CAP_META_CAPTURE |
> 					V4L2_CAP_VIDEO_CAPTURE |
> 					V4L2_CAP_VIDEO_CAPTURE_MPLANE);
> 	}
>
> 	bool isOutput() const
> 	{
> 		return device_caps() & (V4L2_CAP_META_OUTPUT |
> 					V4L2_CAP_VIDEO_OUTPUT |
> 					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> 	}
>
> 	bool isMeta() const
> 	{
> 		return device_caps() & (V4L2_CAP_META_CAPTURE |
> 					V4L2_CAP_META_OUTPUT);
> 	}
>
> 	bool isVideo() const
> 	{
> 		return device_caps() & (V4L2_CAP_VIDEO_CAPTURE |
> 					V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> 					V4L2_CAP_VIDEO_OUTPUT |
> 					V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> 	}
>
> 	bool isMetaCapture() const
> 	{
> 		return isMeta() && isCapture();
> 	}
>
> 	bool isMetaOutput() const
> 	{
> 		return isMeta() && isOutput();
> 	}
>
> 	bool isVideoCapture() const
> 	{
> 		return isVideo() && isCapture();
> 	}
>
> 	bool isVideoOutput() const
> 	{
> 		return isVideo() && isOutput();
> 	}
>
> and then use those four last functions below.

I see. I don't feel like adding 4 functions and one level of
indirection is super useful considering this is
used in a single place, but if that would help moving forward I'll
change this.

>
> > >>
> > >> 	 * 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 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;
> > >> 	}
> > >>
>
> --
> Regards,
>
> Laurent Pinchart
-------------- 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/20190301/5569daa7/attachment-0001.sig>