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

[Laurent Pinchart]

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/ ?

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

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

> + */
> +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.

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