| Message ID | 20260731133327.126689-1-robert.mader@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert On Fri, Jul 31, 2026 at 03:33:27PM +0200, Robert Mader wrote: > ColorSpace::Smpte170m notably has YcbcrEncoding::Rec601, which is what > libyuv::ARGBToNV12() - used in TestPatternGenerator - uses internally. > Newer/upcoming versions of libyuv will have ARGBToNV12Matrix(), allowing > us to explicitly request the corresponding matrix for > ColorSpace::Rec709, however for now let's honor to the implicit value in > order to allow color space aware clients to render as correctly as > possible. Am I correct that Rec709 is preferred, but to actually use it we'll have to request it to libyuv explicitly (once new version of libyuv will gain ARGBToNV12Matrix()) ? > > Note that I'm not fully sure if the same applies to libyuv::MJPGToNV12() in > ImageFrameGenerator, however I *think* so. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/virtual/virtual.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 81d2dddab815..9d59c26e2290 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -208,8 +208,8 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } > > - if (cfg.colorSpace != ColorSpace::Rec709) { > - cfg.colorSpace = ColorSpace::Rec709; > + if (cfg.colorSpace != ColorSpace::Smpte170m) { > + cfg.colorSpace = ColorSpace::Smpte170m; > status = Adjusted; > adjusted = true; > } > @@ -283,7 +283,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > cfg.pixelFormat = pixelFormat; > cfg.size = data->config_.maxResolutionSize; > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > - cfg.colorSpace = ColorSpace::Rec709; > + cfg.colorSpace = ColorSpace::Smpte170m; So this basically aligns the configuration to what is actually happenning. If that's the case, I think it's worth doing so Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > config->addConfiguration(cfg); > } > -- > 2.55.0 >
Hi Jacopo, On 03.08.26 11:09, Jacopo Mondi wrote: > Hi Robert > > On Fri, Jul 31, 2026 at 03:33:27PM +0200, Robert Mader wrote: >> ColorSpace::Smpte170m notably has YcbcrEncoding::Rec601, which is what >> libyuv::ARGBToNV12() - used in TestPatternGenerator - uses internally. >> Newer/upcoming versions of libyuv will have ARGBToNV12Matrix(), allowing >> us to explicitly request the corresponding matrix for >> ColorSpace::Rec709, however for now let's honor to the implicit value in >> order to allow color space aware clients to render as correctly as >> possible. > Am I correct that Rec709 is preferred, but to actually use it we'll > have to request it to libyuv explicitly (once new version of libyuv > will gain ARGBToNV12Matrix()) ? Correct. > >> Note that I'm not fully sure if the same applies to libyuv::MJPGToNV12() in >> ImageFrameGenerator, however I *think* so. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/virtual/virtual.cpp | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp >> index 81d2dddab815..9d59c26e2290 100644 >> --- a/src/libcamera/pipeline/virtual/virtual.cpp >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp >> @@ -208,8 +208,8 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() >> adjusted = true; >> } >> >> - if (cfg.colorSpace != ColorSpace::Rec709) { >> - cfg.colorSpace = ColorSpace::Rec709; >> + if (cfg.colorSpace != ColorSpace::Smpte170m) { >> + cfg.colorSpace = ColorSpace::Smpte170m; >> status = Adjusted; >> adjusted = true; >> } >> @@ -283,7 +283,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, >> cfg.pixelFormat = pixelFormat; >> cfg.size = data->config_.maxResolutionSize; >> cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; >> - cfg.colorSpace = ColorSpace::Rec709; >> + cfg.colorSpace = ColorSpace::Smpte170m; > So this basically aligns the configuration to what is actually > happenning. If that's the case, I think it's worth doing so > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j Thanks! > >> config->addConfiguration(cfg); >> } >> -- >> 2.55.0 >>
Hi 2026. 07. 31. 15:33 keltezéssel, Robert Mader írta: > ColorSpace::Smpte170m notably has YcbcrEncoding::Rec601, which is what > libyuv::ARGBToNV12() - used in TestPatternGenerator - uses internally. > Newer/upcoming versions of libyuv will have ARGBToNV12Matrix(), allowing libyuv does not really have releases as far as I'm aware, so manual function checking in meson is probably needed. Nonetheless it could be interesting to use the color space based on the application preference. > us to explicitly request the corresponding matrix for > ColorSpace::Rec709, however for now let's honor to the implicit value in > order to allow color space aware clients to render as correctly as > possible. > > Note that I'm not fully sure if the same applies to libyuv::MJPGToNV12() in > ImageFrameGenerator, however I *think* so. Looking at e.g. `libyuv::MJPGToARGB()`, it seems to use `I422ToARGB()`, which uses `kYuvI601Constants`, which seems to be what we want. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- According to `V4L2Device::toColorSpace()`, `ColorSpace::Smpte170m` corresponds to `V4L2_COLORSPACE_SMPTE170M`, which is described at https://docs.kernel.org/userspace-api/media/v4l/colorspaces-details.html#colorspace-smpte-170m-v4l2-colorspace-smpte170m and that says The transfer function defined for SMPTE 170M is the same as the one defined in Rec. 709. [...] This conversion to Y’CbCr is identical to the one defined in the ITU BT.601 standard [...] and `libyuv::ARGBToNV12()` uses `kArgbI601Constants`, which is documented as // BT.601 limited range RGB to YUV coefficients in https://chromium.googlesource.com/libyuv/libyuv/+/7138e0a2a7eab0ea7e200e49f047b80e6d7c474e/source/row_common.cc#1499 So based on my understanding, this looks like a better fit. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > src/libcamera/pipeline/virtual/virtual.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 81d2dddab815..9d59c26e2290 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -208,8 +208,8 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } > > - if (cfg.colorSpace != ColorSpace::Rec709) { > - cfg.colorSpace = ColorSpace::Rec709; > + if (cfg.colorSpace != ColorSpace::Smpte170m) { > + cfg.colorSpace = ColorSpace::Smpte170m; > status = Adjusted; > adjusted = true; > } > @@ -283,7 +283,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > cfg.pixelFormat = pixelFormat; > cfg.size = data->config_.maxResolutionSize; > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > - cfg.colorSpace = ColorSpace::Rec709; > + cfg.colorSpace = ColorSpace::Smpte170m; > > config->addConfiguration(cfg); > }
On 04.08.26 13:17, Barnabás Pőcze wrote: > Hi > > 2026. 07. 31. 15:33 keltezéssel, Robert Mader írta: >> ColorSpace::Smpte170m notably has YcbcrEncoding::Rec601, which is what >> libyuv::ARGBToNV12() - used in TestPatternGenerator - uses internally. >> Newer/upcoming versions of libyuv will have ARGBToNV12Matrix(), allowing > > libyuv does not really have releases as far as I'm aware, so manual > function > checking in meson is probably needed. Nonetheless it could be > interesting to > use the color space based on the application preference. > > >> us to explicitly request the corresponding matrix for >> ColorSpace::Rec709, however for now let's honor to the implicit value in >> order to allow color space aware clients to render as correctly as >> possible. >> >> Note that I'm not fully sure if the same applies to >> libyuv::MJPGToNV12() in >> ImageFrameGenerator, however I *think* so. > > Looking at e.g. `libyuv::MJPGToARGB()`, it seems to use > `I422ToARGB()`, which uses > `kYuvI601Constants`, which seems to be what we want. > > >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- > > According to `V4L2Device::toColorSpace()`, `ColorSpace::Smpte170m` > corresponds to > `V4L2_COLORSPACE_SMPTE170M`, which is described at > https://docs.kernel.org/userspace-api/media/v4l/colorspaces-details.html#colorspace-smpte-170m-v4l2-colorspace-smpte170m > and that says > > The transfer function defined for SMPTE 170M is the same as the one > defined in Rec. 709. > [...] > This conversion to Y’CbCr is identical to the one defined in the ITU > BT.601 standard [...] > > and `libyuv::ARGBToNV12()` uses `kArgbI601Constants`, which is > documented as > > // BT.601 limited range RGB to YUV coefficients > > in > https://chromium.googlesource.com/libyuv/libyuv/+/7138e0a2a7eab0ea7e200e49f047b80e6d7c474e/source/row_common.cc#1499 > > So based on my understanding, this looks like a better fit. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Thanks! FTR.: I also opened https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/12214 in order to ensure Gstreamer prints a named colorimetry when running `gst-device-monitor-1.0 Video/Source` or printing negotiated caps in combination with libcamerasrc or pipewiresrc. Without this patch it correctly printed "bt709", while now it prints `2:4:5:4` instead of "bt601". > >> src/libcamera/pipeline/virtual/virtual.cpp | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp >> b/src/libcamera/pipeline/virtual/virtual.cpp >> index 81d2dddab815..9d59c26e2290 100644 >> --- a/src/libcamera/pipeline/virtual/virtual.cpp >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp >> @@ -208,8 +208,8 @@ CameraConfiguration::Status >> VirtualCameraConfiguration::validate() >> adjusted = true; >> } >> - if (cfg.colorSpace != ColorSpace::Rec709) { >> - cfg.colorSpace = ColorSpace::Rec709; >> + if (cfg.colorSpace != ColorSpace::Smpte170m) { >> + cfg.colorSpace = ColorSpace::Smpte170m; >> status = Adjusted; >> adjusted = true; >> } >> @@ -283,7 +283,7 @@ >> PipelineHandlerVirtual::generateConfiguration(Camera *camera, >> cfg.pixelFormat = pixelFormat; >> cfg.size = data->config_.maxResolutionSize; >> cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; >> - cfg.colorSpace = ColorSpace::Rec709; >> + cfg.colorSpace = ColorSpace::Smpte170m; >> config->addConfiguration(cfg); >> } >
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 81d2dddab815..9d59c26e2290 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -208,8 +208,8 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() adjusted = true; } - if (cfg.colorSpace != ColorSpace::Rec709) { - cfg.colorSpace = ColorSpace::Rec709; + if (cfg.colorSpace != ColorSpace::Smpte170m) { + cfg.colorSpace = ColorSpace::Smpte170m; status = Adjusted; adjusted = true; } @@ -283,7 +283,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, cfg.pixelFormat = pixelFormat; cfg.size = data->config_.maxResolutionSize; cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; - cfg.colorSpace = ColorSpace::Rec709; + cfg.colorSpace = ColorSpace::Smpte170m; config->addConfiguration(cfg); }
ColorSpace::Smpte170m notably has YcbcrEncoding::Rec601, which is what libyuv::ARGBToNV12() - used in TestPatternGenerator - uses internally. Newer/upcoming versions of libyuv will have ARGBToNV12Matrix(), allowing us to explicitly request the corresponding matrix for ColorSpace::Rec709, however for now let's honor to the implicit value in order to allow color space aware clients to render as correctly as possible. Note that I'm not fully sure if the same applies to libyuv::MJPGToNV12() in ImageFrameGenerator, however I *think* so. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/libcamera/pipeline/virtual/virtual.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)