[v2] libcamera: software_isp: Replace malloc() with std::vector<>
diff mbox series

Message ID 20240804133605.8234-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 57396a0e3f08a5cd1ea6f48c66f3decdfeaa7dfd
Headers show
Series
  • [v2] libcamera: software_isp: Replace malloc() with std::vector<>
Related show

Commit Message

Laurent Pinchart Aug. 4, 2024, 1:36 p.m. UTC 
libcamera is implemented in C++, use std::vector<> to manage the
dynamically allocated line buffers instead of malloc() and free(). This
simplifies the code and improves memory safety by ensuring no allocation
will be leaked.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Use std::vector instead of new[]() and delete[]()
---
 src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
 src/libcamera/software_isp/debayer_cpu.h   |  2 +-
 2 files changed, 13 insertions(+), 20 deletions(-)


base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Comments

Kieran Bingham Aug. 5, 2024, 8:51 a.m. UTC | #1 
Quoting Laurent Pinchart (2024-08-04 14:36:05)
> libcamera is implemented in C++, use std::vector<> to manage the
> dynamically allocated line buffers instead of malloc() and free(). This
> simplifies the code and improves memory safety by ensuring no allocation
> will be leaked.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Use std::vector instead of new[]() and delete[]()
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index f8d2677d657a..077f7f4bc45b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>         /* Initialize color lookup tables */
>         for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>                 red_[i] = green_[i] = blue_[i] = i;
> -
> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -               lineBuffers_[i] = nullptr;
>  }
>  
> -DebayerCpu::~DebayerCpu()
> -{
> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -               free(lineBuffers_[i]);
> -}
> +DebayerCpu::~DebayerCpu() = default;
>  
>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>         const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>         lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>         lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>                             2 * lineBufferPadding_;
> -       for (unsigned int i = 0;
> -            i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
> -            i++) {
> -               free(lineBuffers_[i]);
> -               lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
> -               if (!lineBuffers_[i])
> -                       return -ENOMEM;
> +
> +       if (enableInputMemcpy_) {
> +               for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
> +                       lineBuffers_[i].resize(lineBufferLength_);

that's easier.

I don't think the previous implementation was doing it, but should any
following lines be resized to 0 if the configuration shrinks?

Worst case, this could grow to full allocation, then the later lines
would remain allocated indefinitely on any system with a long running
camera manager (i.e. pipewire). The memory wouldn't be lost, as it would
be released when the camera manager is destroyed, but the memory
wouldn't be usable...

Could be a patch on top as the previous implementation didn't do this
and it's just a conversion patch here.

So I think this patch still steps in a good way forwards:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>         }
>  
>         measuredFrames_ = 0;
> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>                 return;
>  
>         for (unsigned int i = 0; i < patternHeight; i++) {
> -               memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
> +               memcpy(lineBuffers_[i].data(),
> +                      linePointers[i + 1] - lineBufferPadding_,
>                        lineBufferLength_);
> -               linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
> +               linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>         }
>  
>         /* Point lineBufferIndex_ to first unused lineBuffer */
> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>         if (!enableInputMemcpy_)
>                 return;
>  
> -       memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
> +       memcpy(lineBuffers_[lineBufferIndex_].data(),
> +              linePointers[patternHeight] - lineBufferPadding_,
>                lineBufferLength_);
> -       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
> +       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
> +                                   + lineBufferPadding_;
>  
>         lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>  }
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 1dac64351f1d..8237a64be733 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -146,7 +146,7 @@ private:
>         DebayerInputConfig inputConfig_;
>         DebayerOutputConfig outputConfig_;
>         std::unique_ptr<SwStatsCpu> stats_;
> -       uint8_t *lineBuffers_[kMaxLineBuffers];
> +       std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>         unsigned int lineBufferLength_;
>         unsigned int lineBufferPadding_;
>         unsigned int lineBufferIndex_;
> 
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
> -- 
> Regards,
> 
> Laurent Pinchart
>
Hans de Goede Aug. 5, 2024, 10:22 a.m. UTC | #2 
Hi,

On 8/4/24 3:36 PM, Laurent Pinchart wrote:
> libcamera is implemented in C++, use std::vector<> to manage the
> dynamically allocated line buffers instead of malloc() and free(). This
> simplifies the code and improves memory safety by ensuring no allocation
> will be leaked.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I have given this a spin with IPU6 + the softISP and everything still works:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

