<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 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 ColorSpace from the StreamConfiguration is now handled<br>
appropriately in the V4L2VideoDevice.<br>
<br>
Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">---<br>
include/libcamera/internal/v4l2_videodevice.h | 2 +<br>
src/libcamera/v4l2_videodevice.cpp | 69 +++++++++++++++++--<br>
2 files changed, 66 insertions(+), 5 deletions(-)<br>
<br>
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h<br>
index efe34d47..34cc9cdd 100644<br>
--- a/include/libcamera/internal/v4l2_videodevice.h<br>
+++ b/include/libcamera/internal/v4l2_videodevice.h<br>
@@ -20,6 +20,7 @@<br>
#include <libcamera/base/log.h><br>
#include <libcamera/base/signal.h><br>
<br>
+#include <libcamera/color_space.h><br>
#include <libcamera/framebuffer.h><br>
#include <libcamera/geometry.h><br>
#include <libcamera/pixel_format.h><br>
@@ -163,6 +164,7 @@ public:<br>
<br>
V4L2PixelFormat fourcc;<br>
Size size;<br>
+ ColorSpace colorSpace;<br>
<br>
std::array<Plane, 3> planes;<br>
unsigned int planesCount = 0;<br>
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp<br>
index ba5f88cd..9e57e257 100644<br>
--- a/src/libcamera/v4l2_videodevice.cpp<br>
+++ b/src/libcamera/v4l2_videodevice.cpp<br>
@@ -364,11 +364,6 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const<br>
* \brief The plane line stride (in bytes)<br>
*/<br>
<br>
-/**<br>
- * \var V4L2DeviceFormat::size<br>
- * \brief The image size in pixels<br>
- */<br>
-<br>
/**<br>
* \var V4L2DeviceFormat::fourcc<br>
* \brief The fourcc code describing the pixel encoding scheme<br>
@@ -377,6 +372,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const<br>
* that identifies the image format pixel encoding scheme.<br>
*/<br>
<br>
+/**<br>
+ * \var V4L2DeviceFormat::size<br>
+ * \brief The image size in pixels<br>
+ */<br>
+<br>
+/**<br>
+ * \var V4L2DeviceFormat::colorSpace<br>
+ * \brief The color space of the pixels<br>
+ *<br>
+ * When setting or trying a format, passing in "Undefined" fields in the<br>
+ * ColorSpace is not permitted because the driver will then make an<br>
+ * arbitrary choice of its own. Choices made by the driver will be<br>
+ * passed back in the normal way, though note that "Undefined" values can<br>
+ * be returned if the device has chosen something that the ColorSpace<br>
+ * class cannot represent.<br>
+ */<br>
+<br>
/**<br>
* \var V4L2DeviceFormat::planes<br>
* \brief The per-plane memory size information<br>
@@ -873,6 +885,12 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)<br>
format->fourcc = V4L2PixelFormat(pix->pixelformat);<br>
format->planesCount = pix->num_planes;<br>
<br>
+ format->colorSpace = v4l2ToColorSpace(*pix);<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Warning)<br>
+ << "Retrieved undefined color space: "<br>
+ << format->colorSpace.toString();<br>
+<br>
for (unsigned int i = 0; i < format->planesCount; ++i) {<br>
format->planes[i].bpl = pix->plane_fmt[i].bytesperline;<br>
format->planes[i].size = pix->plane_fmt[i].sizeimage;<br>
@@ -887,6 +905,11 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)<br>
struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;<br>
int ret;<br>
<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Error)<br>
+ << "Trying to set undefined color space: "<br>
+ << format->colorSpace.toString();<br>
+<br></blockquote><div><br></div><div>Should we return with an error code in the above if clause? Similar for the other setFormat() calls.</div><div>Other than that:</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div><br></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">
v4l2Format.type = bufferType_;<br>
pix->width = format->size.width;<br>
pix->height = format->size.height;<br>
@@ -894,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)<br>
pix->num_planes = format->planesCount;<br>
pix->field = V4L2_FIELD_NONE;<br>
<br>
+ ret = colorSpaceToV4l2(format->colorSpace, *pix);<br>
+ if (ret < 0)<br>
+ LOG(V4L2, Warning)<br>
+ << "Setting color space unrecognised by V4L2: "<br>
+ << format->colorSpace.toString();<br>
+<br>
ASSERT(pix->num_planes <= std::size(pix->plane_fmt));<br>
<br>
for (unsigned int i = 0; i < pix->num_planes; ++i) {<br>
@@ -922,6 +951,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)<br>
format->planes[i].size = pix->plane_fmt[i].sizeimage;<br>
}<br>
<br>
+ format->colorSpace = v4l2ToColorSpace(*pix);<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Warning)<br>
+ << "Undefined color space has been set: "<br>
+ << format->colorSpace.toString();<br>
+<br>
return 0;<br>
}<br>
<br>
@@ -945,6 +980,12 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)<br>
format->planes[0].bpl = pix->bytesperline;<br>
format->planes[0].size = pix->sizeimage;<br>
<br>
+ format->colorSpace = v4l2ToColorSpace(*pix);<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Warning)<br>
+ << "Retrieved undefined color space: "<br>
+ << format->colorSpace.toString();<br>
+<br>
return 0;<br>
}<br>
<br>
@@ -954,12 +995,24 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)<br>
struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;<br>
int ret;<br>
<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Error)<br>
+ << "Trying to set undefined color space: "<br>
+ << format->colorSpace.toString();<br>
+<br>
v4l2Format.type = bufferType_;<br>
pix->width = format->size.width;<br>
pix->height = format->size.height;<br>
pix->pixelformat = format->fourcc;<br>
pix->bytesperline = format->planes[0].bpl;<br>
pix->field = V4L2_FIELD_NONE;<br>
+<br>
+ ret = colorSpaceToV4l2(format->colorSpace, *pix);<br>
+ if (ret < 0)<br>
+ LOG(V4L2, Warning)<br>
+ << "Set color space unrecognised by V4L2: "<br>
+ << format->colorSpace.toString();<br>
+<br>
ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);<br>
if (ret) {<br>
LOG(V4L2, Error)<br>
@@ -979,6 +1032,12 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)<br>
format->planes[0].bpl = pix->bytesperline;<br>
format->planes[0].size = pix->sizeimage;<br>
<br>
+ format->colorSpace = v4l2ToColorSpace(*pix);<br>
+ if (!format->colorSpace.isFullyDefined())<br>
+ LOG(V4L2, Warning)<br>
+ << "Undefined color space has been set: "<br>
+ << format->colorSpace.toString();<br>
+<br>
return 0;<br>
}<br>
<br>
-- <br>
2.20.1<br>
<br>
</blockquote></div></div>