[libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Fix handling of colour spaces
Naushir Patuck
naush at raspberrypi.com
Fri Jan 6 11:09:32 CET 2023
Hi David,
Thank you for your work.
On Tue, 3 Jan 2023 at 11:33, David Plowman via libcamera-devel <
libcamera-devel at lists.libcamera.org> wrote:
> We implement a custom validateColorSpaces method that forces all
> (non-raw) streams to same colour space, whilst distinguishing RGB
> streams from YUV ones, as the former must have the YCbCr encoding and
> range over-written.
>
> When we apply the colour space, we always send the full YUV version as
> that gets converted correctly to what our hardware drivers expect. It
> is also careful to check what comes back as the YCbCr information gets
> overwritten again.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> .../pipeline/raspberrypi/raspberrypi.cpp | 99 ++++++++++++++++++-
> 1 file changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8569df17..bb574afc 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public
> CameraConfiguration
> public:
> RPiCameraConfiguration(const RPiCameraData *data);
>
> + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags
> flags);
> Status validate() override;
>
> /* Cache the combinedTransform_ that will be applied to the sensor
> */
> @@ -317,6 +318,13 @@ public:
>
> private:
> const RPiCameraData *data_;
> +
> + /*
> + * Store the colour spaces that all our streams will have. RGB
> format streams
> + * will be the same but need the YCbCr fields cleared.
> + */
> + std::optional<ColorSpace> yuvColorSpace_;
> + std::optional<ColorSpace> rgbColorSpace_;
> };
>
Should these fields live in RPiCameraData for the multi-camera cases?
>
> class PipelineHandlerRPi : public PipelineHandler
> @@ -357,6 +365,89 @@ RPiCameraConfiguration::RPiCameraConfiguration(const
> RPiCameraData *data)
> {
> }
>
> +static const std::vector<ColorSpace> validColorSpaces = {
> + ColorSpace::Sycc,
> + ColorSpace::Smpte170m,
> + ColorSpace::Rec709
> +};
> +
> +static std::optional<ColorSpace> findValidColorSpace(const ColorSpace
> &colourSpace)
> +{
> + for (auto cs : validColorSpaces) {
> + if (colourSpace.primaries == cs.primaries &&
> + colourSpace.transferFunction == cs.transferFunction)
> + return cs;
> + }
> +
> + return std::nullopt;
> +}
> +
> +static bool isRgb(const PixelFormat &pixFmt)
> +{
> + /*
> + * Be careful not to use this when a format might be raw because
> it returns
> + * the wrong result for mono raw streams.
> + */
>
Could we add an assert that tests this?
With those addressed (if appropriate):
Reviewed-by: Naushir Patuck <naushir at raspberrypi.com>
> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;
> +}
> +
> +static bool isYuv(const PixelFormat &pixFmt)
> +{
> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;
> +}
> +
> +CameraConfiguration::Status
> RPiCameraConfiguration::validateColorSpaces([[maybe_unused]]
> ColorSpaceFlags flags)
> +{
> + Status status = Valid;
> + yuvColorSpace_.reset();
> +
> + for (auto cfg : config_) {
> + /* First fix up raw streams to have the "raw" colour
> space. */
> + if (isRaw(cfg.pixelFormat) && cfg.colorSpace !=
> ColorSpace::Raw) {
> + /* If there was no value here, that doesn't count
> as "adjusted". */
> + if (cfg.colorSpace)
> + status = Adjusted;
> + cfg.colorSpace = ColorSpace::Raw;
> + }
> +
> + /* Next we need to find our shared colour space. The first
> valid one will do. */
> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw &&
> !yuvColorSpace_)
> + yuvColorSpace_ =
> findValidColorSpace(cfg.colorSpace.value());
> + }
> +
> + /* If no colour space was given anywhere, choose sYCC. */
> + if (!yuvColorSpace_)
> + yuvColorSpace_ = ColorSpace::Sycc;
> +
> + /* Note the version of this that any RGB streams will have to use.
> */
> + rgbColorSpace_ = yuvColorSpace_;
> + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> + rgbColorSpace_->range = ColorSpace::Range::Full;
> +
> + /* Go through the streams again and force everyone to the same
> colour space. */
> + for (auto cfg : config_) {
> + if (cfg.colorSpace == ColorSpace::Raw)
> + continue;
> +
> + if (isYuv(cfg.pixelFormat) && cfg.colorSpace !=
> yuvColorSpace_) {
> + /* Again, no value means "not adjusted". */
> + if (cfg.colorSpace)
> + status = Adjusted;
> + cfg.colorSpace = yuvColorSpace_;
> + }
> + if (isRgb(cfg.pixelFormat) && cfg.colorSpace !=
> rgbColorSpace_) {
> + /* Be nice, and let the YUV version count as
> non-adjusted too. */
> + if (cfg.colorSpace && cfg.colorSpace !=
> yuvColorSpace_)
> + status = Adjusted;
> + cfg.colorSpace = rgbColorSpace_;
> + }
> + }
> +
> + return status;
> +}
> +
> CameraConfiguration::Status RPiCameraConfiguration::validate()
> {
> Status status = Valid;
> @@ -533,7 +624,8 @@ CameraConfiguration::Status
> RPiCameraConfiguration::validate()
> V4L2DeviceFormat format;
> format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> - format.colorSpace = cfg.colorSpace;
> + /* We want to send the associated YCbCr info through to
> the driver. */
> + format.colorSpace = yuvColorSpace_;
>
> LOG(RPI, Debug)
> << "Try color space " <<
> ColorSpace::toString(cfg.colorSpace);
> @@ -542,6 +634,11 @@ CameraConfiguration::Status
> RPiCameraConfiguration::validate()
> if (ret)
> return Invalid;
>
> + /*
> + * But for RGB streams, the YCbCr info gets overwritten on
> the way back
> + * so we must check against what the stream cfg says, not
> what we actually
> + * requested (which carefully included the YCbCr info)!
> + */
> if (cfg.colorSpace != format.colorSpace) {
> status = Adjusted;
> LOG(RPI, Debug)
> --
> 2.30.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230106/96809039/attachment.htm>
More information about the libcamera-devel
mailing list