[v3,09/17] libcamera: converter: Add functions to adjust config

22201 diff mbox series

From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Add to the Converter interface two functions used by pipeline handlers
to validate and adjust the converter input and output configurations
by specifying the desired alignment for the adjustment.

Add the adjustInputSize() and adjustOutputSize() functions that allows
to adjust the converter input/output sizes with the desired alignment.

Add a validateOutput() function meant to be used by the pipeline
handler implementations of validate(). The function adjusts a
StreamConfiguration to a valid configuration produced by the Converter.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

---
Changes in v3:
- Added this patch
---
 include/libcamera/internal/converter.h        |  17 ++
 .../internal/converter/converter_v4l2_m2m.h   |  11 ++
 src/libcamera/converter.cpp                   |  43 +++++
 .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
 4 files changed, 240 insertions(+)

Hi Jacopo,

Thank you for the patch :-)

On Fri, Dec 06, 2024 at 11:13:31AM +0100, Stefan Klug wrote:
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Add to the Converter interface two functions used by pipeline handlers
> to validate and adjust the converter input and output configurations
> by specifying the desired alignment for the adjustment.
> 
> Add the adjustInputSize() and adjustOutputSize() functions that allows
> to adjust the converter input/output sizes with the desired alignment.
> 
> Add a validateOutput() function meant to be used by the pipeline
> handler implementations of validate(). The function adjusts a
> StreamConfiguration to a valid configuration produced by the Converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> ---
> Changes in v3:
> - Added this patch
> ---
>  include/libcamera/internal/converter.h        |  17 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  11 ++
>  src/libcamera/converter.cpp                   |  43 +++++
>  .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
>  4 files changed, 240 insertions(+)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index ffbb6f345cd5..9213ae5b9c33 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -41,6 +41,13 @@ public:
>  
>  	using Features = Flags<Feature>;
>  
> +	enum class Alignment {
> +		Down = 0,
> +		Up = (1 << 0),
> +	};
> +
> +	using Alignments = Flags<Alignment>;
> +
>  	Converter(MediaDevice *media, Features features = Feature::None);
>  	virtual ~Converter();
>  
> @@ -51,9 +58,19 @@ public:
>  	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
>  	virtual SizeRange sizes(const Size &input) = 0;
>  
> +	virtual Size adjustInputSize(const PixelFormat &pixFmt,
> +				     const Size &size,
> +				     Alignments align = Alignment::Down) = 0;
> +	virtual Size adjustOutputSize(const PixelFormat &pixFmt,
> +				      const Size &size,
> +				      Alignments align = Alignment::Down) = 0;
> +
>  	virtual std::tuple<unsigned int, unsigned int>
>  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
>  
> +	virtual int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +				   Alignments align = Alignment::Down) = 0;
> +

I'm wondering when to use non-const references and when a pointer. Is
there a reason cfg is a pointer?

