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

[Laurent Pinchart]

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.

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

-- 
Regards,

Laurent Pinchart