[v5,3/3] virtual: Support more common pixel formats
diff mbox series

Message ID 20261019224902.177382-4-robert.mader@collabora.com
State Accepted
Headers show
Series
  • virtual: Support more common pixel formats
Related show

Commit Message

Robert Mader Oct. 19, 2026, 10:49 p.m. UTC 
While NV12 is the most common native format for hardware video encoders,
YUV420 is the equivalent for most software video encoders. Thus
supporting both is great for testing.

Similarly having some RGB formats with and without alpha can be very
useful, especially as the swISP is currently limited to such formats.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 .../pipeline/virtual/frame_generator.h        |  3 +-
 .../virtual/image_frame_generator.cpp         | 61 ++++++++++++++++---
 .../pipeline/virtual/image_frame_generator.h  |  2 +-
 .../virtual/test_pattern_generator.cpp        | 52 +++++++++++++---
 .../pipeline/virtual/test_pattern_generator.h |  2 +-
 src/libcamera/pipeline/virtual/virtual.cpp    | 45 +++++++++++---
 6 files changed, 135 insertions(+), 30 deletions(-)

Comments

Laurent Pinchart Oct. 19, 2026, 11:20 p.m. UTC | #1 
On Tue, Oct 20, 2026 at 12:49:02AM +0200, Robert Mader wrote:
> While NV12 is the most common native format for hardware video encoders,
> YUV420 is the equivalent for most software video encoders. Thus
> supporting both is great for testing.
> 
> Similarly having some RGB formats with and without alpha can be very
> useful, especially as the swISP is currently limited to such formats.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../pipeline/virtual/frame_generator.h        |  3 +-
>  .../virtual/image_frame_generator.cpp         | 61 ++++++++++++++++---
>  .../pipeline/virtual/image_frame_generator.h  |  2 +-
>  .../virtual/test_pattern_generator.cpp        | 52 +++++++++++++---
>  .../pipeline/virtual/test_pattern_generator.h |  2 +-
>  src/libcamera/pipeline/virtual/virtual.cpp    | 45 +++++++++++---
>  6 files changed, 135 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
> index a0658c45cd82..fd34a91a5e50 100644
> --- a/src/libcamera/pipeline/virtual/frame_generator.h
> +++ b/src/libcamera/pipeline/virtual/frame_generator.h
> @@ -9,6 +9,7 @@
>  
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/geometry.h>
> +#include <libcamera/stream.h>
>  
>  namespace libcamera {
>  
> @@ -19,7 +20,7 @@ public:
>  
>  	virtual void configure(const Size &size) = 0;
>  
> -	virtual int generateFrame(const Size &size,
> +	virtual int generateFrame(const StreamConfiguration &cfg,
>  				  const FrameBuffer *buffer) = 0;
>  
>  protected:
> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> index 5caad8b72763..7a9bc9ac1ebd 100644
> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> @@ -126,7 +126,7 @@ void ImageFrameGenerator::configure(const Size &size)
>  	}
>  }
>  
> -int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)
> +int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer)
>  {
>  	ASSERT(!scaledFrameDatas_.empty());
>  
> @@ -143,17 +143,62 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
>  					    MappedFrameBuffer::MapFlag::Write);
>  
>  	const auto &planes = mappedFrameBuffer.planes();
> -	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
> +	const Size size = cfg.size;

Sorry, I forgot the & in my review of v4:

	const Size &size = cfg.size;

> +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  
>  	/* Loop only around the number of images available */
>  	frameIndex_ %= imageFrameDatas_.size();
>  
> +	int ret;
> +	auto Y = scaledFrameDatas_[frameIndex_].Y.get();
> +	auto UV = scaledFrameDatas_[frameIndex_].UV.get();

Lowercase for variables please.

> +
>  	/* Write the scaledY and scaledUV to the mapped frame buffer */
> -	libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width,
> -			 scaledFrameDatas_[frameIndex_].UV.get(), size.width,
> -			 planes[0].data(), info.stride(size.width, 0),
> -			 planes[1].data(), info.stride(size.width, 1),
> -			 size.width, size.height);
> +	switch (cfg.pixelFormat) {
> +	case formats::NV12:
> +		ret = libyuv::NV12Copy(Y,
> +				       size.width,
> +				       UV,
> +				       size.width,
> +				       planes[0].data(),
> +				       info.stride(size.width, 0),
> +				       planes[1].data(),
> +				       info.stride(size.width, 1),
> +				       size.width, size.height);

I think the following would be more readable as it groups related
arguments without making lines too long:

		ret = libyuv::NV12Copy(Y, size.width, UV, size.width,
				       planes[0].data(), info.stride(size.width, 0),
				       planes[1].data(), info.stride(size.width, 1),
				       size.width, size.height);

Same below.

> +		break;
> +
> +	case formats::YUV420:
> +		ret = libyuv::NV12ToI420(Y,
> +					 size.width,
> +					 UV,
> +					 size.width,
> +					 planes[0].data(),
> +					 info.stride(size.width, 0),
> +					 planes[1].data(),
> +					 info.stride(size.width, 1),
> +					 planes[2].data(),
> +					 info.stride(size.width, 2),
> +					 size.width, size.height);
> +		break;
> +
> +	case formats::ABGR8888:
> +	case formats::XBGR8888:
> +		ret = libyuv::NV12ToABGR(Y,
> +					 size.width,
> +					 UV,
> +					 size.width,
> +					 planes[0].data(),
> +					 info.stride(size.width, 0),
> +					 size.width, size.height);
> +		break;
> +
> +	default:
> +		LOG(Virtual, Error) << "Unsupported format";
> +		return -1;
> +	}
> +
> +	if (ret != 0)
> +		LOG(Virtual, Error) << "Conversation failed with " << ret;
>  
>  	/* Proceed to the next image every 4 frames */
>  	/* \todo Consider setting the frameRepeat in the config file  */
> @@ -161,7 +206,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
>  	if (parameter_ % frameRepeat == 0)
>  		frameIndex_++;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h
> index 42a077ba803c..19b6fe38654e 100644
> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h
> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
> @@ -37,7 +37,7 @@ private:
>  	};
>  
>  	void configure(const Size &size) override;
> -	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
> +	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
>  
>  	std::vector<ImageFrameData> imageFrameDatas_;
>  	std::vector<ImageFrameData> scaledFrameDatas_;
> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> index 571be4fbf65d..e214048d288a 100644
> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> @@ -44,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual)
>  
>  static const unsigned int kARGBSize = 4;
>  
> -int TestPatternGenerator::generateFrame(const Size &size,
> +int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
>  					const FrameBuffer *buffer)
>  {
>  	/*
> @@ -60,19 +60,53 @@ int TestPatternGenerator::generateFrame(const Size &size,
>  					    MappedFrameBuffer::MapFlag::Write);
>  
>  	const auto &planes = mappedFrameBuffer.planes();
> -	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
> +	const Size size = cfg.size;

	const Size &size = cfg.size;

> +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  
>  	rotateLeft1Column<kARGBSize>(size, template_.get());
>  
> +	int ret;
> +
>  	/* Convert the template_ to the frame buffer */
> -	int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
> -				     planes[0].data(),
> -				     info.stride(size.width, 0),
> -				     planes[1].data(),
> -				     info.stride(size.width, 1),
> -				     size.width, size.height);
> +	switch (cfg.pixelFormat) {
> +	case formats::NV12:
> +		ret = libyuv::ARGBToNV12(template_.get(),
> +					 size.width * kARGBSize,
> +					 planes[0].data(),
> +					 info.stride(size.width, 0),
> +					 planes[1].data(),
> +					 info.stride(size.width, 1),
> +					 size.width, size.height);

Similarly as above,

		ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
					 planes[0].data(), info.stride(size.width, 0),
					 planes[1].data(), info.stride(size.width, 1),
					 size.width, size.height);

