[libcamera-devel] [PATCH v2 1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Aug 27 14:36:05 CEST 2021
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 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.
>>> - *
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 at ideasonboard.com>
>> 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