[libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2: Consolidate information about formats

[Jacopo Mondi]

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) ?

> > > > > +		[](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