[PATCH v1 1/3] libcamera: virtual: Avoid some copies

[Barnabás Pőcze]

Hi


2024. december 11., szerda 17:40 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Wed, Dec 11, 2024 at 03:25:45PM +0000, Barnabás Pőcze wrote:
> > There is no reason make copies, these functions return
> > const lvalue references, access the data through those.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> with one comment below.
> 
> > ---
> >  src/libcamera/pipeline/virtual/image_frame_generator.cpp  | 2 +-
> >  src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +-
> >  src/libcamera/pipeline/virtual/virtual.cpp                | 3 +--
> >  3 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > index 277efbb09..d1545b5d9 100644
> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
> >
> >  	MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);
> >
> > -	auto planes = mappedFrameBuffer.planes();
> > +	const auto &planes = mappedFrameBuffer.planes();
> >
> >  	/* Loop only around the number of images available */
> >  	frameIndex_ %= imageFrameDatas_.size();
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > index 7bc2b338c..47d341919 100644
> > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size,
> >  	MappedFrameBuffer mappedFrameBuffer(buffer,
> >  					    MappedFrameBuffer::MapFlag::Write);
> >
> > -	auto planes = mappedFrameBuffer.planes();
> > +	const auto &planes = mappedFrameBuffer.planes();
> >
> >  	shiftLeft(size);
> >
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> > index 1b7cd5cb3..3126bdd7d 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
> >  		return -ENOBUFS;
> >
> >  	const StreamConfiguration &config = stream->configuration();
> > -
> > -	auto info = PixelFormatInfo::info(config.pixelFormat);
> > +	const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat);
> 
> There's also a similar issue in src/libcamera/pipeline/imx8-isi/imx8-isi.cpp:
> 
> 	const PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat);
> 
> It would be nice to disable the PixelFormatInfo copy constructor (using
> LIBCAMERA_DISABLE_COPY() or even LIBCAMERA_DISABLE_COPY_AND_MOVE()), but
> that interferes with the designated initializer syntax we use in
> formats.cpp to populate the pixelFormatInfo array as aggregate
> initialization is disabled for types that have user-declared
> constructors (even if deleted). One option would be to define a
> constructor for the class, but we then lose the ability to use
> designated initializers. Maybe that's not a big issue ?
> 
> Could you have a look at this ?
> [...]

That map is initialized from an `std::initializer_list` and that provides a `const`
view of the data, so the map copy constructs the elements from the list. Deleting
the copy constructor would make that not work. I think using an array and `std::move_iterator`
could maybe work around that.

Returning a pointer from `PixelFormatInfo::info()` would also eliminate this kind of issue.

Also, there is the "performance-unnecessary-copy-initialization" clang-tidy check,
but it seems it does not want to diagnose this situation. (Seemingly because
the reference is returned by a static member function.)


Regards,
Barnabás Pőcze