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

[Kieran Bingham]

Hi Niklas,

On 01/07/2020 22:56, Niklas Söderlund wrote:
> 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).
> 


It seems I've read Jacopo's response first, so now I have a better
understanding of what is needed.

No objections to returning / using a vector as it simplifies things ;-)

--
Kieran




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