<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 8 Jun 2021 at 15:44, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</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">The Raspberry Pi pipeline handler now sets colour spaces correctly.<br>
<br>
In generateConfiguration() is sets them to reasonable default values<br></blockquote><div><br></div><div>Typo:</div><div>s\is\it\</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">
based on the stream role. Note how video recording streams use the<br>
"VIDEO" YCbCr encoding, which will later be fixed up according to the<br>
requested resolution.<br>
<br>
validate() and configure() check the colour space is valid, and also<br>
force all (non-raw) output streams to share the same colour space<br>
(which is a hardware restriction). Finally, the "VIDEO" YCbCr encoding<br>
is corrected by configure() to be one of REC601, REC709 or REC2020.<br>
---<br>
.../pipeline/raspberrypi/raspberrypi.cpp | 84 +++++++++++++++++++<br>
1 file changed, 84 insertions(+)<br>
<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index a65b4568..acb4a1c3 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -279,6 +279,24 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)<br>
{<br>
}<br>
<br>
+static bool validateColorSpace(ColorSpace &colorSpace)<br>
+{<br>
+ const std::vector<ColorSpace> validColorSpaces = {<br>
+ ColorSpace::JFIF,<br>
+ ColorSpace::SMPTE170M,<br>
+ ColorSpace::REC709,<br>
+ ColorSpace::REC2020,<br>
+ ColorSpace::VIDEO,<br>
+ };<br>
+ auto it = std::find_if(validColorSpaces.begin(), validColorSpaces.end(),<br>
+ [&colorSpace](const ColorSpace &item) { return colorSpace == item; });<br></blockquote><div><br></div><div>You could use std::find() for fewer characters to type :-)</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">
+ if (it != validColorSpaces.end())<br>
+ return true;<br>
+<br>
+ colorSpace = ColorSpace::JFIF;<br>
+ return false;<br>
+}<br>
+<br>
CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
{<br>
Status status = Valid;<br>
@@ -344,6 +362,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;<br>
std::pair<int, Size> outSize[2];<br>
Size maxSize;<br>
+ ColorSpace colorSpace; /* colour space for all non-raw output streams */<br>
for (StreamConfiguration &cfg : config_) {<br>
if (isRaw(cfg.pixelFormat)) {<br>
/*<br>
@@ -352,6 +371,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
*/<br>
V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();<br>
V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);<br>
+ sensorFormat.colorSpace = ColorSpace::RAW;<br>
int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);<br>
if (ret)<br>
return Invalid;<br>
@@ -390,6 +410,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
if (maxSize < cfg.size) {<br>
maxSize = cfg.size;<br>
maxIndex = outCount;<br>
+ colorSpace = cfg.colorSpace;<br>
}<br>
outCount++;<br>
}<br>
@@ -403,6 +424,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
}<br>
}<br>
<br>
+ /* Ensure the output colour space (if present) is one we handle. */<br>
+ if (outCount)<br>
+ validateColorSpace(colorSpace);<br>
+<br>
/*<br>
* Now do any fixups needed. For the two ISP outputs, one stream must be<br>
* equal or smaller than the other in all dimensions.<br>
@@ -444,10 +469,26 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
status = Adjusted;<br>
}<br>
<br>
+ /* Output streams must share the same colour space. */<br>
+ if (cfg.colorSpace != colorSpace) {<br>
+ LOG(RPI, Warning) << "Output stream " << (i == maxIndex ? 0 : 1)<br>
+ << " colour space changed";<br>
+ cfg.colorSpace = colorSpace;<br>
+ status = Adjusted;<br>
+ }<br>
+<br>
V4L2DeviceFormat format;<br>
format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);<br>
format.size = cfg.size;<br>
<br>
+ /*<br>
+ * Request a sensible colour space. Note that "VIDEO" isn't a real<br>
+ * encoding, so substitute something else sensible.<br>
+ */<br>
+ format.colorSpace = colorSpace;<br>
+ if (format.colorSpace.encoding == ColorSpace::Encoding::VIDEO)<br>
+ format.colorSpace.encoding = ColorSpace::Encoding::REC601;<br>
+<br>
int ret = dev->tryFormat(&format);<br>
if (ret)<br>
return Invalid;<br>
@@ -473,6 +514,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
V4L2DeviceFormat sensorFormat;<br>
unsigned int bufferCount;<br>
PixelFormat pixelFormat;<br>
+ ColorSpace colorSpace;<br>
V4L2VideoDevice::Formats fmts;<br>
Size size;<br>
<br>
@@ -488,6 +530,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
fmts = data->unicam_[Unicam::Image].dev()->formats();<br>
sensorFormat = findBestMode(fmts, size);<br>
pixelFormat = sensorFormat.fourcc.toPixelFormat();<br>
+ colorSpace = ColorSpace::RAW;<br>
ASSERT(pixelFormat.isValid());<br>
bufferCount = 2;<br>
rawCount++;<br>
@@ -499,6 +542,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
/* Return the largest sensor resolution. */<br>
size = data->sensor_->resolution();<br>
bufferCount = 1;<br>
+ colorSpace = ColorSpace::JFIF;<br>
outCount++;<br>
break;<br>
<br>
@@ -515,6 +559,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
pixelFormat = formats::YUV420;<br>
size = { 1920, 1080 };<br>
bufferCount = 4;<br>
+ colorSpace = ColorSpace::VIDEO;<br>
outCount++;<br>
break;<br>
<br>
@@ -523,6 +568,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
pixelFormat = formats::ARGB8888;<br>
size = { 800, 600 };<br>
bufferCount = 4;<br>
+ colorSpace = ColorSpace::JFIF;<br>
outCount++;<br>
break;<br>
<br>
@@ -552,6 +598,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
StreamConfiguration cfg(formats);<br>
cfg.size = size;<br>
cfg.pixelFormat = pixelFormat;<br>
+ cfg.colorSpace = colorSpace;<br>
cfg.bufferCount = bufferCount;<br>
config->addConfiguration(cfg);<br>
}<br>
@@ -573,6 +620,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
Size maxSize, sensorSize;<br>
unsigned int maxIndex = 0;<br>
bool rawStream = false;<br>
+ ColorSpace colorSpace; /* colour space for all non-raw output streams */<br>
<br>
/*<br>
* Look for the RAW stream (if given) size as well as the largest<br>
@@ -591,14 +639,40 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
} else {<br>
if (cfg.size > maxSize) {<br>
maxSize = config->at(i).size;<br>
+ colorSpace = config->at(i).colorSpace;<br>
maxIndex = i;<br>
}<br>
}<br>
}<br>
<br>
+ if (maxSize.isNull()) {<br>
+ /*<br>
+ * No non-raw streams, so some will get made below. Doesn't matter<br>
+ * what colour space we assign to them.<br>
+ */<br>
+ colorSpace = ColorSpace::JFIF;<br>
+ } else {<br>
+ /* Make sure we can handle this colour space. */<br>
+ validateColorSpace(colorSpace);<br>
+<br>
+ /*<br>
+ * The "VIDEO" colour encoding means that we choose one of REC601,<br>
+ * REC709 or REC2020 automatically according to the resolution.<br>
+ */<br>
+ if (colorSpace.encoding == ColorSpace::Encoding::VIDEO) {<br>
+ if (maxSize.width >= 3840)<br>
+ colorSpace.encoding = ColorSpace::Encoding::REC2020;<br>
+ else if (maxSize.width >= 1280)<br>
+ colorSpace.encoding = ColorSpace::Encoding::REC709;<br>
+ else<br>
+ colorSpace.encoding = ColorSpace::Encoding::REC601;<br>
+ }<br>
+ }<br>
+<br>
/* First calculate the best sensor mode we can use based on the user request. */<br>
V4L2VideoDevice::Formats fmts = data->unicam_[Unicam::Image].dev()->formats();<br>
V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ? sensorSize : maxSize);<br>
+ sensorFormat.colorSpace = ColorSpace::RAW;<br>
<br>
/*<br>
* Unicam image output format. The ISP input format gets set at start,<br>
@@ -636,11 +710,18 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
StreamConfiguration &cfg = config->at(i);<br>
<br>
if (isRaw(cfg.pixelFormat)) {<br>
+ cfg.colorSpace = ColorSpace::RAW;<br>
cfg.setStream(&data->unicam_[Unicam::Image]);<br>
data->unicam_[Unicam::Image].setExternal(true);<br>
continue;<br>
}<br>
<br>
+ /* All other streams share the same colour space. */<br>
+ if (cfg.colorSpace != colorSpace) {<br>
+ LOG(RPI, Warning) << "Stream " << i << " colour space changed";<br></blockquote><div><br></div><div>Perhaps this should be a Info log level?</div><div><br></div><div>All things here are minors so </div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@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">
+ cfg.colorSpace = colorSpace;<br>
+ }<br>
+<br>
/* The largest resolution gets routed to the ISP Output 0 node. */<br>
RPi::Stream *stream = i == maxIndex ? &data->isp_[Isp::Output0]<br>
: &data->isp_[Isp::Output1];<br>
@@ -648,6 +729,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);<br>
format.size = cfg.size;<br>
format.fourcc = fourcc;<br>
+ format.colorSpace = cfg.colorSpace;<br>
<br>
LOG(RPI, Debug) << "Setting " << stream->name() << " to "<br>
<< format.toString();<br>
@@ -687,6 +769,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
format = {};<br>
format.size = maxSize;<br>
format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420, false);<br>
+ format.colorSpace = colorSpace;<br>
ret = data->isp_[Isp::Output0].dev()->setFormat(&format);<br>
if (ret) {<br>
LOG(RPI, Error)<br>
@@ -716,6 +799,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
const Size limit = maxDimensions.boundedToAspectRatio(format.size);<br>
<br>
output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);<br>
+ output1Format.colorSpace = colorSpace;<br>
<br>
LOG(RPI, Debug) << "Setting ISP Output1 (internal) to "<br>
<< output1Format.toString();<br>
-- <br>
2.20.1<br>
<br>
</blockquote></div></div>