[libcamera-devel] [PATCH v2 1/3] libcamera: v4l2_videodevice: Zero-initialize planes in V4L2DeviceFormat

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-11-04 09:48:39 +0200, Laurent Pinchart wrote:
> The V4L2DeviceFormat class doesn't have a default constructor, neither
> does it specifies default member initializers for the plane-related
> members. This results in the planes array and planesCount members being
> uninitialized by default, leading to undefined behaviour if the user of
> the class doesn't initialize it explicitly.
> 
> Most users initialize V4L2DeviceFormat instances, but some don't. We
> could fix them, but that would likely turn into a game of whack-a-mole.
> As there's no use case for instantiating a large number of
> V4L2DeviceFormat instances in a performance-critical code path, let's
> instead add default initializers to avoid future issues.
> 
> While at it, define a type of the structures containing plane
> information, and use an std::array.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  include/libcamera/internal/v4l2_videodevice.h | 13 ++++++++-----
>  src/libcamera/v4l2_videodevice.cpp            |  9 +++++++++
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 53f6a2d5515b..dcb9654a34b8 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_INTERNAL_V4L2_VIDEODEVICE_H__
>  #define __LIBCAMERA_INTERNAL_V4L2_VIDEODEVICE_H__
>  
> +#include <array>
>  #include <atomic>
>  #include <memory>
>  #include <stdint.h>
> @@ -153,14 +154,16 @@ private:
>  class V4L2DeviceFormat
>  {
>  public:
> +	struct Plane {
> +		uint32_t size = 0;
> +		uint32_t bpl = 0;
> +	};
> +
>  	V4L2PixelFormat fourcc;
>  	Size size;
>  
> -	struct {
> -		uint32_t size;
> -		uint32_t bpl;
> -	} planes[3];
> -	unsigned int planesCount;
> +	std::array<Plane, 3> planes;
> +	unsigned int planesCount = 0;
>  
>  	const std::string toString() const;
>  };
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 36d7d9a0f27a..d07c3530eea5 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -351,6 +351,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>   * V4L2DeviceFormat::planesCount value.
>   */
>  
> +/**
> + * \struct V4L2DeviceFormat::Plane
> + * \brief Per-plane memory size information
> + * \var V4L2DeviceFormat::Plane::size
> + * \brief The plane total memory size (in bytes)
> + * \var V4L2DeviceFormat::Plane::bpl
> + * \brief The plane line stride (in bytes)
> + */
> +
>  /**
>   * \var V4L2DeviceFormat::size
>   * \brief The image size in pixels
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund