[libcamera-devel,v3,01/10] libcamera: camera_sensor: Make mbusCodes() return a set
diff mbox series

Message ID 20200623020930.1781469-2-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: ipu3: Allow zero-copy RAW stream
Related show

Commit Message

Niklas Söderlund June 23, 2020, 2:09 a.m. UTC 
It makes little sens for a CameraSensor to support the same media bus
format multiple times, change the container from a vector<> to a set<>.

This was discovered while working with the IPU3 pipeline handler that
Unnecessarily had to carry code to sort vectors in order to find
intersections. As this change requires switching the container to set<>
in that pipeline handler as well fix this while we are at it.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/internal/camera_sensor.h |  4 ++--
 include/libcamera/internal/formats.h       |  2 +-
 src/libcamera/camera_sensor.cpp            |  1 -
 src/libcamera/formats.cpp                  |  7 +++----
 src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
 test/camera-sensor.cpp                     |  2 +-
 6 files changed, 12 insertions(+), 17 deletions(-)

Comments

Jacopo Mondi June 24, 2020, 9:18 a.m. UTC | #1 
Hi Niklas,

On Tue, Jun 23, 2020 at 04:09:21AM +0200, Niklas Söderlund wrote:
> It makes little sens for a CameraSensor to support the same media bus

sense
s/a CameraSensor/the CameraSensor class/
> format multiple times, change the container from a vector<> to a set<>.
>
> This was discovered while working with the IPU3 pipeline handler that
> Unnecessarily had to carry code to sort vectors in order to find

s/Un/un/

> intersections. As this change requires switching the container to set<>
> in that pipeline handler as well fix this while we are at it.

as well,

Which sensor does report mulitple times the same mbus_code ? Isn't
this a driver bug if it happens ?

I see  V4L2Subdevice::formats() populating the image formats with the
mbus codes returned by V4L2Subdevice::enumPadCodes() and when I print
out on Soraka the codes there collected I only get MEDIA_BUS_FMT_SGRBG10_1X10.

