[libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2: Consolidate information about formats
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jun 25 23:33:18 CEST 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-06-25 09:56:05 +0200, Jacopo Mondi wrote:
> Hello,
>
> On Thu, Jun 25, 2020 at 04:23:46AM +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote:
> > > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:
> > > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:
> > > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:
> > > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC
> > > > > > all over the CIO2 code collect it in a single map and extract the data
> > > > > > from it. This is done in preparation of adding PixelFormat information
> > > > > > to the mix.
> > > > > >
> > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > > > ---
> > > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------
> > > > > > 1 file changed, 24 insertions(+), 26 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644
> > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > > > @@ -18,6 +18,17 @@ namespace libcamera {
> > > > > >
> > > > > > LOG_DECLARE_CATEGORY(IPU3)
> > > > > >
> > > > > > +namespace {
> > > > > > +
> > > > > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> > > > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> > > > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> > > > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> > > > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace */
> > > > > > +
> > > > >
> > > > > Ok, I see where this is going with the next patch, but I'm not super
> > > > > excited by having to iterate the map just to extract keys (and anyway,
> > > > > why doesn't std::map have a keys() function ??).
> > > >
> > > > Possibly because its implementation would be O(n), and STL generally
> > > > tries to avoid hiding expensive operations behind apparently simple
> > > > functions. Just a speculation.
> > > >
>
> Could be!
>
> > > > > > CIO2Device::CIO2Device()
> > > > > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> > > > > > {
> > > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > > > > * Make sure the sensor produces at least one format compatible with
> > > > > > * the CIO2 requirements.
> > > > > > */
> > > > > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 };
> > > > > > + std::set<unsigned int> cio2Codes;
> > > > >
> > > > > why a set ?
> > > >
> > > > To keep this efficient, given that std::map is sorted by key, this could
> > > > be a vector, and we could call
> > > >
> > > > cio2Codes.reserve(mbusCodesToInfo.size());
> > >
> > > We would then also need to remember to sort the vector as that is a
> > > requirement for utils::set_overlap() where this is used. I think it's
> > > much nicer to use a set from the start to make sure nothing goes wrong.
> >
>
> Sorry but, we're extracting keys from a sorted map with unique
> entries. The resulting vector of keys will be sorted as well, isn't it ?
>
> And the requirement of being sorted depends on what you will use the
> vector for anway, it's not something an "extract keys" function should
> care about (and again, if' I'm not mistaken, the vector of keys will
> be anyhow sorted and unique, being the result of iterating an
> std::map)
>
> > I agree std::set has that benefit. Note that here we wouldn't need to
> > sort the vector explicitly. I thus don't really see a reason to use a
> > set in this specific case. However, maybe adding
> >
> > /*
> > * utils::set_overlap() requires the containers to be sorted,
> > * this is guaranteed for cio2Codes as it gets generated from a
> > * sorted map with a loop that preserves the order.
> > */
> >
> > could avoid surprises in the future.
> >
> > > I'm feeling a bit of resistance against set in this series. Is there a
> > > downside to using it I don't see? I think they are neat as a
> > > specialization of vector where we get guarantees there is only one
> > > occurrence of each value and the readout/sorting order will be the same
> > > no mater where the container where created. Am I missing something?
> >
> > std::set is (usually) implemented as a red-black tree. That will consume
> > more memory than a vector, and for small sets, will likely be slower
> > than creating a vector and sorting it (if needed). I'm not necessarily
> > opposed to usage of std::set in non-hot paths, but as noted in another
> > reply in this series, we would need to make that consistent then,
> > passing a set to CameraSensor::getFormats(), and possibly more
> > functions. I fear std::set would quickly spread through lots of places.
> >
>
> That's my fear as well. If we have utility functions returning set,
> callers will use set too, and indeed they're not efficient as vectors.
>
> > > > before the std::transform(). I think a set is indeed overkill.
> > > >
> > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > > > > + std::inserter(cio2Codes, cio2Codes.begin()),
> >
> > btw shouldn't this be end() and not begin() ?
> >
>
> As commented on Naush's patch, can this be handled with an
> std::back_inserter(cio2Codes) ?
If its a vector yes, if its a set no :-) Sets does not implement
push_back().
Since the goal of this series is not to create a (in my view) better API
for the CameraSensor I will will use vectors for this interface in the
next incarnation and will thus use the back_inserter here.
>
> > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > > > >
> > > > > you could
> > > > > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });
> > > > > or since C++14
> > > > > [](auto &pair){ return pair.first; });
> > > >
> > > > Much nicer indeed.
> > >
> > > Neat.
> > >
> > > > > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > > > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > > > > > cio2Codes.begin(), cio2Codes.end())) {
> > > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > > > > > * Apply the selected format to the sensor, the CSI-2 receiver and
> > > > > > * the CIO2 output device.
> > > > > > */
> > > > > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 },
> > > > > > - size);
> > > > > > + std::vector<unsigned int> mbusCodes;
> > > > >
> > > > > You could reserve space in the vector knowing the map size
> > > >
> > > > Seems we agree :-)
> > > >
> > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > > > > + std::back_inserter(mbusCodes),
> > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > > > > > +
> > > > > > + sensorFormat = sensor_->getFormat(mbusCodes, size);
> > > > > > ret = sensor_->setFormat(&sensorFormat);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > > > > > if (ret)
> > > > > > return ret;
> > > > > >
> > > > > > - V4L2PixelFormat v4l2Format;
> > > > > > - switch (sensorFormat.mbus_code) {
> > > > > > - case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> > > > > > - break;
> > > > > > - case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> > > > > > - break;
> > > > > > - case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> > > > > > - break;
> > > > > > - case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> > > > > > - break;
> > > > > > - default:
> > > > > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);
> > > > > > + if (itInfo == mbusCodesToInfo.end())
> > > > > > return -EINVAL;
> > > > > > - }
> > > > > >
> > > > > > - outputFormat->fourcc = v4l2Format;
> > > > > > + outputFormat->fourcc = itInfo->second;
> > > > >
> > > > > I would be tempted to suggest you to add helper functions around that
> > > > > map, not sure it's worth the effort, but those transform to just
> > > > > retreive keys are not nice imho.
> > > >
> > > > I was about to mention that, thinking that the vector of keys could then
> > > > be cached, but given that the code is only used in init paths, it may
> > > > not be worth it.
> > >
> > > I see two options for this problem, the one used in this patch or
> > > creating multiple data structures in the anonyms namespace, one for each
> > > way to lookup something. The later consumes less CPU but consumes more
> > > memory, I'm not sure which on is the best option here. I went with this
> > > option as it prevents possible differences between the multiple data
> > > structures for lookup. I'm open to switching to that option if that is
> > > the consensus.
> >
> > If it was used in more places I'd cache the keys the first time they're
> > used, but it's not worth it here. Same for reverse lookups, I'd create
> > and cache a reverse map on the first call to the helper, but only if it
> > really makes a difference.
> >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > > That apart
> > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > >
> > > > > > outputFormat->size = sensorFormat.size;
> > > > > > outputFormat->planesCount = 1;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list