[libcamera-devel,PATCH/RFC,08/12] libcamera: stream: Add StreamFormats

1225 diff mbox series

From: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Add a StreamFormats which describes all the formats a stream can
support. The object does not collect any formation itself but can
simplify users interaction with formats as it's able to translate a
stream format range into discrete steps.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/stream.h |  30 ++++++
 src/libcamera/stream.cpp   | 191 +++++++++++++++++++++++++++++++++++++
 2 files changed, 221 insertions(+)

Hi Niklas,

Thank you for the patch.

On Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:
> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Add a StreamFormats which describes all the formats a stream can
> support. The object does not collect any formation itself but can
> simplify users interaction with formats as it's able to translate a
> stream format range into discrete steps.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/stream.h |  30 ++++++
>  src/libcamera/stream.cpp   | 191 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 221 insertions(+)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 47c007ed52e2..138ed649ba8c 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_STREAM_H__
>  #define __LIBCAMERA_STREAM_H__
>  
> +#include <map>
>  #include <string>
>  #include <vector>
>  
> @@ -39,6 +40,35 @@ enum StreamRole {
>  	Viewfinder,
>  };
>  
> +class StreamFormats
> +{
> +public:
> +	struct Range {
> +		Size min;
> +		Size max;
> +		unsigned int stepWidth;
> +		unsigned int stepHeight;
> +	};

This is very similar to the SizeRange structure defined in geometry.h.
Would it make sense to extend that structure instead ?

I would also rename stepWidth and stepHeight to hStep and vStep
respectively, but that may just be me.

> +
> +	StreamFormats() {}
> +
> +	StreamFormats(const std::map<unsigned int, Range> &rangeSizes)
> +		: rangeSizes_(rangeSizes) {}
> +
> +	StreamFormats(const std::map<unsigned int, std::vector<Size>> &discreteSizes)
> +		: discreteSizes_(discreteSizes) {}

Would it be useful to give an alias to these types, the same way we have
FormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may
then want to rename StreamFormats to something that doesn't use the word
Stream, as it's not just related to streams.

> +
> +	std::vector<unsigned int> formats() const;
> +	std::vector<Size> sizes(unsigned int pixelformat) const;
> +
> +	bool isRange() const { return !rangeSizes_.empty(); }
> +	Range range(unsigned int pixelformat) const;

You should return a const Range & to optimise the case where the caller
only wants to inspect the range and doesn't need a copy.

> +
> +private:
> +	std::map<unsigned int, std::vector<Size>> discreteSizes_;
> +	std::map<unsigned int, Range> rangeSizes_;
> +};
> +
>  using StreamRoles = std::vector<StreamRole>;
>  
>  class Stream
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 0c59a31a3a05..e82fa8143b8f 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <libcamera/stream.h>
>  
> +#include <algorithm>
>  #include <iomanip>
>  #include <sstream>
>  
> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const
>   * \brief A vector of StreamRole
>   */
>  
> +/**
> + * \class StreamFormats
> + * \brief Hold information about supported stream formats
> + *
> + * The StreamFormats class holds information about pixel formats and frame
> + * sizes a stream supports. The class groups size information by the pixel
> + * format which can produce it. There are two types of size information which
> + * can be described in the StreamFormats object discrete and range sizes.
> + *
> + * The discrete sizes are a list of fixed sizes of the only resolutions the
> + * stream can produce. While the range description contains a max and min
> + * size together with a stepping. The range information can either be consumed
> + * raw which allows users to calculate a size which the stream could support
> + * or be accessed thru the sizes() helper which will compute a list of common
> + * discrete sizes which can be produced within the range.
> + */

I think we'll have to refine both the API and its documentation, to
explain better how this operates and is supposed to be used. Let's
discuss the issues pointed out through this patch first.

