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

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2020-07-01 22:42:38 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> 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 would prefer it to be a std::set, but par the discussion around v1 of 
this patch and the last two versions of the series that introduce 
CIO2Devcice (which is now merged) it seemed the quickest way to avoid 
bikeshedding making it a std::vector.

We can always later switch the container to a std::set if it becomes 
less trouble then remember to call std::sort() where it mattes, or for 
that matter start with calling std::sort() in the helper. As it is now 
the helper leaves no guarantees of the order of the keys so we are not 
committing to anything (yet).

> 
> --
> Kieran
> 
> 
> 
> > 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
> --
> Kieran

-- 
Regards,
Niklas Söderlund