[libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2: Consolidate information about formats
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jun 25 02:12:59 CEST 2020
Hi Laurent and Jacopo,
On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:
> Hi Jacopo and Niklas,
>
> 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.
>
> > > 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.
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?
>
> before the std::transform(). I think a set is indeed overkill.
>
> > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > + std::inserter(cio2Codes, cio2Codes.begin()),
> > > + [](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.
>
> 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