Same below.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I think we can handle that when applying if you let us know if you're
fine with the small changes.

> +		break;
> +
> +	case formats::YUV420:
> +		ret = libyuv::ARGBToI420(template_.get(),
> +					 size.width * kARGBSize,
> +					 planes[0].data(),
> +					 info.stride(size.width, 0),
> +					 planes[1].data(),
> +					 info.stride(size.width, 1),
> +					 planes[2].data(),
> +					 info.stride(size.width, 2),
> +					 size.width, size.height);
> +		break;
> +
> +	case formats::ABGR8888:
> +	case formats::XBGR8888:
> +		ret = libyuv::ARGBCopy(template_.get(),
> +				       size.width * kARGBSize,
> +				       planes[0].data(),
> +				       info.stride(size.width, 0),
> +				       size.width, size.height);
> +		break;
> +
> +	default:
> +		LOG(Virtual, Error) << "Unsupported format";
> +		return -1;
> +	}
> +
>  	if (ret != 0)
> -		LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
> +		LOG(Virtual, Error) << "Conversation failed with " << ret;
>  
>  	return ret;
>  }
> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> index 2a51bd31abdf..9f72d652ab2c 100644
> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h
> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> @@ -24,7 +24,7 @@ enum class TestPattern : char {
>  class TestPatternGenerator : public FrameGenerator
>  {
>  public:
> -	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
> +	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
>  
>  protected:
>  	/* Buffer of test pattern template */
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index e25dd85a7391..f35af97b103d 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -47,6 +47,13 @@ LOG_DEFINE_CATEGORY(Virtual)
>  
>  namespace {
>  
> +constexpr PixelFormat outputFormats[] = {
> +	formats::NV12,
> +	formats::YUV420,
> +	formats::ABGR8888,
> +	formats::XBGR8888,
> +};
> +
>  uint64_t currentTimestamp()
>  {
>  	const auto now = std::chrono::steady_clock::now();
> @@ -148,7 +155,7 @@ void VirtualCameraData::processRequest(Request *request)
>  				found = true;
>  
>  				if (streamConfig.frameGenerator->generateFrame(
> -					    stream->configuration().size, buffer))
> +					    stream->configuration(), buffer))
>  					fmd.status = FrameMetadata::Status::FrameError;
>  
>  				bufferCompleted.emit(buffer);
> @@ -202,18 +209,37 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>  			adjusted = true;
>  		}
>  
> -		if (cfg.pixelFormat != formats::NV12) {
> -			cfg.pixelFormat = formats::NV12;
> +		if (std::find(std::begin(outputFormats),
> +			      std::end(outputFormats),
> +			      cfg.pixelFormat) == std::end(outputFormats)) {
> +			cfg.pixelFormat = outputFormats[0];
>  			status = Adjusted;
>  			adjusted = true;
>  		}
>  
> -		if (cfg.colorSpace != ColorSpace::Smpte170m) {
> -			cfg.colorSpace = ColorSpace::Smpte170m;
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		std::optional<ColorSpace> colorSpace;
> +
> +		switch (info.colourEncoding) {
> +		case PixelFormatInfo::ColourEncodingRGB:
> +			colorSpace = ColorSpace::Srgb;
> +			break;
> +
> +		case PixelFormatInfo::ColourEncodingYUV:
> +			colorSpace = ColorSpace::Smpte170m;
> +			break;
> +
> +		default:
> +			ASSERT(false);
> +		}
> +
> +		if (cfg.colorSpace && cfg.colorSpace != colorSpace) {
>  			status = Adjusted;
>  			adjusted = true;
>  		}
>  
> +		cfg.colorSpace = colorSpace;
> +
>  		if (validateColorSpaces() == Adjusted) {
>  			status = Adjusted;
>  			adjusted = true;
> @@ -223,7 +249,6 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>  			LOG(Virtual, Info)
>  				<< "Stream configuration adjusted to " << cfg.toString();
>  
> -		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  		cfg.stride = info.stride(cfg.size.width, 0, 1);
>  		cfg.frameSize = info.frameSize(cfg.size, 1);
>  
> @@ -275,12 +300,12 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
>  		}
>  
>  		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -		PixelFormat pixelFormat = formats::NV12;
> -		streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
> -						 data->config_.maxResolutionSize } };
> +		for (auto pixelFormat : outputFormats)
> +			streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
> +							 data->config_.maxResolutionSize } };
>  		StreamFormats formats(streamFormats);
>  		StreamConfiguration cfg(formats);
> -		cfg.pixelFormat = pixelFormat;
> +		cfg.pixelFormat = outputFormats[0];
>  		cfg.size = data->config_.maxResolutionSize;
>  		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>  		cfg.colorSpace = ColorSpace::Smpte170m;
Robert Mader Oct. 20, 2026, 7:01 a.m. UTC | #2 
Hi,

