[libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add StreamFormats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 18 20:13:34 CEST 2019
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 at 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 at 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list