>  	virtual int configure(const StreamConfiguration &inputCfg,
>  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
>  	virtual int exportBuffers(const Stream *stream, unsigned int count,
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index a5286166f574..89bd2878b190 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -47,6 +47,11 @@ public:
>  	std::tuple<unsigned int, unsigned int>
>  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
>  
> +	Size adjustInputSize(const PixelFormat &pixFmt,
> +			     const Size &size, Alignments align = Alignment::Down) override;
> +	Size adjustOutputSize(const PixelFormat &pixFmt,
> +			      const Size &size, Alignments align = Alignment::Down) override;
> +
>  	int configure(const StreamConfiguration &inputCfg,
>  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
>  	int exportBuffers(const Stream *stream, unsigned int count,
> @@ -55,6 +60,9 @@ public:
>  	int start();
>  	void stop();
>  
> +	int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +			   Alignments align = Alignment::Down) override;
> +
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>  
> @@ -101,6 +109,9 @@ private:
>  		std::pair<Rectangle, Rectangle> inputCropBounds_;
>  	};
>  
> +	Size adjustSizes(const Size &size, const std::vector<SizeRange> &ranges,
> +			 Alignments align);
> +
>  	std::unique_ptr<V4L2M2MDevice> m2m_;
>  
>  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index aa16970cd446..c3da162b7de7 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -50,6 +50,21 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief A bitwise combination of features supported by the converter
>   */
>  
> +/**
> + * \enum Converter::Alignment
> + * \brief The alignment mode specified when adjusting the converter input or
> + * output sizes
> + * \var Converter::Alignment::Down
> + * \brief Adjust the Converter sizes to a smaller valid size
> + * \var Converter::Alignment::Up
> + * \brief Adjust the Converter sizes to a larger valid size
> + */
> +
> +/**
> + * \typedef Converter::Alignments
> + * \brief A bitwise combination of alignments supported by the converter
> + */
> +
>  /**
>   * \brief Construct a Converter instance
>   * \param[in] media The media device implementing the converter
> @@ -110,6 +125,24 @@ Converter::~Converter()
>   * \return A range of output image sizes
>   */
>  
> +/**
> + * \fn Converter::adjustInputSize()
> + * \brief Adjust the converter input \a size to a valid value
> + * \param[in] pixFmt The pixel format of the converter input stream
> + * \param[in] size The converter input size to adjust to a valid value
> + * \param[in] align The desired alignment
> + * \return The adjusted converter input size
> + */

What gets returned if there is no valid side for the request?

My comments where all nits. So

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

> +
> +/**
> + * \fn Converter::adjustOutputSize()
> + * \brief Adjust the converter output \a size to a valid value
> + * \param[in] pixFmt The pixel format of the converter output stream
> + * \param[in] size The converter output size to adjust to a valid value
> + * \param[in] align The desired alignment
> + * \return The adjusted converter output size
> + */
> +
>  /**
>   * \fn Converter::strideAndFrameSize()
>   * \brief Retrieve the output stride and frame size for an input configutation
> @@ -118,6 +151,16 @@ Converter::~Converter()
>   * \return A tuple indicating the stride and frame size or an empty tuple on error
>   */
>  
> +/**
> + * \fn Converter::validateOutput()
> + * \brief Validate and possibily adjust \a cfg to a valid converter output
> + * \param[inout] cfg The StreamConfiguration to validate and adjust
> + * \param[out] adjusted Set to true if \a cfg has been adjusted
> + * \param[in] align The desired alignment
> + * \return 0 if \a cfg is valid or has been adjusted, a negative error code
> + * otherwise if \a cfg cannot be adjusted
> + */
> +
>  /**
>   * \fn Converter::configure()
>   * \brief Configure a set of output stream conversion from an input stream
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index bd7e5cce600d..6857472b29f2 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -8,6 +8,7 @@
>  
>  #include "libcamera/internal/converter/converter_v4l2_m2m.h"
>  
> +#include <algorithm>
>  #include <limits.h>
>  
>  #include <libcamera/base/log.h>
> @@ -400,6 +401,127 @@ V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>  	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::adjustInputSize
> + */
> +Size V4L2M2MConverter::adjustInputSize(const PixelFormat &pixFmt,
> +				       const Size &size, Alignments align)
> +{
> +	auto formats = m2m_->output()->formats();
> +	V4L2PixelFormat v4l2PixFmt = m2m_->output()->toV4L2PixelFormat(pixFmt);
> +
> +	auto it = formats.find(v4l2PixFmt);
> +	if (it == formats.end()) {
> +		LOG(Converter, Info)
> +			<< "Unsupported pixel format " << pixFmt;
> +		return {};
> +	}
> +
> +	return adjustSizes(size, it->second, align);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::adjustOutputSize
> + */
> +Size V4L2M2MConverter::adjustOutputSize(const PixelFormat &pixFmt,
> +					const Size &size, Alignments align)
> +{
> +	auto formats = m2m_->capture()->formats();
> +	V4L2PixelFormat v4l2PixFmt = m2m_->capture()->toV4L2PixelFormat(pixFmt);
> +
> +	auto it = formats.find(v4l2PixFmt);
> +	if (it == formats.end()) {
> +		LOG(Converter, Info)
> +			<< "Unsupported pixel format " << pixFmt;
> +		return {};
> +	}
> +
> +	return adjustSizes(size, it->second, align);
> +}
> +
> +Size V4L2M2MConverter::adjustSizes(const Size &cfgSize,
> +				   const std::vector<SizeRange> &ranges,
> +				   Alignments align)
> +{
> +	Size size = cfgSize;
> +
> +	if (ranges.size() == 1) {
> +		/*
> +		 * The device supports either V4L2_FRMSIZE_TYPE_CONTINUOUS or
> +		 * V4L2_FRMSIZE_TYPE_STEPWISE.
> +		 */
> +		const SizeRange &range = *ranges.begin();
> +
> +		size.width = std::clamp(size.width, range.min.width,
> +					range.max.width);
> +		size.height = std::clamp(size.height, range.min.height,
> +					 range.max.height);
> +
> +		/*
> +		 * Check if any alignment is needed. If the sizes are already
> +		 * aligned, or the device supports V4L2_FRMSIZE_TYPE_CONTINUOUS
> +		 * with hStep and vStep equal to 1, we're done here.
> +		 */
> +		int widthR = size.width % range.hStep;
> +		int heightR = size.height % range.vStep;
> +
> +		/* Align up or down according to the caller request. */
> +
> +		if (widthR != 0)
> +			size.width = size.width - widthR
> +				   + ((align == Alignment::Up) ? range.hStep : 0);
> +
> +		if (heightR != 0)
> +			size.height = size.height - heightR
> +				    + ((align == Alignment::Up) ? range.vStep : 0);
> +	} else {
> +		/*
> +		 * The device supports V4L2_FRMSIZE_TYPE_DISCRETE, find the
> +		 * size closer to the requested output configuration.
> +		 *
> +		 * The size ranges vector is not ordered, so we sort it first.
> +		 * If we align up, start from the larger element.
> +		 */
> +		std::vector<Size> sizes(ranges.size());
> +		std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> +			       [](const SizeRange &range) { return range.max; });
> +		std::sort(sizes.begin(), sizes.end());
> +
> +		if (align == Alignment::Up)
> +			std::reverse(sizes.begin(), sizes.end());
> +
> +		/*
> +		 * Return true if s2 is valid according to the desired
> +		 * alignment: smaller than s1 if we align down, larger than s1
> +		 * if we align up.
> +		 */
> +		auto nextSizeValid = [](const Size &s1, const Size &s2, Alignments a) {
> +			return a == Alignment::Down
> +				? (s1.width > s2.width && s1.height > s2.height)
> +				: (s1.width < s2.width && s1.height < s2.height);
> +		};
> +
> +		Size newSize;
> +		for (const Size &sz : sizes) {
> +			if (!nextSizeValid(size, sz, align))
> +				break;
> +
> +			newSize = sz;
> +		}
> +
> +		if (newSize.isNull()) {
> +			LOG(Converter, Error)
> +				<< "Cannot adjust " << cfgSize
> +				<< " to a supported converter size";
> +			return {};
> +		}
> +
> +		size = newSize;
> +	}
> +
> +	return size;
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::configure
>   */
> @@ -507,6 +629,53 @@ void V4L2M2MConverter::stop()
>  		iter.second->stop();
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::validateOutput
> + */
> +int V4L2M2MConverter::validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +				     Alignments align)
> +{
> +	V4L2VideoDevice *capture = m2m_->capture();
> +	V4L2VideoDevice::Formats fmts = capture->formats();
> +
> +	if (adjusted)
> +		*adjusted = false;
> +
> +	PixelFormat fmt = cfg->pixelFormat;
> +	V4L2PixelFormat v4l2PixFmt = capture->toV4L2PixelFormat(fmt);
> +
> +	auto it = fmts.find(v4l2PixFmt);
> +	if (it == fmts.end()) {
> +		it = fmts.begin();
> +		v4l2PixFmt = it->first;
> +		cfg->pixelFormat = v4l2PixFmt.toPixelFormat();
> +
> +		if (adjusted)
> +			*adjusted = true;
> +
> +		LOG(Converter, Info)
> +			<< "Converter output pixel format adjusted to "
> +			<< cfg->pixelFormat;
> +	}
> +
> +	const Size cfgSize = cfg->size;
> +	cfg->size = adjustSizes(cfgSize, it->second, align);
> +
> +	if (cfg->size.isNull())
> +		return -EINVAL;
> +
> +	if (cfg->size.width != cfgSize.width ||
> +	    cfg->size.height != cfgSize.height) {
> +		LOG(Converter, Info)
> +			<< "Converter size adjusted to "
> +			<< cfg->size;
> +		if (adjusted)
> +			*adjusted = true;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::queueBuffers
>   */
> -- 
> 2.43.0
>

On Fri, Dec 06, 2024 at 11:13:31AM +0100, Stefan Klug wrote:
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Add to the Converter interface two functions used by pipeline handlers
> to validate and adjust the converter input and output configurations
> by specifying the desired alignment for the adjustment.
> 
> Add the adjustInputSize() and adjustOutputSize() functions that allows
> to adjust the converter input/output sizes with the desired alignment.
> 
> Add a validateOutput() function meant to be used by the pipeline
> handler implementations of validate(). The function adjusts a
> StreamConfiguration to a valid configuration produced by the Converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> ---
> Changes in v3:
> - Added this patch
> ---
>  include/libcamera/internal/converter.h        |  17 ++
>  .../internal/converter/converter_v4l2_m2m.h   |  11 ++
>  src/libcamera/converter.cpp                   |  43 +++++
>  .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
>  4 files changed, 240 insertions(+)
> 
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index ffbb6f345cd5..9213ae5b9c33 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -41,6 +41,13 @@ public:
>  
>  	using Features = Flags<Feature>;
>  
> +	enum class Alignment {
> +		Down = 0,
> +		Up = (1 << 0),
> +	};
> +
> +	using Alignments = Flags<Alignment>;
> +
>  	Converter(MediaDevice *media, Features features = Feature::None);
>  	virtual ~Converter();
>  
> @@ -51,9 +58,19 @@ public:
>  	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
>  	virtual SizeRange sizes(const Size &input) = 0;
>  
> +	virtual Size adjustInputSize(const PixelFormat &pixFmt,
> +				     const Size &size,
> +				     Alignments align = Alignment::Down) = 0;
> +	virtual Size adjustOutputSize(const PixelFormat &pixFmt,
> +				      const Size &size,
> +				      Alignments align = Alignment::Down) = 0;
> +
>  	virtual std::tuple<unsigned int, unsigned int>
>  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
>  
> +	virtual int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +				   Alignments align = Alignment::Down) = 0;

I'm confused. Here (and everywhere else it's used), since the type is
Alignments (Flags<Alignment>), doesn't that mean that
Alignment::Down | Alignment::Up is a valid input? Wouldn't that cause
unexpected behavior?

Although I suppose if Alignment::Down | Alignment::Up was actually
passed then it wouldn't be equal Alignment::Down nor Alignment::Up so
nothing would happen? Except maybe the ternary operators ig, those would
just activate the false clause.

Am I missing something here?


Paul


> +
>  	virtual int configure(const StreamConfiguration &inputCfg,
>  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
>  	virtual int exportBuffers(const Stream *stream, unsigned int count,
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index a5286166f574..89bd2878b190 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -47,6 +47,11 @@ public:
>  	std::tuple<unsigned int, unsigned int>
>  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
>  
> +	Size adjustInputSize(const PixelFormat &pixFmt,
> +			     const Size &size, Alignments align = Alignment::Down) override;
> +	Size adjustOutputSize(const PixelFormat &pixFmt,
> +			      const Size &size, Alignments align = Alignment::Down) override;
> +
>  	int configure(const StreamConfiguration &inputCfg,
>  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
>  	int exportBuffers(const Stream *stream, unsigned int count,
> @@ -55,6 +60,9 @@ public:
>  	int start();
>  	void stop();
>  
> +	int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +			   Alignments align = Alignment::Down) override;
> +
>  	int queueBuffers(FrameBuffer *input,
>  			 const std::map<const Stream *, FrameBuffer *> &outputs);
>  
> @@ -101,6 +109,9 @@ private:
>  		std::pair<Rectangle, Rectangle> inputCropBounds_;
>  	};
>  
> +	Size adjustSizes(const Size &size, const std::vector<SizeRange> &ranges,
> +			 Alignments align);
> +
>  	std::unique_ptr<V4L2M2MDevice> m2m_;
>  
>  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index aa16970cd446..c3da162b7de7 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -50,6 +50,21 @@ LOG_DEFINE_CATEGORY(Converter)
>   * \brief A bitwise combination of features supported by the converter
>   */
>  
> +/**
> + * \enum Converter::Alignment
> + * \brief The alignment mode specified when adjusting the converter input or
> + * output sizes
> + * \var Converter::Alignment::Down
> + * \brief Adjust the Converter sizes to a smaller valid size
> + * \var Converter::Alignment::Up
> + * \brief Adjust the Converter sizes to a larger valid size
> + */
> +
> +/**
> + * \typedef Converter::Alignments
> + * \brief A bitwise combination of alignments supported by the converter
> + */
> +
>  /**
>   * \brief Construct a Converter instance
>   * \param[in] media The media device implementing the converter
> @@ -110,6 +125,24 @@ Converter::~Converter()
>   * \return A range of output image sizes
>   */
>  
> +/**
> + * \fn Converter::adjustInputSize()
> + * \brief Adjust the converter input \a size to a valid value
> + * \param[in] pixFmt The pixel format of the converter input stream
> + * \param[in] size The converter input size to adjust to a valid value
> + * \param[in] align The desired alignment
> + * \return The adjusted converter input size
> + */
> +
> +/**
> + * \fn Converter::adjustOutputSize()
> + * \brief Adjust the converter output \a size to a valid value
> + * \param[in] pixFmt The pixel format of the converter output stream
> + * \param[in] size The converter output size to adjust to a valid value
> + * \param[in] align The desired alignment
> + * \return The adjusted converter output size
> + */
> +
>  /**
>   * \fn Converter::strideAndFrameSize()
>   * \brief Retrieve the output stride and frame size for an input configutation
> @@ -118,6 +151,16 @@ Converter::~Converter()
>   * \return A tuple indicating the stride and frame size or an empty tuple on error
>   */
>  
> +/**
> + * \fn Converter::validateOutput()
> + * \brief Validate and possibily adjust \a cfg to a valid converter output
> + * \param[inout] cfg The StreamConfiguration to validate and adjust
> + * \param[out] adjusted Set to true if \a cfg has been adjusted
> + * \param[in] align The desired alignment
> + * \return 0 if \a cfg is valid or has been adjusted, a negative error code
> + * otherwise if \a cfg cannot be adjusted
> + */
> +
>  /**
>   * \fn Converter::configure()
>   * \brief Configure a set of output stream conversion from an input stream
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index bd7e5cce600d..6857472b29f2 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -8,6 +8,7 @@
>  
>  #include "libcamera/internal/converter/converter_v4l2_m2m.h"
>  
> +#include <algorithm>
>  #include <limits.h>
>  
>  #include <libcamera/base/log.h>
> @@ -400,6 +401,127 @@ V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>  	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::adjustInputSize
> + */
> +Size V4L2M2MConverter::adjustInputSize(const PixelFormat &pixFmt,
> +				       const Size &size, Alignments align)
> +{
> +	auto formats = m2m_->output()->formats();
> +	V4L2PixelFormat v4l2PixFmt = m2m_->output()->toV4L2PixelFormat(pixFmt);
> +
> +	auto it = formats.find(v4l2PixFmt);
> +	if (it == formats.end()) {
> +		LOG(Converter, Info)
> +			<< "Unsupported pixel format " << pixFmt;
> +		return {};
> +	}
> +
> +	return adjustSizes(size, it->second, align);
> +}
> +
> +/**
> + * \copydoc libcamera::Converter::adjustOutputSize
> + */
> +Size V4L2M2MConverter::adjustOutputSize(const PixelFormat &pixFmt,
> +					const Size &size, Alignments align)
> +{
> +	auto formats = m2m_->capture()->formats();
> +	V4L2PixelFormat v4l2PixFmt = m2m_->capture()->toV4L2PixelFormat(pixFmt);
> +
> +	auto it = formats.find(v4l2PixFmt);
> +	if (it == formats.end()) {
> +		LOG(Converter, Info)
> +			<< "Unsupported pixel format " << pixFmt;
> +		return {};
> +	}
> +
> +	return adjustSizes(size, it->second, align);
> +}
> +
> +Size V4L2M2MConverter::adjustSizes(const Size &cfgSize,
> +				   const std::vector<SizeRange> &ranges,
> +				   Alignments align)
> +{
> +	Size size = cfgSize;
> +
> +	if (ranges.size() == 1) {
> +		/*
> +		 * The device supports either V4L2_FRMSIZE_TYPE_CONTINUOUS or
> +		 * V4L2_FRMSIZE_TYPE_STEPWISE.
> +		 */
> +		const SizeRange &range = *ranges.begin();
> +
> +		size.width = std::clamp(size.width, range.min.width,
> +					range.max.width);
> +		size.height = std::clamp(size.height, range.min.height,
> +					 range.max.height);
> +
> +		/*
> +		 * Check if any alignment is needed. If the sizes are already
> +		 * aligned, or the device supports V4L2_FRMSIZE_TYPE_CONTINUOUS
> +		 * with hStep and vStep equal to 1, we're done here.
> +		 */
> +		int widthR = size.width % range.hStep;
> +		int heightR = size.height % range.vStep;
> +
> +		/* Align up or down according to the caller request. */
> +
> +		if (widthR != 0)
> +			size.width = size.width - widthR
> +				   + ((align == Alignment::Up) ? range.hStep : 0);
> +
> +		if (heightR != 0)
> +			size.height = size.height - heightR
> +				    + ((align == Alignment::Up) ? range.vStep : 0);
> +	} else {
> +		/*
> +		 * The device supports V4L2_FRMSIZE_TYPE_DISCRETE, find the
> +		 * size closer to the requested output configuration.
> +		 *
> +		 * The size ranges vector is not ordered, so we sort it first.
> +		 * If we align up, start from the larger element.
> +		 */
> +		std::vector<Size> sizes(ranges.size());
> +		std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> +			       [](const SizeRange &range) { return range.max; });
> +		std::sort(sizes.begin(), sizes.end());
> +
> +		if (align == Alignment::Up)
> +			std::reverse(sizes.begin(), sizes.end());
> +
> +		/*
> +		 * Return true if s2 is valid according to the desired
> +		 * alignment: smaller than s1 if we align down, larger than s1
> +		 * if we align up.
> +		 */
> +		auto nextSizeValid = [](const Size &s1, const Size &s2, Alignments a) {
> +			return a == Alignment::Down
> +				? (s1.width > s2.width && s1.height > s2.height)
> +				: (s1.width < s2.width && s1.height < s2.height);
> +		};
> +
> +		Size newSize;
> +		for (const Size &sz : sizes) {
> +			if (!nextSizeValid(size, sz, align))
> +				break;
> +
> +			newSize = sz;
> +		}
> +
> +		if (newSize.isNull()) {
> +			LOG(Converter, Error)
> +				<< "Cannot adjust " << cfgSize
> +				<< " to a supported converter size";
> +			return {};
> +		}
> +
> +		size = newSize;
> +	}
> +
> +	return size;
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::configure
>   */
> @@ -507,6 +629,53 @@ void V4L2M2MConverter::stop()
>  		iter.second->stop();
>  }
>  
> +/**
> + * \copydoc libcamera::Converter::validateOutput
> + */
> +int V4L2M2MConverter::validateOutput(StreamConfiguration *cfg, bool *adjusted,
> +				     Alignments align)
> +{
> +	V4L2VideoDevice *capture = m2m_->capture();
> +	V4L2VideoDevice::Formats fmts = capture->formats();
> +
> +	if (adjusted)
> +		*adjusted = false;
> +
> +	PixelFormat fmt = cfg->pixelFormat;
> +	V4L2PixelFormat v4l2PixFmt = capture->toV4L2PixelFormat(fmt);
> +
> +	auto it = fmts.find(v4l2PixFmt);
> +	if (it == fmts.end()) {
> +		it = fmts.begin();
> +		v4l2PixFmt = it->first;
> +		cfg->pixelFormat = v4l2PixFmt.toPixelFormat();
> +
> +		if (adjusted)
> +			*adjusted = true;
> +
> +		LOG(Converter, Info)
> +			<< "Converter output pixel format adjusted to "
> +			<< cfg->pixelFormat;
> +	}
> +
> +	const Size cfgSize = cfg->size;
> +	cfg->size = adjustSizes(cfgSize, it->second, align);
> +
> +	if (cfg->size.isNull())
> +		return -EINVAL;
> +
> +	if (cfg->size.width != cfgSize.width ||
> +	    cfg->size.height != cfgSize.height) {
> +		LOG(Converter, Info)
> +			<< "Converter size adjusted to "
> +			<< cfg->size;
> +		if (adjusted)
> +			*adjusted = true;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::queueBuffers
>   */
> -- 
> 2.43.0
>

Hi Stefan

On Wed, Dec 11, 2024 at 05:55:22PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the patch :-)
>
> On Fri, Dec 06, 2024 at 11:13:31AM +0100, Stefan Klug wrote:
> > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Add to the Converter interface two functions used by pipeline handlers
> > to validate and adjust the converter input and output configurations
> > by specifying the desired alignment for the adjustment.
> >
> > Add the adjustInputSize() and adjustOutputSize() functions that allows
> > to adjust the converter input/output sizes with the desired alignment.
> >
> > Add a validateOutput() function meant to be used by the pipeline
> > handler implementations of validate(). The function adjusts a
> > StreamConfiguration to a valid configuration produced by the Converter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - Added this patch
> > ---
> >  include/libcamera/internal/converter.h        |  17 ++
> >  .../internal/converter/converter_v4l2_m2m.h   |  11 ++
> >  src/libcamera/converter.cpp                   |  43 +++++
> >  .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
> >  4 files changed, 240 insertions(+)
> >
> > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > index ffbb6f345cd5..9213ae5b9c33 100644
> > --- a/include/libcamera/internal/converter.h
> > +++ b/include/libcamera/internal/converter.h
> > @@ -41,6 +41,13 @@ public:
> >
> >  	using Features = Flags<Feature>;
> >
> > +	enum class Alignment {
> > +		Down = 0,
> > +		Up = (1 << 0),
> > +	};
> > +
> > +	using Alignments = Flags<Alignment>;
> > +
> >  	Converter(MediaDevice *media, Features features = Feature::None);
> >  	virtual ~Converter();
> >
> > @@ -51,9 +58,19 @@ public:
> >  	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
> >  	virtual SizeRange sizes(const Size &input) = 0;
> >
> > +	virtual Size adjustInputSize(const PixelFormat &pixFmt,
> > +				     const Size &size,
> > +				     Alignments align = Alignment::Down) = 0;
> > +	virtual Size adjustOutputSize(const PixelFormat &pixFmt,
> > +				      const Size &size,
> > +				      Alignments align = Alignment::Down) = 0;
> > +
> >  	virtual std::tuple<unsigned int, unsigned int>
> >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> >
> > +	virtual int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +				   Alignments align = Alignment::Down) = 0;
> > +
>
> I'm wondering when to use non-const references and when a pointer. Is
> there a reason cfg is a pointer?
>

I presume the idea is to use * if the parameter can be modified and
const & for non-modifiable ones.

I presume the idea was that being used to the pass-by-value syntax from C,
it felt natural to convey the mutable nature of an argument using pointers.

However it's more of an habit than something we enforce constantly
throughout the code base.


> >  	virtual int configure(const StreamConfiguration &inputCfg,
> >  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> >  	virtual int exportBuffers(const Stream *stream, unsigned int count,
> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > index a5286166f574..89bd2878b190 100644
> > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > @@ -47,6 +47,11 @@ public:
> >  	std::tuple<unsigned int, unsigned int>
> >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> >
> > +	Size adjustInputSize(const PixelFormat &pixFmt,
> > +			     const Size &size, Alignments align = Alignment::Down) override;
> > +	Size adjustOutputSize(const PixelFormat &pixFmt,
> > +			      const Size &size, Alignments align = Alignment::Down) override;
> > +
> >  	int configure(const StreamConfiguration &inputCfg,
> >  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> >  	int exportBuffers(const Stream *stream, unsigned int count,
> > @@ -55,6 +60,9 @@ public:
> >  	int start();
> >  	void stop();
> >
> > +	int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +			   Alignments align = Alignment::Down) override;
> > +
> >  	int queueBuffers(FrameBuffer *input,
> >  			 const std::map<const Stream *, FrameBuffer *> &outputs);
> >
> > @@ -101,6 +109,9 @@ private:
> >  		std::pair<Rectangle, Rectangle> inputCropBounds_;
> >  	};
> >
> > +	Size adjustSizes(const Size &size, const std::vector<SizeRange> &ranges,
> > +			 Alignments align);
> > +
> >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> >
> >  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index aa16970cd446..c3da162b7de7 100644
> > --- a/src/libcamera/converter.cpp
> > +++ b/src/libcamera/converter.cpp
> > @@ -50,6 +50,21 @@ LOG_DEFINE_CATEGORY(Converter)
> >   * \brief A bitwise combination of features supported by the converter
> >   */
> >
> > +/**
> > + * \enum Converter::Alignment
> > + * \brief The alignment mode specified when adjusting the converter input or
> > + * output sizes
> > + * \var Converter::Alignment::Down
> > + * \brief Adjust the Converter sizes to a smaller valid size
> > + * \var Converter::Alignment::Up
> > + * \brief Adjust the Converter sizes to a larger valid size
> > + */
> > +
> > +/**
> > + * \typedef Converter::Alignments
> > + * \brief A bitwise combination of alignments supported by the converter
> > + */
> > +
> >  /**
> >   * \brief Construct a Converter instance
> >   * \param[in] media The media device implementing the converter
> > @@ -110,6 +125,24 @@ Converter::~Converter()
> >   * \return A range of output image sizes
> >   */
> >
> > +/**
> > + * \fn Converter::adjustInputSize()
> > + * \brief Adjust the converter input \a size to a valid value
> > + * \param[in] pixFmt The pixel format of the converter input stream
> > + * \param[in] size The converter input size to adjust to a valid value
> > + * \param[in] align The desired alignment
> > + * \return The adjusted converter input size
> > + */
>
> What gets returned if there is no valid side for the request?

You're right, I should probably add

  * \return The adjusted converter input size or a null Size if \a
  * size cannot be adjusted

I would use "null Size" because of Size::isNull().

>
> My comments where all nits. So
>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
>

Thanks, I'll send a fixup on top.

> > +
> > +/**
> > + * \fn Converter::adjustOutputSize()
> > + * \brief Adjust the converter output \a size to a valid value
> > + * \param[in] pixFmt The pixel format of the converter output stream
> > + * \param[in] size The converter output size to adjust to a valid value
> > + * \param[in] align The desired alignment
> > + * \return The adjusted converter output size
> > + */
> > +
> >  /**
> >   * \fn Converter::strideAndFrameSize()
> >   * \brief Retrieve the output stride and frame size for an input configutation
> > @@ -118,6 +151,16 @@ Converter::~Converter()
> >   * \return A tuple indicating the stride and frame size or an empty tuple on error
> >   */
> >
> > +/**
> > + * \fn Converter::validateOutput()
> > + * \brief Validate and possibily adjust \a cfg to a valid converter output
> > + * \param[inout] cfg The StreamConfiguration to validate and adjust
> > + * \param[out] adjusted Set to true if \a cfg has been adjusted
> > + * \param[in] align The desired alignment
> > + * \return 0 if \a cfg is valid or has been adjusted, a negative error code
> > + * otherwise if \a cfg cannot be adjusted
> > + */
> > +
> >  /**
> >   * \fn Converter::configure()
> >   * \brief Configure a set of output stream conversion from an input stream
> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index bd7e5cce600d..6857472b29f2 100644
> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > @@ -8,6 +8,7 @@
> >
> >  #include "libcamera/internal/converter/converter_v4l2_m2m.h"
> >
> > +#include <algorithm>
> >  #include <limits.h>
> >
> >  #include <libcamera/base/log.h>
> > @@ -400,6 +401,127 @@ V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> >  	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
> >  }
> >
> > +/**
> > + * \copydoc libcamera::Converter::adjustInputSize
> > + */
> > +Size V4L2M2MConverter::adjustInputSize(const PixelFormat &pixFmt,
> > +				       const Size &size, Alignments align)
> > +{
> > +	auto formats = m2m_->output()->formats();
> > +	V4L2PixelFormat v4l2PixFmt = m2m_->output()->toV4L2PixelFormat(pixFmt);
> > +
> > +	auto it = formats.find(v4l2PixFmt);
> > +	if (it == formats.end()) {
> > +		LOG(Converter, Info)
> > +			<< "Unsupported pixel format " << pixFmt;
> > +		return {};
> > +	}
> > +
> > +	return adjustSizes(size, it->second, align);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::Converter::adjustOutputSize
> > + */
> > +Size V4L2M2MConverter::adjustOutputSize(const PixelFormat &pixFmt,
> > +					const Size &size, Alignments align)
> > +{
> > +	auto formats = m2m_->capture()->formats();
> > +	V4L2PixelFormat v4l2PixFmt = m2m_->capture()->toV4L2PixelFormat(pixFmt);
> > +
> > +	auto it = formats.find(v4l2PixFmt);
> > +	if (it == formats.end()) {
> > +		LOG(Converter, Info)
> > +			<< "Unsupported pixel format " << pixFmt;
> > +		return {};
> > +	}
> > +
> > +	return adjustSizes(size, it->second, align);
> > +}
> > +
> > +Size V4L2M2MConverter::adjustSizes(const Size &cfgSize,
> > +				   const std::vector<SizeRange> &ranges,
> > +				   Alignments align)
> > +{
> > +	Size size = cfgSize;
> > +
> > +	if (ranges.size() == 1) {
> > +		/*
> > +		 * The device supports either V4L2_FRMSIZE_TYPE_CONTINUOUS or
> > +		 * V4L2_FRMSIZE_TYPE_STEPWISE.
> > +		 */
> > +		const SizeRange &range = *ranges.begin();
> > +
> > +		size.width = std::clamp(size.width, range.min.width,
> > +					range.max.width);
> > +		size.height = std::clamp(size.height, range.min.height,
> > +					 range.max.height);
> > +
> > +		/*
> > +		 * Check if any alignment is needed. If the sizes are already
> > +		 * aligned, or the device supports V4L2_FRMSIZE_TYPE_CONTINUOUS
> > +		 * with hStep and vStep equal to 1, we're done here.
> > +		 */
> > +		int widthR = size.width % range.hStep;
> > +		int heightR = size.height % range.vStep;
> > +
> > +		/* Align up or down according to the caller request. */
> > +
> > +		if (widthR != 0)
> > +			size.width = size.width - widthR
> > +				   + ((align == Alignment::Up) ? range.hStep : 0);
> > +
> > +		if (heightR != 0)
> > +			size.height = size.height - heightR
> > +				    + ((align == Alignment::Up) ? range.vStep : 0);
> > +	} else {
> > +		/*
> > +		 * The device supports V4L2_FRMSIZE_TYPE_DISCRETE, find the
> > +		 * size closer to the requested output configuration.
> > +		 *
> > +		 * The size ranges vector is not ordered, so we sort it first.
> > +		 * If we align up, start from the larger element.
> > +		 */
> > +		std::vector<Size> sizes(ranges.size());
> > +		std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> > +			       [](const SizeRange &range) { return range.max; });
> > +		std::sort(sizes.begin(), sizes.end());
> > +
> > +		if (align == Alignment::Up)
> > +			std::reverse(sizes.begin(), sizes.end());
> > +
> > +		/*
> > +		 * Return true if s2 is valid according to the desired
> > +		 * alignment: smaller than s1 if we align down, larger than s1
> > +		 * if we align up.
> > +		 */
> > +		auto nextSizeValid = [](const Size &s1, const Size &s2, Alignments a) {
> > +			return a == Alignment::Down
> > +				? (s1.width > s2.width && s1.height > s2.height)
> > +				: (s1.width < s2.width && s1.height < s2.height);
> > +		};
> > +
> > +		Size newSize;
> > +		for (const Size &sz : sizes) {
> > +			if (!nextSizeValid(size, sz, align))
> > +				break;
> > +
> > +			newSize = sz;
> > +		}
> > +
> > +		if (newSize.isNull()) {
> > +			LOG(Converter, Error)
> > +				<< "Cannot adjust " << cfgSize
> > +				<< " to a supported converter size";
> > +			return {};
> > +		}
> > +
> > +		size = newSize;
> > +	}
> > +
> > +	return size;
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::configure
> >   */
> > @@ -507,6 +629,53 @@ void V4L2M2MConverter::stop()
> >  		iter.second->stop();
> >  }
> >
> > +/**
> > + * \copydoc libcamera::Converter::validateOutput
> > + */
> > +int V4L2M2MConverter::validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +				     Alignments align)
> > +{
> > +	V4L2VideoDevice *capture = m2m_->capture();
> > +	V4L2VideoDevice::Formats fmts = capture->formats();
> > +
> > +	if (adjusted)
> > +		*adjusted = false;
> > +
> > +	PixelFormat fmt = cfg->pixelFormat;
> > +	V4L2PixelFormat v4l2PixFmt = capture->toV4L2PixelFormat(fmt);
> > +
> > +	auto it = fmts.find(v4l2PixFmt);
> > +	if (it == fmts.end()) {
> > +		it = fmts.begin();
> > +		v4l2PixFmt = it->first;
> > +		cfg->pixelFormat = v4l2PixFmt.toPixelFormat();
> > +
> > +		if (adjusted)
> > +			*adjusted = true;
> > +
> > +		LOG(Converter, Info)
> > +			<< "Converter output pixel format adjusted to "
> > +			<< cfg->pixelFormat;
> > +	}
> > +
> > +	const Size cfgSize = cfg->size;
> > +	cfg->size = adjustSizes(cfgSize, it->second, align);
> > +
> > +	if (cfg->size.isNull())
> > +		return -EINVAL;
> > +
> > +	if (cfg->size.width != cfgSize.width ||
> > +	    cfg->size.height != cfgSize.height) {
> > +		LOG(Converter, Info)
> > +			<< "Converter size adjusted to "
> > +			<< cfg->size;
> > +		if (adjusted)
> > +			*adjusted = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::queueBuffers
> >   */
> > --
> > 2.43.0
> >

Hi Paul

On Thu, Dec 12, 2024 at 08:43:15PM +0900, Paul Elder wrote:
> On Fri, Dec 06, 2024 at 11:13:31AM +0100, Stefan Klug wrote:
> > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Add to the Converter interface two functions used by pipeline handlers
> > to validate and adjust the converter input and output configurations
> > by specifying the desired alignment for the adjustment.
> >
> > Add the adjustInputSize() and adjustOutputSize() functions that allows
> > to adjust the converter input/output sizes with the desired alignment.
> >
> > Add a validateOutput() function meant to be used by the pipeline
> > handler implementations of validate(). The function adjusts a
> > StreamConfiguration to a valid configuration produced by the Converter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - Added this patch
> > ---
> >  include/libcamera/internal/converter.h        |  17 ++
> >  .../internal/converter/converter_v4l2_m2m.h   |  11 ++
> >  src/libcamera/converter.cpp                   |  43 +++++
> >  .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
> >  4 files changed, 240 insertions(+)
> >
> > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > index ffbb6f345cd5..9213ae5b9c33 100644
> > --- a/include/libcamera/internal/converter.h
> > +++ b/include/libcamera/internal/converter.h
> > @@ -41,6 +41,13 @@ public:
> >
> >  	using Features = Flags<Feature>;
> >
> > +	enum class Alignment {
> > +		Down = 0,
> > +		Up = (1 << 0),
> > +	};
> > +
> > +	using Alignments = Flags<Alignment>;
> > +
> >  	Converter(MediaDevice *media, Features features = Feature::None);
> >  	virtual ~Converter();
> >
> > @@ -51,9 +58,19 @@ public:
> >  	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
> >  	virtual SizeRange sizes(const Size &input) = 0;
> >
> > +	virtual Size adjustInputSize(const PixelFormat &pixFmt,
> > +				     const Size &size,
> > +				     Alignments align = Alignment::Down) = 0;
> > +	virtual Size adjustOutputSize(const PixelFormat &pixFmt,
> > +				      const Size &size,
> > +				      Alignments align = Alignment::Down) = 0;
> > +
> >  	virtual std::tuple<unsigned int, unsigned int>
> >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> >
> > +	virtual int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +				   Alignments align = Alignment::Down) = 0;
>
> I'm confused. Here (and everywhere else it's used), since the type is
> Alignments (Flags<Alignment>), doesn't that mean that
> Alignment::Down | Alignment::Up is a valid input? Wouldn't that cause
> unexpected behavior?
>
> Although I suppose if Alignment::Down | Alignment::Up was actually
> passed then it wouldn't be equal Alignment::Down nor Alignment::Up so
> nothing would happen? Except maybe the ternary operators ig, those would
> just activate the false clause.
>
> Am I missing something here?
>

nah, you're absolutely right, Alignments should be a Flag<>, it's fine
as an enum!

I'll send fixups

>
> Paul
>
>
> > +
> >  	virtual int configure(const StreamConfiguration &inputCfg,
> >  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> >  	virtual int exportBuffers(const Stream *stream, unsigned int count,
> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > index a5286166f574..89bd2878b190 100644
> > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > @@ -47,6 +47,11 @@ public:
> >  	std::tuple<unsigned int, unsigned int>
> >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> >
> > +	Size adjustInputSize(const PixelFormat &pixFmt,
> > +			     const Size &size, Alignments align = Alignment::Down) override;
> > +	Size adjustOutputSize(const PixelFormat &pixFmt,
> > +			      const Size &size, Alignments align = Alignment::Down) override;
> > +
> >  	int configure(const StreamConfiguration &inputCfg,
> >  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> >  	int exportBuffers(const Stream *stream, unsigned int count,
> > @@ -55,6 +60,9 @@ public:
> >  	int start();
> >  	void stop();
> >
> > +	int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +			   Alignments align = Alignment::Down) override;
> > +
> >  	int queueBuffers(FrameBuffer *input,
> >  			 const std::map<const Stream *, FrameBuffer *> &outputs);
> >
> > @@ -101,6 +109,9 @@ private:
> >  		std::pair<Rectangle, Rectangle> inputCropBounds_;
> >  	};
> >
> > +	Size adjustSizes(const Size &size, const std::vector<SizeRange> &ranges,
> > +			 Alignments align);
> > +
> >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> >
> >  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index aa16970cd446..c3da162b7de7 100644
> > --- a/src/libcamera/converter.cpp
> > +++ b/src/libcamera/converter.cpp
> > @@ -50,6 +50,21 @@ LOG_DEFINE_CATEGORY(Converter)
> >   * \brief A bitwise combination of features supported by the converter
> >   */
> >
> > +/**
> > + * \enum Converter::Alignment
> > + * \brief The alignment mode specified when adjusting the converter input or
> > + * output sizes
> > + * \var Converter::Alignment::Down
> > + * \brief Adjust the Converter sizes to a smaller valid size
> > + * \var Converter::Alignment::Up
> > + * \brief Adjust the Converter sizes to a larger valid size
> > + */
> > +
> > +/**
> > + * \typedef Converter::Alignments
> > + * \brief A bitwise combination of alignments supported by the converter
> > + */
> > +
> >  /**
> >   * \brief Construct a Converter instance
> >   * \param[in] media The media device implementing the converter
> > @@ -110,6 +125,24 @@ Converter::~Converter()
> >   * \return A range of output image sizes
> >   */
> >
> > +/**
> > + * \fn Converter::adjustInputSize()
> > + * \brief Adjust the converter input \a size to a valid value
> > + * \param[in] pixFmt The pixel format of the converter input stream
> > + * \param[in] size The converter input size to adjust to a valid value
> > + * \param[in] align The desired alignment
> > + * \return The adjusted converter input size
> > + */
> > +
> > +/**
> > + * \fn Converter::adjustOutputSize()
> > + * \brief Adjust the converter output \a size to a valid value
> > + * \param[in] pixFmt The pixel format of the converter output stream
> > + * \param[in] size The converter output size to adjust to a valid value
> > + * \param[in] align The desired alignment
> > + * \return The adjusted converter output size
> > + */
> > +
> >  /**
> >   * \fn Converter::strideAndFrameSize()
> >   * \brief Retrieve the output stride and frame size for an input configutation
> > @@ -118,6 +151,16 @@ Converter::~Converter()
> >   * \return A tuple indicating the stride and frame size or an empty tuple on error
> >   */
> >
> > +/**
> > + * \fn Converter::validateOutput()
> > + * \brief Validate and possibily adjust \a cfg to a valid converter output
> > + * \param[inout] cfg The StreamConfiguration to validate and adjust
> > + * \param[out] adjusted Set to true if \a cfg has been adjusted
> > + * \param[in] align The desired alignment
> > + * \return 0 if \a cfg is valid or has been adjusted, a negative error code
> > + * otherwise if \a cfg cannot be adjusted
> > + */
> > +
> >  /**
> >   * \fn Converter::configure()
> >   * \brief Configure a set of output stream conversion from an input stream
> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index bd7e5cce600d..6857472b29f2 100644
> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > @@ -8,6 +8,7 @@
> >
> >  #include "libcamera/internal/converter/converter_v4l2_m2m.h"
> >
> > +#include <algorithm>
> >  #include <limits.h>
> >
> >  #include <libcamera/base/log.h>
> > @@ -400,6 +401,127 @@ V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> >  	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
> >  }
> >
> > +/**
> > + * \copydoc libcamera::Converter::adjustInputSize
> > + */
> > +Size V4L2M2MConverter::adjustInputSize(const PixelFormat &pixFmt,
> > +				       const Size &size, Alignments align)
> > +{
> > +	auto formats = m2m_->output()->formats();
> > +	V4L2PixelFormat v4l2PixFmt = m2m_->output()->toV4L2PixelFormat(pixFmt);
> > +
> > +	auto it = formats.find(v4l2PixFmt);
> > +	if (it == formats.end()) {
> > +		LOG(Converter, Info)
> > +			<< "Unsupported pixel format " << pixFmt;
> > +		return {};
> > +	}
> > +
> > +	return adjustSizes(size, it->second, align);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::Converter::adjustOutputSize
> > + */
> > +Size V4L2M2MConverter::adjustOutputSize(const PixelFormat &pixFmt,
> > +					const Size &size, Alignments align)
> > +{
> > +	auto formats = m2m_->capture()->formats();
> > +	V4L2PixelFormat v4l2PixFmt = m2m_->capture()->toV4L2PixelFormat(pixFmt);
> > +
> > +	auto it = formats.find(v4l2PixFmt);
> > +	if (it == formats.end()) {
> > +		LOG(Converter, Info)
> > +			<< "Unsupported pixel format " << pixFmt;
> > +		return {};
> > +	}
> > +
> > +	return adjustSizes(size, it->second, align);
> > +}
> > +
> > +Size V4L2M2MConverter::adjustSizes(const Size &cfgSize,
> > +				   const std::vector<SizeRange> &ranges,
> > +				   Alignments align)
> > +{
> > +	Size size = cfgSize;
> > +
> > +	if (ranges.size() == 1) {
> > +		/*
> > +		 * The device supports either V4L2_FRMSIZE_TYPE_CONTINUOUS or
> > +		 * V4L2_FRMSIZE_TYPE_STEPWISE.
> > +		 */
> > +		const SizeRange &range = *ranges.begin();
> > +
> > +		size.width = std::clamp(size.width, range.min.width,
> > +					range.max.width);
> > +		size.height = std::clamp(size.height, range.min.height,
> > +					 range.max.height);
> > +
> > +		/*
> > +		 * Check if any alignment is needed. If the sizes are already
> > +		 * aligned, or the device supports V4L2_FRMSIZE_TYPE_CONTINUOUS
> > +		 * with hStep and vStep equal to 1, we're done here.
> > +		 */
> > +		int widthR = size.width % range.hStep;
> > +		int heightR = size.height % range.vStep;
> > +
> > +		/* Align up or down according to the caller request. */
> > +
> > +		if (widthR != 0)
> > +			size.width = size.width - widthR
> > +				   + ((align == Alignment::Up) ? range.hStep : 0);
> > +
> > +		if (heightR != 0)
> > +			size.height = size.height - heightR
> > +				    + ((align == Alignment::Up) ? range.vStep : 0);
> > +	} else {
> > +		/*
> > +		 * The device supports V4L2_FRMSIZE_TYPE_DISCRETE, find the
> > +		 * size closer to the requested output configuration.
> > +		 *
> > +		 * The size ranges vector is not ordered, so we sort it first.
> > +		 * If we align up, start from the larger element.
> > +		 */
> > +		std::vector<Size> sizes(ranges.size());
> > +		std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> > +			       [](const SizeRange &range) { return range.max; });
> > +		std::sort(sizes.begin(), sizes.end());
> > +
> > +		if (align == Alignment::Up)
> > +			std::reverse(sizes.begin(), sizes.end());
> > +
> > +		/*
> > +		 * Return true if s2 is valid according to the desired
> > +		 * alignment: smaller than s1 if we align down, larger than s1
> > +		 * if we align up.
> > +		 */
> > +		auto nextSizeValid = [](const Size &s1, const Size &s2, Alignments a) {
> > +			return a == Alignment::Down
> > +				? (s1.width > s2.width && s1.height > s2.height)
> > +				: (s1.width < s2.width && s1.height < s2.height);
> > +		};
> > +
> > +		Size newSize;
> > +		for (const Size &sz : sizes) {
> > +			if (!nextSizeValid(size, sz, align))
> > +				break;
> > +
> > +			newSize = sz;
> > +		}
> > +
> > +		if (newSize.isNull()) {
> > +			LOG(Converter, Error)
> > +				<< "Cannot adjust " << cfgSize
> > +				<< " to a supported converter size";
> > +			return {};
> > +		}
> > +
> > +		size = newSize;
> > +	}
> > +
> > +	return size;
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::configure
> >   */
> > @@ -507,6 +629,53 @@ void V4L2M2MConverter::stop()
> >  		iter.second->stop();
> >  }
> >
> > +/**
> > + * \copydoc libcamera::Converter::validateOutput
> > + */
> > +int V4L2M2MConverter::validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > +				     Alignments align)
> > +{
> > +	V4L2VideoDevice *capture = m2m_->capture();
> > +	V4L2VideoDevice::Formats fmts = capture->formats();
> > +
> > +	if (adjusted)
> > +		*adjusted = false;
> > +
> > +	PixelFormat fmt = cfg->pixelFormat;
> > +	V4L2PixelFormat v4l2PixFmt = capture->toV4L2PixelFormat(fmt);
> > +
> > +	auto it = fmts.find(v4l2PixFmt);
> > +	if (it == fmts.end()) {
> > +		it = fmts.begin();
> > +		v4l2PixFmt = it->first;
> > +		cfg->pixelFormat = v4l2PixFmt.toPixelFormat();
> > +
> > +		if (adjusted)
> > +			*adjusted = true;
> > +
> > +		LOG(Converter, Info)
> > +			<< "Converter output pixel format adjusted to "
> > +			<< cfg->pixelFormat;
> > +	}
> > +
> > +	const Size cfgSize = cfg->size;
> > +	cfg->size = adjustSizes(cfgSize, it->second, align);
> > +
> > +	if (cfg->size.isNull())
> > +		return -EINVAL;
> > +
> > +	if (cfg->size.width != cfgSize.width ||
> > +	    cfg->size.height != cfgSize.height) {
> > +		LOG(Converter, Info)
> > +			<< "Converter size adjusted to "
> > +			<< cfg->size;
> > +		if (adjusted)
> > +			*adjusted = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::Converter::queueBuffers
> >   */
> > --
> > 2.43.0
> >

On Thu, Dec 12, 2024 at 06:28:16PM +0100, Jacopo Mondi wrote:
> Hi Paul
> 
> On Thu, Dec 12, 2024 at 08:43:15PM +0900, Paul Elder wrote:
> > On Fri, Dec 06, 2024 at 11:13:31AM +0100, Stefan Klug wrote:
> > > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > Add to the Converter interface two functions used by pipeline handlers
> > > to validate and adjust the converter input and output configurations
> > > by specifying the desired alignment for the adjustment.
> > >
> > > Add the adjustInputSize() and adjustOutputSize() functions that allows
> > > to adjust the converter input/output sizes with the desired alignment.
> > >
> > > Add a validateOutput() function meant to be used by the pipeline
> > > handler implementations of validate(). The function adjusts a
> > > StreamConfiguration to a valid configuration produced by the Converter.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > ---
> > > Changes in v3:
> > > - Added this patch
> > > ---
> > >  include/libcamera/internal/converter.h        |  17 ++
> > >  .../internal/converter/converter_v4l2_m2m.h   |  11 ++
> > >  src/libcamera/converter.cpp                   |  43 +++++
> > >  .../converter/converter_v4l2_m2m.cpp          | 169 ++++++++++++++++++
> > >  4 files changed, 240 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> > > index ffbb6f345cd5..9213ae5b9c33 100644
> > > --- a/include/libcamera/internal/converter.h
> > > +++ b/include/libcamera/internal/converter.h
> > > @@ -41,6 +41,13 @@ public:
> > >
> > >  	using Features = Flags<Feature>;
> > >
> > > +	enum class Alignment {
> > > +		Down = 0,
> > > +		Up = (1 << 0),
> > > +	};
> > > +
> > > +	using Alignments = Flags<Alignment>;
> > > +
> > >  	Converter(MediaDevice *media, Features features = Feature::None);
> > >  	virtual ~Converter();
> > >
> > > @@ -51,9 +58,19 @@ public:
> > >  	virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
> > >  	virtual SizeRange sizes(const Size &input) = 0;
> > >
> > > +	virtual Size adjustInputSize(const PixelFormat &pixFmt,
> > > +				     const Size &size,
> > > +				     Alignments align = Alignment::Down) = 0;
> > > +	virtual Size adjustOutputSize(const PixelFormat &pixFmt,
> > > +				      const Size &size,
> > > +				      Alignments align = Alignment::Down) = 0;
> > > +
> > >  	virtual std::tuple<unsigned int, unsigned int>
> > >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> > >
> > > +	virtual int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > > +				   Alignments align = Alignment::Down) = 0;
> >
> > I'm confused. Here (and everywhere else it's used), since the type is
> > Alignments (Flags<Alignment>), doesn't that mean that
> > Alignment::Down | Alignment::Up is a valid input? Wouldn't that cause
> > unexpected behavior?
> >
> > Although I suppose if Alignment::Down | Alignment::Up was actually
> > passed then it wouldn't be equal Alignment::Down nor Alignment::Up so
> > nothing would happen? Except maybe the ternary operators ig, those would
> > just activate the false clause.
> >
> > Am I missing something here?
> >
> 
> nah, you're absolutely right, Alignments should be a Flag<>, it's fine
> as an enum!
> 
> I'll send fixups

Fixup looks good! With that,

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> >
> >
> > > +
> > >  	virtual int configure(const StreamConfiguration &inputCfg,
> > >  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> > >  	virtual int exportBuffers(const Stream *stream, unsigned int count,
> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > index a5286166f574..89bd2878b190 100644
> > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> > > @@ -47,6 +47,11 @@ public:
> > >  	std::tuple<unsigned int, unsigned int>
> > >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> > >
> > > +	Size adjustInputSize(const PixelFormat &pixFmt,
> > > +			     const Size &size, Alignments align = Alignment::Down) override;
> > > +	Size adjustOutputSize(const PixelFormat &pixFmt,
> > > +			      const Size &size, Alignments align = Alignment::Down) override;
> > > +
> > >  	int configure(const StreamConfiguration &inputCfg,
> > >  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> > >  	int exportBuffers(const Stream *stream, unsigned int count,
> > > @@ -55,6 +60,9 @@ public:
> > >  	int start();
> > >  	void stop();
> > >
> > > +	int validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > > +			   Alignments align = Alignment::Down) override;
> > > +
> > >  	int queueBuffers(FrameBuffer *input,
> > >  			 const std::map<const Stream *, FrameBuffer *> &outputs);
> > >
> > > @@ -101,6 +109,9 @@ private:
> > >  		std::pair<Rectangle, Rectangle> inputCropBounds_;
> > >  	};
> > >
> > > +	Size adjustSizes(const Size &size, const std::vector<SizeRange> &ranges,
> > > +			 Alignments align);
> > > +
> > >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> > >
> > >  	std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > > index aa16970cd446..c3da162b7de7 100644
> > > --- a/src/libcamera/converter.cpp
> > > +++ b/src/libcamera/converter.cpp
> > > @@ -50,6 +50,21 @@ LOG_DEFINE_CATEGORY(Converter)
> > >   * \brief A bitwise combination of features supported by the converter
> > >   */
> > >
> > > +/**
> > > + * \enum Converter::Alignment
> > > + * \brief The alignment mode specified when adjusting the converter input or
> > > + * output sizes
> > > + * \var Converter::Alignment::Down
> > > + * \brief Adjust the Converter sizes to a smaller valid size
> > > + * \var Converter::Alignment::Up
> > > + * \brief Adjust the Converter sizes to a larger valid size
> > > + */
> > > +
> > > +/**
> > > + * \typedef Converter::Alignments
> > > + * \brief A bitwise combination of alignments supported by the converter
> > > + */
> > > +
> > >  /**
> > >   * \brief Construct a Converter instance
> > >   * \param[in] media The media device implementing the converter
> > > @@ -110,6 +125,24 @@ Converter::~Converter()
> > >   * \return A range of output image sizes
> > >   */
> > >
> > > +/**
> > > + * \fn Converter::adjustInputSize()
> > > + * \brief Adjust the converter input \a size to a valid value
> > > + * \param[in] pixFmt The pixel format of the converter input stream
> > > + * \param[in] size The converter input size to adjust to a valid value
> > > + * \param[in] align The desired alignment
> > > + * \return The adjusted converter input size
> > > + */
> > > +
> > > +/**
> > > + * \fn Converter::adjustOutputSize()
> > > + * \brief Adjust the converter output \a size to a valid value
> > > + * \param[in] pixFmt The pixel format of the converter output stream
> > > + * \param[in] size The converter output size to adjust to a valid value
> > > + * \param[in] align The desired alignment
> > > + * \return The adjusted converter output size
> > > + */
> > > +
> > >  /**
> > >   * \fn Converter::strideAndFrameSize()
> > >   * \brief Retrieve the output stride and frame size for an input configutation
> > > @@ -118,6 +151,16 @@ Converter::~Converter()
> > >   * \return A tuple indicating the stride and frame size or an empty tuple on error
> > >   */
> > >
> > > +/**
> > > + * \fn Converter::validateOutput()
> > > + * \brief Validate and possibily adjust \a cfg to a valid converter output
> > > + * \param[inout] cfg The StreamConfiguration to validate and adjust
> > > + * \param[out] adjusted Set to true if \a cfg has been adjusted
> > > + * \param[in] align The desired alignment
> > > + * \return 0 if \a cfg is valid or has been adjusted, a negative error code
> > > + * otherwise if \a cfg cannot be adjusted
> > > + */
> > > +
> > >  /**
> > >   * \fn Converter::configure()
> > >   * \brief Configure a set of output stream conversion from an input stream
> > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > index bd7e5cce600d..6857472b29f2 100644
> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > > @@ -8,6 +8,7 @@
> > >
> > >  #include "libcamera/internal/converter/converter_v4l2_m2m.h"
> > >
> > > +#include <algorithm>
> > >  #include <limits.h>
> > >
> > >  #include <libcamera/base/log.h>
> > > @@ -400,6 +401,127 @@ V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> > >  	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
> > >  }
> > >
> > > +/**
> > > + * \copydoc libcamera::Converter::adjustInputSize
> > > + */
> > > +Size V4L2M2MConverter::adjustInputSize(const PixelFormat &pixFmt,
> > > +				       const Size &size, Alignments align)
> > > +{
> > > +	auto formats = m2m_->output()->formats();
> > > +	V4L2PixelFormat v4l2PixFmt = m2m_->output()->toV4L2PixelFormat(pixFmt);
> > > +
> > > +	auto it = formats.find(v4l2PixFmt);
> > > +	if (it == formats.end()) {
> > > +		LOG(Converter, Info)
> > > +			<< "Unsupported pixel format " << pixFmt;
> > > +		return {};
> > > +	}
> > > +
> > > +	return adjustSizes(size, it->second, align);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::Converter::adjustOutputSize
> > > + */
> > > +Size V4L2M2MConverter::adjustOutputSize(const PixelFormat &pixFmt,
> > > +					const Size &size, Alignments align)
> > > +{
> > > +	auto formats = m2m_->capture()->formats();
> > > +	V4L2PixelFormat v4l2PixFmt = m2m_->capture()->toV4L2PixelFormat(pixFmt);
> > > +
> > > +	auto it = formats.find(v4l2PixFmt);
> > > +	if (it == formats.end()) {
> > > +		LOG(Converter, Info)
> > > +			<< "Unsupported pixel format " << pixFmt;
> > > +		return {};
> > > +	}
> > > +
> > > +	return adjustSizes(size, it->second, align);
> > > +}
> > > +
> > > +Size V4L2M2MConverter::adjustSizes(const Size &cfgSize,
> > > +				   const std::vector<SizeRange> &ranges,
> > > +				   Alignments align)
> > > +{
> > > +	Size size = cfgSize;
> > > +
> > > +	if (ranges.size() == 1) {
> > > +		/*
> > > +		 * The device supports either V4L2_FRMSIZE_TYPE_CONTINUOUS or
> > > +		 * V4L2_FRMSIZE_TYPE_STEPWISE.
> > > +		 */
> > > +		const SizeRange &range = *ranges.begin();
> > > +
> > > +		size.width = std::clamp(size.width, range.min.width,
> > > +					range.max.width);
> > > +		size.height = std::clamp(size.height, range.min.height,
> > > +					 range.max.height);
> > > +
> > > +		/*
> > > +		 * Check if any alignment is needed. If the sizes are already
> > > +		 * aligned, or the device supports V4L2_FRMSIZE_TYPE_CONTINUOUS
> > > +		 * with hStep and vStep equal to 1, we're done here.
> > > +		 */
> > > +		int widthR = size.width % range.hStep;
> > > +		int heightR = size.height % range.vStep;
> > > +
> > > +		/* Align up or down according to the caller request. */
> > > +
> > > +		if (widthR != 0)
> > > +			size.width = size.width - widthR
> > > +				   + ((align == Alignment::Up) ? range.hStep : 0);
> > > +
> > > +		if (heightR != 0)
> > > +			size.height = size.height - heightR
> > > +				    + ((align == Alignment::Up) ? range.vStep : 0);
> > > +	} else {
> > > +		/*
> > > +		 * The device supports V4L2_FRMSIZE_TYPE_DISCRETE, find the
> > > +		 * size closer to the requested output configuration.
> > > +		 *
> > > +		 * The size ranges vector is not ordered, so we sort it first.
> > > +		 * If we align up, start from the larger element.
> > > +		 */
> > > +		std::vector<Size> sizes(ranges.size());
> > > +		std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> > > +			       [](const SizeRange &range) { return range.max; });
> > > +		std::sort(sizes.begin(), sizes.end());
> > > +
> > > +		if (align == Alignment::Up)
> > > +			std::reverse(sizes.begin(), sizes.end());
> > > +
> > > +		/*
> > > +		 * Return true if s2 is valid according to the desired
> > > +		 * alignment: smaller than s1 if we align down, larger than s1
> > > +		 * if we align up.
> > > +		 */
> > > +		auto nextSizeValid = [](const Size &s1, const Size &s2, Alignments a) {
> > > +			return a == Alignment::Down
> > > +				? (s1.width > s2.width && s1.height > s2.height)
> > > +				: (s1.width < s2.width && s1.height < s2.height);
> > > +		};
> > > +
> > > +		Size newSize;
> > > +		for (const Size &sz : sizes) {
> > > +			if (!nextSizeValid(size, sz, align))
> > > +				break;
> > > +
> > > +			newSize = sz;
> > > +		}
> > > +
> > > +		if (newSize.isNull()) {
> > > +			LOG(Converter, Error)
> > > +				<< "Cannot adjust " << cfgSize
> > > +				<< " to a supported converter size";
> > > +			return {};
> > > +		}
> > > +
> > > +		size = newSize;
> > > +	}
> > > +
> > > +	return size;
> > > +}
> > > +
> > >  /**
> > >   * \copydoc libcamera::Converter::configure
> > >   */
> > > @@ -507,6 +629,53 @@ void V4L2M2MConverter::stop()
> > >  		iter.second->stop();
> > >  }
> > >
> > > +/**
> > > + * \copydoc libcamera::Converter::validateOutput
> > > + */
> > > +int V4L2M2MConverter::validateOutput(StreamConfiguration *cfg, bool *adjusted,
> > > +				     Alignments align)
> > > +{
> > > +	V4L2VideoDevice *capture = m2m_->capture();
> > > +	V4L2VideoDevice::Formats fmts = capture->formats();
> > > +
> > > +	if (adjusted)
> > > +		*adjusted = false;
> > > +
> > > +	PixelFormat fmt = cfg->pixelFormat;
> > > +	V4L2PixelFormat v4l2PixFmt = capture->toV4L2PixelFormat(fmt);
> > > +
> > > +	auto it = fmts.find(v4l2PixFmt);
> > > +	if (it == fmts.end()) {
> > > +		it = fmts.begin();
> > > +		v4l2PixFmt = it->first;
> > > +		cfg->pixelFormat = v4l2PixFmt.toPixelFormat();
> > > +
> > > +		if (adjusted)
> > > +			*adjusted = true;
> > > +
> > > +		LOG(Converter, Info)
> > > +			<< "Converter output pixel format adjusted to "
> > > +			<< cfg->pixelFormat;
> > > +	}
> > > +
> > > +	const Size cfgSize = cfg->size;
> > > +	cfg->size = adjustSizes(cfgSize, it->second, align);
> > > +
> > > +	if (cfg->size.isNull())
> > > +		return -EINVAL;
> > > +
> > > +	if (cfg->size.width != cfgSize.width ||
> > > +	    cfg->size.height != cfgSize.height) {
> > > +		LOG(Converter, Info)
> > > +			<< "Converter size adjusted to "
> > > +			<< cfg->size;
> > > +		if (adjusted)
> > > +			*adjusted = true;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * \copydoc libcamera::Converter::queueBuffers
> > >   */
> > > --
> > > 2.43.0
> > >