On 20.10.26 01:20, Laurent Pinchart wrote:
> On Tue, Oct 20, 2026 at 12:49:02AM +0200, Robert Mader wrote:
>> While NV12 is the most common native format for hardware video encoders,
>> YUV420 is the equivalent for most software video encoders. Thus
>> supporting both is great for testing.
>>
>> Similarly having some RGB formats with and without alpha can be very
>> useful, especially as the swISP is currently limited to such formats.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../pipeline/virtual/frame_generator.h        |  3 +-
>>   .../virtual/image_frame_generator.cpp         | 61 ++++++++++++++++---
>>   .../pipeline/virtual/image_frame_generator.h  |  2 +-
>>   .../virtual/test_pattern_generator.cpp        | 52 +++++++++++++---
>>   .../pipeline/virtual/test_pattern_generator.h |  2 +-
>>   src/libcamera/pipeline/virtual/virtual.cpp    | 45 +++++++++++---
>>   6 files changed, 135 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
>> index a0658c45cd82..fd34a91a5e50 100644
>> --- a/src/libcamera/pipeline/virtual/frame_generator.h
>> +++ b/src/libcamera/pipeline/virtual/frame_generator.h
>> @@ -9,6 +9,7 @@
>>   
>>   #include <libcamera/framebuffer.h>
>>   #include <libcamera/geometry.h>
>> +#include <libcamera/stream.h>
>>   
>>   namespace libcamera {
>>   
>> @@ -19,7 +20,7 @@ public:
>>   
>>   	virtual void configure(const Size &size) = 0;
>>   
>> -	virtual int generateFrame(const Size &size,
>> +	virtual int generateFrame(const StreamConfiguration &cfg,
>>   				  const FrameBuffer *buffer) = 0;
>>   
>>   protected:
>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
>> index 5caad8b72763..7a9bc9ac1ebd 100644
>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
>> @@ -126,7 +126,7 @@ void ImageFrameGenerator::configure(const Size &size)
>>   	}
>>   }
>>   
>> -int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)
>> +int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer)
>>   {
>>   	ASSERT(!scaledFrameDatas_.empty());
>>   
>> @@ -143,17 +143,62 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
>>   					    MappedFrameBuffer::MapFlag::Write);
>>   
>>   	const auto &planes = mappedFrameBuffer.planes();
>> -	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
>> +	const Size size = cfg.size;
> Sorry, I forgot the & in my review of v4:
>
> 	const Size &size = cfg.size;
>
>> +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>>   
>>   	/* Loop only around the number of images available */
>>   	frameIndex_ %= imageFrameDatas_.size();
>>   
>> +	int ret;
>> +	auto Y = scaledFrameDatas_[frameIndex_].Y.get();
>> +	auto UV = scaledFrameDatas_[frameIndex_].UV.get();
> Lowercase for variables please.
>
>> +
>>   	/* Write the scaledY and scaledUV to the mapped frame buffer */
>> -	libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width,
>> -			 scaledFrameDatas_[frameIndex_].UV.get(), size.width,
>> -			 planes[0].data(), info.stride(size.width, 0),
>> -			 planes[1].data(), info.stride(size.width, 1),
>> -			 size.width, size.height);
>> +	switch (cfg.pixelFormat) {
>> +	case formats::NV12:
>> +		ret = libyuv::NV12Copy(Y,
>> +				       size.width,
>> +				       UV,
>> +				       size.width,
>> +				       planes[0].data(),
>> +				       info.stride(size.width, 0),
>> +				       planes[1].data(),
>> +				       info.stride(size.width, 1),
>> +				       size.width, size.height);
> I think the following would be more readable as it groups related
> arguments without making lines too long:
>
> 		ret = libyuv::NV12Copy(Y, size.width, UV, size.width,
> 				       planes[0].data(), info.stride(size.width, 0),
> 				       planes[1].data(), info.stride(size.width, 1),
> 				       size.width, size.height);
>
> Same below.
>
>> +		break;
>> +
>> +	case formats::YUV420:
>> +		ret = libyuv::NV12ToI420(Y,
>> +					 size.width,
>> +					 UV,
>> +					 size.width,
>> +					 planes[0].data(),
>> +					 info.stride(size.width, 0),
>> +					 planes[1].data(),
>> +					 info.stride(size.width, 1),
>> +					 planes[2].data(),
>> +					 info.stride(size.width, 2),
>> +					 size.width, size.height);
>> +		break;
>> +
>> +	case formats::ABGR8888:
>> +	case formats::XBGR8888:
>> +		ret = libyuv::NV12ToABGR(Y,
>> +					 size.width,
>> +					 UV,
>> +					 size.width,
>> +					 planes[0].data(),
>> +					 info.stride(size.width, 0),
>> +					 size.width, size.height);
>> +		break;
>> +
>> +	default:
>> +		LOG(Virtual, Error) << "Unsupported format";
>> +		return -1;
>> +	}
>> +
>> +	if (ret != 0)
>> +		LOG(Virtual, Error) << "Conversation failed with " << ret;
>>   
>>   	/* Proceed to the next image every 4 frames */
>>   	/* \todo Consider setting the frameRepeat in the config file  */
>> @@ -161,7 +206,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
>>   	if (parameter_ % frameRepeat == 0)
>>   		frameIndex_++;
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   /*
>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h
>> index 42a077ba803c..19b6fe38654e 100644
>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h
>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
>> @@ -37,7 +37,7 @@ private:
>>   	};
>>   
>>   	void configure(const Size &size) override;
>> -	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
>> +	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
>>   
>>   	std::vector<ImageFrameData> imageFrameDatas_;
>>   	std::vector<ImageFrameData> scaledFrameDatas_;
>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
>> index 571be4fbf65d..e214048d288a 100644
>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
>> @@ -44,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual)
>>   
>>   static const unsigned int kARGBSize = 4;
>>   
>> -int TestPatternGenerator::generateFrame(const Size &size,
>> +int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
>>   					const FrameBuffer *buffer)
>>   {
>>   	/*
>> @@ -60,19 +60,53 @@ int TestPatternGenerator::generateFrame(const Size &size,
>>   					    MappedFrameBuffer::MapFlag::Write);
>>   
>>   	const auto &planes = mappedFrameBuffer.planes();
>> -	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
>> +	const Size size = cfg.size;
> 	const Size &size = cfg.size;
>
>> +	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>>   
>>   	rotateLeft1Column<kARGBSize>(size, template_.get());
>>   
>> +	int ret;
>> +
>>   	/* Convert the template_ to the frame buffer */
>> -	int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
>> -				     planes[0].data(),
>> -				     info.stride(size.width, 0),
>> -				     planes[1].data(),
>> -				     info.stride(size.width, 1),
>> -				     size.width, size.height);
>> +	switch (cfg.pixelFormat) {
>> +	case formats::NV12:
>> +		ret = libyuv::ARGBToNV12(template_.get(),
>> +					 size.width * kARGBSize,
>> +					 planes[0].data(),
>> +					 info.stride(size.width, 0),
>> +					 planes[1].data(),
>> +					 info.stride(size.width, 1),
>> +					 size.width, size.height);
> Similarly as above,
>
> 		ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
> 					 planes[0].data(), info.stride(size.width, 0),
> 					 planes[1].data(), info.stride(size.width, 1),
> 					 size.width, size.height);
>
> Same below.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I think we can handle that when applying if you let us know if you're
> fine with the small changes.
Sure, and thanks!
>
>> +		break;
>> +
>> +	case formats::YUV420:
>> +		ret = libyuv::ARGBToI420(template_.get(),
>> +					 size.width * kARGBSize,
>> +					 planes[0].data(),
>> +					 info.stride(size.width, 0),
>> +					 planes[1].data(),
>> +					 info.stride(size.width, 1),
>> +					 planes[2].data(),
>> +					 info.stride(size.width, 2),
>> +					 size.width, size.height);
>> +		break;
>> +
>> +	case formats::ABGR8888:
>> +	case formats::XBGR8888:
>> +		ret = libyuv::ARGBCopy(template_.get(),
>> +				       size.width * kARGBSize,
>> +				       planes[0].data(),
>> +				       info.stride(size.width, 0),
>> +				       size.width, size.height);
>> +		break;
>> +
>> +	default:
>> +		LOG(Virtual, Error) << "Unsupported format";
>> +		return -1;
>> +	}
>> +
>>   	if (ret != 0)
>> -		LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
>> +		LOG(Virtual, Error) << "Conversation failed with " << ret;
>>   
>>   	return ret;
>>   }
>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h
>> index 2a51bd31abdf..9f72d652ab2c 100644
>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h
>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
>> @@ -24,7 +24,7 @@ enum class TestPattern : char {
>>   class TestPatternGenerator : public FrameGenerator
>>   {
>>   public:
>> -	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
>> +	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
>>   
>>   protected:
>>   	/* Buffer of test pattern template */
>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
>> index e25dd85a7391..f35af97b103d 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>> @@ -47,6 +47,13 @@ LOG_DEFINE_CATEGORY(Virtual)
>>   
>>   namespace {
>>   
>> +constexpr PixelFormat outputFormats[] = {
>> +	formats::NV12,
>> +	formats::YUV420,
>> +	formats::ABGR8888,
>> +	formats::XBGR8888,
>> +};
>> +
>>   uint64_t currentTimestamp()
>>   {
>>   	const auto now = std::chrono::steady_clock::now();
>> @@ -148,7 +155,7 @@ void VirtualCameraData::processRequest(Request *request)
>>   				found = true;
>>   
>>   				if (streamConfig.frameGenerator->generateFrame(
>> -					    stream->configuration().size, buffer))
>> +					    stream->configuration(), buffer))
>>   					fmd.status = FrameMetadata::Status::FrameError;
>>   
>>   				bufferCompleted.emit(buffer);
>> @@ -202,18 +209,37 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>>   			adjusted = true;
>>   		}
>>   
>> -		if (cfg.pixelFormat != formats::NV12) {
>> -			cfg.pixelFormat = formats::NV12;
>> +		if (std::find(std::begin(outputFormats),
>> +			      std::end(outputFormats),
>> +			      cfg.pixelFormat) == std::end(outputFormats)) {
>> +			cfg.pixelFormat = outputFormats[0];
>>   			status = Adjusted;
>>   			adjusted = true;
>>   		}
>>   
>> -		if (cfg.colorSpace != ColorSpace::Smpte170m) {
>> -			cfg.colorSpace = ColorSpace::Smpte170m;
>> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>> +		std::optional<ColorSpace> colorSpace;
>> +
>> +		switch (info.colourEncoding) {
>> +		case PixelFormatInfo::ColourEncodingRGB:
>> +			colorSpace = ColorSpace::Srgb;
>> +			break;
>> +
>> +		case PixelFormatInfo::ColourEncodingYUV:
>> +			colorSpace = ColorSpace::Smpte170m;
>> +			break;
>> +
>> +		default:
>> +			ASSERT(false);
>> +		}
>> +
>> +		if (cfg.colorSpace && cfg.colorSpace != colorSpace) {
>>   			status = Adjusted;
>>   			adjusted = true;
>>   		}
>>   
>> +		cfg.colorSpace = colorSpace;
>> +
>>   		if (validateColorSpaces() == Adjusted) {
>>   			status = Adjusted;
>>   			adjusted = true;
>> @@ -223,7 +249,6 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>>   			LOG(Virtual, Info)
>>   				<< "Stream configuration adjusted to " << cfg.toString();
>>   
>> -		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>>   		cfg.stride = info.stride(cfg.size.width, 0, 1);
>>   		cfg.frameSize = info.frameSize(cfg.size, 1);
>>   
>> @@ -275,12 +300,12 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
>>   		}
>>   
>>   		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
>> -		PixelFormat pixelFormat = formats::NV12;
>> -		streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
>> -						 data->config_.maxResolutionSize } };
>> +		for (auto pixelFormat : outputFormats)
>> +			streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
>> +							 data->config_.maxResolutionSize } };
>>   		StreamFormats formats(streamFormats);
>>   		StreamConfiguration cfg(formats);
>> -		cfg.pixelFormat = pixelFormat;
>> +		cfg.pixelFormat = outputFormats[0];
>>   		cfg.size = data->config_.maxResolutionSize;
>>   		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>>   		cfg.colorSpace = ColorSpace::Smpte170m;
Kieran Bingham Oct. 20, 2026, 10:49 a.m. UTC | #3 
Quoting Laurent Pinchart (2026-10-20 00:20:12)
> On Tue, Oct 20, 2026 at 12:49:02AM +0200, Robert Mader wrote:
> > While NV12 is the most common native format for hardware video encoders,
> > YUV420 is the equivalent for most software video encoders. Thus
> > supporting both is great for testing.
> > 
> > Similarly having some RGB formats with and without alpha can be very
> > useful, especially as the swISP is currently limited to such formats.
> > 
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  .../pipeline/virtual/frame_generator.h        |  3 +-
> >  .../virtual/image_frame_generator.cpp         | 61 ++++++++++++++++---
> >  .../pipeline/virtual/image_frame_generator.h  |  2 +-
> >  .../virtual/test_pattern_generator.cpp        | 52 +++++++++++++---
> >  .../pipeline/virtual/test_pattern_generator.h |  2 +-
> >  src/libcamera/pipeline/virtual/virtual.cpp    | 45 +++++++++++---
> >  6 files changed, 135 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
> > index a0658c45cd82..fd34a91a5e50 100644
> > --- a/src/libcamera/pipeline/virtual/frame_generator.h
> > +++ b/src/libcamera/pipeline/virtual/frame_generator.h
> > @@ -9,6 +9,7 @@
> >  
> >  #include <libcamera/framebuffer.h>
> >  #include <libcamera/geometry.h>
> > +#include <libcamera/stream.h>
> >  
> >  namespace libcamera {
> >  
> > @@ -19,7 +20,7 @@ public:
> >  
> >       virtual void configure(const Size &size) = 0;
> >  
> > -     virtual int generateFrame(const Size &size,
> > +     virtual int generateFrame(const StreamConfiguration &cfg,
> >                                 const FrameBuffer *buffer) = 0;
> >  
> >  protected:
> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > index 5caad8b72763..7a9bc9ac1ebd 100644
> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> > @@ -126,7 +126,7 @@ void ImageFrameGenerator::configure(const Size &size)
> >       }
> >  }
> >  
> > -int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)
> > +int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer)
> >  {
> >       ASSERT(!scaledFrameDatas_.empty());
> >  
> > @@ -143,17 +143,62 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
> >                                           MappedFrameBuffer::MapFlag::Write);
> >  
> >       const auto &planes = mappedFrameBuffer.planes();
> > -     const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
> > +     const Size size = cfg.size;
> 
> Sorry, I forgot the & in my review of v4:
> 
>         const Size &size = cfg.size;
> 
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> >  
> >       /* Loop only around the number of images available */
> >       frameIndex_ %= imageFrameDatas_.size();
> >  
> > +     int ret;
> > +     auto Y = scaledFrameDatas_[frameIndex_].Y.get();
> > +     auto UV = scaledFrameDatas_[frameIndex_].UV.get();
> 
> Lowercase for variables please.
> 
> > +
> >       /* Write the scaledY and scaledUV to the mapped frame buffer */
> > -     libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width,
> > -                      scaledFrameDatas_[frameIndex_].UV.get(), size.width,
> > -                      planes[0].data(), info.stride(size.width, 0),
> > -                      planes[1].data(), info.stride(size.width, 1),
> > -                      size.width, size.height);
> > +     switch (cfg.pixelFormat) {
> > +     case formats::NV12:
> > +             ret = libyuv::NV12Copy(Y,
> > +                                    size.width,
> > +                                    UV,
> > +                                    size.width,
> > +                                    planes[0].data(),
> > +                                    info.stride(size.width, 0),
> > +                                    planes[1].data(),
> > +                                    info.stride(size.width, 1),
> > +                                    size.width, size.height);
> 
> I think the following would be more readable as it groups related
> arguments without making lines too long:
> 
>                 ret = libyuv::NV12Copy(Y, size.width, UV, size.width,
>                                        planes[0].data(), info.stride(size.width, 0),
>                                        planes[1].data(), info.stride(size.width, 1),
>                                        size.width, size.height);
> 
> Same below.
> 
> > +             break;
> > +
> > +     case formats::YUV420:
> > +             ret = libyuv::NV12ToI420(Y,
> > +                                      size.width,
> > +                                      UV,
> > +                                      size.width,
> > +                                      planes[0].data(),
> > +                                      info.stride(size.width, 0),
> > +                                      planes[1].data(),
> > +                                      info.stride(size.width, 1),
> > +                                      planes[2].data(),
> > +                                      info.stride(size.width, 2),
> > +                                      size.width, size.height);
> > +             break;
> > +
> > +     case formats::ABGR8888:
> > +     case formats::XBGR8888:
> > +             ret = libyuv::NV12ToABGR(Y,
> > +                                      size.width,
> > +                                      UV,
> > +                                      size.width,
> > +                                      planes[0].data(),
> > +                                      info.stride(size.width, 0),
> > +                                      size.width, size.height);
> > +             break;
> > +
> > +     default:
> > +             LOG(Virtual, Error) << "Unsupported format";
> > +             return -1;
> > +     }
> > +
> > +     if (ret != 0)
> > +             LOG(Virtual, Error) << "Conversation failed with " << ret;

As much as I love a good chat, I'll change this to 'Conversion' ;-)

--
Kieran


> >  
> >       /* Proceed to the next image every 4 frames */
> >       /* \todo Consider setting the frameRepeat in the config file  */
> > @@ -161,7 +206,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
> >       if (parameter_ % frameRepeat == 0)
> >               frameIndex_++;
> >  
> > -     return 0;
> > +     return ret;
> >  }
> >  
> >  /*
> > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h
> > index 42a077ba803c..19b6fe38654e 100644
> > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h
> > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
> > @@ -37,7 +37,7 @@ private:
> >       };
> >  
> >       void configure(const Size &size) override;
> > -     int generateFrame(const Size &size, const FrameBuffer *buffer) override;
> > +     int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
> >  
> >       std::vector<ImageFrameData> imageFrameDatas_;
> >       std::vector<ImageFrameData> scaledFrameDatas_;
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > index 571be4fbf65d..e214048d288a 100644
> > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
> > @@ -44,7 +44,7 @@ LOG_DECLARE_CATEGORY(Virtual)
> >  
> >  static const unsigned int kARGBSize = 4;
> >  
> > -int TestPatternGenerator::generateFrame(const Size &size,
> > +int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
> >                                       const FrameBuffer *buffer)
> >  {
> >       /*
> > @@ -60,19 +60,53 @@ int TestPatternGenerator::generateFrame(const Size &size,
> >                                           MappedFrameBuffer::MapFlag::Write);
> >  
> >       const auto &planes = mappedFrameBuffer.planes();
> > -     const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
> > +     const Size size = cfg.size;
> 
>         const Size &size = cfg.size;
> 
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> >  
> >       rotateLeft1Column<kARGBSize>(size, template_.get());
> >  
> > +     int ret;
> > +
> >       /* Convert the template_ to the frame buffer */
> > -     int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
> > -                                  planes[0].data(),
> > -                                  info.stride(size.width, 0),
> > -                                  planes[1].data(),
> > -                                  info.stride(size.width, 1),
> > -                                  size.width, size.height);
> > +     switch (cfg.pixelFormat) {
> > +     case formats::NV12:
> > +             ret = libyuv::ARGBToNV12(template_.get(),
> > +                                      size.width * kARGBSize,
> > +                                      planes[0].data(),
> > +                                      info.stride(size.width, 0),
> > +                                      planes[1].data(),
> > +                                      info.stride(size.width, 1),
> > +                                      size.width, size.height);
> 
> Similarly as above,
> 
>                 ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
>                                          planes[0].data(), info.stride(size.width, 0),
>                                          planes[1].data(), info.stride(size.width, 1),
>                                          size.width, size.height);
> 
> Same below.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I think we can handle that when applying if you let us know if you're
> fine with the small changes.

I'm going to apply these fixups, as I also prefer keeping the
data/stride for each plane on a single line. Far more readable IMO.

And also I found a typo below, so I'll do that too

> 
> > +             break;
> > +
> > +     case formats::YUV420:
> > +             ret = libyuv::ARGBToI420(template_.get(),
> > +                                      size.width * kARGBSize,
> > +                                      planes[0].data(),
> > +                                      info.stride(size.width, 0),
> > +                                      planes[1].data(),
> > +                                      info.stride(size.width, 1),
> > +                                      planes[2].data(),
> > +                                      info.stride(size.width, 2),
> > +                                      size.width, size.height);
> > +             break;
> > +
> > +     case formats::ABGR8888:
> > +     case formats::XBGR8888:
> > +             ret = libyuv::ARGBCopy(template_.get(),
> > +                                    size.width * kARGBSize,
> > +                                    planes[0].data(),
> > +                                    info.stride(size.width, 0),
> > +                                    size.width, size.height);
> > +             break;
> > +
> > +     default:
> > +             LOG(Virtual, Error) << "Unsupported format";
> > +             return -1;
> > +     }
> > +
> >       if (ret != 0)
> > -             LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
> > +             LOG(Virtual, Error) << "Conversation failed with " << ret;
> >  
> >       return ret;
> >  }
> > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > index 2a51bd31abdf..9f72d652ab2c 100644
> > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
> > @@ -24,7 +24,7 @@ enum class TestPattern : char {
> >  class TestPatternGenerator : public FrameGenerator
> >  {
> >  public:
> > -     int generateFrame(const Size &size, const FrameBuffer *buffer) override;
> > +     int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
> >  
> >  protected:
> >       /* Buffer of test pattern template */
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> > index e25dd85a7391..f35af97b103d 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -47,6 +47,13 @@ LOG_DEFINE_CATEGORY(Virtual)
> >  
> >  namespace {
> >  
> > +constexpr PixelFormat outputFormats[] = {
> > +     formats::NV12,
> > +     formats::YUV420,
> > +     formats::ABGR8888,
> > +     formats::XBGR8888,
> > +};
> > +
> >  uint64_t currentTimestamp()
> >  {
> >       const auto now = std::chrono::steady_clock::now();
> > @@ -148,7 +155,7 @@ void VirtualCameraData::processRequest(Request *request)
> >                               found = true;
> >  
> >                               if (streamConfig.frameGenerator->generateFrame(
> > -                                         stream->configuration().size, buffer))
> > +                                         stream->configuration(), buffer))
> >                                       fmd.status = FrameMetadata::Status::FrameError;
> >  
> >                               bufferCompleted.emit(buffer);
> > @@ -202,18 +209,37 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
> >                       adjusted = true;
> >               }
> >  
> > -             if (cfg.pixelFormat != formats::NV12) {
> > -                     cfg.pixelFormat = formats::NV12;
> > +             if (std::find(std::begin(outputFormats),
> > +                           std::end(outputFormats),
> > +                           cfg.pixelFormat) == std::end(outputFormats)) {
> > +                     cfg.pixelFormat = outputFormats[0];
> >                       status = Adjusted;
> >                       adjusted = true;
> >               }
> >  
> > -             if (cfg.colorSpace != ColorSpace::Smpte170m) {
> > -                     cfg.colorSpace = ColorSpace::Smpte170m;
> > +             const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> > +             std::optional<ColorSpace> colorSpace;
> > +
> > +             switch (info.colourEncoding) {
> > +             case PixelFormatInfo::ColourEncodingRGB:
> > +                     colorSpace = ColorSpace::Srgb;
> > +                     break;
> > +
> > +             case PixelFormatInfo::ColourEncodingYUV:
> > +                     colorSpace = ColorSpace::Smpte170m;
> > +                     break;
> > +
> > +             default:
> > +                     ASSERT(false);
> > +             }
> > +
> > +             if (cfg.colorSpace && cfg.colorSpace != colorSpace) {
> >                       status = Adjusted;
> >                       adjusted = true;
> >               }
> >  
> > +             cfg.colorSpace = colorSpace;
> > +
> >               if (validateColorSpaces() == Adjusted) {
> >                       status = Adjusted;
> >                       adjusted = true;
> > @@ -223,7 +249,6 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
> >                       LOG(Virtual, Info)
> >                               << "Stream configuration adjusted to " << cfg.toString();
> >  
> > -             const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> >               cfg.stride = info.stride(cfg.size.width, 0, 1);
> >               cfg.frameSize = info.frameSize(cfg.size, 1);
> >  
> > @@ -275,12 +300,12 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> >               }
> >  
> >               std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > -             PixelFormat pixelFormat = formats::NV12;
> > -             streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
> > -                                              data->config_.maxResolutionSize } };
> > +             for (auto pixelFormat : outputFormats)
> > +                     streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
> > +                                                      data->config_.maxResolutionSize } };
> >               StreamFormats formats(streamFormats);
> >               StreamConfiguration cfg(formats);
> > -             cfg.pixelFormat = pixelFormat;
> > +             cfg.pixelFormat = outputFormats[0];
> >               cfg.size = data->config_.maxResolutionSize;
> >               cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> >               cfg.colorSpace = ColorSpace::Smpte170m;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Kieran Bingham Oct. 20, 2026, 11:47 a.m. UTC | #4 
I've made the local fixups and pushed to CI to retest, but they were
more substantial in volume than I expected, though I don't
expect/believe there to be any specific risk to the code.