p.s.

I do see an *unrelated* performance regression where the softISP processing
time for 1920x1080 goes up from 3 ms / frame to 6 ms / frame. 1 ms of that
is caused by the new 32bpp RGB sparse output support which qcam prefers
over the previous 24bpp packed support forcing 24 bpp packed output fmt
resolves the 1 ms difference which is expected since 32 bpp sparse writes
33% more data. But that still leaves 2 ms unaccounted for.

I'll try to dig into where those 2 ms are coming from. With moderen CPU
turboing behavior it is hard to tell really. Might even be a kernel issue...






> ---
> Changes since v1:
> 
> - Use std::vector instead of new[]() and delete[]()
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index f8d2677d657a..077f7f4bc45b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>  	/* Initialize color lookup tables */
>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>  		red_[i] = green_[i] = blue_[i] = i;
> -
> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -		lineBuffers_[i] = nullptr;
>  }
>  
> -DebayerCpu::~DebayerCpu()
> -{
> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -		free(lineBuffers_[i]);
> -}
> +DebayerCpu::~DebayerCpu() = default;
>  
>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>  	const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>  	lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>  	lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>  			    2 * lineBufferPadding_;
> -	for (unsigned int i = 0;
> -	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
> -	     i++) {
> -		free(lineBuffers_[i]);
> -		lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
> -		if (!lineBuffers_[i])
> -			return -ENOMEM;
> +
> +	if (enableInputMemcpy_) {
> +		for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
> +			lineBuffers_[i].resize(lineBufferLength_);
>  	}
>  
>  	measuredFrames_ = 0;
> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>  		return;
>  
>  	for (unsigned int i = 0; i < patternHeight; i++) {
> -		memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
> +		memcpy(lineBuffers_[i].data(),
> +		       linePointers[i + 1] - lineBufferPadding_,
>  		       lineBufferLength_);
> -		linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
> +		linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>  	}
>  
>  	/* Point lineBufferIndex_ to first unused lineBuffer */
> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>  	if (!enableInputMemcpy_)
>  		return;
>  
> -	memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
> +	memcpy(lineBuffers_[lineBufferIndex_].data(),
> +	       linePointers[patternHeight] - lineBufferPadding_,
>  	       lineBufferLength_);
> -	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
> +	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
> +				    + lineBufferPadding_;
>  
>  	lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>  }
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 1dac64351f1d..8237a64be733 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -146,7 +146,7 @@ private:
>  	DebayerInputConfig inputConfig_;
>  	DebayerOutputConfig outputConfig_;
>  	std::unique_ptr<SwStatsCpu> stats_;
> -	uint8_t *lineBuffers_[kMaxLineBuffers];
> +	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>  	unsigned int lineBufferLength_;
>  	unsigned int lineBufferPadding_;
>  	unsigned int lineBufferIndex_;
> 
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
Laurent Pinchart Aug. 5, 2024, 10:32 a.m. UTC | #3 
Hi Hans,

On Mon, Aug 05, 2024 at 12:22:02PM +0200, Hans de Goede wrote:
> On 8/4/24 3:36 PM, Laurent Pinchart wrote:
> > libcamera is implemented in C++, use std::vector<> to manage the
> > dynamically allocated line buffers instead of malloc() and free(). This
> > simplifies the code and improves memory safety by ensuring no allocation
> > will be leaked.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I have given this a spin with IPU6 + the softISP and everything still works:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Thank you.

> p.s.
> 
> I do see an *unrelated* performance regression where the softISP processing
> time for 1920x1080 goes up from 3 ms / frame to 6 ms / frame. 1 ms of that
> is caused by the new 32bpp RGB sparse output support which qcam prefers
> over the previous 24bpp packed support forcing 24 bpp packed output fmt
> resolves the 1 ms difference which is expected since 32 bpp sparse writes
> 33% more data. But that still leaves 2 ms unaccounted for.
> 
> I'll try to dig into where those 2 ms are coming from. With moderen CPU
> turboing behavior it is hard to tell really. Might even be a kernel issue...

Is that a regression introduced by this patch, or a regression between a
particular point in the past and the last master branch ?

