Message ID | 20211206105032.13876-8-david.plowman@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi David On Mon, Dec 06, 2021 at 10:50:30AM +0000, David Plowman wrote: > This method forces raw streams to have the "raw" color space, and also > optionally makes all output streams to share the same color space as > some platforms may require this. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 2 ++ > src/libcamera/camera.cpp | 59 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 61 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index a7759ccb..fdcb66ab 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -69,6 +69,8 @@ public: > protected: > CameraConfiguration(); > > + Status validateColorSpaces(bool shareOutputColorSpaces); > + > std::vector<StreamConfiguration> config_; > }; > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 400a7cf0..388af4f7 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -14,12 +14,14 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/thread.h> > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_controls.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > > /** > @@ -314,6 +316,63 @@ std::size_t CameraConfiguration::size() const > return config_.size(); > } > > +static bool isRaw(const PixelFormat &pixFmt) > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.isValid() && > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > +} > + > +/** > + * \brief Check and update the color spaces requested for each stream > + * \param[in] shareOutputColorSpaces Force all non-raw output streams to share > + * the same color space > + * > + * This function performs certain consistency checks on the color spaces of > + * the streams and may adjust them so that: > + * > + * - Any raw streams have the Raw color space > + * - If shareOutputColorSpaces is set, all output streams are forced to share > + * the same color space (this may be a constraint on some platforms). > + * > + * It is optional for a pipeline handler to use this method. > + * > + * \return A CameraConfiguration::Status value that describes the validation > + * status (same as CameraConfiguration::validate). > + */ > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces) > +{ > + Status status = Valid; > + > + /* > + * Set all raw streams to the Raw color space, and make a note of the largest > + * non-raw stream with a defined color space (if there is one). > + */ > + int index = -1; > + for (auto [i, cfg] : utils::enumerate(config_)) { > + if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) { > + cfg.colorSpace = ColorSpace::Raw; > + status = Adjusted; > + } > + else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) Note for when applying: if () { } else if { } > + index = i; > + } > + > + if (index < 0 || !shareOutputColorSpaces) > + return status; > + > + /* Make all output color spaces the same, if requested. */ > + for (auto &cfg : config_) { > + if (!isRaw(cfg.pixelFormat) && > + cfg.colorSpace != config_[index].colorSpace) { > + cfg.colorSpace = config_[index].colorSpace; > + status = Adjusted; > + } > + } > + > + return status; > +} > + > /** > * \var CameraConfiguration::transform > * \brief User-specified transform to be applied to the image > -- > 2.20.1 >
Hi David, Thank you for the patch. On Mon, Dec 06, 2021 at 10:50:30AM +0000, David Plowman wrote: > This method forces raw streams to have the "raw" color space, and also s/method/function/ (same below) > optionally makes all output streams to share the same color space as > some platforms may require this. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 2 ++ > src/libcamera/camera.cpp | 59 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 61 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index a7759ccb..fdcb66ab 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -69,6 +69,8 @@ public: > protected: > CameraConfiguration(); > > + Status validateColorSpaces(bool shareOutputColorSpaces); > + I'm not a big fan of boolean parameters for this kind of use case, as it's not clear what they mean when you look at the callers. Explicit flags are better: enum class ColorSpaceFlag { None, StreamsShareColorSpace, -- or a better name }; using ColorSpaceFlags = Flags<ColorSpaceFlag>; Status validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None); It may be overkill to use the Flags class for a single flag, the enum alone would be enough. > std::vector<StreamConfiguration> config_; > }; > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 400a7cf0..388af4f7 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -14,12 +14,14 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/thread.h> > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_controls.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > > /** > @@ -314,6 +316,63 @@ std::size_t CameraConfiguration::size() const > return config_.size(); > } > > +static bool isRaw(const PixelFormat &pixFmt) > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.isValid() && > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > +} namespace { bool isRaw(const PixelFormat &pixFmt) { ... } } /* namespace */ would be the C++ way. > + > +/** > + * \brief Check and update the color spaces requested for each stream > + * \param[in] shareOutputColorSpaces Force all non-raw output streams to share > + * the same color space > + * > + * This function performs certain consistency checks on the color spaces of > + * the streams and may adjust them so that: > + * > + * - Any raw streams have the Raw color space > + * - If shareOutputColorSpaces is set, all output streams are forced to share > + * the same color space (this may be a constraint on some platforms). > + * > + * It is optional for a pipeline handler to use this method. > + * > + * \return A CameraConfiguration::Status value that describes the validation > + * status (same as CameraConfiguration::validate). > + */ > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces) > +{ > + Status status = Valid; > + > + /* > + * Set all raw streams to the Raw color space, and make a note of the largest > + * non-raw stream with a defined color space (if there is one). > + */ > + int index = -1; > + for (auto [i, cfg] : utils::enumerate(config_)) { > + if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) { > + cfg.colorSpace = ColorSpace::Raw; > + status = Adjusted; > + } > + else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) As Jacopo said :-) Any particular reason to go for the largest stream ? > + index = i; > + } > + > + if (index < 0 || !shareOutputColorSpaces) > + return status; > + > + /* Make all output color spaces the same, if requested. */ > + for (auto &cfg : config_) { > + if (!isRaw(cfg.pixelFormat) && > + cfg.colorSpace != config_[index].colorSpace) { > + cfg.colorSpace = config_[index].colorSpace; > + status = Adjusted; > + } > + } > + > + return status; > +} > + > /** > * \var CameraConfiguration::transform > * \brief User-specified transform to be applied to the image
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index a7759ccb..fdcb66ab 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -69,6 +69,8 @@ public: protected: CameraConfiguration(); + Status validateColorSpaces(bool shareOutputColorSpaces); + std::vector<StreamConfiguration> config_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 400a7cf0..388af4f7 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -14,12 +14,14 @@ #include <libcamera/base/log.h> #include <libcamera/base/thread.h> +#include <libcamera/color_space.h> #include <libcamera/framebuffer_allocator.h> #include <libcamera/request.h> #include <libcamera/stream.h> #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_controls.h" +#include "libcamera/internal/formats.h" #include "libcamera/internal/pipeline_handler.h" /** @@ -314,6 +316,63 @@ std::size_t CameraConfiguration::size() const return config_.size(); } +static bool isRaw(const PixelFormat &pixFmt) +{ + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); + return info.isValid() && + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; +} + +/** + * \brief Check and update the color spaces requested for each stream + * \param[in] shareOutputColorSpaces Force all non-raw output streams to share + * the same color space + * + * This function performs certain consistency checks on the color spaces of + * the streams and may adjust them so that: + * + * - Any raw streams have the Raw color space + * - If shareOutputColorSpaces is set, all output streams are forced to share + * the same color space (this may be a constraint on some platforms). + * + * It is optional for a pipeline handler to use this method. + * + * \return A CameraConfiguration::Status value that describes the validation + * status (same as CameraConfiguration::validate). + */ +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces) +{ + Status status = Valid; + + /* + * Set all raw streams to the Raw color space, and make a note of the largest + * non-raw stream with a defined color space (if there is one). + */ + int index = -1; + for (auto [i, cfg] : utils::enumerate(config_)) { + if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) { + cfg.colorSpace = ColorSpace::Raw; + status = Adjusted; + } + else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) + index = i; + } + + if (index < 0 || !shareOutputColorSpaces) + return status; + + /* Make all output color spaces the same, if requested. */ + for (auto &cfg : config_) { + if (!isRaw(cfg.pixelFormat) && + cfg.colorSpace != config_[index].colorSpace) { + cfg.colorSpace = config_[index].colorSpace; + status = Adjusted; + } + } + + return status; +} + /** * \var CameraConfiguration::transform * \brief User-specified transform to be applied to the image