Changes below

Quoting Laurent Pinchart (2026-10-20 00:20:12)
> On Tue, Oct 20, 2026 at 12:49:02AM +0200, Robert Mader wrote:
> > While NV12 is the most common native format for hardware video encoders,
> > YUV420 is the equivalent for most software video encoders. Thus
> > supporting both is great for testing.
> > 
> > Similarly having some RGB formats with and without alpha can be very
> > useful, especially as the swISP is currently limited to such formats.

I'll add:

The ImageFrameData members Y and UV are converted to lowercase as part
of the implementation.

> > 
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  .../pipeline/virtual/frame_generator.h        |  3 +-
> >  .../virtual/image_frame_generator.cpp         | 61 ++++++++++++++++---
> >  .../pipeline/virtual/image_frame_generator.h  |  2 +-
> >  .../virtual/test_pattern_generator.cpp        | 52 +++++++++++++---
> >  .../pipeline/virtual/test_pattern_generator.h |  2 +-
> >  src/libcamera/pipeline/virtual/virtual.cpp    | 45 +++++++++++---
> >  6 files changed, 135 insertions(+), 30 deletions(-)
> > 

Applying the following fixups to this patch:

So I worked through this and it became a big diff, which I think should
be squashed into this commit:

diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
index 7a9bc9ac1ebd..a368e8ed0b90 100644
--- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
+++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
@@ -115,8 +115,8 @@ void ImageFrameGenerator::configure(const Size &size)
                  * 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,
+		libyuv::NV12Scale(src.y.get(), src.size.width,
+				  src.uv.get(), src.size.width,
 				  src.size.width, src.size.height,
 				  scaledY.get(), size.width, scaledUV.get(), size.width,
 				  size.width, size.height, libyuv::FilterMode::kFilterBilinear);
@@ -143,52 +143,37 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra
 					    MappedFrameBuffer::MapFlag::Write);

 	const auto &planes = mappedFrameBuffer.planes();
-	const Size size = cfg.size;
+	const Size &size = cfg.size;
 	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);

 	/* Loop only around the number of images available */
 	frameIndex_ %= imageFrameDatas_.size();

 	int ret;