> > ---
> > Changes since v1:
> > 
> > - Use std::vector instead of new[]() and delete[]()
> > ---
> >  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
> >  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
> >  2 files changed, 13 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> > index f8d2677d657a..077f7f4bc45b 100644
> > --- a/src/libcamera/software_isp/debayer_cpu.cpp
> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> > @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> >  	/* Initialize color lookup tables */
> >  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
> >  		red_[i] = green_[i] = blue_[i] = i;
> > -
> > -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> > -		lineBuffers_[i] = nullptr;
> >  }
> >  
> > -DebayerCpu::~DebayerCpu()
> > -{
> > -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> > -		free(lineBuffers_[i]);
> > -}
> > +DebayerCpu::~DebayerCpu() = default;
> >  
> >  #define DECLARE_SRC_POINTERS(pixel_t)                            \
> >  	const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
> > @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
> >  	lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
> >  	lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
> >  			    2 * lineBufferPadding_;
> > -	for (unsigned int i = 0;
> > -	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
> > -	     i++) {
> > -		free(lineBuffers_[i]);
> > -		lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
> > -		if (!lineBuffers_[i])
> > -			return -ENOMEM;
> > +
> > +	if (enableInputMemcpy_) {
> > +		for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
> > +			lineBuffers_[i].resize(lineBufferLength_);
> >  	}
> >  
> >  	measuredFrames_ = 0;
> > @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
> >  		return;
> >  
> >  	for (unsigned int i = 0; i < patternHeight; i++) {
> > -		memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
> > +		memcpy(lineBuffers_[i].data(),
> > +		       linePointers[i + 1] - lineBufferPadding_,
> >  		       lineBufferLength_);
> > -		linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
> > +		linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
> >  	}
> >  
> >  	/* Point lineBufferIndex_ to first unused lineBuffer */
> > @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
> >  	if (!enableInputMemcpy_)
> >  		return;
> >  
> > -	memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
> > +	memcpy(lineBuffers_[lineBufferIndex_].data(),
> > +	       linePointers[patternHeight] - lineBufferPadding_,
> >  	       lineBufferLength_);
> > -	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
> > +	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
> > +				    + lineBufferPadding_;
> >  
> >  	lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
> >  }
> > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> > index 1dac64351f1d..8237a64be733 100644
> > --- a/src/libcamera/software_isp/debayer_cpu.h
> > +++ b/src/libcamera/software_isp/debayer_cpu.h
> > @@ -146,7 +146,7 @@ private:
> >  	DebayerInputConfig inputConfig_;
> >  	DebayerOutputConfig outputConfig_;
> >  	std::unique_ptr<SwStatsCpu> stats_;
> > -	uint8_t *lineBuffers_[kMaxLineBuffers];
> > +	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
> >  	unsigned int lineBufferLength_;
> >  	unsigned int lineBufferPadding_;
> >  	unsigned int lineBufferIndex_;
> > 
> > base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
Milan Zamazal Aug. 5, 2024, 10:35 a.m. UTC | #4 
Hi Laurent,

thank you for the patch.

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> libcamera is implemented in C++, use std::vector<> to manage the
> dynamically allocated line buffers instead of malloc() and free(). This
> simplifies the code and improves memory safety by ensuring no allocation
> will be leaked.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

(I thought that malloc was used here to be able to detect allocation
failures when exceptions are not used but it's probably not an issue
with total size of line buffers not being much more than ~100 kB
usually).

