[libcamera-devel] [PATCH 1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()

[Jacopo Mondi]

Hi Umang,

On Tue, Aug 03, 2021 at 07:02:02PM +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.
>
> To know all the frame sizes of the CameraSensor as required in IPU3
> case Cio2Device::sizes(), one would now require to enumerate all the
> media bus codes (can be retrieved by CameraSensor::mbusCodes()) with
> CameraSensor::size(mbusCode). The result can be inserted in a
> std::set<> to avoid duplicates.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 +-
>  src/libcamera/camera_sensor.cpp            | 36 ++++++++++++++++------
>  src/libcamera/pipeline/ipu3/cio2.cpp       | 10 +++---
>  test/camera-sensor.cpp                     |  2 +-
>  4 files changed, 34 insertions(+), 16 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 cde431cc..3c3ceff3 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -471,16 +471,6 @@ int CameraSensor::initProperties()
>   * \return The supported media bus codes sorted in increasing order
>   */
>
> -/**
> - * \fn CameraSensor::sizes()
> - * \brief Retrieve the frame sizes supported by the camera sensor
> - *
> - * 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
> - */
> -
>  /**
>   * \brief Retrieve the camera sensor resolution
>   *
> @@ -594,6 +584,32 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  	return format;
>  }
>
> +/**
> + * \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 for a particular \a mbusCode are supported by the camera
> + * sensor.

I would drop this

> + *
> + * \return The supported frame sizes for \a mbusCode sorted in increasing order
> + */
> +const std::vector<Size> CameraSensor::sizes(unsigned int mbusCode) const

Why have you moved this ? Let's respect the declaration order

> +{
> +	std::vector<Size> sizes;
> +
> +	const auto format = formats_.find(mbusCode);

&format

> +	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 Set the sensor output format
>   * \param[in] format The desired sensor output format
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 1bcd580e..6f2ef321 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -69,11 +69,13 @@ std::vector<SizeRange> CIO2Device::sizes() const
>  	if (!sensor_)
>  		return {};
>
> -	std::vector<SizeRange> sizes;
> -	for (const Size &size : sensor_->sizes())
> -		sizes.emplace_back(size, size);
> +	std::set<Size> allSizes;
> +	for (unsigned int code : sensor_->mbusCodes()) {
> +		for (Size sz : sensor_->sizes(code))

const Size &sz

> +			allSizes.insert(sz);
> +	}
>
> -	return sizes;
> +	return std::vector<SizeRange>(allSizes.begin(), allSizes.end());

Looking at how CIO2::sizes() is used in IPU3::generateConfiguration()
I wonder if we shouldn't pass the desired PixelFormat to
CIO2::sizes() so that we can actually enumerate the per-format sizes
for real (and avoid going through the set->vector conversion).

>  }
>
>  /**
> 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);

This changes the semantic of the test from "let's check if a format
supports 4096x2160" to "let's check if ARGB8888_1X32 supports
4096x2160". I think it's better now, and as long as the test passes,
since it uses VIMC, we should be good!

Thanks
   j

>  		auto iter2 = std::find(sizes.begin(), sizes.end(),
>  				       Size(4096, 2160));
>  		if (iter2 == sizes.end()) {
> --
> 2.31.0
>