[libcamera-devel] [PATCH/RFC 08/12] libcamera: stream: Add StreamFormats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun May 19 16:51:56 CEST 2019
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 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?
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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list