> +
> +/**
> + * \class StreamFormats::Range
> + * \brief Hold information about stream format range
> + */
> +
> +/**
> + * \var StreamFormats::Range::min
> + * \brief Range minimum size
> + */
> +
> +/**
> + * \var StreamFormats::Range::max
> + * \brief Range maximum size
> + */
> +
> +/**
> + * \var StreamFormats::Range::stepWidth
> + * \brief Range width step length in pixels
> + */
> +
> +/**
> + * \var StreamFormats::Range::stepHeight
> + * \brief Range height step length in pixels
> + */
> +
> +/**
> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)
> + * \brief Constrict a ranged based StreamFormats object

s/Constrict/Construct/

> + * \param[in] rangeSizes A map of pixel format to a ranged description
> + * \sa StreamFormats::Range
> + */
> +
> +/**
> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)
> + * \brief Constrict a discrete based StreamFormats object
> + * \param[in] discreteSizes A map of pixel format to a list of frame sizes
> + */
> +
> +/**
> + * \brief Retrive a list of pixel formats supported by the stram

s/Retrive/Retrieve/ (in multiple locations)
s/a list/the list/
s/stram/stream/

> + * \returns A list of pixel formats
> + */
> +std::vector<unsigned int> StreamFormats::formats() const
> +{
> +	std::vector<unsigned int> formats;
> +
> +	if (isRange()) {
> +		for (auto const &it : rangeSizes_)
> +			formats.push_back(it.first);
> +	} else {
> +		for (auto const &it : discreteSizes_)
> +			formats.push_back(it.first);
> +	}
> +
> +	return formats;
> +}
> +
> +/**
> + * \brief Retrive a list of frame sizes
> + * \param[in] pixelformat Pixel format to retrive sizes for

This needs to be expanded to explain the mechanism for range-based
sizes.

> + * \returns A list of frame sizes
> + */
> +std::vector<Size> StreamFormats::sizes(unsigned int pixelformat) const
> +{
> +	std::vector<Size> sizes;
> +
> +	/*
> +	 * Sizes to try and extract from ranges.
> +	 * \todo Verify list of resolutions are good

Could you explain where they come from ?

> +	 */
> +	static const std::vector<Size> rangeDescreteSizes = {
> +		Size(160, 120),
> +		Size(240, 160),
> +		Size(320, 240),
> +		Size(400, 240),
> +		Size(480, 320),
> +		Size(640, 360),
> +		Size(640, 480),
> +		Size(720, 480),
> +		Size(720, 576),
> +		Size(768, 480),
> +		Size(800, 600),
> +		Size(854, 480),
> +		Size(960, 540),
> +		Size(960, 640),
> +		Size(1024, 576),
> +		Size(1024, 600),
> +		Size(1024, 768),
> +		Size(1152, 864),
> +		Size(1280, 1024),
> +		Size(1280, 1080),
> +		Size(1280, 720),
> +		Size(1280, 800),
> +		Size(1360, 768),
> +		Size(1366, 768),
> +		Size(1400, 1050),
> +		Size(1440, 900),
> +		Size(1536, 864),
> +		Size(1600, 1200),
> +		Size(1600, 900),
> +		Size(1680, 1050),
> +		Size(1920, 1080),
> +		Size(1920, 1200),
> +		Size(2048, 1080),
> +		Size(2048, 1152),
> +		Size(2048, 1536),
> +		Size(2160, 1080),
> +		Size(2560, 1080),
> +		Size(2560, 1440),
> +		Size(2560, 1600),
> +		Size(2560, 2048),
> +		Size(2960, 1440),
> +		Size(3200, 1800),
> +		Size(3200, 2048),
> +		Size(3200, 2400),
> +		Size(3440, 1440),
> +		Size(3840, 1080),
> +		Size(3840, 1600),
> +		Size(3840, 2160),
> +		Size(3840, 2400),
> +		Size(4096, 2160),
> +		Size(5120, 2160),
> +		Size(5120, 2880),
> +		Size(7680, 4320),
> +	};
> +
> +	if (!isRange()) {
> +		auto const &it = discreteSizes_.find(pixelformat);
> +		if (it == discreteSizes_.end())
> +			return {};
> +		sizes = it->second;
> +	} else {
> +		Range limit = range(pixelformat);
> +
> +		for (const Size &size : rangeDescreteSizes) {
> +			if (size.width < limit.min.width ||
> +			    size.width > limit.max.width ||
> +			    size.height < limit.min.height ||
> +			    size.height > limit.max.height ||

Would it be useful to add a contains() method to the Range class ?

> +			    size.width % limit.stepWidth ||
> +			    size.height % limit.stepHeight)

Is this correct ? I would have considered the step differently than an
alignment, as in the range containing all min + i * step values up to
max. If min is not aligned to step, your check leads to a different
result. In any case this should be clearly documented in the Range
class.

> +				continue;
> +
> +			sizes.push_back(size);
> +		}
> +	}
> +
> +	std::sort(sizes.begin(), sizes.end());
> +
> +	return sizes;
> +}
> +
> +/**
> + * \fn StreamFormats::isRange()
> + * \brief Check if the StreamFormat is a range description
> + * \returns True if the description is a range, false otherwise
> + */
> +
> +/**
> + * \brief Retrive the range description
> + * \param[in] pixelformat Pixel format to retrive description for

This doesn't document that the function is only available for
range-based StreamFormats instances, and what happens in the pixel
format is not supported. Furthermore, I think the method should also
support the discrete sizes, by returning a range made from the minimum
and maximum size. Otherwise it gets quite inconvenient for applications
that want to quickly check what the minimum and maximum sizes here.
We'll run into the issue of what to report as a step in that case, one
option would be to set the step to 0 to report that there's no single
step value, but other options may be possible.

All in all I think this class suffers from similar issues than
FormatEnum, we need to think more about how it will be used to offer a
nice, clean and clear (and clearly documented) API to applications.

> + * \returns The range description
> + */
> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const
> +{
> +	auto const it = rangeSizes_.find(pixelformat);
> +
> +	if (it == rangeSizes_.end())
> +		return {};
> +
> +	return it->second;
> +}
> +
>  /**
>   * \class Stream
>   * \brief Video stream for a camera

Hi Niklas,

On 18/05/2019 19:13, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:
>> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>
>> Add a StreamFormats which describes all the formats a stream can
>> support. The object does not collect any formation itself but can
>> simplify users interaction with formats as it's able to translate a
>> stream format range into discrete steps.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> ---
>>  include/libcamera/stream.h |  30 ++++++
>>  src/libcamera/stream.cpp   | 191 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 221 insertions(+)
>>
>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
>> index 47c007ed52e2..138ed649ba8c 100644
>> --- a/include/libcamera/stream.h
>> +++ b/include/libcamera/stream.h
>> @@ -7,6 +7,7 @@
>>  #ifndef __LIBCAMERA_STREAM_H__
>>  #define __LIBCAMERA_STREAM_H__
>>  
>> +#include <map>
>>  #include <string>
>>  #include <vector>
>>  
>> @@ -39,6 +40,35 @@ enum StreamRole {
>>  	Viewfinder,
>>  };
>>  
>> +class StreamFormats
>> +{
>> +public:
>> +	struct Range {
>> +		Size min;
>> +		Size max;
>> +		unsigned int stepWidth;
>> +		unsigned int stepHeight;
>> +	};
> 
> This is very similar to the SizeRange structure defined in geometry.h.
> Would it make sense to extend that structure instead ?
> 
> I would also rename stepWidth and stepHeight to hStep and vStep
> respectively, but that may just be me.
> 
>> +
>> +	StreamFormats() {}
>> +
>> +	StreamFormats(const std::map<unsigned int, Range> &rangeSizes)
>> +		: rangeSizes_(rangeSizes) {}
>> +
>> +	StreamFormats(const std::map<unsigned int, std::vector<Size>> &discreteSizes)
>> +		: discreteSizes_(discreteSizes) {}
> 
> Would it be useful to give an alias to these types, the same way we have
> FormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may
> then want to rename StreamFormats to something that doesn't use the word
> Stream, as it's not just related to streams.
> 
>> +
>> +	std::vector<unsigned int> formats() const;
>> +	std::vector<Size> sizes(unsigned int pixelformat) const;
>> +
>> +	bool isRange() const { return !rangeSizes_.empty(); }
>> +	Range range(unsigned int pixelformat) const;
> 
> You should return a const Range & to optimise the case where the caller
> only wants to inspect the range and doesn't need a copy.
> 
>> +
>> +private:
>> +	std::map<unsigned int, std::vector<Size>> discreteSizes_;
>> +	std::map<unsigned int, Range> rangeSizes_;
>> +};
>> +
>>  using StreamRoles = std::vector<StreamRole>;
>>  
>>  class Stream
>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>> index 0c59a31a3a05..e82fa8143b8f 100644
>> --- a/src/libcamera/stream.cpp
>> +++ b/src/libcamera/stream.cpp
>> @@ -7,6 +7,7 @@
>>  
>>  #include <libcamera/stream.h>
>>  
>> +#include <algorithm>
>>  #include <iomanip>
>>  #include <sstream>
>>  
>> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const
>>   * \brief A vector of StreamRole
>>   */
>>  
>> +/**
>> + * \class StreamFormats
>> + * \brief Hold information about supported stream formats
>> + *
>> + * The StreamFormats class holds information about pixel formats and frame
>> + * sizes a stream supports. The class groups size information by the pixel
>> + * format which can produce it. There are two types of size information which
>> + * can be described in the StreamFormats object discrete and range sizes.
>> + *
>> + * The discrete sizes are a list of fixed sizes of the only resolutions the
>> + * stream can produce. While the range description contains a max and min
>> + * size together with a stepping. The range information can either be consumed
>> + * raw which allows users to calculate a size which the stream could support
>> + * or be accessed thru the sizes() helper which will compute a list of common
>> + * discrete sizes which can be produced within the range.
>> + */
> 
> I think we'll have to refine both the API and its documentation, to
> explain better how this operates and is supposed to be used. Let's
> discuss the issues pointed out through this patch first.
> 
>> +
>> +/**
>> + * \class StreamFormats::Range
>> + * \brief Hold information about stream format range
>> + */
>> +
>> +/**
>> + * \var StreamFormats::Range::min
>> + * \brief Range minimum size
>> + */
>> +
>> +/**
>> + * \var StreamFormats::Range::max
>> + * \brief Range maximum size
>> + */
>> +
>> +/**
>> + * \var StreamFormats::Range::stepWidth
>> + * \brief Range width step length in pixels
>> + */
>> +
>> +/**
>> + * \var StreamFormats::Range::stepHeight
>> + * \brief Range height step length in pixels
>> + */
>> +
>> +/**
>> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)
>> + * \brief Constrict a ranged based StreamFormats object
> 
> s/Constrict/Construct/
> 
>> + * \param[in] rangeSizes A map of pixel format to a ranged description
>> + * \sa StreamFormats::Range
>> + */
>> +
>> +/**
>> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)
>> + * \brief Constrict a discrete based StreamFormats object
>> + * \param[in] discreteSizes A map of pixel format to a list of frame sizes
>> + */
>> +
>> +/**
>> + * \brief Retrive a list of pixel formats supported by the stram
> 
> s/Retrive/Retrieve/ (in multiple locations)
> s/a list/the list/
> s/stram/stream/
> 
>> + * \returns A list of pixel formats
>> + */
>> +std::vector<unsigned int> StreamFormats::formats() const
>> +{
>> +	std::vector<unsigned int> formats;
>> +
>> +	if (isRange()) {
>> +		for (auto const &it : rangeSizes_)
>> +			formats.push_back(it.first);
>> +	} else {
>> +		for (auto const &it : discreteSizes_)
>> +			formats.push_back(it.first);
>> +	}
>> +
>> +	return formats;
>> +}
>> +
>> +/**
>> + * \brief Retrive a list of frame sizes
>> + * \param[in] pixelformat Pixel format to retrive sizes for
> 
> This needs to be expanded to explain the mechanism for range-based
> sizes.
> 
>> + * \returns A list of frame sizes
>> + */
>> +std::vector<Size> StreamFormats::sizes(unsigned int pixelformat) const
>> +{
>> +	std::vector<Size> sizes;
>> +
>> +	/*
>> +	 * Sizes to try and extract from ranges.
>> +	 * \todo Verify list of resolutions are good
> 
> Could you explain where they come from ?
> 
>> +	 */
>> +	static const std::vector<Size> rangeDescreteSizes = {


s/Descrete/Discrete/


>> +		Size(160, 120),
>> +		Size(240, 160),
>> +		Size(320, 240),
>> +		Size(400, 240),
>> +		Size(480, 320),
>> +		Size(640, 360),
>> +		Size(640, 480),
>> +		Size(720, 480),
>> +		Size(720, 576),
>> +		Size(768, 480),
>> +		Size(800, 600),
>> +		Size(854, 480),
>> +		Size(960, 540),
>> +		Size(960, 640),
>> +		Size(1024, 576),
>> +		Size(1024, 600),
>> +		Size(1024, 768),
>> +		Size(1152, 864),
>> +		Size(1280, 1024),
>> +		Size(1280, 1080),
>> +		Size(1280, 720),
>> +		Size(1280, 800),
>> +		Size(1360, 768),
>> +		Size(1366, 768),
>> +		Size(1400, 1050),
>> +		Size(1440, 900),
>> +		Size(1536, 864),
>> +		Size(1600, 1200),
>> +		Size(1600, 900),
>> +		Size(1680, 1050),
>> +		Size(1920, 1080),
>> +		Size(1920, 1200),
>> +		Size(2048, 1080),
>> +		Size(2048, 1152),
>> +		Size(2048, 1536),
>> +		Size(2160, 1080),
>> +		Size(2560, 1080),
>> +		Size(2560, 1440),
>> +		Size(2560, 1600),
>> +		Size(2560, 2048),
>> +		Size(2960, 1440),
>> +		Size(3200, 1800),
>> +		Size(3200, 2048),
>> +		Size(3200, 2400),
>> +		Size(3440, 1440),
>> +		Size(3840, 1080),
>> +		Size(3840, 1600),
>> +		Size(3840, 2160),
>> +		Size(3840, 2400),
>> +		Size(4096, 2160),
>> +		Size(5120, 2160),
>> +		Size(5120, 2880),
>> +		Size(7680, 4320),
>> +	};

Is that a list that we could generate somehow instead from some conditions?

Perhaps generate based on aspect ratios and known widths and multipliers
or something... not sure it will be any better - but it might be more
readable or easy to maintain?


>> +
>> +	if (!isRange()) {
>> +		auto const &it = discreteSizes_.find(pixelformat);
>> +		if (it == discreteSizes_.end())
>> +			return {};
>> +		sizes = it->second;
>> +	} else {
>> +		Range limit = range(pixelformat);
>> +
>> +		for (const Size &size : rangeDescreteSizes) {
>> +			if (size.width < limit.min.width ||
>> +			    size.width > limit.max.width ||
>> +			    size.height < limit.min.height ||
>> +			    size.height > limit.max.height ||
> 
> Would it be useful to add a contains() method to the Range class ?
> 
>> +			    size.width % limit.stepWidth ||
>> +			    size.height % limit.stepHeight)
> 
> Is this correct ? I would have considered the step differently than an
> alignment, as in the range containing all min + i * step values up to
> max. If min is not aligned to step, your check leads to a different
> result. In any case this should be clearly documented in the Range
> class.
> 
>> +				continue;
>> +
>> +			sizes.push_back(size);
>> +		}
>> +	}
>> +
>> +	std::sort(sizes.begin(), sizes.end());
>> +
>> +	return sizes;
>> +}
>> +
>> +/**
>> + * \fn StreamFormats::isRange()
>> + * \brief Check if the StreamFormat is a range description
>> + * \returns True if the description is a range, false otherwise
>> + */
>> +
>> +/**
>> + * \brief Retrive the range description
>> + * \param[in] pixelformat Pixel format to retrive description for
> 
> This doesn't document that the function is only available for
> range-based StreamFormats instances, and what happens in the pixel
> format is not supported. Furthermore, I think the method should also
> support the discrete sizes, by returning a range made from the minimum
> and maximum size. Otherwise it gets quite inconvenient for applications
> that want to quickly check what the minimum and maximum sizes here.
> We'll run into the issue of what to report as a step in that case, one
> option would be to set the step to 0 to report that there's no single
> step value, but other options may be possible.
> 
> All in all I think this class suffers from similar issues than
> FormatEnum, we need to think more about how it will be used to offer a
> nice, clean and clear (and clearly documented) API to applications.
> 
>> + * \returns The range description
>> + */
>> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const
>> +{
>> +	auto const it = rangeSizes_.find(pixelformat);
>> +
>> +	if (it == rangeSizes_.end())
>> +		return {};
>> +
>> +	return it->second;
>> +}
>> +
>>  /**
>>   * \class Stream
>>   * \brief Video stream for a camera
> 

Would we later use this class somehow to handle 'format negotiation'
between two entities? I.e. given two sets of formats, return a third set
which is the intersection of both?

Hi Kieran,

On Sun, May 19, 2019 at 11:23:38AM +0100, Kieran Bingham wrote:
> On 18/05/2019 19:13, Laurent Pinchart wrote:
> > On Sat, May 18, 2019 at 02:06:17AM +0300, Laurent Pinchart wrote:
> >> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>
> >> Add a StreamFormats which describes all the formats a stream can
> >> support. The object does not collect any formation itself but can
> >> simplify users interaction with formats as it's able to translate a
> >> stream format range into discrete steps.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> ---
> >>  include/libcamera/stream.h |  30 ++++++
> >>  src/libcamera/stream.cpp   | 191 +++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 221 insertions(+)
> >>
> >> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> >> index 47c007ed52e2..138ed649ba8c 100644
> >> --- a/include/libcamera/stream.h
> >> +++ b/include/libcamera/stream.h
> >> @@ -7,6 +7,7 @@
> >>  #ifndef __LIBCAMERA_STREAM_H__
> >>  #define __LIBCAMERA_STREAM_H__
> >>  
> >> +#include <map>
> >>  #include <string>
> >>  #include <vector>
> >>  
> >> @@ -39,6 +40,35 @@ enum StreamRole {
> >>  	Viewfinder,
> >>  };
> >>  
> >> +class StreamFormats
> >> +{
> >> +public:
> >> +	struct Range {
> >> +		Size min;
> >> +		Size max;
> >> +		unsigned int stepWidth;
> >> +		unsigned int stepHeight;
> >> +	};
> > 
> > This is very similar to the SizeRange structure defined in geometry.h.
> > Would it make sense to extend that structure instead ?
> > 
> > I would also rename stepWidth and stepHeight to hStep and vStep
> > respectively, but that may just be me.
> > 
> >> +
> >> +	StreamFormats() {}
> >> +
> >> +	StreamFormats(const std::map<unsigned int, Range> &rangeSizes)
> >> +		: rangeSizes_(rangeSizes) {}
> >> +
> >> +	StreamFormats(const std::map<unsigned int, std::vector<Size>> &discreteSizes)
> >> +		: discreteSizes_(discreteSizes) {}
> > 
> > Would it be useful to give an alias to these types, the same way we have
> > FormatEnum ? And shouldn't FormatEnum be replaced by this class ? We may
> > then want to rename StreamFormats to something that doesn't use the word
> > Stream, as it's not just related to streams.
> > 
> >> +
> >> +	std::vector<unsigned int> formats() const;
> >> +	std::vector<Size> sizes(unsigned int pixelformat) const;
> >> +
> >> +	bool isRange() const { return !rangeSizes_.empty(); }
> >> +	Range range(unsigned int pixelformat) const;
> > 
> > You should return a const Range & to optimise the case where the caller
> > only wants to inspect the range and doesn't need a copy.
> > 
> >> +
> >> +private:
> >> +	std::map<unsigned int, std::vector<Size>> discreteSizes_;
> >> +	std::map<unsigned int, Range> rangeSizes_;
> >> +};
> >> +
> >>  using StreamRoles = std::vector<StreamRole>;
> >>  
> >>  class Stream
> >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> >> index 0c59a31a3a05..e82fa8143b8f 100644
> >> --- a/src/libcamera/stream.cpp
> >> +++ b/src/libcamera/stream.cpp
> >> @@ -7,6 +7,7 @@
> >>  
> >>  #include <libcamera/stream.h>
> >>  
> >> +#include <algorithm>
> >>  #include <iomanip>
> >>  #include <sstream>
> >>  
> >> @@ -122,6 +123,196 @@ std::string StreamConfiguration::toString() const
> >>   * \brief A vector of StreamRole
> >>   */
> >>  
> >> +/**
> >> + * \class StreamFormats
> >> + * \brief Hold information about supported stream formats
> >> + *
> >> + * The StreamFormats class holds information about pixel formats and frame
> >> + * sizes a stream supports. The class groups size information by the pixel
> >> + * format which can produce it. There are two types of size information which
> >> + * can be described in the StreamFormats object discrete and range sizes.
> >> + *
> >> + * The discrete sizes are a list of fixed sizes of the only resolutions the
> >> + * stream can produce. While the range description contains a max and min
> >> + * size together with a stepping. The range information can either be consumed
> >> + * raw which allows users to calculate a size which the stream could support
> >> + * or be accessed thru the sizes() helper which will compute a list of common
> >> + * discrete sizes which can be produced within the range.
> >> + */
> > 
> > I think we'll have to refine both the API and its documentation, to
> > explain better how this operates and is supposed to be used. Let's
> > discuss the issues pointed out through this patch first.
> > 
> >> +
> >> +/**
> >> + * \class StreamFormats::Range
> >> + * \brief Hold information about stream format range
> >> + */
> >> +
> >> +/**
> >> + * \var StreamFormats::Range::min
> >> + * \brief Range minimum size
> >> + */
> >> +
> >> +/**
> >> + * \var StreamFormats::Range::max
> >> + * \brief Range maximum size
> >> + */
> >> +
> >> +/**
> >> + * \var StreamFormats::Range::stepWidth
> >> + * \brief Range width step length in pixels
> >> + */
> >> +
> >> +/**
> >> + * \var StreamFormats::Range::stepHeight
> >> + * \brief Range height step length in pixels
> >> + */
> >> +
> >> +/**
> >> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, Range > &rangeSizes)
> >> + * \brief Constrict a ranged based StreamFormats object
> > 
> > s/Constrict/Construct/
> > 
> >> + * \param[in] rangeSizes A map of pixel format to a ranged description
> >> + * \sa StreamFormats::Range
> >> + */
> >> +
> >> +/**
> >> + * \fn StreamFormats::StreamFormats(const std::map< unsigned int, std::vector< Size >> &discreteSizes)
> >> + * \brief Constrict a discrete based StreamFormats object
> >> + * \param[in] discreteSizes A map of pixel format to a list of frame sizes
> >> + */
> >> +
> >> +/**
> >> + * \brief Retrive a list of pixel formats supported by the stram
> > 
> > s/Retrive/Retrieve/ (in multiple locations)
> > s/a list/the list/
> > s/stram/stream/
> > 
> >> + * \returns A list of pixel formats
> >> + */
> >> +std::vector<unsigned int> StreamFormats::formats() const
> >> +{
> >> +	std::vector<unsigned int> formats;
> >> +
> >> +	if (isRange()) {
> >> +		for (auto const &it : rangeSizes_)
> >> +			formats.push_back(it.first);
> >> +	} else {
> >> +		for (auto const &it : discreteSizes_)
> >> +			formats.push_back(it.first);
> >> +	}
> >> +
> >> +	return formats;
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrive a list of frame sizes
> >> + * \param[in] pixelformat Pixel format to retrive sizes for
> > 
> > This needs to be expanded to explain the mechanism for range-based
> > sizes.
> > 
> >> + * \returns A list of frame sizes
> >> + */
> >> +std::vector<Size> StreamFormats::sizes(unsigned int pixelformat) const
> >> +{
> >> +	std::vector<Size> sizes;
> >> +
> >> +	/*
> >> +	 * Sizes to try and extract from ranges.
> >> +	 * \todo Verify list of resolutions are good
> > 
> > Could you explain where they come from ?
> > 
> >> +	 */
> >> +	static const std::vector<Size> rangeDescreteSizes = {
> 
> s/Descrete/Discrete/
> 
> >> +		Size(160, 120),
> >> +		Size(240, 160),
> >> +		Size(320, 240),
> >> +		Size(400, 240),
> >> +		Size(480, 320),
> >> +		Size(640, 360),
> >> +		Size(640, 480),
> >> +		Size(720, 480),
> >> +		Size(720, 576),
> >> +		Size(768, 480),
> >> +		Size(800, 600),
> >> +		Size(854, 480),
> >> +		Size(960, 540),
> >> +		Size(960, 640),
> >> +		Size(1024, 576),
> >> +		Size(1024, 600),
> >> +		Size(1024, 768),
> >> +		Size(1152, 864),
> >> +		Size(1280, 1024),
> >> +		Size(1280, 1080),
> >> +		Size(1280, 720),
> >> +		Size(1280, 800),
> >> +		Size(1360, 768),
> >> +		Size(1366, 768),
> >> +		Size(1400, 1050),
> >> +		Size(1440, 900),
> >> +		Size(1536, 864),
> >> +		Size(1600, 1200),
> >> +		Size(1600, 900),
> >> +		Size(1680, 1050),
> >> +		Size(1920, 1080),
> >> +		Size(1920, 1200),
> >> +		Size(2048, 1080),
> >> +		Size(2048, 1152),
> >> +		Size(2048, 1536),
> >> +		Size(2160, 1080),
> >> +		Size(2560, 1080),
> >> +		Size(2560, 1440),
> >> +		Size(2560, 1600),
> >> +		Size(2560, 2048),
> >> +		Size(2960, 1440),
> >> +		Size(3200, 1800),
> >> +		Size(3200, 2048),
> >> +		Size(3200, 2400),
> >> +		Size(3440, 1440),
> >> +		Size(3840, 1080),
> >> +		Size(3840, 1600),
> >> +		Size(3840, 2160),
> >> +		Size(3840, 2400),
> >> +		Size(4096, 2160),
> >> +		Size(5120, 2160),
> >> +		Size(5120, 2880),
> >> +		Size(7680, 4320),
> >> +	};
> 
> Is that a list that we could generate somehow instead from some conditions?
> 
> Perhaps generate based on aspect ratios and known widths and multipliers
> or something... not sure it will be any better - but it might be more
> readable or easy to maintain?
> 
> >> +
> >> +	if (!isRange()) {
> >> +		auto const &it = discreteSizes_.find(pixelformat);
> >> +		if (it == discreteSizes_.end())
> >> +			return {};
> >> +		sizes = it->second;
> >> +	} else {
> >> +		Range limit = range(pixelformat);
> >> +
> >> +		for (const Size &size : rangeDescreteSizes) {
> >> +			if (size.width < limit.min.width ||
> >> +			    size.width > limit.max.width ||
> >> +			    size.height < limit.min.height ||
> >> +			    size.height > limit.max.height ||
> > 
> > Would it be useful to add a contains() method to the Range class ?
> > 
> >> +			    size.width % limit.stepWidth ||
> >> +			    size.height % limit.stepHeight)
> > 
> > Is this correct ? I would have considered the step differently than an
> > alignment, as in the range containing all min + i * step values up to
> > max. If min is not aligned to step, your check leads to a different
> > result. In any case this should be clearly documented in the Range
> > class.
> > 
> >> +				continue;
> >> +
> >> +			sizes.push_back(size);
> >> +		}
> >> +	}
> >> +
> >> +	std::sort(sizes.begin(), sizes.end());
> >> +
> >> +	return sizes;
> >> +}
> >> +
> >> +/**
> >> + * \fn StreamFormats::isRange()
> >> + * \brief Check if the StreamFormat is a range description
> >> + * \returns True if the description is a range, false otherwise
> >> + */
> >> +
> >> +/**
> >> + * \brief Retrive the range description
> >> + * \param[in] pixelformat Pixel format to retrive description for
> > 
> > This doesn't document that the function is only available for
> > range-based StreamFormats instances, and what happens in the pixel
> > format is not supported. Furthermore, I think the method should also
> > support the discrete sizes, by returning a range made from the minimum
> > and maximum size. Otherwise it gets quite inconvenient for applications
> > that want to quickly check what the minimum and maximum sizes here.
> > We'll run into the issue of what to report as a step in that case, one
> > option would be to set the step to 0 to report that there's no single
> > step value, but other options may be possible.
> > 
> > All in all I think this class suffers from similar issues than
> > FormatEnum, we need to think more about how it will be used to offer a
> > nice, clean and clear (and clearly documented) API to applications.
> > 
> >> + * \returns The range description
> >> + */
> >> +StreamFormats::Range StreamFormats::range(unsigned int pixelformat) const
> >> +{
> >> +	auto const it = rangeSizes_.find(pixelformat);
> >> +
> >> +	if (it == rangeSizes_.end())
> >> +		return {};
> >> +
> >> +	return it->second;
> >> +}
> >> +
> >>  /**
> >>   * \class Stream
> >>   * \brief Video stream for a camera
> > 
> 
> Would we later use this class somehow to handle 'format negotiation'
> between two entities? I.e. given two sets of formats, return a third set
> which is the intersection of both?

I think we need classes to help with format propagation along the
pipeline. I'm more and more wondering if we need two classes for this, a
V4L2-oriented class to be used internally, and a friendlier class
exposed to applications.