Message ID | 20211104135805.5269-7-david.plowman@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting David Plowman (2021-11-04 13:58:04) > This method checks that the requested color spaces are sensible and do > not contain any undefined enum values. It also initialises the > "actual" color space field to the same value as the one requested, in > the expectation that the rest of the validate() method will be able to > check this. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/camera.h | 2 ++ > src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 601ee46e..fdab4410 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -69,6 +69,8 @@ public: > protected: > CameraConfiguration(); > > + Status validateColorSpaces(bool sharedColorSpace); > + > std::vector<StreamConfiguration> config_; > }; > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 400a7cf0..90e9460b 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -20,6 +20,7 @@ > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_controls.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > > /** > @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const > return config_.size(); > } > > +static bool isRaw(const PixelFormat &pixFmt) This looks like it should be a helper added to the PixelFormat class as a patch of it's own? I'm not sure it belongs here in the camera class. > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.isValid() && > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > +} > + I think this is missing doxygen documentation. What's the definition of a 'sharedColorSpace' ? > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace) > +{ > + Status status = Valid; > + > + /* Find the largest non-raw stream (if any). */ > + int index = -1; > + for (unsigned int i = 0; i < config_.size(); i++) { > + const StreamConfiguration &cfg = config_[i]; > + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size)) > + index = i; > + } > + Given the below usage of index, I'd be tempted to put an ASSERT(index != -1); here. > + /* > + * Here we force raw streams to the correct color space and signal > + * an error if we encounter anything undefined. We handle the case > + * where all output streams are to share a color space, which we > + * choose to be the color space of the largest stream. > + */ > + for (auto &cfg : config_) { > + ColorSpace initialColorSpace = cfg.requestedColorSpace; > + > + if (isRaw(cfg.pixelFormat)) > + cfg.requestedColorSpace = ColorSpace::Raw; > + else if (!cfg.requestedColorSpace.isFullyDefined()) { > + LOG(Camera, Error) << "Stream has undefined color space"; > + cfg.requestedColorSpace = ColorSpace::Jpeg; > + } else if (sharedColorSpace) > + cfg.requestedColorSpace = config_[index].requestedColorSpace; Can index ever be -1 here? > + > + if (cfg.requestedColorSpace != initialColorSpace) > + status = Adjusted; > + > + /* > + * We also initialise the actual color space as if the > + * hardware can do what we want. But note that the rest > + * of the validate() method may change this. > + */ > + cfg.actualColorSpace = cfg.requestedColorSpace; > + } > + > + return status; > +} > + > /** > * \var CameraConfiguration::transform > * \brief User-specified transform to be applied to the image > -- > 2.20.1 >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 601ee46e..fdab4410 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -69,6 +69,8 @@ public: protected: CameraConfiguration(); + Status validateColorSpaces(bool sharedColorSpace); + std::vector<StreamConfiguration> config_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 400a7cf0..90e9460b 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -20,6 +20,7 @@ #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_controls.h" +#include "libcamera/internal/formats.h" #include "libcamera/internal/pipeline_handler.h" /** @@ -314,6 +315,56 @@ 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; +} + +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace) +{ + Status status = Valid; + + /* Find the largest non-raw stream (if any). */ + int index = -1; + for (unsigned int i = 0; i < config_.size(); i++) { + const StreamConfiguration &cfg = config_[i]; + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size)) + index = i; + } + + /* + * Here we force raw streams to the correct color space and signal + * an error if we encounter anything undefined. We handle the case + * where all output streams are to share a color space, which we + * choose to be the color space of the largest stream. + */ + for (auto &cfg : config_) { + ColorSpace initialColorSpace = cfg.requestedColorSpace; + + if (isRaw(cfg.pixelFormat)) + cfg.requestedColorSpace = ColorSpace::Raw; + else if (!cfg.requestedColorSpace.isFullyDefined()) { + LOG(Camera, Error) << "Stream has undefined color space"; + cfg.requestedColorSpace = ColorSpace::Jpeg; + } else if (sharedColorSpace) + cfg.requestedColorSpace = config_[index].requestedColorSpace; + + if (cfg.requestedColorSpace != initialColorSpace) + status = Adjusted; + + /* + * We also initialise the actual color space as if the + * hardware can do what we want. But note that the rest + * of the validate() method may change this. + */ + cfg.actualColorSpace = cfg.requestedColorSpace; + } + + return status; +} + /** * \var CameraConfiguration::transform * \brief User-specified transform to be applied to the image