| Message ID | 20200701210948.999363-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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() ? -- Kieran > Signed-off-by: Niklas Söderlund <niklas.soderlund@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"; >
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@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
Hi Kieran On Wed, Jul 01, 2020 at 10:42:38PM +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'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 :( > > -- > Kieran > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 02/07/2020 09:07, Jacopo Mondi wrote: > Hi Kieran > > On Wed, Jul 01, 2020 at 10:42:38PM +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'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. Thanks, that's a lot clearer. I wonder if we should capture some of that in the commit message at 1/2 ... > It's a shame we can't overload on return value :( Yes, I guess being able to return a set or a vector could also make 'every one happy' ... but I'm not overly concerned. A vector should be fine. Thanks for the description. Kieran. >> -- >> Kieran >> >> >> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@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 >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
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@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 >
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. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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";
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Laurent, On 02/07/2020 22:47, Niklas Söderlund wrote: > 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. Yes, I think that was the agreement in the other mails ;-) > > I like this, if we can scrape together some R-b we can take this to the > next level :-) Ha, ok well I suppose I could actually provide a tag too ... For both patches in this series: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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";
Use a helper instead of local code to retrieve all keys from a map. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/cio2.cpp | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)