> ---
> Changes since v1:
>
> - Use std::vector instead of new[]() and delete[]()
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>  2 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index f8d2677d657a..077f7f4bc45b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>  	/* Initialize color lookup tables */
>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>  		red_[i] = green_[i] = blue_[i] = i;
> -
> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -		lineBuffers_[i] = nullptr;
>  }
>  
> -DebayerCpu::~DebayerCpu()
> -{
> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> -		free(lineBuffers_[i]);
> -}
> +DebayerCpu::~DebayerCpu() = default;
>  
>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>  	const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>  	lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>  	lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>  			    2 * lineBufferPadding_;
> -	for (unsigned int i = 0;
> -	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
> -	     i++) {
> -		free(lineBuffers_[i]);
> -		lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
> -		if (!lineBuffers_[i])
> -			return -ENOMEM;
> +
> +	if (enableInputMemcpy_) {
> +		for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
> +			lineBuffers_[i].resize(lineBufferLength_);
>  	}
>  
>  	measuredFrames_ = 0;
> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>  		return;
>  
>  	for (unsigned int i = 0; i < patternHeight; i++) {
> -		memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
> +		memcpy(lineBuffers_[i].data(),
> +		       linePointers[i + 1] - lineBufferPadding_,
>  		       lineBufferLength_);
> -		linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
> +		linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>  	}
>  
>  	/* Point lineBufferIndex_ to first unused lineBuffer */
> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>  	if (!enableInputMemcpy_)
>  		return;
>  
> -	memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
> +	memcpy(lineBuffers_[lineBufferIndex_].data(),
> +	       linePointers[patternHeight] - lineBufferPadding_,
>  	       lineBufferLength_);
> -	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
> +	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
> +				    + lineBufferPadding_;
>  
>  	lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>  }
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 1dac64351f1d..8237a64be733 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -146,7 +146,7 @@ private:
>  	DebayerInputConfig inputConfig_;
>  	DebayerOutputConfig outputConfig_;
>  	std::unique_ptr<SwStatsCpu> stats_;
> -	uint8_t *lineBuffers_[kMaxLineBuffers];
> +	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>  	unsigned int lineBufferLength_;
>  	unsigned int lineBufferPadding_;
>  	unsigned int lineBufferIndex_;
>
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
Milan Zamazal Aug. 5, 2024, 10:37 a.m. UTC | #5 
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Laurent Pinchart (2024-08-04 14:36:05)
>> libcamera is implemented in C++, use std::vector<> to manage the
>> dynamically allocated line buffers instead of malloc() and free(). This
>
>> simplifies the code and improves memory safety by ensuring no allocation
>> will be leaked.
>> 
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>> Changes since v1:
>> 
>> - Use std::vector instead of new[]() and delete[]()
>> ---
>>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>>  2 files changed, 13 insertions(+), 20 deletions(-)
>> 
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index f8d2677d657a..077f7f4bc45b 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>>         /* Initialize color lookup tables */
>>         for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>>                 red_[i] = green_[i] = blue_[i] = i;
>> -
>> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>> -               lineBuffers_[i] = nullptr;
>>  }
>>  
>> -DebayerCpu::~DebayerCpu()
>> -{
>> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>> -               free(lineBuffers_[i]);
>> -}
>> +DebayerCpu::~DebayerCpu() = default;
>>  
>>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>>         const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
>> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>         lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>>         lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>>                             2 * lineBufferPadding_;
>> -       for (unsigned int i = 0;
>> -            i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
>> -            i++) {
>> -               free(lineBuffers_[i]);
>> -               lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
>> -               if (!lineBuffers_[i])
>> -                       return -ENOMEM;
>> +
>> +       if (enableInputMemcpy_) {
>> +               for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
>> +                       lineBuffers_[i].resize(lineBufferLength_);
>
> that's easier.
>
> I don't think the previous implementation was doing it, but should any
> following lines be resized to 0 if the configuration shrinks?
>
> Worst case, this could grow to full allocation, then the later lines
> would remain allocated indefinitely on any system with a long running
> camera manager (i.e. pipewire). The memory wouldn't be lost, as it would
> be released when the camera manager is destroyed, but the memory
> wouldn't be usable...

Can the Bayer pattern actually change when changing the configuration?

> Could be a patch on top as the previous implementation didn't do this
> and it's just a conversion patch here.
>
> So I think this patch still steps in a good way forwards:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
>>         }
>>  
>>         measuredFrames_ = 0;
>> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>>                 return;
>>  
>>         for (unsigned int i = 0; i < patternHeight; i++) {
>> -               memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
>> +               memcpy(lineBuffers_[i].data(),
>> +                      linePointers[i + 1] - lineBufferPadding_,
>>                        lineBufferLength_);
>> -               linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
>> +               linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>>         }
>>  
>>         /* Point lineBufferIndex_ to first unused lineBuffer */
>> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>>         if (!enableInputMemcpy_)
>>                 return;
>>  
>> -       memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
>> +       memcpy(lineBuffers_[lineBufferIndex_].data(),
>> +              linePointers[patternHeight] - lineBufferPadding_,
>>                lineBufferLength_);
>> -       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
>> +       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
>> +                                   + lineBufferPadding_;
>>  
>>         lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>>  }
>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> index 1dac64351f1d..8237a64be733 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -146,7 +146,7 @@ private:
>>         DebayerInputConfig inputConfig_;
>>         DebayerOutputConfig outputConfig_;
>>         std::unique_ptr<SwStatsCpu> stats_;
>> -       uint8_t *lineBuffers_[kMaxLineBuffers];
>> +       std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>>         unsigned int lineBufferLength_;
>>         unsigned int lineBufferPadding_;
>>         unsigned int lineBufferIndex_;
>> 
>> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
>> -- 
>> Regards,
>> 
>> Laurent Pinchart
>>
Hans de Goede Aug. 5, 2024, 10:56 a.m. UTC | #6 
Hi,

