[libcamera-devel] [PATCH 2/2] libcamera: pipeline: simple: Initialize V4L2DeviceFormat before use

[Laurent Pinchart]

On Wed, Oct 21, 2020 at 07:30:53PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Wed, Oct 21, 2020 at 03:13:55PM +0200, Jacopo Mondi wrote:
> > On Wed, Oct 21, 2020 at 05:47:44AM +0300, Laurent Pinchart wrote:
> > > The V4L2DeviceFormat has no default constructor. Zero it before use when
> > 
> > Isn't it implicitly generated by the compiler ?
> 
> It is, but the implicitly-defined constructor won't help us here:
> 
> "If the implicitly-declared default constructor is not defined as
> deleted, it is defined (that is, a function body is generated and
> compiled) by the compiler if odr-used, and it has the same effect as a
> user-defined constructor with empty body and empty initializer list."
> 
> (https://en.cppreference.com/w/cpp/language/default_constructor is an
> interesting read)
> 
> > > setting formats. Failure to do so leaves the planes uninitialized,
> > > potentially causing memory corruption.
> > 
> > If not, isn't it better to add it in the class ?
> 
> I had considered that, but wasn't sure, and was hoping nobody would ask
> ;-)
> 
> It could make sense to replace the planes array and planesCount field
> with an std::vector. A bit of a shame to have to use dynamic allocation
> when we know there will never be more than 3 planes though. I wish there
> was a dynamic array container, with fixed memory preallocation and
> dynamic size up to the number of elements.
> 
> Any preference between using a vector, and keeping it as-is with a
> constructor that sets numPlanes to 0 ?

By the way, we also have the option of using a default member
initializer
(https://en.cppreference.com/w/cpp/language/data_members#Member_initialization):

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 40ed87e17cfa..12eaf45a31ec 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -160,7 +160,7 @@ public:
 		uint32_t size;
 		uint32_t bpl;
 	} planes[3];
-	unsigned int planesCount;
+	unsigned int planesCount = 0;
 
 	const std::string toString() const;
 };

Tomi recently pointed this out. I don't like it much when there are
explicit constructors (as initialization is then split between the
header file and the cpp file), but for structures with no explicitly
defined constructor, I don't object.

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/simple/converter.cpp | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > > index b4ee021f6210..b3705abcd626 100644
> > > --- a/src/libcamera/pipeline/simple/converter.cpp
> > > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > > @@ -72,7 +72,7 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> > >  	 * Set the format on the input side (V4L2 output) of the converter to
> > >  	 * enumerate the conversion capabilities on its output (V4L2 capture).
> > >  	 */
> > > -	V4L2DeviceFormat format;
> > > +	V4L2DeviceFormat format = {};
> > >  	format.fourcc = m2m_->output()->toV4L2PixelFormat(input);
> > >  	format.size = { 1, 1 };
> > >
> > > @@ -103,7 +103,7 @@ SizeRange SimpleConverter::sizes(const Size &input)
> > >  	 * Set the size on the input side (V4L2 output) of the converter to
> > >  	 * enumerate the scaling capabilities on its output (V4L2 capture).
> > >  	 */
> > > -	V4L2DeviceFormat format;
> > > +	V4L2DeviceFormat format = {};
> > >  	format.fourcc = V4L2PixelFormat();
> > >  	format.size = input;
> > >
> > > @@ -142,7 +142,7 @@ SizeRange SimpleConverter::sizes(const Size &input)
> > >  int SimpleConverter::configure(PixelFormat inputFormat, const Size &inputSize,
> > >  			       StreamConfiguration *cfg)
> > >  {
> > > -	V4L2DeviceFormat format;
> > > +	V4L2DeviceFormat format = {};
> > >  	int ret;
> > >
> > >  	V4L2PixelFormat videoFormat = m2m_->output()->toV4L2PixelFormat(inputFormat);

-- 
Regards,

Laurent Pinchart