| Message ID | 20230120162258.7039-2-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman via libcamera-devel (2023-01-20 16:22:58) > 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 on the way back. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8569df17..f768dced 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_; > }; > > class PipelineHandlerRPi : public PipelineHandler > @@ -357,6 +365,105 @@ 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) > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > +} > + > +static bool isYuv(const PixelFormat &pixFmt) > +{ > + /* The code below would return true for raw mono streams, so weed those out first. */ > + if (isRaw(pixFmt)) > + return false; > + > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > +} > + > +/* > + * Raspberry Pi drivers expect the following colour spaces: > + * - V4L2_COLORSPACE_RAW for raw streams. > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and > + * quantisation are not used. > + * > + * The libcamera colour spaces that we wish to use corresponding to these are therefore: > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > + */ > + > +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)) { > + /* If there was no value here, that doesn't count as "adjusted". */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { > + status = Adjusted; > + cfg.colorSpace = ColorSpace::Raw; > + } > + continue; > + } > + > + /* Next we need to find our shared colour space. The first valid one will do. */ > + if (cfg.colorSpace && !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_; > + } Nit blank line? Or is this hugging of two scopes intentional? > + if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) { > + /* Be nice, and let the YUV version count as non-adjusted too. */ I do wonder if we should update the Adjusted to a set of Flags that say 'what' has been adjusted instead ... Then an application can identify if it cares about the change or not. But - I don't think that matters to this patch. The fact we're making small adjustments but not reporting to the application may worry me a little, but I expect if this passes your metric for worrying about colorspace, I shouldn't worry, as you're bar must be higher than mine! So I'd say: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_) > + status = Adjusted; > + cfg.colorSpace = rgbColorSpace_; > + } > + } > + > + return status; > +} > + > CameraConfiguration::Status RPiCameraConfiguration::validate() > { > Status status = Valid; > @@ -533,7 +640,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 +650,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 >
Hi David, On 1/20/23 9:52 PM, David Plowman via libcamera-devel 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 on the way back. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8569df17..f768dced 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. Would re-pharse it like: /* * Store the colour spaces that all our streams will have. RGB format streams * will have the same colorspace as YUV streams, with YCbCr field cleared and * range set to full. */ > + */ > + std::optional<ColorSpace> yuvColorSpace_; > + std::optional<ColorSpace> rgbColorSpace_; > }; > > class PipelineHandlerRPi : public PipelineHandler > @@ -357,6 +365,105 @@ 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) > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > +} > + > +static bool isYuv(const PixelFormat &pixFmt) > +{ > + /* The code below would return true for raw mono streams, so weed those out first. */ > + if (isRaw(pixFmt)) > + return false; > + > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > +} > + > +/* > + * Raspberry Pi drivers expect the following colour spaces: > + * - V4L2_COLORSPACE_RAW for raw streams. > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and > + * quantisation are not used. > + * > + * The libcamera colour spaces that we wish to use corresponding to these are therefore: > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > + */ Thank you for the information. This makes the expectations clearer. With that, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> I can fixup the comment while applying the patch (if you are OK with the comment's explanation). > + > +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)) { > + /* If there was no value here, that doesn't count as "adjusted". */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { > + status = Adjusted; > + cfg.colorSpace = ColorSpace::Raw; > + } > + continue; > + } > + > + /* Next we need to find our shared colour space. The first valid one will do. */ > + if (cfg.colorSpace && !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 +640,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 +650,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)
Hi David, There's a small question below that is holding this back from being merged. I'm planning a release tomorrow, so I'd like to get this colourspace fix in. Quoting Umang Jain via libcamera-devel (2023-01-25 06:48:19) > Hi David, > > On 1/20/23 9:52 PM, David Plowman via libcamera-devel 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 on the way back. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > > 1 file changed, 114 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 8569df17..f768dced 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. > > Would re-pharse it like: > > /* > * Store the colour spaces that all our streams will have. RGB > format streams > * will have the same colorspace as YUV streams, with YCbCr > field cleared and > * range set to full. > */ Are you happy with the expansion on this comment? -- Kieran > > > + */ > > + std::optional<ColorSpace> yuvColorSpace_; > > + std::optional<ColorSpace> rgbColorSpace_; > > }; > > > > class PipelineHandlerRPi : public PipelineHandler > > @@ -357,6 +365,105 @@ 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) > > +{ > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > > +} > > + > > +static bool isYuv(const PixelFormat &pixFmt) > > +{ > > + /* The code below would return true for raw mono streams, so weed those out first. */ > > + if (isRaw(pixFmt)) > > + return false; > > + > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > > +} > > + > > +/* > > + * Raspberry Pi drivers expect the following colour spaces: > > + * - V4L2_COLORSPACE_RAW for raw streams. > > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for > > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and > > + * quantisation are not used. > > + * > > + * The libcamera colour spaces that we wish to use corresponding to these are therefore: > > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > > + */ > > Thank you for the information. This makes the expectations clearer. > > With that, > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > I can fixup the comment while applying the patch (if you are OK with the > comment's explanation). > > > + > > +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)) { > > + /* If there was no value here, that doesn't count as "adjusted". */ > > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { > > + status = Adjusted; > > + cfg.colorSpace = ColorSpace::Raw; > > + } > > + continue; > > + } > > + > > + /* Next we need to find our shared colour space. The first valid one will do. */ > > + if (cfg.colorSpace && !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 +640,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 +650,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) >
Hi Kieran, Umag I didn't spot the question, sorry about that. But yes, more complete comments always welcome! Thanks David On Mon, 30 Jan 2023 at 11:04, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, > > There's a small question below that is holding this back from being merged. > > I'm planning a release tomorrow, so I'd like to get this colourspace fix > in. > > Quoting Umang Jain via libcamera-devel (2023-01-25 06:48:19) > > Hi David, > > > > On 1/20/23 9:52 PM, David Plowman via libcamera-devel 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 on the way back. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > > > 1 file changed, 114 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 8569df17..f768dced 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. > > > > Would re-pharse it like: > > > > /* > > * Store the colour spaces that all our streams will have. RGB > > format streams > > * will have the same colorspace as YUV streams, with YCbCr > > field cleared and > > * range set to full. > > */ > > Are you happy with the expansion on this comment? > > > -- > Kieran > > > > > > > > + */ > > > + std::optional<ColorSpace> yuvColorSpace_; > > > + std::optional<ColorSpace> rgbColorSpace_; > > > }; > > > > > > class PipelineHandlerRPi : public PipelineHandler > > > @@ -357,6 +365,105 @@ 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) > > > +{ > > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > > > +} > > > + > > > +static bool isYuv(const PixelFormat &pixFmt) > > > +{ > > > + /* The code below would return true for raw mono streams, so weed those out first. */ > > > + if (isRaw(pixFmt)) > > > + return false; > > > + > > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > > > +} > > > + > > > +/* > > > + * Raspberry Pi drivers expect the following colour spaces: > > > + * - V4L2_COLORSPACE_RAW for raw streams. > > > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for > > > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and > > > + * quantisation are not used. > > > + * > > > + * The libcamera colour spaces that we wish to use corresponding to these are therefore: > > > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > > > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > > > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > > > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > > > + */ > > > > Thank you for the information. This makes the expectations clearer. > > > > With that, > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > I can fixup the comment while applying the patch (if you are OK with the > > comment's explanation). > > > > > + > > > +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)) { > > > + /* If there was no value here, that doesn't count as "adjusted". */ > > > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { > > > + status = Adjusted; > > > + cfg.colorSpace = ColorSpace::Raw; > > > + } > > > + continue; > > > + } > > > + > > > + /* Next we need to find our shared colour space. The first valid one will do. */ > > > + if (cfg.colorSpace && !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 +640,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 +650,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) > >
Hi David, Thank you for the patch. On Fri, Jan 20, 2023 at 04:22:58PM +0000, David Plowman via libcamera-devel 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 on the way back. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8569df17..f768dced 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_; > }; > > class PipelineHandlerRPi : public PipelineHandler > @@ -357,6 +365,105 @@ 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) > +{ > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > +} > + > +static bool isYuv(const PixelFormat &pixFmt) > +{ > + /* The code below would return true for raw mono streams, so weed those out first. */ > + if (isRaw(pixFmt)) > + return false; > + > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > +} > + > +/* > + * Raspberry Pi drivers expect the following colour spaces: > + * - V4L2_COLORSPACE_RAW for raw streams. > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and > + * quantisation are not used. > + * > + * The libcamera colour spaces that we wish to use corresponding to these are therefore: > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > + */ > + > +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)) { > + /* If there was no value here, that doesn't count as "adjusted". */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { > + status = Adjusted; > + cfg.colorSpace = ColorSpace::Raw; Agreed for the Adjusted flag, but shouldn't you set cfg.colorSpace to raw even if !cfg.colorSpace ? > + } > + continue; > + } > + > + /* Next we need to find our shared colour space. The first valid one will do. */ > + if (cfg.colorSpace && !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 +640,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 +650,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)
Hi Laurent Whoops, bit of a slip in the little "optimisation" in the v2 version. Thanks for spotting it, I'll send a small patch. David On Mon, 6 Feb 2023 at 17:56, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi David, > > Thank you for the patch. > > On Fri, Jan 20, 2023 at 04:22:58PM +0000, David Plowman via > libcamera-devel 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 on the way back. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++- > > 1 file changed, 114 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 8569df17..f768dced 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_; > > }; > > > > class PipelineHandlerRPi : public PipelineHandler > > @@ -357,6 +365,105 @@ > 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) > > +{ > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; > > +} > > + > > +static bool isYuv(const PixelFormat &pixFmt) > > +{ > > + /* The code below would return true for raw mono streams, so weed > those out first. */ > > + if (isRaw(pixFmt)) > > + return false; > > + > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; > > +} > > + > > +/* > > + * Raspberry Pi drivers expect the following colour spaces: > > + * - V4L2_COLORSPACE_RAW for raw streams. > > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, > V4L2_COLORSPACE_REC709 for > > + * non-raw streams. Other fields such as transfer function, YCbCr > encoding and > > + * quantisation are not used. > > + * > > + * The libcamera colour spaces that we wish to use corresponding to > these are therefore: > > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW > > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG > > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M > > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 > > + */ > > + > > +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)) { > > + /* If there was no value here, that doesn't count > as "adjusted". */ > > + if (cfg.colorSpace && cfg.colorSpace != > ColorSpace::Raw) { > > + status = Adjusted; > > + cfg.colorSpace = ColorSpace::Raw; > > Agreed for the Adjusted flag, but shouldn't you set cfg.colorSpace to > raw even if !cfg.colorSpace ? > > > + } > > + continue; > > + } > > + > > + /* Next we need to find our shared colour space. The first > valid one will do. */ > > + if (cfg.colorSpace && !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 +640,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 +650,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) > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 8569df17..f768dced 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_; }; class PipelineHandlerRPi : public PipelineHandler @@ -357,6 +365,105 @@ 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) +{ + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB; +} + +static bool isYuv(const PixelFormat &pixFmt) +{ + /* The code below would return true for raw mono streams, so weed those out first. */ + if (isRaw(pixFmt)) + return false; + + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV; +} + +/* + * Raspberry Pi drivers expect the following colour spaces: + * - V4L2_COLORSPACE_RAW for raw streams. + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for + * non-raw streams. Other fields such as transfer function, YCbCr encoding and + * quantisation are not used. + * + * The libcamera colour spaces that we wish to use corresponding to these are therefore: + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709 + */ + +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)) { + /* If there was no value here, that doesn't count as "adjusted". */ + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) { + status = Adjusted; + cfg.colorSpace = ColorSpace::Raw; + } + continue; + } + + /* Next we need to find our shared colour space. The first valid one will do. */ + if (cfg.colorSpace && !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 +640,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 +650,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)