[PATCH v2] libcamera: software_isp: Replace malloc() with std::vector<>

[Hans de Goede]

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 at ideasonboard.com>
>>
>> I have given this a spin with IPU6 + the softISP and everything still works:
>>
>> Tested-by: Hans de Goede <hdegoede at 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
>