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

[Milan Zamazal]

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

> On Mon, Aug 05, 2024 at 12:37:50PM +0200, Milan Zamazal wrote:
>> Kieran Bingham <kieran.bingham at 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 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...
>> 
>> 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 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