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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jan 22, 2019 at 02:44:16PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 21, 2019 at 10:31:59PM +0200, Laurent Pinchart wrote:
> > 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.

Inlines should be used with caution as they can generate a fair amount
of code in C++, for instance when they copy full objects behind the
scene, or for virtual destructors. In this specific case I think they're
fine.

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

6 cases of \return true, and 6 cases of \return True :-) I'd go for the
later to capitalize all \return expressions.

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