[libcamera-devel,v2,1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()
diff mbox series

Message ID 20210810075854.86191-2-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • ipu3: Change sensor size selection policy
Related show

Commit Message

Umang Jain Aug. 10, 2021, 7:58 a.m. UTC 
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@ideasonboard.com>
Tested-by: Umang Jain <umang.jain@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 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(-)

Comments

Jacopo Mondi Aug. 25, 2021, 10:09 a.m. UTC | #1 
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@ideasonboard.com>
> Tested-by: Umang Jain <umang.jain@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@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@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
>
Jacopo Mondi Aug. 25, 2021, 1:01 p.m. UTC | #2 
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@ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain@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@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@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
> >
Kieran Bingham Aug. 27, 2021, 12:36 p.m. UTC | #3 
Hi Umang,

On 25/08/2021 14:01, Jacopo Mondi wrote:
> 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@ideasonboard.com>
>>> Tested-by: Umang Jain <umang.jain@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@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.
>>> - *

The fact that this statement is removed makes this patch sound like a
good idea!

>>> - * \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 ?

Agreed, we should keep that doc consistent.

>>
>>>  {
>>> +	std::vector<SizeRange> szRange;
>>
>> Why have you moved it up ? I would also keep the same name
>>
>>> +	int mbusCode = -1;
>>> +

is 0 a valid mbusCode?

If not, we could use that and keep mbusCode as unsigned.


https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html#v4l2-mbus-pixelcode
shows a table of valid mbus codes, and I don't think its likely that 0x0
would ever be added there.


>>>  	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

Then this wouldn't be a problem...


>>
>> With this fixed:

likewise

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>> Reviewed-by: Jacopo Mondi <jacopo@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
>>>

Patch
diff mbox series 

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
 {
+	std::vector<SizeRange> szRange;
+	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;
+	}
+
+	if (!mbusCode)
+		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()) {