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

[Laurent Pinchart]

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 ?

>  
>  	std::vector<unsigned int> planeSizes;
>  	for (size_t i = 0; i < info.planes.size(); ++i)

-- 
Regards,

Laurent Pinchart