| Message ID | 20250407085639.16180-4-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 04. 07. 10:56 keltezéssel, Milan Zamazal írta: > 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. As far as I can see, there are a lot of other places where this check is "open coded", I believe either all of them or none of them should be replaced. For example, `IPARkISP1::configure()`, `ISICameraConfiguration::validate()`, `IPU3CameraConfiguration::validate()`, etc. And there is also `PipelineHandlerBase::isRaw()` in the rpi pipeline. I am wondering if adding `PixelFormatInfo::isRaw()` could be useful as well. Regards, Barnabás Pőcze > > Signed-off-by: Milan Zamazal <mzamazal@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 4e66b336..9ff11a41 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 52633fe3..a5b613bb 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -536,8 +536,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; > @@ -551,9 +550,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; > } >
Hi Barnabás, thank you for review. Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Hi > > 2025. 04. 07. 10:56 keltezéssel, Milan Zamazal írta: >> 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. > > As far as I can see, there are a lot of other places where this check is "open coded", > I believe either all of them or none of them should be replaced. For example, > `IPARkISP1::configure()`, `ISICameraConfiguration::validate()`, > `IPU3CameraConfiguration::validate()`, etc. > > And there is also `PipelineHandlerBase::isRaw()` in the rpi pipeline. IIRC I tried to replace only the given pattern and not simpler constructs like when the info is already retrieved, to not complicate the patches more than necessary. But the rest can certainly be also replaced, I can do it in v5 if there are no objections. > I am wondering if adding `PixelFormatInfo::isRaw()` could be useful as well. I think so, with the broader replacement. > Regards, > Barnabás Pőcze > > >> Signed-off-by: Milan Zamazal <mzamazal@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 4e66b336..9ff11a41 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 52633fe3..a5b613bb 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -536,8 +536,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; >> @@ -551,9 +550,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; >> } >>
Quoting Milan Zamazal (2025-05-13 09:10:08) > Hi Barnabás, > > thank you for review. > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > > > Hi > > > > 2025. 04. 07. 10:56 keltezéssel, Milan Zamazal írta: > >> 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. > > > > As far as I can see, there are a lot of other places where this check is "open coded", > > I believe either all of them or none of them should be replaced. For example, > > `IPARkISP1::configure()`, `ISICameraConfiguration::validate()`, > > `IPU3CameraConfiguration::validate()`, etc. > > > > And there is also `PipelineHandlerBase::isRaw()` in the rpi pipeline. > > IIRC I tried to replace only the given pattern and not simpler > constructs like when the info is already retrieved, to not complicate > the patches more than necessary. But the rest can certainly be also > replaced, I can do it in v5 if there are no objections. > > > I am wondering if adding `PixelFormatInfo::isRaw()` could be useful as well. > > I think so, with the broader replacement. I'm sure I recall this used to be a global helper - and somehow/ for some reason it got moved to being handled by pipeline handlers specfically. I think the intention was that pipeline handlers would know which formats are raw or not ... but I can't find anything specific in the history yet.. Even if that was the case, maybe that needs to be revisited if in the end every pipeline handler ends up implementing it in exactly the same way... -- Kieran > > > Regards, > > Barnabás Pőcze > > > > > >> +/** > >> + * \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; > >> +} > >> +
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 4e66b336..9ff11a41 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 52633fe3..a5b613bb 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -536,8 +536,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; @@ -551,9 +550,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; }
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. Signed-off-by: Milan Zamazal <mzamazal@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(-)