[libcamera-devel] [PATCH v3 01/10] libcamera: camera_sensor: Make mbusCodes() return a set
Jacopo Mondi
jacopo at jmondi.org
Thu Jun 25 09:48:35 CEST 2020
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 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