On 8/5/24 12:32 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Aug 05, 2024 at 12:22:02PM +0200, Hans de Goede wrote:
>> On 8/4/24 3:36 PM, Laurent Pinchart wrote:
>>> libcamera is implemented in C++, use std::vector<> to manage the
>>> dynamically allocated line buffers instead of malloc() and free(). This
>>> simplifies the code and improves memory safety by ensuring no allocation
>>> will be leaked.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> I have given this a spin with IPU6 + the softISP and everything still works:
>>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thank you.
> 
>> p.s.
>>
>> I do see an *unrelated* performance regression where the softISP processing
>> time for 1920x1080 goes up from 3 ms / frame to 6 ms / frame. 1 ms of that
>> is caused by the new 32bpp RGB sparse output support which qcam prefers
>> over the previous 24bpp packed support forcing 24 bpp packed output fmt
>> resolves the 1 ms difference which is expected since 32 bpp sparse writes
>> 33% more data. But that still leaves 2 ms unaccounted for.
>>
>> I'll try to dig into where those 2 ms are coming from. With moderen CPU
>> turboing behavior it is hard to tell really. Might even be a kernel issue...
> 
> Is that a regression introduced by this patch, or a regression between a
> particular point in the past and the last master branch ?

The latter: "a regression between a particular point in the past and
the last master branch". I was hoping that the "*unrelated*" above made
that clear.

It could even be something outside of libcamera, like e.g. the kernel
scheduling the softISP thread on the E-cores instead of on the P-cores...

IOW this is homework for me to figure out where this comes from,
this does show that the builtin mini benchmark really is quite useful.

Regards,

