| Message ID | 20251026125650.117468-2-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 26. 13:56 keltezéssel, Umang Jain írta: > Virtual pipeline handler should provide colorSpace in > generateConfiguration() and validate the colorspace in validate(). > It is mandatory for a pipeline handler to set the colorspace if it > is unset in the stream configuration, during validate(). > > For choosing the colorspace for the generated NV12 frames, following > points have been taken into account: > - The transfer function should be Rec.709 for NV12 > - The YCbCr encoding has been chosen Rec.709 as it is the most common more ? And the "The YCbCr encoding has been chosen Rec.709" part is not clear to me. > than Rec.601/Rec.2020 > - Range should be 'Limited' as with the NV12 pixel format. > > Hence, the closest colorspace match is ColorSpace::Rec709 which is > set for the virtual pipeline handler. > > Signed-off-by: Umang Jain <uajain@igalia.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Tested-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 23eae852..c0247b4d 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -214,6 +214,18 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } > > + if (!cfg.colorSpace || > + cfg.colorSpace != ColorSpace::Rec709) { You can drop the `!cfg.colorSpace` part. An empty optional is only equal to another empty optional. > + cfg.colorSpace = ColorSpace::Rec709; > + status = Adjusted; > + adjusted = true; > + } > + > + if (validateColorSpaces() == Adjusted) { This will always be called with `formats::NV12` and `ColorSpace::Rec709` on every stream, and so it will never make any adjustments, right? I think it's probably fine to leave it here even in that case, should this pipeline handler be extended to support more things; just want to confirm it. Regards, Barnabás Pőcze > + status = Adjusted; > + adjusted = true; > + } > + > if (adjusted) > LOG(Virtual, Info) > << "Stream configuration adjusted to " << cfg.toString(); > @@ -278,6 +290,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > cfg.pixelFormat = pixelFormat; > cfg.size = data->config_.maxResolutionSize; > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > + cfg.colorSpace = ColorSpace::Rec709; > > config->addConfiguration(cfg); > } > -- > 2.51.0 >
Hi, On 10/27/25 11:02, Barnabás Pőcze wrote: > Hi > > 2025. 10. 26. 13:56 keltezéssel, Umang Jain írta: >> Virtual pipeline handler should provide colorSpace in >> generateConfiguration() and validate the colorspace in validate(). >> It is mandatory for a pipeline handler to set the colorspace if it >> is unset in the stream configuration, during validate(). >> >> For choosing the colorspace for the generated NV12 frames, following >> points have been taken into account: >> - The transfer function should be Rec.709 for NV12 >> - The YCbCr encoding has been chosen Rec.709 as it is the most common > more ? > > And the "The YCbCr encoding has been chosen Rec.709" part is not clear > to me. > > >> than Rec.601/Rec.2020 >> - Range should be 'Limited' as with the NV12 pixel format. >> >> Hence, the closest colorspace match is ColorSpace::Rec709 which is >> set for the virtual pipeline handler. >> >> Signed-off-by: Umang Jain <uajain@igalia.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Tested-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp >> b/src/libcamera/pipeline/virtual/virtual.cpp >> index 23eae852..c0247b4d 100644 >> --- a/src/libcamera/pipeline/virtual/virtual.cpp >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp >> @@ -214,6 +214,18 @@ CameraConfiguration::Status >> VirtualCameraConfiguration::validate() >> adjusted = true; >> } >> >> + if (!cfg.colorSpace || >> + cfg.colorSpace != ColorSpace::Rec709) { > > You can drop the `!cfg.colorSpace` part. An empty optional is only > equal to another empty optional. FTR., other pipelines like the rkisp1 only set the adjusted state if cfg.colorSpace was not empty - so that might be worth doing here as well. But the current form is indeed redundant. > >> + cfg.colorSpace = ColorSpace::Rec709; >> + status = Adjusted; >> + adjusted = true; >> + } >> + >> + if (validateColorSpaces() == Adjusted) { > > This will always be called with `formats::NV12` and `ColorSpace::Rec709` > on every stream, and so it will never make any adjustments, right? > > I think it's probably fine to leave it here even in that case, should > this > pipeline handler be extended to support more things; just want to > confirm it. > > > Regards, > Barnabás Pőcze > >> + status = Adjusted; >> + adjusted = true; >> + } >> + >> if (adjusted) >> LOG(Virtual, Info) >> << "Stream configuration adjusted to " << >> cfg.toString(); >> @@ -278,6 +290,7 @@ >> PipelineHandlerVirtual::generateConfiguration(Camera *camera, >> cfg.pixelFormat = pixelFormat; >> cfg.size = data->config_.maxResolutionSize; >> cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; >> + cfg.colorSpace = ColorSpace::Rec709; >> >> config->addConfiguration(cfg); >> } >> -- >> 2.51.0 >> >
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 23eae852..c0247b4d 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -214,6 +214,18 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() adjusted = true; } + if (!cfg.colorSpace || + cfg.colorSpace != ColorSpace::Rec709) { + cfg.colorSpace = ColorSpace::Rec709; + status = Adjusted; + adjusted = true; + } + + if (validateColorSpaces() == Adjusted) { + status = Adjusted; + adjusted = true; + } + if (adjusted) LOG(Virtual, Info) << "Stream configuration adjusted to " << cfg.toString(); @@ -278,6 +290,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, cfg.pixelFormat = pixelFormat; cfg.size = data->config_.maxResolutionSize; cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; + cfg.colorSpace = ColorSpace::Rec709; config->addConfiguration(cfg); }