[libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add StreamFormats
Kieran Bingham
kieran.bingham at ideasonboard.com
Sun May 19 12:23:38 CEST 2019
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 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 = {
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?
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list