Hans



 
>>> ---
>>> Changes since v1:
>>>
>>> - Use std::vector instead of new[]() and delete[]()
>>> ---
>>>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>>>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>>>  2 files changed, 13 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>> index f8d2677d657a..077f7f4bc45b 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>>>  	/* Initialize color lookup tables */
>>>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>>>  		red_[i] = green_[i] = blue_[i] = i;
>>> -
>>> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>>> -		lineBuffers_[i] = nullptr;
>>>  }
>>>  
>>> -DebayerCpu::~DebayerCpu()
>>> -{
>>> -	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>>> -		free(lineBuffers_[i]);
>>> -}
>>> +DebayerCpu::~DebayerCpu() = default;
>>>  
>>>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>>>  	const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
>>> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>>  	lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>>>  	lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>>>  			    2 * lineBufferPadding_;
>>> -	for (unsigned int i = 0;
>>> -	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
>>> -	     i++) {
>>> -		free(lineBuffers_[i]);
>>> -		lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
>>> -		if (!lineBuffers_[i])
>>> -			return -ENOMEM;
>>> +
>>> +	if (enableInputMemcpy_) {
>>> +		for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
>>> +			lineBuffers_[i].resize(lineBufferLength_);
>>>  	}
>>>  
>>>  	measuredFrames_ = 0;
>>> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>>>  		return;
>>>  
>>>  	for (unsigned int i = 0; i < patternHeight; i++) {
>>> -		memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
>>> +		memcpy(lineBuffers_[i].data(),
>>> +		       linePointers[i + 1] - lineBufferPadding_,
>>>  		       lineBufferLength_);
>>> -		linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
>>> +		linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>>>  	}
>>>  
>>>  	/* Point lineBufferIndex_ to first unused lineBuffer */
>>> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>>>  	if (!enableInputMemcpy_)
>>>  		return;
>>>  
>>> -	memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
>>> +	memcpy(lineBuffers_[lineBufferIndex_].data(),
>>> +	       linePointers[patternHeight] - lineBufferPadding_,
>>>  	       lineBufferLength_);
>>> -	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
>>> +	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
>>> +				    + lineBufferPadding_;
>>>  
>>>  	lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>>>  }
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>>> index 1dac64351f1d..8237a64be733 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.h
>>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>>> @@ -146,7 +146,7 @@ private:
>>>  	DebayerInputConfig inputConfig_;
>>>  	DebayerOutputConfig outputConfig_;
>>>  	std::unique_ptr<SwStatsCpu> stats_;
>>> -	uint8_t *lineBuffers_[kMaxLineBuffers];
>>> +	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>>>  	unsigned int lineBufferLength_;
>>>  	unsigned int lineBufferPadding_;
>>>  	unsigned int lineBufferIndex_;
>>>
>>> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
>
Laurent Pinchart Aug. 5, 2024, 4:25 p.m. UTC | #7 
On Mon, Aug 05, 2024 at 12:37:50PM +0200, Milan Zamazal wrote:
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> 
> > Quoting Laurent Pinchart (2024-08-04 14:36:05)
> >> libcamera is implemented in C++, use std::vector<> to manage the
> >> dynamically allocated line buffers instead of malloc() and free(). This
> >
> >> simplifies the code and improves memory safety by ensuring no allocation
> >> will be leaked.
> >> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> Changes since v1:
> >> 
> >> - Use std::vector instead of new[]() and delete[]()
> >> ---
> >>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
> >>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
> >>  2 files changed, 13 insertions(+), 20 deletions(-)
> >> 
> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> >> index f8d2677d657a..077f7f4bc45b 100644
> >> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> >> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> >>         /* Initialize color lookup tables */
> >>         for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
> >>                 red_[i] = green_[i] = blue_[i] = i;
> >> -
> >> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> >> -               lineBuffers_[i] = nullptr;
> >>  }
> >>  
> >> -DebayerCpu::~DebayerCpu()
> >> -{
> >> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> >> -               free(lineBuffers_[i]);
> >> -}
> >> +DebayerCpu::~DebayerCpu() = default;
> >>  
> >>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
> >>         const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
> >> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
> >>         lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
> >>         lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
> >>                             2 * lineBufferPadding_;
> >> -       for (unsigned int i = 0;
> >> -            i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
> >> -            i++) {
> >> -               free(lineBuffers_[i]);
> >> -               lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
> >> -               if (!lineBuffers_[i])
> >> -                       return -ENOMEM;
> >> +
> >> +       if (enableInputMemcpy_) {
> >> +               for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
> >> +                       lineBuffers_[i].resize(lineBufferLength_);
> >
> > that's easier.
> >
> > I don't think the previous implementation was doing it, but should any
> > following lines be resized to 0 if the configuration shrinks?
> >
> > Worst case, this could grow to full allocation, then the later lines
> > would remain allocated indefinitely on any system with a long running
> > camera manager (i.e. pipewire). The memory wouldn't be lost, as it would
> > be released when the camera manager is destroyed, but the memory
> > wouldn't be usable...
> 
> Can the Bayer pattern actually change when changing the configuration?

If you apply horizontal or vertical flipping, the pattern can change
unless you adjust the crop rectangle by one pixel to keep the pattern
identical. Some sensors do so automatically at the hardware (or
firmware) level. We haven't really standardized on how it should be
handled in libcamera, but we should.

> > Could be a patch on top as the previous implementation didn't do this
> > and it's just a conversion patch here.

Probably a good idea of a patch on top.

> > So I think this patch still steps in a good way forwards:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >>         }
> >>  
> >>         measuredFrames_ = 0;
> >> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
> >>                 return;
> >>  
> >>         for (unsigned int i = 0; i < patternHeight; i++) {
> >> -               memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
> >> +               memcpy(lineBuffers_[i].data(),
> >> +                      linePointers[i + 1] - lineBufferPadding_,
> >>                        lineBufferLength_);
> >> -               linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
> >> +               linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
> >>         }
> >>  
> >>         /* Point lineBufferIndex_ to first unused lineBuffer */
> >> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
> >>         if (!enableInputMemcpy_)
> >>                 return;
> >>  
> >> -       memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
> >> +       memcpy(lineBuffers_[lineBufferIndex_].data(),
> >> +              linePointers[patternHeight] - lineBufferPadding_,
> >>                lineBufferLength_);
> >> -       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
> >> +       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
> >> +                                   + lineBufferPadding_;
> >>  
> >>         lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
> >>  }
> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> >> index 1dac64351f1d..8237a64be733 100644
> >> --- a/src/libcamera/software_isp/debayer_cpu.h
> >> +++ b/src/libcamera/software_isp/debayer_cpu.h
> >> @@ -146,7 +146,7 @@ private:
> >>         DebayerInputConfig inputConfig_;
> >>         DebayerOutputConfig outputConfig_;
> >>         std::unique_ptr<SwStatsCpu> stats_;
> >> -       uint8_t *lineBuffers_[kMaxLineBuffers];
> >> +       std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
> >>         unsigned int lineBufferLength_;
> >>         unsigned int lineBufferPadding_;
> >>         unsigned int lineBufferIndex_;
> >> 
> >> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
Milan Zamazal Aug. 5, 2024, 8:54 p.m. UTC | #8 
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Mon, Aug 05, 2024 at 12:37:50PM +0200, Milan Zamazal wrote:
>> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
>> 
>
>> > Quoting Laurent Pinchart (2024-08-04 14:36:05)
>> >> libcamera is implemented in C++, use std::vector<> to manage the
>> >> dynamically allocated line buffers instead of malloc() and free(). This
>> >
>> >> simplifies the code and improves memory safety by ensuring no allocation
>> >> will be leaked.
>> >> 
>> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >> ---
>> >> Changes since v1:
>> >> 
>> >> - Use std::vector instead of new[]() and delete[]()
>> >> ---
>> >>  src/libcamera/software_isp/debayer_cpu.cpp | 31 +++++++++-------------
>> >>  src/libcamera/software_isp/debayer_cpu.h   |  2 +-
>> >>  2 files changed, 13 insertions(+), 20 deletions(-)
>> >> 
>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> >> index f8d2677d657a..077f7f4bc45b 100644
>> >> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> >> @@ -49,16 +49,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>> >>         /* Initialize color lookup tables */
>> >>         for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>> >>                 red_[i] = green_[i] = blue_[i] = i;
>> >> -
>> >> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>> >> -               lineBuffers_[i] = nullptr;
>> >>  }
>> >>  
>> >> -DebayerCpu::~DebayerCpu()
>> >> -{
>> >> -       for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>> >> -               free(lineBuffers_[i]);
>> >> -}
>> >> +DebayerCpu::~DebayerCpu() = default;
>> >>  
>> >>  #define DECLARE_SRC_POINTERS(pixel_t)                            \
>> >>         const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
>> >> @@ -526,13 +519,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>> >>         lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
>> >>         lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
>> >>                             2 * lineBufferPadding_;
>> >> -       for (unsigned int i = 0;
>> >> -            i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
>> >> -            i++) {
>> >> -               free(lineBuffers_[i]);
>> >> -               lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
>> >> -               if (!lineBuffers_[i])
>> >> -                       return -ENOMEM;
>> >> +
>> >> +       if (enableInputMemcpy_) {
>> >> +               for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
>> >> +                       lineBuffers_[i].resize(lineBufferLength_);
>> >
>> > that's easier.
>> >
>> > I don't think the previous implementation was doing it, but should any
>> > following lines be resized to 0 if the configuration shrinks?
>> >
>> > Worst case, this could grow to full allocation, then the later lines
>> > would remain allocated indefinitely on any system with a long running
>> > camera manager (i.e. pipewire). The memory wouldn't be lost, as it would
>> > be released when the camera manager is destroyed, but the memory
>> > wouldn't be usable...
>> 
>> Can the Bayer pattern actually change when changing the configuration?
>
> If you apply horizontal or vertical flipping, the pattern can change
> unless you adjust the crop rectangle by one pixel to keep the pattern
> identical. Some sensors do so automatically at the hardware (or
> firmware) level. We haven't really standardized on how it should be
> handled in libcamera, but we should.

I see, OK.  But I suppose the pattern size remains the same in those
cases so we needn't worry much about shrinking the allocated line
buffers.

>> > Could be a patch on top as the previous implementation didn't do this
>> > and it's just a conversion patch here.
>
> Probably a good idea of a patch on top.
>
>> > So I think this patch still steps in a good way forwards:
>> >
>> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >
>> >>         }
>> >>  
>> >>         measuredFrames_ = 0;
>> >> @@ -587,9 +577,10 @@ void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
>> >>                 return;
>> >>  
>> >>         for (unsigned int i = 0; i < patternHeight; i++) {
>> >> -               memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
>> >> +               memcpy(lineBuffers_[i].data(),
>> >> +                      linePointers[i + 1] - lineBufferPadding_,
>> >>                        lineBufferLength_);
>> >> -               linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
>> >> +               linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
>> >>         }
>> >>  
>> >>         /* Point lineBufferIndex_ to first unused lineBuffer */
>> >> @@ -614,9 +605,11 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
>> >>         if (!enableInputMemcpy_)
>> >>                 return;
>> >>  
>> >> -       memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
>> >> +       memcpy(lineBuffers_[lineBufferIndex_].data(),
>> >> +              linePointers[patternHeight] - lineBufferPadding_,
>> >>                lineBufferLength_);
>> >> -       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
>> >> +       linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
>> >> +                                   + lineBufferPadding_;
>> >>  
>> >>         lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
>> >>  }
>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> >> index 1dac64351f1d..8237a64be733 100644
>> >> --- a/src/libcamera/software_isp/debayer_cpu.h
>> >> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> >> @@ -146,7 +146,7 @@ private:
>> >>         DebayerInputConfig inputConfig_;
>> >>         DebayerOutputConfig outputConfig_;
>> >>         std::unique_ptr<SwStatsCpu> stats_;
>> >> -       uint8_t *lineBuffers_[kMaxLineBuffers];
>> >> +       std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>> >>         unsigned int lineBufferLength_;
>> >>         unsigned int lineBufferPadding_;
>> >>         unsigned int lineBufferIndex_;
>> >> 
>> >> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index f8d2677d657a..077f7f4bc45b 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -49,16 +49,9 @@  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
 	/* Initialize color lookup tables */
 	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
 		red_[i] = green_[i] = blue_[i] = i;
