[libcamera-devel] [PATCH v2 1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()
Jacopo Mondi
jacopo at jmondi.org
Wed Aug 25 15:01:38 CEST 2021
Hi Umang,
one additional comment
On Wed, Aug 25, 2021 at 12:09:35PM +0200, Jacopo Mondi wrote:
> Hi Umang,
>
> On Tue, Aug 10, 2021 at 01:28:51PM +0530, Umang Jain wrote:
> > In CameraSensor, the mbusCodes() and sizes() accessor functions
> > retrieves all the supported media bus codes and the supported sizes
> > respectively. However, this is quite limiting since the caller
> > probably isn't in a position to match which range of sizes are
> > supported for a particular mbusCode.
> >
> > Hence, the caller is most likely interested to know about the sizes
> > supported for a particular media bus code. This patch transforms the
> > existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to
> > achieve that goal.
> >
> > The patch also transforms existing CIO2Device::sizes() in IPU3 pipeline
> > handler to CIO2Device::sizes(PixelFormat) on a similar principle. The
> > function is then plumbed to CameraSensor::sizes(mbusCode) to enumerate
> > the per-format sizes as required in
> > PipelineHandlerIPU3::generateConfiguration().
> >
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain at ideasonboard.com>
>
> Well, I hope you test your patches before sending them. Do we need
> Tested-by tags from the author ?
>
> > Tested-by: Jacopo Mondi <jacopo at jmondi.org>
>
> I was sure I had reviewed the series in full I'm sorry,
>
>
> > ---
> > include/libcamera/internal/camera_sensor.h | 2 +-
> > src/libcamera/camera_sensor.cpp | 25 ++++++++++++++++------
> > src/libcamera/pipeline/ipu3/cio2.cpp | 20 ++++++++++++-----
> > src/libcamera/pipeline/ipu3/cio2.h | 2 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > test/camera-sensor.cpp | 2 +-
> > 6 files changed, 38 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index db12b07e..d25a1165 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -38,7 +38,7 @@ public:
> > const std::string &id() const { return id_; }
> > const MediaEntity *entity() const { return entity_; }
> > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > - const std::vector<Size> &sizes() const { return sizes_; }
> > + const std::vector<Size> sizes(unsigned int mbusCode) const;
> > Size resolution() const;
> > const std::vector<int32_t> &testPatternModes() const
> > {
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 7a386415..87668509 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -472,14 +472,27 @@ int CameraSensor::initProperties()
> > */
> >
> > /**
> > - * \fn CameraSensor::sizes()
> > - * \brief Retrieve the frame sizes supported by the camera sensor
> > + * \brief Retrieve the supported frame sizes for a media bus code
> > + * \param[in] mbusCode The media bus code for which sizes are requested
> > *
> > - * The reported sizes span all media bus codes supported by the camera sensor.
> > - * Not all sizes may be supported by all media bus codes.
> > - *
> > - * \return The supported frame sizes sorted in increasing order
> > + * \return The supported frame sizes for \a mbusCode sorted in increasing order
> > */
> > +const std::vector<Size> CameraSensor::sizes(unsigned int mbusCode) const
> > +{
> > + std::vector<Size> sizes;
> > +
> > + const auto &format = formats_.find(mbusCode);
> > + if (format == formats_.end())
> > + return sizes;
> > +
> > + const std::vector<SizeRange> &ranges = format->second;
> > + std::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),
> > + [](const SizeRange &range) { return range.max; });
> > +
> > + std::sort(sizes.begin(), sizes.end());
> > +
> > + return sizes;
> > +}
> >
> > /**
> > * \brief Retrieve the camera sensor resolution
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 1bcd580e..8a40e955 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -64,16 +64,26 @@ std::vector<PixelFormat> CIO2Device::formats() const
> > * \brief Retrieve the list of supported size ranges
> > * \return The list of supported SizeRange
> > */
> > -std::vector<SizeRange> CIO2Device::sizes() const
> > +std::vector<SizeRange> CIO2Device::sizes(const PixelFormat &format) const
>
> I know we don't really doxygen, but as it's there, what about
> documenting it ?
>
> > {
> > + std::vector<SizeRange> szRange;
>
> Why have you moved it up ? I would also keep the same name
>
> > + int mbusCode = -1;
> > +
> > if (!sensor_)
> > return {};
> >
> > - std::vector<SizeRange> sizes;
> > - for (const Size &size : sensor_->sizes())
> > - sizes.emplace_back(size, size);
> > + for (const auto &iter : mbusCodesToPixelFormat) {
> > + if (iter.second == format)
> > + mbusCode = iter.first;
>
> There should be only one match, right ? Then
> if (iter.second != format)
> continue;
>
> mbusCode = iter.first;
> break;
>
> > + }
> > +
> > + if (!mbusCode)
>
> if (-1) evaluates to true
>
> With this fixed:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > + return {};
> > +
> > + for (const Size &sz: sensor_->sizes(mbusCode))
checkstyle reports:
- for (const Size &sz: sensor_->sizes(mbusCode))
+ for (const Size &sz : sensor_->sizes(mbusCode))
szRange.emplace_back(sz);
> > + szRange.emplace_back(sz);
> >
> > - return sizes;
> > + return szRange;
> > }
> >
> > /**
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index f28e9f1d..24272dc5 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -35,7 +35,7 @@ public:
> > CIO2Device();
> >
> > std::vector<PixelFormat> formats() const;
> > - std::vector<SizeRange> sizes() const;
> > + std::vector<SizeRange> sizes(const PixelFormat &format) const;
> >
> > int init(const MediaDevice *media, unsigned int index);
> > int configure(const Size &size, V4L2DeviceFormat *outputFormat);
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 19cb5c4e..c73540fb 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -455,7 +455,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > bufferCount = cio2Config.bufferCount;
> >
> > for (const PixelFormat &format : data->cio2_.formats())
> > - streamFormats[format] = data->cio2_.sizes();
> > + streamFormats[format] = data->cio2_.sizes(format);
> >
> > break;
> > }
> > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > index a8dcad82..372ee4af 100644
> > --- a/test/camera-sensor.cpp
> > +++ b/test/camera-sensor.cpp
> > @@ -76,7 +76,7 @@ protected:
> > return TestFail;
> > }
> >
> > - const std::vector<Size> &sizes = sensor_->sizes();
> > + const std::vector<Size> &sizes = sensor_->sizes(*iter);
> > auto iter2 = std::find(sizes.begin(), sizes.end(),
> > Size(4096, 2160));
> > if (iter2 == sizes.end()) {
> > --
> > 2.31.1
> >
More information about the libcamera-devel
mailing list