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

[Laurent Pinchart]

Hi Barnabás,

On Wed, Dec 11, 2024 at 05:10:23PM +0000, Barnabás Pőcze wrote:
> 2024. december 11., szerda 17:40 keltezéssel, Laurent Pinchart írta:
> > 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.)

OK, let's ignore this for the time being then.

-- 
Regards,

Laurent Pinchart