[4/4] virtual: Align strides to 256 bytes

28422 diff mbox series

This patch was originally developed to help debugging and testing
https://webrtc-review.googlesource.com/c/src/+/491680, however I see
some benefits in always doing it:
1. It is common for hardware ISPs to have some stride alignment - even
   though usually somewhat lower. Using the given alignment should thus
   help ensure that clients implement stride handling correctly,
   preventing cases like the one linked above where a major implemention
   went without that - unnocited - for quite a long time.
2. It makes it more likely that output buffers can be imported by GPUs
   and display engines.
2. It paves the way for GPU-accelerated frame generators (similar to
   DebayerEGL), which may become desirable going forward in order to
   provide high resolution / high framerate / high bit-depth / HDR
   content.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 .../pipeline/virtual/frame_generator.h        |  1 +
 .../virtual/image_frame_generator.cpp         | 23 ++++++++++---------
 .../virtual/test_pattern_generator.cpp        | 18 ++++++++++-----
 src/libcamera/pipeline/virtual/virtual.cpp    |  9 +++++---
 4 files changed, 31 insertions(+), 20 deletions(-)

On 08.10.26 23:11, Robert Mader wrote:
> This patch was originally developed to help debugging and testing
> https://webrtc-review.googlesource.com/c/src/+/491680, however I see
> some benefits in always doing it:
> 1. It is common for hardware ISPs to have some stride alignment - even
>     though usually somewhat lower. Using the given alignment should thus
>     help ensure that clients implement stride handling correctly,
>     preventing cases like the one linked above where a major implemention
>     went without that - unnocited - for quite a long time.
> 2. It makes it more likely that output buffers can be imported by GPUs
>     and display engines.
> 2. It paves the way for GPU-accelerated frame generators (similar to
>     DebayerEGL), which may become desirable going forward in order to
>     provide high resolution / high framerate / high bit-depth / HDR
>     content.
>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> ---
>   .../pipeline/virtual/frame_generator.h        |  1 +
>   .../virtual/image_frame_generator.cpp         | 23 ++++++++++---------
>   .../virtual/test_pattern_generator.cpp        | 18 ++++++++++-----
>   src/libcamera/pipeline/virtual/virtual.cpp    |  9 +++++---
>   4 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
> index 70cd92907b63..31eea6392008 100644
> --- a/src/libcamera/pipeline/virtual/frame_generator.h
> +++ b/src/libcamera/pipeline/virtual/frame_generator.h
> @@ -23,6 +23,7 @@ public:
>   	virtual int generateFrame(const StreamConfiguration *cfg,
>   				  const FrameBuffer *buffer) = 0;
>   
> +	static constexpr unsigned int kStrideAlignment = 256;
>   protected:
>   	FrameGenerator() {}
>   };
> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> index dcef90702e18..685c11a61029 100644
> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> @@ -110,11 +110,6 @@ void ImageFrameGenerator::configure(const Size &size)
>   			std::make_unique<uint8_t[]>(halfSizeWidth * halfSizeHeight * 2);
>   		auto &src = imageFrameDatas_[i];
>   
> -		/*
> -		 * \todo Some platforms might enforce stride due to GPU.
> -                 * The width needs to be a multiple of the stride to work
> -                 * properly for now.
> -		 */
>   		libyuv::NV12Scale(src.Y.get(), src.size.width,
>   				  src.UV.get(), src.size.width,
>   				  src.size.width, src.size.height,
> @@ -159,9 +154,11 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration *cfg, const Fra
>   				       scaledFrameDatas_[frameIndex_].UV.get(),
>   				       size.width,
>   				       planes[0].data(),
> -				       info.stride(size.width, 0),
> +				       info.stride(size.width, 0,
> +						   kStrideAlignment),
>   				       planes[1].data(),
> -				       info.stride(size.width, 1),
> +				       info.stride(size.width, 1,
> +						   kStrideAlignment),
>   				       size.width, size.height);
>   		break;
>   	case formats::YUV420:
> @@ -170,11 +167,14 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration *cfg, const Fra
>   					 scaledFrameDatas_[frameIndex_].UV.get(),
>   					 size.width,
>   					 planes[0].data(),
> -					 info.stride(size.width, 0),
> +					 info.stride(size.width, 0,
> +						     kStrideAlignment),
>   					 planes[1].data(),
> -					 info.stride(size.width, 1),
> +					 info.stride(size.width, 1,
> +						     kStrideAlignment),
There's an issue with the computed value here, see 
https://gitlab.freedesktop.org/camera/libcamera/-/work_items/360
>   					 planes[2].data(),
> -					 info.stride(size.width, 2),
> +					 info.stride(size.width, 2,
> +						     kStrideAlignment),
>   					 size.width, size.height);
>   		break;
>   	case formats::ABGR8888:
> @@ -184,7 +184,8 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration *cfg, const Fra
>   					 scaledFrameDatas_[frameIndex_].UV.get(),
>   					 size.width,
>   					 planes[0].data(),
> -					 info.stride(size.width, 0),
> +					 info.stride(size.width, 0,
> +						     kStrideAlignment),
>   					 size.width, size.height);
>   		break;
>   	default:
> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> index d8cdc1c91c24..1892854d63d3 100644
> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> @@ -72,20 +72,25 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration *cfg,
>   		ret = libyuv::ARGBToNV12(template_.get(),
>   					 size.width * kARGBSize,
>   					 planes[0].data(),
> -					 info.stride(size.width, 0),
> +					 info.stride(size.width, 0,
> +						     kStrideAlignment),
>   					 planes[1].data(),
> -					 info.stride(size.width, 1),
> +					 info.stride(size.width, 1,
> +						     kStrideAlignment),
>   					 size.width, size.height);
>   		break;
>   	case formats::YUV420:
>   		ret = libyuv::ARGBToI420(template_.get(),
>   					 size.width * kARGBSize,
>   					 planes[0].data(),
> -					 info.stride(size.width, 0),
> +					 info.stride(size.width, 0,
> +						     kStrideAlignment),
>   					 planes[1].data(),
> -					 info.stride(size.width, 1),
> +					 info.stride(size.width, 1,
> +						     kStrideAlignment),
>   					 planes[2].data(),
> -					 info.stride(size.width, 2),
> +					 info.stride(size.width, 2,
> +						     kStrideAlignment),
>   					 size.width, size.height);
>   		break;
>   	case formats::ABGR8888:
> @@ -93,7 +98,8 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration *cfg,
>   		ret = libyuv::ARGBCopy(template_.get(),
>   				       size.width * kARGBSize,
>   				       planes[0].data(),
> -				       info.stride(size.width, 0),
> +				       info.stride(size.width, 0,
> +						   kStrideAlignment),
>   				       size.width, size.height);
>   		break;
>   	default:
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 349cb88a3f05..ac6c5ca67aa8 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -231,8 +231,10 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>   			adjusted = true;
>   		}
>   
> -		auto stride = info.stride(cfg.size.width, 0);
> -		auto frameSize = info.frameSize(cfg.size);
> +		auto stride = info.stride(cfg.size.width, 0,
> +					  FrameGenerator::kStrideAlignment);
> +		auto frameSize = info.frameSize(cfg.size,
> +						FrameGenerator::kStrideAlignment);
>   		if ((cfg.stride && cfg.stride != stride) ||
>   		    (cfg.frameSize && cfg.frameSize != frameSize)) {
>   			status = Adjusted;
> @@ -342,7 +344,8 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
>   
>   	std::vector<unsigned int> planeSizes;
>   	for (size_t i = 0; i < info.numPlanes(); ++i)
> -		planeSizes.push_back(info.planeSize(config.size, i));
> +		planeSizes.push_back(info.planeSize(config.size, i,
> +						    FrameGenerator::kStrideAlignment));
>   
>   	return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);
>   }