Looking at the drivers of the two there installed sensors confirms
that only one format is supported..

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/internal/camera_sensor.h |  4 ++--
>  include/libcamera/internal/formats.h       |  2 +-
>  src/libcamera/camera_sensor.cpp            |  1 -
>  src/libcamera/formats.cpp                  |  7 +++----
>  src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
>  test/camera-sensor.cpp                     |  2 +-
>  6 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 7f07413f95602881..6d92818f16e4b752 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -50,7 +50,7 @@ public:
>
>  	const std::string &model() const { return model_; }
>  	const MediaEntity *entity() const { return entity_; }
> -	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> +	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> &sizes() const { return sizes_; }
>  	const Size &resolution() const { return resolution_; }
>
> @@ -77,7 +77,7 @@ private:
>
>  	ImageFormats formats_;
>  	Size resolution_;
> -	std::vector<unsigned int> mbusCodes_;
> +	std::set<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>
>  	ControlList properties_;
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 4b172efc6588d374..e8aa555dcbe99350 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -24,7 +24,7 @@ public:
>  	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
>
>  	bool isEmpty() const;
> -	std::vector<unsigned int> formats() const;
> +	std::set<unsigned int> formats() const;
>  	const std::vector<SizeRange> &sizes(unsigned int format) const;
>  	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index b14b4051dca606b9..5ab50de660534f3f 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -251,7 +251,6 @@ int CameraSensor::init()
>  	}
>
>  	mbusCodes_ = formats_.formats();
> -	std::sort(mbusCodes_.begin(), mbusCodes_.end());
>
>  	for (const auto &format : formats_.data()) {
>  		const std::vector<SizeRange> &ranges = format.second;
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 1272de29c802c539..6234fd98f3e2002c 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -68,14 +68,13 @@ bool ImageFormats::isEmpty() const
>   * \brief Retrieve a list of all supported image formats
>   * \return List of pixel formats or media bus codes
>   */
> -std::vector<unsigned int> ImageFormats::formats() const
> +std::set<unsigned int> ImageFormats::formats() const
>  {
> -	std::vector<unsigned int> formats;
> -	formats.reserve(data_.size());
> +	std::set<unsigned int> formats;
>
>  	/* \todo: Should this be cached instead of computed each time? */
>  	for (auto const &it : data_)
> -		formats.push_back(it.first);
> +		formats.insert(it.first);
>
>  	return formats;
>  }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5544288b14f739c0..1a59de0c58975b3c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1441,15 +1441,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	/*
>  	 * Make sure the sensor produces at least one format compatible with
>  	 * the CIO2 requirements.
> -	 *
> -	 * utils::set_overlap requires the ranges to be sorted, keep the
> -	 * cio2Codes vector sorted in ascending order.
>  	 */
> -	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> -						   MEDIA_BUS_FMT_SGRBG10_1X10,
> -						   MEDIA_BUS_FMT_SGBRG10_1X10,
> -						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> -	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> +	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 };
> +	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
>  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
>  				cio2Codes.begin(), cio2Codes.end())) {
>  		LOG(IPU3, Error)
> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
> --- a/test/camera-sensor.cpp
> +++ b/test/camera-sensor.cpp
> @@ -67,7 +67,7 @@ protected:
>  			return TestFail;
>  		}
>
> -		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
> +		const std::set<unsigned int> &codes = sensor_->mbusCodes();
>  		auto iter = std::find(codes.begin(), codes.end(),
>  				      MEDIA_BUS_FMT_ARGB8888_1X32);
>  		if (iter == codes.end()) {
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund June 24, 2020, 5:53 p.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2020-06-24 11:18:10 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Jun 23, 2020 at 04:09:21AM +0200, Niklas Söderlund wrote:
> > It makes little sens for a CameraSensor to support the same media bus
> 
> sense
> s/a CameraSensor/the CameraSensor class/
> > format multiple times, change the container from a vector<> to a set<>.
> >
> > This was discovered while working with the IPU3 pipeline handler that
> > Unnecessarily had to carry code to sort vectors in order to find
> 
> s/Un/un/
> 
> > intersections. As this change requires switching the container to set<>
> > in that pipeline handler as well fix this while we are at it.
> 
> as well,
> 
> Which sensor does report mulitple times the same mbus_code ? Isn't
> this a driver bug if it happens ?

None does so today, and if it happens I would indeed consider it a 
driver bug. That is the reason for this change, to us an API to 
demonstrate that it's impossible to have the same format twice.

As an added bonus the set will always be sorted so the possibility that 
an implementation somewhere forgets to sort the vector is completely 
avoided.

> 
> I see  V4L2Subdevice::formats() populating the image formats with the
> mbus codes returned by V4L2Subdevice::enumPadCodes() and when I print
> out on Soraka the codes there collected I only get MEDIA_BUS_FMT_SGRBG10_1X10.
> 
> Looking at the drivers of the two there installed sensors confirms
> that only one format is supported..
> 
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  4 ++--
> >  include/libcamera/internal/formats.h       |  2 +-
> >  src/libcamera/camera_sensor.cpp            |  1 -
> >  src/libcamera/formats.cpp                  |  7 +++----
> >  src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
> >  test/camera-sensor.cpp                     |  2 +-
> >  6 files changed, 12 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 7f07413f95602881..6d92818f16e4b752 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -50,7 +50,7 @@ public:
> >
> >  	const std::string &model() const { return model_; }
> >  	const MediaEntity *entity() const { return entity_; }
> > -	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > +	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
> >  	const std::vector<Size> &sizes() const { return sizes_; }
> >  	const Size &resolution() const { return resolution_; }
> >
> > @@ -77,7 +77,7 @@ private:
> >
> >  	ImageFormats formats_;
> >  	Size resolution_;
> > -	std::vector<unsigned int> mbusCodes_;
> > +	std::set<unsigned int> mbusCodes_;
> >  	std::vector<Size> sizes_;
> >
> >  	ControlList properties_;
> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > index 4b172efc6588d374..e8aa555dcbe99350 100644
> > --- a/include/libcamera/internal/formats.h
> > +++ b/include/libcamera/internal/formats.h
> > @@ -24,7 +24,7 @@ public:
> >  	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> >
> >  	bool isEmpty() const;
> > -	std::vector<unsigned int> formats() const;
> > +	std::set<unsigned int> formats() const;
> >  	const std::vector<SizeRange> &sizes(unsigned int format) const;
> >  	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index b14b4051dca606b9..5ab50de660534f3f 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -251,7 +251,6 @@ int CameraSensor::init()
> >  	}
> >
> >  	mbusCodes_ = formats_.formats();
> > -	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> >
> >  	for (const auto &format : formats_.data()) {
> >  		const std::vector<SizeRange> &ranges = format.second;
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 1272de29c802c539..6234fd98f3e2002c 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -68,14 +68,13 @@ bool ImageFormats::isEmpty() const
> >   * \brief Retrieve a list of all supported image formats
> >   * \return List of pixel formats or media bus codes
> >   */
> > -std::vector<unsigned int> ImageFormats::formats() const
> > +std::set<unsigned int> ImageFormats::formats() const
> >  {
> > -	std::vector<unsigned int> formats;
> > -	formats.reserve(data_.size());
> > +	std::set<unsigned int> formats;
> >
> >  	/* \todo: Should this be cached instead of computed each time? */
> >  	for (auto const &it : data_)
> > -		formats.push_back(it.first);
> > +		formats.insert(it.first);
> >
> >  	return formats;
> >  }
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 5544288b14f739c0..1a59de0c58975b3c 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1441,15 +1441,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >  	/*
> >  	 * Make sure the sensor produces at least one format compatible with
> >  	 * the CIO2 requirements.
> > -	 *
> > -	 * utils::set_overlap requires the ranges to be sorted, keep the
> > -	 * cio2Codes vector sorted in ascending order.
> >  	 */
> > -	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > -						   MEDIA_BUS_FMT_SGRBG10_1X10,
> > -						   MEDIA_BUS_FMT_SGBRG10_1X10,
> > -						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> > -	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > +	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 };
> > +	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> >  				cio2Codes.begin(), cio2Codes.end())) {
> >  		LOG(IPU3, Error)
> > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
> > --- a/test/camera-sensor.cpp
> > +++ b/test/camera-sensor.cpp
> > @@ -67,7 +67,7 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > -		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
> > +		const std::set<unsigned int> &codes = sensor_->mbusCodes();
> >  		auto iter = std::find(codes.begin(), codes.end(),
> >  				      MEDIA_BUS_FMT_ARGB8888_1X32);
> >  		if (iter == codes.end()) {
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 24, 2020, 11:44 p.m. UTC | #3 
Hi Niklas,

On Wed, Jun 24, 2020 at 07:53:37PM +0200, Niklas Söderlund wrote:
> On 2020-06-24 11:18:10 +0200, Jacopo Mondi wrote:
> > On Tue, Jun 23, 2020 at 04:09:21AM +0200, Niklas Söderlund wrote:
> > > It makes little sens for a CameraSensor to support the same media bus
> > 
> > sense
> > s/a CameraSensor/the CameraSensor class/

Or "for a camera sensor".

> > > format multiple times, change the container from a vector<> to a set<>.
> > >
> > > This was discovered while working with the IPU3 pipeline handler that
> > > Unnecessarily had to carry code to sort vectors in order to find
> > 
> > s/Un/un/
> > 
> > > intersections. As this change requires switching the container to set<>
> > > in that pipeline handler as well fix this while we are at it.
> > 
> > as well,
> > 
> > Which sensor does report mulitple times the same mbus_code ? Isn't
> > this a driver bug if it happens ?
> 
> None does so today, and if it happens I would indeed consider it a 
> driver bug. That is the reason for this change, to us an API to 
> demonstrate that it's impossible to have the same format twice.
> 
> As an added bonus the set will always be sorted so the possibility that 
> an implementation somewhere forgets to sort the vector is completely 
> avoided.

std::set is however more complex than std::vector. None of this should
run in a hot path, so I'm ok with the change, but I think we need to
take complexity into account in API design.

> > I see  V4L2Subdevice::formats() populating the image formats with the
> > mbus codes returned by V4L2Subdevice::enumPadCodes() and when I print
> > out on Soraka the codes there collected I only get MEDIA_BUS_FMT_SGRBG10_1X10.
> > 
> > Looking at the drivers of the two there installed sensors confirms
> > that only one format is supported..
> > 
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  4 ++--
> > >  include/libcamera/internal/formats.h       |  2 +-
> > >  src/libcamera/camera_sensor.cpp            |  1 -
> > >  src/libcamera/formats.cpp                  |  7 +++----
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
> > >  test/camera-sensor.cpp                     |  2 +-
> > >  6 files changed, 12 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 7f07413f95602881..6d92818f16e4b752 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -50,7 +50,7 @@ public:
> > >
> > >  	const std::string &model() const { return model_; }
> > >  	const MediaEntity *entity() const { return entity_; }
> > > -	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > +	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > >  	const std::vector<Size> &sizes() const { return sizes_; }
> > >  	const Size &resolution() const { return resolution_; }
> > >
> > > @@ -77,7 +77,7 @@ private:
> > >
> > >  	ImageFormats formats_;
> > >  	Size resolution_;
> > > -	std::vector<unsigned int> mbusCodes_;
> > > +	std::set<unsigned int> mbusCodes_;
> > >  	std::vector<Size> sizes_;
> > >
> > >  	ControlList properties_;
> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > index 4b172efc6588d374..e8aa555dcbe99350 100644
> > > --- a/include/libcamera/internal/formats.h
> > > +++ b/include/libcamera/internal/formats.h
> > > @@ -24,7 +24,7 @@ public:
> > >  	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> > >
> > >  	bool isEmpty() const;
> > > -	std::vector<unsigned int> formats() const;
> > > +	std::set<unsigned int> formats() const;
> > >  	const std::vector<SizeRange> &sizes(unsigned int format) const;
> > >  	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index b14b4051dca606b9..5ab50de660534f3f 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -251,7 +251,6 @@ int CameraSensor::init()
> > >  	}
> > >
> > >  	mbusCodes_ = formats_.formats();
> > > -	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> > >
> > >  	for (const auto &format : formats_.data()) {
> > >  		const std::vector<SizeRange> &ranges = format.second;
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index 1272de29c802c539..6234fd98f3e2002c 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -68,14 +68,13 @@ bool ImageFormats::isEmpty() const
> > >   * \brief Retrieve a list of all supported image formats
> > >   * \return List of pixel formats or media bus codes
> > >   */
> > > -std::vector<unsigned int> ImageFormats::formats() const
> > > +std::set<unsigned int> ImageFormats::formats() const
> > >  {
> > > -	std::vector<unsigned int> formats;
> > > -	formats.reserve(data_.size());
> > > +	std::set<unsigned int> formats;
> > >
> > >  	/* \todo: Should this be cached instead of computed each time? */
> > >  	for (auto const &it : data_)
> > > -		formats.push_back(it.first);
> > > +		formats.insert(it.first);
> > >
> > >  	return formats;
> > >  }
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 5544288b14f739c0..1a59de0c58975b3c 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1441,15 +1441,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > >  	/*
> > >  	 * Make sure the sensor produces at least one format compatible with
> > >  	 * the CIO2 requirements.
> > > -	 *
> > > -	 * utils::set_overlap requires the ranges to be sorted, keep the
> > > -	 * cio2Codes vector sorted in ascending order.
> > >  	 */
> > > -	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > > -						   MEDIA_BUS_FMT_SGRBG10_1X10,
> > > -						   MEDIA_BUS_FMT_SGBRG10_1X10,
> > > -						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> > > -	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > +	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 };

Should we make this static const ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > +	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > >  				cio2Codes.begin(), cio2Codes.end())) {
> > >  		LOG(IPU3, Error)
> > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > > index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
> > > --- a/test/camera-sensor.cpp
> > > +++ b/test/camera-sensor.cpp
> > > @@ -67,7 +67,7 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >
> > > -		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
> > > +		const std::set<unsigned int> &codes = sensor_->mbusCodes();
> > >  		auto iter = std::find(codes.begin(), codes.end(),
> > >  				      MEDIA_BUS_FMT_ARGB8888_1X32);
> > >  		if (iter == codes.end()) {
Jacopo Mondi June 25, 2020, 7:48 a.m. UTC | #4 
Hi

On Thu, Jun 25, 2020 at 02:44:36AM +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Wed, Jun 24, 2020 at 07:53:37PM +0200, Niklas Söderlund wrote:
> > On 2020-06-24 11:18:10 +0200, Jacopo Mondi wrote:
> > > On Tue, Jun 23, 2020 at 04:09:21AM +0200, Niklas Söderlund wrote:
> > > > It makes little sens for a CameraSensor to support the same media bus
> > >
> > > sense
> > > s/a CameraSensor/the CameraSensor class/
>
> Or "for a camera sensor".
>
> > > > format multiple times, change the container from a vector<> to a set<>.
> > > >
> > > > This was discovered while working with the IPU3 pipeline handler that
> > > > Unnecessarily had to carry code to sort vectors in order to find
> > >
> > > s/Un/un/
> > >
> > > > intersections. As this change requires switching the container to set<>
> > > > in that pipeline handler as well fix this while we are at it.
> > >
> > > as well,
> > >
> > > Which sensor does report mulitple times the same mbus_code ? Isn't
> > > this a driver bug if it happens ?
> >
> > None does so today, and if it happens I would indeed consider it a
> > driver bug. That is the reason for this change, to us an API to
> > demonstrate that it's impossible to have the same format twice.
> >
> > As an added bonus the set will always be sorted so the possibility that
> > an implementation somewhere forgets to sort the vector is completely
> > avoided.
>
> std::set is however more complex than std::vector. None of this should
> run in a hot path, so I'm ok with the change, but I think we need to
> take complexity into account in API design.

I still don't think we need any of this. If a driver reports two
identical mbus codes, it's buggy and should be fixed. There's no
reason I can see to work around it "just in case" here.

>
> > > I see  V4L2Subdevice::formats() populating the image formats with the
> > > mbus codes returned by V4L2Subdevice::enumPadCodes() and when I print
> > > out on Soraka the codes there collected I only get MEDIA_BUS_FMT_SGRBG10_1X10.
> > >
> > > Looking at the drivers of the two there installed sensors confirms
> > > that only one format is supported..
> > >
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h |  4 ++--
> > > >  include/libcamera/internal/formats.h       |  2 +-
> > > >  src/libcamera/camera_sensor.cpp            |  1 -
> > > >  src/libcamera/formats.cpp                  |  7 +++----
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
> > > >  test/camera-sensor.cpp                     |  2 +-
> > > >  6 files changed, 12 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index 7f07413f95602881..6d92818f16e4b752 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -50,7 +50,7 @@ public:
> > > >
> > > >  	const std::string &model() const { return model_; }
> > > >  	const MediaEntity *entity() const { return entity_; }
> > > > -	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > > +	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > >  	const std::vector<Size> &sizes() const { return sizes_; }
> > > >  	const Size &resolution() const { return resolution_; }
> > > >
> > > > @@ -77,7 +77,7 @@ private:
> > > >
> > > >  	ImageFormats formats_;
> > > >  	Size resolution_;
> > > > -	std::vector<unsigned int> mbusCodes_;
> > > > +	std::set<unsigned int> mbusCodes_;
> > > >  	std::vector<Size> sizes_;
> > > >
> > > >  	ControlList properties_;
> > > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > > index 4b172efc6588d374..e8aa555dcbe99350 100644
> > > > --- a/include/libcamera/internal/formats.h
> > > > +++ b/include/libcamera/internal/formats.h
> > > > @@ -24,7 +24,7 @@ public:
> > > >  	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> > > >
> > > >  	bool isEmpty() const;
> > > > -	std::vector<unsigned int> formats() const;
> > > > +	std::set<unsigned int> formats() const;
> > > >  	const std::vector<SizeRange> &sizes(unsigned int format) const;
> > > >  	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index b14b4051dca606b9..5ab50de660534f3f 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -251,7 +251,6 @@ int CameraSensor::init()
> > > >  	}
> > > >
> > > >  	mbusCodes_ = formats_.formats();
> > > > -	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> > > >
> > > >  	for (const auto &format : formats_.data()) {
> > > >  		const std::vector<SizeRange> &ranges = format.second;
> > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > > index 1272de29c802c539..6234fd98f3e2002c 100644
> > > > --- a/src/libcamera/formats.cpp
> > > > +++ b/src/libcamera/formats.cpp
> > > > @@ -68,14 +68,13 @@ bool ImageFormats::isEmpty() const
> > > >   * \brief Retrieve a list of all supported image formats
> > > >   * \return List of pixel formats or media bus codes
> > > >   */
> > > > -std::vector<unsigned int> ImageFormats::formats() const
> > > > +std::set<unsigned int> ImageFormats::formats() const
> > > >  {
> > > > -	std::vector<unsigned int> formats;
> > > > -	formats.reserve(data_.size());
> > > > +	std::set<unsigned int> formats;
> > > >
> > > >  	/* \todo: Should this be cached instead of computed each time? */
> > > >  	for (auto const &it : data_)
> > > > -		formats.push_back(it.first);
> > > > +		formats.insert(it.first);
> > > >
> > > >  	return formats;
> > > >  }
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 5544288b14f739c0..1a59de0c58975b3c 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -1441,15 +1441,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > >  	/*
> > > >  	 * Make sure the sensor produces at least one format compatible with
> > > >  	 * the CIO2 requirements.
> > > > -	 *
> > > > -	 * utils::set_overlap requires the ranges to be sorted, keep the
> > > > -	 * cio2Codes vector sorted in ascending order.
> > > >  	 */
> > > > -	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > -						   MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > -						   MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > -						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> > > > -	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > > +	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 };
>
> Should we make this static const ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > > > +	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > > >  				cio2Codes.begin(), cio2Codes.end())) {
> > > >  		LOG(IPU3, Error)
> > > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > > > index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
> > > > --- a/test/camera-sensor.cpp
> > > > +++ b/test/camera-sensor.cpp
> > > > @@ -67,7 +67,7 @@ protected:
> > > >  			return TestFail;
> > > >  		}
> > > >
> > > > -		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
> > > > +		const std::set<unsigned int> &codes = sensor_->mbusCodes();
> > > >  		auto iter = std::find(codes.begin(), codes.end(),
> > > >  				      MEDIA_BUS_FMT_ARGB8888_1X32);
> > > >  		if (iter == codes.end()) {
>
> --
> Regards,
>
> Laurent Pinchart
Niklas Söderlund June 25, 2020, 7:09 p.m. UTC | #5 
Hi,

On 2020-06-25 09:48:35 +0200, Jacopo Mondi wrote:
> Hi
> 
> On Thu, Jun 25, 2020 at 02:44:36AM +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > On Wed, Jun 24, 2020 at 07:53:37PM +0200, Niklas Söderlund wrote:
> > > On 2020-06-24 11:18:10 +0200, Jacopo Mondi wrote:
> > > > On Tue, Jun 23, 2020 at 04:09:21AM +0200, Niklas Söderlund wrote:
> > > > > It makes little sens for a CameraSensor to support the same media bus
> > > >
> > > > sense
> > > > s/a CameraSensor/the CameraSensor class/
> >
> > Or "for a camera sensor".
> >
> > > > > format multiple times, change the container from a vector<> to a set<>.
> > > > >
> > > > > This was discovered while working with the IPU3 pipeline handler that
> > > > > Unnecessarily had to carry code to sort vectors in order to find
> > > >
> > > > s/Un/un/
> > > >
> > > > > intersections. As this change requires switching the container to set<>
> > > > > in that pipeline handler as well fix this while we are at it.
> > > >
> > > > as well,
> > > >
> > > > Which sensor does report mulitple times the same mbus_code ? Isn't
> > > > this a driver bug if it happens ?
> > >
> > > None does so today, and if it happens I would indeed consider it a
> > > driver bug. That is the reason for this change, to us an API to
> > > demonstrate that it's impossible to have the same format twice.
> > >
> > > As an added bonus the set will always be sorted so the possibility that
> > > an implementation somewhere forgets to sort the vector is completely
> > > avoided.
> >
> > std::set is however more complex than std::vector. None of this should
> > run in a hot path, so I'm ok with the change, but I think we need to
> > take complexity into account in API design.
> 
> I still don't think we need any of this. If a driver reports two
> identical mbus codes, it's buggy and should be fixed. There's no
> reason I can see to work around it "just in case" here.

I think it's more then "just in case". IMHO it's a better API because it 
tells the user it's impossible to have multiple entries of the same 
value. Without this in the API the user must know this as a fact of the 
V4L2 API and if it would find duplicates that the bug is in the driver 
and not somewhere else.

> 
> >
> > > > I see  V4L2Subdevice::formats() populating the image formats with the
> > > > mbus codes returned by V4L2Subdevice::enumPadCodes() and when I print
> > > > out on Soraka the codes there collected I only get MEDIA_BUS_FMT_SGRBG10_1X10.
> > > >
> > > > Looking at the drivers of the two there installed sensors confirms
> > > > that only one format is supported..
> > > >
> > > > >
> > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > > ---
> > > > >  include/libcamera/internal/camera_sensor.h |  4 ++--
> > > > >  include/libcamera/internal/formats.h       |  2 +-
> > > > >  src/libcamera/camera_sensor.cpp            |  1 -
> > > > >  src/libcamera/formats.cpp                  |  7 +++----
> > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp       | 13 +++++--------
> > > > >  test/camera-sensor.cpp                     |  2 +-
> > > > >  6 files changed, 12 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > > index 7f07413f95602881..6d92818f16e4b752 100644
> > > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > > @@ -50,7 +50,7 @@ public:
> > > > >
> > > > >  	const std::string &model() const { return model_; }
> > > > >  	const MediaEntity *entity() const { return entity_; }
> > > > > -	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > > > +	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > > >  	const std::vector<Size> &sizes() const { return sizes_; }
> > > > >  	const Size &resolution() const { return resolution_; }
> > > > >
> > > > > @@ -77,7 +77,7 @@ private:
> > > > >
> > > > >  	ImageFormats formats_;
> > > > >  	Size resolution_;
> > > > > -	std::vector<unsigned int> mbusCodes_;
> > > > > +	std::set<unsigned int> mbusCodes_;
> > > > >  	std::vector<Size> sizes_;
> > > > >
> > > > >  	ControlList properties_;
> > > > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > > > index 4b172efc6588d374..e8aa555dcbe99350 100644
> > > > > --- a/include/libcamera/internal/formats.h
> > > > > +++ b/include/libcamera/internal/formats.h
> > > > > @@ -24,7 +24,7 @@ public:
> > > > >  	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> > > > >
> > > > >  	bool isEmpty() const;
> > > > > -	std::vector<unsigned int> formats() const;
> > > > > +	std::set<unsigned int> formats() const;
> > > > >  	const std::vector<SizeRange> &sizes(unsigned int format) const;
> > > > >  	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> > > > >
> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > > index b14b4051dca606b9..5ab50de660534f3f 100644
> > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > @@ -251,7 +251,6 @@ int CameraSensor::init()
> > > > >  	}
> > > > >
> > > > >  	mbusCodes_ = formats_.formats();
> > > > > -	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> > > > >
> > > > >  	for (const auto &format : formats_.data()) {
> > > > >  		const std::vector<SizeRange> &ranges = format.second;
> > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > > > index 1272de29c802c539..6234fd98f3e2002c 100644
> > > > > --- a/src/libcamera/formats.cpp
> > > > > +++ b/src/libcamera/formats.cpp
> > > > > @@ -68,14 +68,13 @@ bool ImageFormats::isEmpty() const
> > > > >   * \brief Retrieve a list of all supported image formats
> > > > >   * \return List of pixel formats or media bus codes
> > > > >   */
> > > > > -std::vector<unsigned int> ImageFormats::formats() const
> > > > > +std::set<unsigned int> ImageFormats::formats() const
> > > > >  {
> > > > > -	std::vector<unsigned int> formats;
> > > > > -	formats.reserve(data_.size());
> > > > > +	std::set<unsigned int> formats;
> > > > >
> > > > >  	/* \todo: Should this be cached instead of computed each time? */
> > > > >  	for (auto const &it : data_)
> > > > > -		formats.push_back(it.first);
> > > > > +		formats.insert(it.first);
> > > > >
> > > > >  	return formats;
> > > > >  }
> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > index 5544288b14f739c0..1a59de0c58975b3c 100644
> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > @@ -1441,15 +1441,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > > > >  	/*
> > > > >  	 * Make sure the sensor produces at least one format compatible with
> > > > >  	 * the CIO2 requirements.
> > > > > -	 *
> > > > > -	 * utils::set_overlap requires the ranges to be sorted, keep the
> > > > > -	 * cio2Codes vector sorted in ascending order.
> > > > >  	 */
> > > > > -	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > -						   MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > -						   MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > -						   MEDIA_BUS_FMT_SRGGB10_1X10 };
> > > > > -	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > > > +	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 };
> >
> > Should we make this static const ?
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > > > +	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > > > >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > > > >  				cio2Codes.begin(), cio2Codes.end())) {
> > > > >  		LOG(IPU3, Error)
> > > > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > > > > index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
> > > > > --- a/test/camera-sensor.cpp
> > > > > +++ b/test/camera-sensor.cpp
> > > > > @@ -67,7 +67,7 @@ protected:
> > > > >  			return TestFail;
> > > > >  		}
> > > > >
> > > > > -		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
> > > > > +		const std::set<unsigned int> &codes = sensor_->mbusCodes();
> > > > >  		auto iter = std::find(codes.begin(), codes.end(),
> > > > >  				      MEDIA_BUS_FMT_ARGB8888_1X32);
> > > > >  		if (iter == codes.end()) {
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 7f07413f95602881..6d92818f16e4b752 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -50,7 +50,7 @@  public:
 
 	const std::string &model() const { return model_; }
 	const MediaEntity *entity() const { return entity_; }
-	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
+	const std::set<unsigned int> &mbusCodes() const { return mbusCodes_; }
 	const std::vector<Size> &sizes() const { return sizes_; }
 	const Size &resolution() const { return resolution_; }
 
@@ -77,7 +77,7 @@  private:
 
 	ImageFormats formats_;
 	Size resolution_;
-	std::vector<unsigned int> mbusCodes_;
+	std::set<unsigned int> mbusCodes_;
 	std::vector<Size> sizes_;
 
 	ControlList properties_;
diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
index 4b172efc6588d374..e8aa555dcbe99350 100644
--- a/include/libcamera/internal/formats.h
+++ b/include/libcamera/internal/formats.h
@@ -24,7 +24,7 @@  public:
 	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
 
 	bool isEmpty() const;
-	std::vector<unsigned int> formats() const;
+	std::set<unsigned int> formats() const;
 	const std::vector<SizeRange> &sizes(unsigned int format) const;
 	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
 
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index b14b4051dca606b9..5ab50de660534f3f 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -251,7 +251,6 @@  int CameraSensor::init()
 	}
 
 	mbusCodes_ = formats_.formats();
