[libcamera-devel] [PATCH v3 01/10] libcamera: camera_sensor: Make mbusCodes() return a set
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 25 01:44:36 CEST 2020
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 at 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 at 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
More information about the libcamera-devel
mailing list