[libcamera-devel] [PATCH v2 2/2] libcamera: ipu3: cio2: Make use of utils::map_keys() helper

[Niklas Söderlund]

Hello,

On 2020-07-02 11:59:18 +0300, Laurent Pinchart wrote:
> Hello everybody,
> 
> On Thu, Jul 02, 2020 at 10:07:44AM +0200, Jacopo Mondi wrote:
> > On Wed, Jul 01, 2020 at 10:42:38PM +0100, Kieran Bingham wrote:
> > > On 01/07/2020 22:09, Niklas Söderlund wrote:
> > > > Use a helper instead of local code to retrieve all keys from a map.
> > > >
> > >
> > > I see this is the reason for changing Laurent's patch to return a
> > > vector, but couldn't this patch update things to use sets locally?
> > >
> > > The keys are 'unique' right? Is there a distinct benefit for returning a
> > > vector over a set?
> > >
> > > Perhaps there is a performance improvement with a vector if it doesn't
> > > need to ensure uniqueness? But I'd be surprised if it was much... but I
> > > don't know.
> > >
> > > Or is it to prevent updating the functions that the set (vector) is then
> > > passed to, i.e. sensor_->getFormat() ?
> > 
> > I'll try to summarize my understanding of the discussion.
> > 
> > When considered in isolation from the context, yes, this function
> > should return an std::set: keys in a map are sorted and unique and
> > std::set makes that explicit so the user doesn't have to inspect what
> > is returned (if it's a vector) to make sure about this.
> > 
> > When this is applied to the context of enumerating image formats (like
> > in the series Niklas has posted yesterday), I think introducing
> > std::set there will very quickly spread to CameraSensor and V4L2
> > video (sub)device and I would be less confortable with that. Mostly
> > because, when we use this for enumerating mbus or fourcc codes, we
> > already have a guarantee that
> > 1) the codes are unique, otherwise is a driver bug
> > 2) the vector is generated iterating the map's key, and the resulting
> > sorting order will be the same.
> > 
> > Using std::set you pay a performance price, as the container needs to be
> > kept sorted, and possibly a small penalty in space occupation as well,
> > as sets are usually implemented using trees instead of being a simpler
> > contigous chunk of allocated space as vectors. That said, we're
> > working with number of item where all these considerations are moot,
> > we have very few elements, so I would consider std::set and
> > std::vector to be equally performant. My main concern is that std::set
> > would soon take over all our APIs and we'll find ourselves to have at
> > some point to convert between set and vectors, which I would really
> > not like to.
> > 
> > It's a shame we can't overload on return value :(
> 
> How about starting with std::vector and seeing where it leads us ? It's
> not like we'll set this in stone.

I like this, if we can scrape together some R-b we can take this to the 
next level :-)

> 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/cio2.cpp | 19 +++----------------
> > > >  1 file changed, 3 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > index aa1459fb3599283b..7941c6845cbc9a9a 100644
> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > @@ -95,11 +95,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > >  	 * utils::set_overlap requires the ranges to be sorted, keep the
> > > >  	 * cio2Codes vector sorted in ascending order.
> > > >  	 */
> > > > -	std::vector<unsigned int> cio2Codes;
> > > > -	cio2Codes.reserve(mbusCodesToInfo.size());
> > > > -	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > > -		       std::back_inserter(cio2Codes),
> > > > -		       [](auto &pair) { return pair.first; });
> > > > +	std::vector<unsigned int> cio2Codes = utils::map_keys(mbusCodesToInfo);
> > > >  	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > > >  				cio2Codes.begin(), cio2Codes.end())) {
> > > > @@ -145,12 +141,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > > >  	 * Apply the selected format to the sensor, the CSI-2 receiver and
> > > >  	 * the CIO2 output device.
> > > >  	 */
> > > > -	std::vector<unsigned int> mbusCodes;
> > > > -	mbusCodes.reserve(mbusCodesToInfo.size());
> > > > -	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > > -		       std::back_inserter(mbusCodes),
> > > > -		       [](auto &pair) { return pair.first; });
> > > > -
> > > > +	std::vector<unsigned int> mbusCodes = utils::map_keys(mbusCodesToInfo);
> > > >  	sensorFormat = sensor_->getFormat(mbusCodes, size);
> > > >  	ret = sensor_->setFormat(&sensorFormat);
> > > >  	if (ret)
> > > > @@ -189,11 +180,7 @@ CIO2Device::generateConfiguration(Size size) const
> > > >  		size = sensor_->resolution();
> > > >
> > > >  	/* Query the sensor static information for closest match. */
> > > > -	std::vector<unsigned int> mbusCodes;
> > > > -	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > > > -		       std::back_inserter(mbusCodes),
> > > > -		       [](auto &pair) { return pair.first; });
> > > > -
> > > > +	std::vector<unsigned int> mbusCodes = utils::map_keys(mbusCodesToInfo);
> > > >  	V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> > > >  	if (!sensorFormat.mbus_code) {
> > > >  		LOG(IPU3, Error) << "Sensor does not support mbus code";
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund