[PATCH v1 3/3] libcamera: virtual: Speed up test pattern animation

[Barnabás Pőcze]

2024. december 17., kedd 17:39 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Wed, Dec 11, 2024 at 03:25:56PM +0000, Barnabás Pőcze wrote:
> > After the initial generation, when a frame is requested,
> > the test pattern generator rotates the image left by 1 column.
> >
> > The current approach has two shortcomings:
> >   (1) it allocates a temporary buffer to hold one column;
> >   (2) it swaps two columns at a time.
> >
> > The test patterns are simple ARGB images, in row-major order,
> > so doing (2) works against memory prefetching. This can be
> > addressed by doing the rotation one row at a time as that way
> > the image is addressed in a purely linear fashion. Doing so also
> > eliminates the need for a dynamically allocated temporary buffer,
> > as the required buffer now only needs to hold one sample,
> > which is 4 bytes in this case.
> >
> > In an optimized build, this results in about a 2x increase in the
> > number of frames per second as reported by `cam`. In an unoptimized,
> > ASAN and UBSAN intrumented build, the difference is even bigger,
> > which is useful for running lc-compliance in CI in a reasonable time.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  .../virtual/test_pattern_generator.cpp        | 68 +++++++++----------
> >  .../pipeline/virtual/test_pattern_generator.h |  4 --
> >  2 files changed, 34 insertions(+), 38 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > index 47d341919..eeadc1646 100644
> > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > @@ -5,6 +5,8 @@
> >   * Derived class of FrameGenerator for generating test patterns
> >   */
> >
> > +#include <string.h>
> > +
> >  #include "test_pattern_generator.h"
> >
> >  #include <libcamera/base/log.h>
> > @@ -13,6 +15,37 @@
> >
> >  #include <libyuv/convert_from_argb.h>
> >
> > +namespace {
> > +
> > +template<size_t SampleSize>
> > +void rotateLeft1Sample(uint8_t *samples, size_t width)
> > +{
> > +	if (width <= 0)
> 
> width is unsigned, so it will never be smaller than 0. As the caller
> already ensures that the width is >= 2, I'd drop this check.
> 
> > +		return;
> > +
> > +	const size_t stride = width * SampleSize;
> 
> You could pass the stride value to this function, instead of recomputing
> it for each line.
> 
> > +	uint8_t first[SampleSize];
> > +
> > +	memcpy(first, &samples[0], SampleSize);
> > +	for (size_t i = 0; i < stride - SampleSize; i += SampleSize)
> > +		memcpy(&samples[i], &samples[i + SampleSize], SampleSize);
> 
> Have you considered replacing this with
> 
> 	memmove(&samples[0], &samples[SampleSize], stride - SampleSize);
> 
> ?

No... but that's a good idea. I'll apply this in the next version.


Regards,
Barnabás Pőcze

> 
> This patch is a good improvement, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> with the above comments taken into account (regardless of whether the
> suggestions are accepted or rejected).
> 
> I think the test pattern generator could do with a whole rewrite. I
> would compute the test pattern image at configure time, and store it in
> NV12 and in the output resolution, and handle the rotation when copying
> it to the frame buffer. That should significantly speed up operation.
> 
> > +	memcpy(&samples[stride - SampleSize], first, SampleSize);
> > +}
> > +
> > +template<size_t SampleSize>
> > +void rotateLeft1Column(const libcamera::Size &size, uint8_t *image)
> > +{
> > +	if (size.width < 2)
> > +		return;
> > +
> > +	const size_t stride = size.width * SampleSize;
> > +
> > +	for (size_t i = 0; i < size.height; i++, image += stride)
> > +		rotateLeft1Sample<SampleSize>(image, size.width);
> > +}
> > +
> > +}
> > +
> >  namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(Virtual)
> > @@ -27,7 +60,7 @@ int TestPatternGenerator::generateFrame(const Size &size,
> >
> >  	const auto &planes = mappedFrameBuffer.planes();
> >
> > -	shiftLeft(size);
> > +	rotateLeft1Column<kARGBSize>(size, template_.get());
> >
> >  	/* Convert the template_ to the frame buffer */
> >  	int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
> > @@ -40,39 +73,6 @@ int TestPatternGenerator::generateFrame(const Size &size,
> >  	return ret;
> >  }
> >
> > -void TestPatternGenerator::shiftLeft(const Size &size)
> > -{
> > -	/* Store the first column temporarily */
> > -	auto firstColumn = std::make_unique<uint8_t[]>(size.height * kARGBSize);
> > -	for (size_t h = 0; h < size.height; h++) {
> > -		unsigned int index = h * size.width * kARGBSize;
> > -		unsigned int index1 = h * kARGBSize;
> > -		firstColumn[index1] = template_[index];
> > -		firstColumn[index1 + 1] = template_[index + 1];
> > -		firstColumn[index1 + 2] = template_[index + 2];
> > -		firstColumn[index1 + 3] = 0x00;
> > -	}
> > -
> > -	/* Overwrite template_ */
> > -	uint8_t *buf = template_.get();
> > -	for (size_t h = 0; h < size.height; h++) {
> > -		for (size_t w = 0; w < size.width - 1; w++) {
> > -			/* Overwrite with the pixel on the right */
> > -			unsigned int index = (h * size.width + w + 1) * kARGBSize;
> > -			*buf++ = template_[index]; /* B */
> > -			*buf++ = template_[index + 1]; /* G */
> > -			*buf++ = template_[index + 2]; /* R */
> > -			*buf++ = 0x00; /* A */
> > -		}
> > -		/* Overwrite the new last column with the original first column */
> > -		unsigned int index1 = h * kARGBSize;
> > -		*buf++ = firstColumn[index1]; /* B */
> > -		*buf++ = firstColumn[index1 + 1]; /* G */
> > -		*buf++ = firstColumn[index1 + 2]; /* R */
> > -		*buf++ = 0x00; /* A */
> > -	}
> > -}
> > -
> >  void ColorBarsGenerator::configure(const Size &size)
> >  {
> >  	constexpr uint8_t kColorBar[8][3] = {
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > index 05f4ab7a7..2a51bd31a 100644
> > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > @@ -29,10 +29,6 @@ public:
> >  protected:
> >  	/* Buffer of test pattern template */
> >  	std::unique_ptr<uint8_t[]> template_;
> > -
> > -private:
> > -	/* Shift the buffer by 1 pixel left each frame */
> > -	void shiftLeft(const Size &size);
> >  };
> >
> >  class ColorBarsGenerator : public TestPatternGenerator
> 
> --
> Regards,
> 
> Laurent Pinchart
>