[PATCH v6 03/12] libcamera: formats: Add a helper to check for a raw pixel format
Milan Zamazal
mzamazal at redhat.com
Mon Jun 2 15:42:08 CEST 2025
Hi Kieran,
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> Quoting Milan Zamazal (2025-05-26 11:15:23)
>> There are several places with the same pattern to check whether a given
>> pixel format is a raw format:
>>
>> return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>> libcamera::PixelFormatInfo::ColourEncodingRAW;
>>
>> Let's move the corresponding isFormatRaw helper from mali-c55.cpp to
>> formats.cpp and use it where applicable. This also avoids a need to
>> introduce the same helper (or the same pattern) in the followup patches.
>
> This was tried before but not completed/successful.
>
> https://patchwork.libcamera.org/project/libcamera/list/?series=4640&state=*
>
> The problem is the definition of 'raw' is too vague.
>
> Maybe we could have something like 'isBayerFormat' if we want to map all
> bayer formats - but a RAW stream being defined on the colour encoding is
> ill-defined...
>
> I still think there's scope for trying to factor out parts here - but we
> seem to need to do better at determining what 'RAW' means ...
Well, we have common patterns and the questions are:
1. Do we want to extract them to common helpers? The answer is probably
yes, because:
- Common patterns should be extracted to helpers generally.
- The helpers should make the purposes of the patterns clearer.
2. Do the same patterns represent the same thing everywhere? And are
they correct?
3. How should the helpers be named and their purpose defined?
As far as I understand the review thread referred above, we don't have
clear answers to 2. + 3. Which is a bit worrying as this means the
current code may not be completely clear or even correct. And 1. is a
good reason to bother anyway.
As for this series, we may drop the mass extraction but we need similar
patterns in this series in either case so at least the meaning and
correctness of the helpers used here should be well defined. I'm not
sure what would be correct; my feeling is that perhaps some of the pixel
format related structures should have a separate member for the purpose,
which looks like a non-trivial change to me with respect to the other
pipelines.
> --
> Kieran
>
>
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>> include/libcamera/internal/formats.h | 2 ++
>> src/libcamera/formats.cpp | 11 +++++++++++
>> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 4 ++--
>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 10 ----------
>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-----
>> 5 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
>> index 6a3e9c16..bc4417d0 100644
>> --- a/include/libcamera/internal/formats.h
>> +++ b/include/libcamera/internal/formats.h
>> @@ -62,4 +62,6 @@ public:
>> std::array<Plane, 3> planes;
>> };
>>
>> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt);
>> +
>> } /* namespace libcamera */
>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
>> index bfcdfc08..c6e0a262 100644
>> --- a/src/libcamera/formats.cpp
>> +++ b/src/libcamera/formats.cpp
>> @@ -1215,4 +1215,15 @@ unsigned int PixelFormatInfo::numPlanes() const
>> return count;
>> }
>>
>> +/**
>> + * \brief Return whether the given pixel format is a raw format
>> + * \param[in] pixFmt The pixel format to examine
>> + * \return True iff the given format is a raw format
>> + */
>> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
>> +{
>> + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>> + libcamera::PixelFormatInfo::ColourEncodingRAW;
>> +}
>> +
>> } /* namespace libcamera */
>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> index ecda426a..ad36ebea 100644
>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> @@ -24,6 +24,7 @@
>> #include "libcamera/internal/camera.h"
>> #include "libcamera/internal/camera_sensor.h"
>> #include "libcamera/internal/device_enumerator.h"
>> +#include "libcamera/internal/formats.h"
>> #include "libcamera/internal/media_device.h"
>> #include "libcamera/internal/pipeline_handler.h"
>> #include "libcamera/internal/v4l2_subdevice.h"
>> @@ -312,8 +313,7 @@ unsigned int ISICameraData::getYuvMediaBusFormat(const PixelFormat &pixelFormat)
>>
>> unsigned int ISICameraData::getMediaBusFormat(PixelFormat *pixelFormat) const
>> {
>> - if (PixelFormatInfo::info(*pixelFormat).colourEncoding ==
>> - PixelFormatInfo::ColourEncodingRAW)
>> + if (isFormatRaw(*pixelFormat))
>> return getRawMediaBusFormat(pixelFormat);
>>
>> return getYuvMediaBusFormat(*pixelFormat);
>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> index a05e11fc..3721fb30 100644
>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> @@ -42,16 +42,6 @@
>> #include "libcamera/internal/v4l2_subdevice.h"
>> #include "libcamera/internal/v4l2_videodevice.h"
>>
>> -namespace {
>> -
>> -bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
>> -{
>> - return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>> - libcamera::PixelFormatInfo::ColourEncodingRAW;
>> -}
>> -
>> -} /* namespace */
>> -
>> namespace libcamera {
>>
>> LOG_DEFINE_CATEGORY(MaliC55)
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 675f0a74..50c83fb7 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -538,8 +538,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>> */
>> if (config_.size() > 1) {
>> for (const auto &cfg : config_) {
>> - if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
>> - PixelFormatInfo::ColourEncodingRAW) {
>> + if (isFormatRaw(cfg.pixelFormat)) {
>> config_.resize(1);
>> status = Adjusted;
>> break;
>> @@ -553,9 +552,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>> * Platforms with dewarper support, such as i.MX8MP, support
>> * only a single stream. We can inspect config_[0] only here.
>> */
>> - bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
>> - PixelFormatInfo::ColourEncodingRAW;
>> - if (!isRaw)
>> + if (!isFormatRaw(config_[0].pixelFormat))
>> useDewarper = true;
>> }
>>
>> --
>> 2.49.0
>>
More information about the libcamera-devel
mailing list