-	std::sort(mbusCodes_.begin(), mbusCodes_.end());
 
 	for (const auto &format : formats_.data()) {
 		const std::vector<SizeRange> &ranges = format.second;
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index 1272de29c802c539..6234fd98f3e2002c 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -68,14 +68,13 @@  bool ImageFormats::isEmpty() const
  * \brief Retrieve a list of all supported image formats
  * \return List of pixel formats or media bus codes
  */
-std::vector<unsigned int> ImageFormats::formats() const
+std::set<unsigned int> ImageFormats::formats() const
 {
-	std::vector<unsigned int> formats;
-	formats.reserve(data_.size());
+	std::set<unsigned int> formats;
 
 	/* \todo: Should this be cached instead of computed each time? */
 	for (auto const &it : data_)
-		formats.push_back(it.first);
+		formats.insert(it.first);
 
 	return formats;
 }
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 5544288b14f739c0..1a59de0c58975b3c 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1441,15 +1441,12 @@  int CIO2Device::init(const MediaDevice *media, unsigned int index)
 	/*
 	 * Make sure the sensor produces at least one format compatible with
 	 * the CIO2 requirements.
-	 *
-	 * utils::set_overlap requires the ranges to be sorted, keep the
-	 * cio2Codes vector sorted in ascending order.
 	 */
-	const std::vector<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
-						   MEDIA_BUS_FMT_SGRBG10_1X10,
-						   MEDIA_BUS_FMT_SGBRG10_1X10,
-						   MEDIA_BUS_FMT_SRGGB10_1X10 };
-	const std::vector<unsigned int> &sensorCodes = sensor_->mbusCodes();
+	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 };
+	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
 	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
 				cio2Codes.begin(), cio2Codes.end())) {
 		LOG(IPU3, Error)
diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
index 8c7fd1d2d4445db3..146b1b7d6c294fdb 100644
--- a/test/camera-sensor.cpp
+++ b/test/camera-sensor.cpp
@@ -67,7 +67,7 @@  protected:
 			return TestFail;
 		}
 
-		const std::vector<unsigned int> &codes = sensor_->mbusCodes();
+		const std::set<unsigned int> &codes = sensor_->mbusCodes();
 		auto iter = std::find(codes.begin(), codes.end(),
 				      MEDIA_BUS_FMT_ARGB8888_1X32);
 		if (iter == codes.end()) {