<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 Wed, 20 Oct 2021 at 12:08, 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 color spaces correctly.<br>
<br>
In generateConfiguration() it sets them to reasonable default values<br>
based on the stream role.<br>
<br>
validate() now calls validateColorSpaces() to ensure that the<br>
requested color spaces are sensible, before proceeding to check what<br>
the hardware can deliver.<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 | 42 +++++++++++++++++++<br>
1 file changed, 42 insertions(+)<br>
<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index 1634ca98..22db54ce 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -288,6 +288,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
if (config_.empty())<br>
return Invalid;<br>
<br>
+ status = validateColorSpaces(true);<br>
+<br>
/*<br>
* What if the platform has a non-90 degree rotation? We can't even<br>
* "adjust" the configuration and carry on. Alternatively, raising an<br>
@@ -354,6 +356,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></blockquote><div><br></div><div>This is going to conflict badly with my media controller changes ;-)</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);<br>
if (ret)<br>
return Invalid;<br>
@@ -449,10 +452,21 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
V4L2DeviceFormat format;<br>
format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);<br>
format.size = cfg.size;<br>
+ format.colorSpace = cfg.requestedColorSpace;<br>
+ LOG(RPI, Debug)<br>
+ << "Try color space " << cfg.requestedColorSpace.toString();<br>
<br>
int ret = dev->tryFormat(&format);<br>
if (ret)<br>
return Invalid;<br>
+ cfg.actualColorSpace = format.colorSpace;<br>
+ if (cfg.actualColorSpace != cfg.requestedColorSpace) {<br>
+ status = Adjusted;<br>
+ LOG(RPI, Warning)<br>
+ << "Color space changed from "<br>
+ << cfg.requestedColorSpace.toString() << " to "<br>
+ << cfg.actualColorSpace.toString();<br>
+ }<br>
<br>
cfg.stride = format.planes[0].bpl;<br>
cfg.frameSize = format.planes[0].size;<br>
@@ -477,6 +491,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
PixelFormat pixelFormat;<br>
V4L2VideoDevice::Formats fmts;<br>
Size size;<br>
+ ColorSpace colorSpace;<br>
<br>
if (roles.empty())<br>
return config;<br>
@@ -491,6 +506,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
sensorFormat = findBestMode(fmts, size);<br>
pixelFormat = sensorFormat.fourcc.toPixelFormat();<br>
ASSERT(pixelFormat.isValid());<br>
+ colorSpace = ColorSpace::Raw;<br>
bufferCount = 2;<br>
rawCount++;<br>
break;<br>
@@ -498,6 +514,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
case StreamRole::StillCapture:<br>
fmts = data->isp_[Isp::Output0].dev()->formats();<br>
pixelFormat = formats::NV12;<br>
+ colorSpace = ColorSpace::Jpeg;<br>
/* Return the largest sensor resolution. */<br>
size = data->sensor_->resolution();<br>
bufferCount = 1;<br>
@@ -515,6 +532,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
*/<br>
fmts = data->isp_[Isp::Output0].dev()->formats();<br>
pixelFormat = formats::YUV420;<br>
+ /* This will be reasonable for many applications. */<br>
+ colorSpace = ColorSpace::Rec709;<br>
size = { 1920, 1080 };<br>
bufferCount = 4;<br>
outCount++;<br>
@@ -523,6 +542,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
case StreamRole::Viewfinder:<br>
fmts = data->isp_[Isp::Output0].dev()->formats();<br>
pixelFormat = formats::ARGB8888;<br>
+ colorSpace = ColorSpace::Jpeg;<br>
size = { 800, 600 };<br>
bufferCount = 4;<br>
outCount++;<br>
@@ -554,6 +574,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
StreamConfiguration cfg(formats);<br>
cfg.size = size;<br>
cfg.pixelFormat = pixelFormat;<br>
+ cfg.requestedColorSpace = colorSpace;<br>
cfg.bufferCount = bufferCount;<br>
config->addConfiguration(cfg);<br>
}<br>
@@ -601,6 +622,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<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>
@@ -650,6 +672,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);<br>
format.size = cfg.size;<br>
format.fourcc = fourcc;<br>
+ format.colorSpace = cfg.requestedColorSpace;<br>
<br>
LOG(RPI, Debug) << "Setting " << stream->name() << " to "<br>
<< format.toString();<br>
@@ -665,6 +688,23 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
return -EINVAL;<br>
}<br>
<br>
+ if (cfg.actualColorSpace != format.colorSpace) {<br>
+ /*<br>
+ * We should have been through validate() before so this<br>
+ * shouldn't be possible, but we mustn't sweep color space<br>
+ * problems under the carpet.<br>
+ */<br>
+ LOG(RPI, Warning)<br>
+ << "Unexpected color space change ("<br>
+ << cfg.actualColorSpace.toString() << " to "<br>
+ << format.colorSpace.toString() << ") in stream "<br>
+ << stream->name();<br>
+ cfg.actualColorSpace = format.colorSpace;<br>
+ }<br>
+ LOG(RPI, Debug)<br>
+ << "Stream " << stream->name() << " has color space "<br>
+ << cfg.actualColorSpace.toString();<br>
+<br>
cfg.setStream(stream);<br>
stream->setExternal(true);<br>
<br>
@@ -689,6 +729,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
format = {};<br>
format.size = maxSize;<br>
format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);<br>
+ /* No one asked for output, so the color space doesn't matter. */<br>
+ format.colorSpace = ColorSpace::Jpeg;<br>
ret = data->isp_[Isp::Output0].dev()->setFormat(&format);<br>
if (ret) {<br>
LOG(RPI, Error)<br>
-- <br>
2.20.1<br>
<br>
</blockquote></div></div>