-
-	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
-		lineBuffers_[i] = nullptr;
 }
 
-DebayerCpu::~DebayerCpu()
-{
-	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
-		free(lineBuffers_[i]);
-}
+DebayerCpu::~DebayerCpu() = default;
 
 #define DECLARE_SRC_POINTERS(pixel_t)                            \
 	const pixel_t *prev = (const pixel_t *)src[0] + xShift_; \
@@ -526,13 +519,10 @@  int DebayerCpu::configure(const StreamConfiguration &inputCfg,
 	lineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;
 	lineBufferLength_ = window_.width * inputConfig_.bpp / 8 +
 			    2 * lineBufferPadding_;
-	for (unsigned int i = 0;
-	     i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;
-	     i++) {
-		free(lineBuffers_[i]);
-		lineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);
-		if (!lineBuffers_[i])
-			return -ENOMEM;
+
+	if (enableInputMemcpy_) {
+		for (unsigned int i = 0; i <= inputConfig_.patternSize.height; i++)
+			lineBuffers_[i].resize(lineBufferLength_);
 	}
 
 	measuredFrames_ = 0;
@@ -587,9 +577,10 @@  void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])
 		return;
 
 	for (unsigned int i = 0; i < patternHeight; i++) {
-		memcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,
+		memcpy(lineBuffers_[i].data(),
+		       linePointers[i + 1] - lineBufferPadding_,
 		       lineBufferLength_);
-		linePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;
+		linePointers[i + 1] = lineBuffers_[i].data() + lineBufferPadding_;
 	}
 
 	/* Point lineBufferIndex_ to first unused lineBuffer */
@@ -614,9 +605,11 @@  void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])
 	if (!enableInputMemcpy_)
 		return;
 
-	memcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,
+	memcpy(lineBuffers_[lineBufferIndex_].data(),
+	       linePointers[patternHeight] - lineBufferPadding_,
 	       lineBufferLength_);
-	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;
+	linePointers[patternHeight] = lineBuffers_[lineBufferIndex_].data()
+				    + lineBufferPadding_;
 
 	lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);
 }
diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
index 1dac64351f1d..8237a64be733 100644
--- a/src/libcamera/software_isp/debayer_cpu.h
+++ b/src/libcamera/software_isp/debayer_cpu.h
@@ -146,7 +146,7 @@  private:
 	DebayerInputConfig inputConfig_;
 	DebayerOutputConfig outputConfig_;
 	std::unique_ptr<SwStatsCpu> stats_;
-	uint8_t *lineBuffers_[kMaxLineBuffers];
+	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
 	unsigned int lineBufferLength_;
 	unsigned int lineBufferPadding_;
 	unsigned int lineBufferIndex_;