-	auto Y = scaledFrameDatas_[frameIndex_].Y.get();
-	auto UV = scaledFrameDatas_[frameIndex_].UV.get();
+	auto y = scaledFrameDatas_[frameIndex_].y.get();
+	auto uv = scaledFrameDatas_[frameIndex_].uv.get();

 	/* Write the scaledY and scaledUV to the mapped frame buffer */
 	switch (cfg.pixelFormat) {
 	case formats::NV12:
-		ret = libyuv::NV12Copy(Y,
-				       size.width,
-				       UV,
-				       size.width,
-				       planes[0].data(),
-				       info.stride(size.width, 0),
-				       planes[1].data(),
-				       info.stride(size.width, 1),
+		ret = libyuv::NV12Copy(y, size.width, uv, size.width,
+				       planes[0].data(), info.stride(size.width, 0),
+				       planes[1].data(), info.stride(size.width, 1),
 				       size.width, size.height);
 		break;

 	case formats::YUV420:
-		ret = libyuv::NV12ToI420(Y,
-					 size.width,
-					 UV,
-					 size.width,
-					 planes[0].data(),
-					 info.stride(size.width, 0),
-					 planes[1].data(),
-					 info.stride(size.width, 1),
-					 planes[2].data(),
-					 info.stride(size.width, 2),
+		ret = libyuv::NV12ToI420(y, size.width, uv, size.width,
+					 planes[0].data(), info.stride(size.width, 0),
+					 planes[1].data(), info.stride(size.width, 1),
+					 planes[2].data(), info.stride(size.width, 2),
 					 size.width, size.height);
 		break;

 	case formats::ABGR8888:
 	case formats::XBGR8888:
-		ret = libyuv::NV12ToABGR(Y,
-					 size.width,
-					 UV,
-					 size.width,
-					 planes[0].data(),
-					 info.stride(size.width, 0),
+		ret = libyuv::NV12ToABGR(y, size.width, uv, size.width,
+					 planes[0].data(), info.stride(size.width, 0),
 					 size.width, size.height);
 		break;

@@ -198,7 +183,7 @@ int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const Fra
 	}

 	if (ret != 0)
-		LOG(Virtual, Error) << "Conversation failed with " << ret;
+		LOG(Virtual, Error) << "Conversion failed with " << ret;

 	/* Proceed to the next image every 4 frames */
 	/* \todo Consider setting the frameRepeat in the config file  */
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h
index 19b6fe38654e..3c70c34b9042 100644
--- a/src/libcamera/pipeline/virtual/image_frame_generator.h
+++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
@@ -31,8 +31,8 @@ private:
 	static constexpr unsigned int frameRepeat = 4;

 	struct ImageFrameData {
-		std::unique_ptr<uint8_t[]> Y;
-		std::unique_ptr<uint8_t[]> UV;
+		std::unique_ptr<uint8_t[]> y;
+		std::unique_ptr<uint8_t[]> uv;
 		Size size;
 	};

diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
index e214048d288a..9e9bc2c5bfef 100644
--- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
+++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
@@ -60,7 +60,7 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
 					    MappedFrameBuffer::MapFlag::Write);

 	const auto &planes = mappedFrameBuffer.planes();
-	const Size size = cfg.size;
+	const Size &size = cfg.size;
 	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);

 	rotateLeft1Column<kARGBSize>(size, template_.get());
@@ -72,22 +72,17 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
 	case formats::NV12:
 		ret = libyuv::ARGBToNV12(template_.get(),
 					 size.width * kARGBSize,
-					 planes[0].data(),
-					 info.stride(size.width, 0),
-					 planes[1].data(),
-					 info.stride(size.width, 1),
+					 planes[0].data(), info.stride(size.width, 0),
+					 planes[1].data(), info.stride(size.width, 1),
 					 size.width, size.height);
 		break;

 	case formats::YUV420:
 		ret = libyuv::ARGBToI420(template_.get(),
 					 size.width * kARGBSize,
-					 planes[0].data(),
-					 info.stride(size.width, 0),
-					 planes[1].data(),
-					 info.stride(size.width, 1),
-					 planes[2].data(),
-					 info.stride(size.width, 2),
+					 planes[0].data(), info.stride(size.width, 0),
+					 planes[1].data(), info.stride(size.width, 1),
+					 planes[2].data(), info.stride(size.width, 2),
 					 size.width, size.height);
 		break;

@@ -95,8 +90,7 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
 	case formats::XBGR8888:
 		ret = libyuv::ARGBCopy(template_.get(),
 				       size.width * kARGBSize,
-				       planes[0].data(),
-				       info.stride(size.width, 0),
+				       planes[0].data(), info.stride(size.width, 0),
 				       size.width, size.height);
 		break;

@@ -106,7 +100,7 @@ int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
 	}

 	if (ret != 0)
-		LOG(Virtual, Error) << "Conversation failed with " << ret;
+		LOG(Virtual, Error) << "Conversion failed with " << ret;

 	return ret;
 }

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/virtual/frame_generator.h b/src/libcamera/pipeline/virtual/frame_generator.h
index a0658c45cd82..fd34a91a5e50 100644
--- a/src/libcamera/pipeline/virtual/frame_generator.h
+++ b/src/libcamera/pipeline/virtual/frame_generator.h
@@ -9,6 +9,7 @@ 
 
 #include <libcamera/framebuffer.h>
 #include <libcamera/geometry.h>
+#include <libcamera/stream.h>
 
 namespace libcamera {
 
@@ -19,7 +20,7 @@  public:
 
 	virtual void configure(const Size &size) = 0;
 
-	virtual int generateFrame(const Size &size,
+	virtual int generateFrame(const StreamConfiguration &cfg,
 				  const FrameBuffer *buffer) = 0;
 
 protected:
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
index 5caad8b72763..7a9bc9ac1ebd 100644
--- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
+++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
@@ -126,7 +126,7 @@  void ImageFrameGenerator::configure(const Size &size)
 	}
 }
 
-int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buffer)
+int ImageFrameGenerator::generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer)
 {
 	ASSERT(!scaledFrameDatas_.empty());
 
@@ -143,17 +143,62 @@  int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
 					    MappedFrameBuffer::MapFlag::Write);
 
 	const auto &planes = mappedFrameBuffer.planes();
-	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
+	const Size size = cfg.size;
+	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
 
 	/* Loop only around the number of images available */
 	frameIndex_ %= imageFrameDatas_.size();
 
+	int ret;
+	auto Y = scaledFrameDatas_[frameIndex_].Y.get();
+	auto UV = scaledFrameDatas_[frameIndex_].UV.get();
+
 	/* Write the scaledY and scaledUV to the mapped frame buffer */
-	libyuv::NV12Copy(scaledFrameDatas_[frameIndex_].Y.get(), size.width,
-			 scaledFrameDatas_[frameIndex_].UV.get(), size.width,
-			 planes[0].data(), info.stride(size.width, 0),
-			 planes[1].data(), info.stride(size.width, 1),
-			 size.width, size.height);
+	switch (cfg.pixelFormat) {
+	case formats::NV12:
+		ret = libyuv::NV12Copy(Y,
+				       size.width,
+				       UV,
+				       size.width,
+				       planes[0].data(),
+				       info.stride(size.width, 0),
+				       planes[1].data(),
+				       info.stride(size.width, 1),
+				       size.width, size.height);
+		break;
+
+	case formats::YUV420:
+		ret = libyuv::NV12ToI420(Y,
+					 size.width,
+					 UV,
+					 size.width,
+					 planes[0].data(),
+					 info.stride(size.width, 0),
+					 planes[1].data(),
+					 info.stride(size.width, 1),
+					 planes[2].data(),
+					 info.stride(size.width, 2),
+					 size.width, size.height);
+		break;
+
+	case formats::ABGR8888:
+	case formats::XBGR8888:
+		ret = libyuv::NV12ToABGR(Y,
+					 size.width,
+					 UV,
+					 size.width,
+					 planes[0].data(),
+					 info.stride(size.width, 0),
+					 size.width, size.height);
+		break;
+
+	default:
+		LOG(Virtual, Error) << "Unsupported format";
+		return -1;
+	}
+
+	if (ret != 0)
+		LOG(Virtual, Error) << "Conversation failed with " << ret;
 
 	/* Proceed to the next image every 4 frames */
 	/* \todo Consider setting the frameRepeat in the config file  */
@@ -161,7 +206,7 @@  int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff
 	if (parameter_ % frameRepeat == 0)
 		frameIndex_++;
 
