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