[libcamera-devel] [PATCH v2 1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()
Jacopo Mondi
jacopo at jmondi.org
Wed Aug 25 12:09:35 CEST 2021
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))
> + 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