-	return 0;
+	return ret;
 }
 
 /*
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h
index 42a077ba803c..19b6fe38654e 100644
--- a/src/libcamera/pipeline/virtual/image_frame_generator.h
+++ b/src/libcamera/pipeline/virtual/image_frame_generator.h
@@ -37,7 +37,7 @@  private:
 	};
 
 	void configure(const Size &size) override;
-	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
+	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
 
 	std::vector<ImageFrameData> imageFrameDatas_;
 	std::vector<ImageFrameData> scaledFrameDatas_;
diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
index 571be4fbf65d..e214048d288a 100644
--- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
+++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp
@@ -44,7 +44,7 @@  LOG_DECLARE_CATEGORY(Virtual)
 
 static const unsigned int kARGBSize = 4;
 
-int TestPatternGenerator::generateFrame(const Size &size,
+int TestPatternGenerator::generateFrame(const StreamConfiguration &cfg,
 					const FrameBuffer *buffer)
 {
 	/*
@@ -60,19 +60,53 @@  int TestPatternGenerator::generateFrame(const Size &size,
 					    MappedFrameBuffer::MapFlag::Write);
 
 	const auto &planes = mappedFrameBuffer.planes();
-	const PixelFormatInfo &info = PixelFormatInfo::info(formats::NV12);
+	const Size size = cfg.size;
+	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
 
 	rotateLeft1Column<kARGBSize>(size, template_.get());
 
+	int ret;
+
 	/* Convert the template_ to the frame buffer */
-	int ret = libyuv::ARGBToNV12(template_.get(), size.width * kARGBSize,
-				     planes[0].data(),
-				     info.stride(size.width, 0),
-				     planes[1].data(),
-				     info.stride(size.width, 1),
-				     size.width, size.height);
+	switch (cfg.pixelFormat) {
+	case formats::NV12:
+		ret = libyuv::ARGBToNV12(template_.get(),
+					 size.width * kARGBSize,
+					 planes[0].data(),
+					 info.stride(size.width, 0),
+					 planes[1].data(),
+					 info.stride(size.width, 1),
+					 size.width, size.height);
+		break;
+
+	case formats::YUV420:
+		ret = libyuv::ARGBToI420(template_.get(),
+					 size.width * kARGBSize,
+					 planes[0].data(),
+					 info.stride(size.width, 0),
+					 planes[1].data(),
+					 info.stride(size.width, 1),
+					 planes[2].data(),
+					 info.stride(size.width, 2),
+					 size.width, size.height);
+		break;
+
+	case formats::ABGR8888:
+	case formats::XBGR8888:
+		ret = libyuv::ARGBCopy(template_.get(),
+				       size.width * kARGBSize,
+				       planes[0].data(),
+				       info.stride(size.width, 0),
+				       size.width, size.height);
+		break;
+
+	default:
+		LOG(Virtual, Error) << "Unsupported format";
+		return -1;
+	}
+
 	if (ret != 0)
-		LOG(Virtual, Error) << "ARGBToNV12() failed with " << ret;
+		LOG(Virtual, Error) << "Conversation failed with " << ret;
 
 	return ret;
 }
diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.h b/src/libcamera/pipeline/virtual/test_pattern_generator.h
index 2a51bd31abdf..9f72d652ab2c 100644
--- a/src/libcamera/pipeline/virtual/test_pattern_generator.h
+++ b/src/libcamera/pipeline/virtual/test_pattern_generator.h
@@ -24,7 +24,7 @@  enum class TestPattern : char {
 class TestPatternGenerator : public FrameGenerator
 {
 public:
-	int generateFrame(const Size &size, const FrameBuffer *buffer) override;
+	int generateFrame(const StreamConfiguration &cfg, const FrameBuffer *buffer) override;
 
 protected:
 	/* Buffer of test pattern template */
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index e25dd85a7391..f35af97b103d 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -47,6 +47,13 @@  LOG_DEFINE_CATEGORY(Virtual)
 
 namespace {
 
+constexpr PixelFormat outputFormats[] = {
+	formats::NV12,
+	formats::YUV420,
+	formats::ABGR8888,
+	formats::XBGR8888,
+};
+
 uint64_t currentTimestamp()
 {
 	const auto now = std::chrono::steady_clock::now();
@@ -148,7 +155,7 @@  void VirtualCameraData::processRequest(Request *request)
 				found = true;
 
 				if (streamConfig.frameGenerator->generateFrame(
-					    stream->configuration().size, buffer))
+					    stream->configuration(), buffer))
 					fmd.status = FrameMetadata::Status::FrameError;
 
 				bufferCompleted.emit(buffer);
@@ -202,18 +209,37 @@  CameraConfiguration::Status VirtualCameraConfiguration::validate()
 			adjusted = true;
 		}
 
-		if (cfg.pixelFormat != formats::NV12) {
-			cfg.pixelFormat = formats::NV12;
+		if (std::find(std::begin(outputFormats),
+			      std::end(outputFormats),
+			      cfg.pixelFormat) == std::end(outputFormats)) {
+			cfg.pixelFormat = outputFormats[0];
 			status = Adjusted;
 			adjusted = true;
 		}
 
-		if (cfg.colorSpace != ColorSpace::Smpte170m) {
-			cfg.colorSpace = ColorSpace::Smpte170m;
+		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
+		std::optional<ColorSpace> colorSpace;
+
+		switch (info.colourEncoding) {
+		case PixelFormatInfo::ColourEncodingRGB:
+			colorSpace = ColorSpace::Srgb;
+			break;
+
+		case PixelFormatInfo::ColourEncodingYUV:
+			colorSpace = ColorSpace::Smpte170m;
+			break;
+
+		default:
+			ASSERT(false);
+		}
+
+		if (cfg.colorSpace && cfg.colorSpace != colorSpace) {
 			status = Adjusted;
 			adjusted = true;
 		}
 
+		cfg.colorSpace = colorSpace;
+
 		if (validateColorSpaces() == Adjusted) {
 			status = Adjusted;
 			adjusted = true;
@@ -223,7 +249,6 @@  CameraConfiguration::Status VirtualCameraConfiguration::validate()
 			LOG(Virtual, Info)
 				<< "Stream configuration adjusted to " << cfg.toString();
 
-		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
 		cfg.stride = info.stride(cfg.size.width, 0, 1);
 		cfg.frameSize = info.frameSize(cfg.size, 1);
 
@@ -275,12 +300,12 @@  PipelineHandlerVirtual::generateConfiguration(Camera *camera,
 		}
 
 		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
-		PixelFormat pixelFormat = formats::NV12;
-		streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
-						 data->config_.maxResolutionSize } };
+		for (auto pixelFormat : outputFormats)
+			streamFormats[pixelFormat] = { { data->config_.minResolutionSize,
+							 data->config_.maxResolutionSize } };
 		StreamFormats formats(streamFormats);
 		StreamConfiguration cfg(formats);
-		cfg.pixelFormat = pixelFormat;
+		cfg.pixelFormat = outputFormats[0];
 		cfg.size = data->config_.maxResolutionSize;
 		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
 		cfg.colorSpace = ColorSpace::Smpte170m;