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

[Kieran Bingham]

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 at 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 at 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
>