[libcamera-devel] [PATCH 3/6] libcamera: v4l2_device: Identify single/multiplane

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 21, 2019 at 10:31:59PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jan 21, 2019 at 06:27:02PM +0100, Jacopo Mondi wrote:
> > Add functions to V4L2Capability to identify if a V4L2 device supports
> > single or multiplane APIs.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_device.h |  6 +++--
> >  src/libcamera/v4l2_device.cpp       | 37 +++++++++++++++++++++++++----
> >  2 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 2787847..0101a4b 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability {
> >  		return reinterpret_cast<const char *>(v4l2_capability::bus_info);
> >  	}
> >
> > -	bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; }
> > -	bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; }
> > +	bool isSinglePlane() const;
> > +	bool isMultiPlane() const;
>
> Maybe s/Plane/Planar/ ?
>

Ok

> > +	bool isCapture() const;
> > +	bool isOutput() const;
>
> Is there a reason to stop inlining the functions ? They should just
> generate a load and an and operation, which should be less costly than a
> function call.
>

I moved them in the cpp unit, then realized I could have moved them back
but I thought it was preferred to have them in the cpp file, as
everytime I suggested inlining something it got pushed back.

I'll move them back.


> >  	bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; }
> >  };
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index aef0996..7cd89d7 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -47,17 +47,46 @@ namespace libcamera {
> >   * \return The string containing the device location
> >   */
> >
> > +
> > +/**
> > + * \brief Identify if the device implements V4L2 single plane APIs
>
> s/implements/implements the/
> s/plane/planar/ ?
>
> and for the following line too.
>
> > + * \return true if the device supports single plane APIs
>
> Don't we start all the return descriptions with a capital letter ?
>
> Those comments hold for the other functions below.

I had looked around and the boolean 'true' is usually spelled with a
lower case 't'.

>
> > + */
> > +bool V4L2Capability::isSinglePlane() const
> > +{
> > +	return (capabilities & V4L2_CAP_VIDEO_CAPTURE) ||
> > +	       (capabilities & V4L2_CAP_VIDEO_OUTPUT);
>
> You could write this
>
> 	return capabilities & (V4L2_CAP_VIDEO_CAPTURE |
> 			       V4L2_CAP_VIDEO_OUTPUT);
>
> I would expect the compiler to perform that optimization itself, but it
> also doesn't hurt to be explicit.
>

ok

Thanks
  j

> > +}
> > +
> > +/**
> > + * \brief Identify if the device implements V4L2 multiplanar APIs
> > + * \return true if the device supports multiplanar APIs
> > + */
> > +bool V4L2Capability::isMultiPlane() const
> > +{
> > +	return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) ||
> > +	       (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> > +}
> > +
> >  /**
> > - * \fn bool V4L2Capability::isCapture()
> >   * \brief Identify if the device is capable of capturing video
> > - * \return True if the device can capture video frames
> > + * \return true if the device can capture video frames
> >   */
> > +bool V4L2Capability::isCapture() const
> > +{
> > +	return (capabilities & V4L2_CAP_VIDEO_CAPTURE) ||
> > +	       (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE);
> > +}
> >
> >  /**
> > - * \fn bool V4L2Capability::isOutput()
> >   * \brief Identify if the device is capable of outputting video
> > - * \return True if the device can output video frames
> > + * \return true if the device can capture video frames
> >   */
> > +bool V4L2Capability::isOutput() const
> > +{
> > +	return (capabilities & V4L2_CAP_VIDEO_OUTPUT) ||
> > +	       (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> > +}
> >
> >  /**
> >   * \fn bool V4L2Capability::hasStreaming()
>
> --
> 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/20190122/77d73fa1/attachment.sig>