| Message ID | 20250701112922.62285-2-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2025-07-01 12:29:21) > 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(). > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 049ebcba..841846b4 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > adjusted = true; > } > > + if (!cfg.colorSpace) { > + cfg.colorSpace = ColorSpace::Sycc; > + status = Adjusted; > + adjusted = true; > + } It looks like virtual only uses NV12 so I think this is fine. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > + if (validateColorSpaces() == Adjusted) { > + status = Adjusted; > + adjusted = true; > + } > + > if (adjusted) > LOG(Virtual, Info) > << "Stream configuration adjusted to " << cfg.toString(); > @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > cfg.pixelFormat = pixelFormat; > cfg.size = data->config_.maxResolutionSize; > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > + cfg.colorSpace = ColorSpace::Sycc; > > config->addConfiguration(cfg); > } > -- > 2.50.0 >
Quoting Kieran Bingham (2025-07-10 08:45:28) > Quoting Umang Jain (2025-07-01 12:29:21) > > 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(). > > > > Signed-off-by: Umang Jain <uajain@igalia.com> From Robert, For patchwork: Tested-by: Robert Mader <robert.mader@collabora.com> > > --- > > src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > index 049ebcba..841846b4 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > adjusted = true; > > } > > > > + if (!cfg.colorSpace) { > > + cfg.colorSpace = ColorSpace::Sycc; > > + status = Adjusted; > > + adjusted = true; > > + } > > It looks like virtual only uses NV12 so I think this is fine. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > + if (validateColorSpaces() == Adjusted) { > > + status = Adjusted; > > + adjusted = true; > > + } > > + > > if (adjusted) > > LOG(Virtual, Info) > > << "Stream configuration adjusted to " << cfg.toString(); > > @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > cfg.pixelFormat = pixelFormat; > > cfg.size = data->config_.maxResolutionSize; > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > > + cfg.colorSpace = ColorSpace::Sycc; > > > > config->addConfiguration(cfg); > > } > > -- > > 2.50.0 > >
On Thu, Jul 10, 2025 at 08:45:28AM +0100, Kieran Bingham wrote: > Quoting Umang Jain (2025-07-01 12:29:21) > > 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(). > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > index 049ebcba..841846b4 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > adjusted = true; > > } > > > > + if (!cfg.colorSpace) { > > + cfg.colorSpace = ColorSpace::Sycc; > > + status = Adjusted; > > + adjusted = true; > > + } > > It looks like virtual only uses NV12 so I think this is fine. Does the pipeline handler produce sYCC though ? This patch looks weird, because it only sets a default colorspace if none is requested, and keeps the application-selected colorspace otherwise. I don't see code in the pipeline handler that takes the colorspace into account to generate frames. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > + if (validateColorSpaces() == Adjusted) { > > + status = Adjusted; > > + adjusted = true; > > + } > > + > > if (adjusted) > > LOG(Virtual, Info) > > << "Stream configuration adjusted to " << cfg.toString(); > > @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > cfg.pixelFormat = pixelFormat; > > cfg.size = data->config_.maxResolutionSize; > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > > + cfg.colorSpace = ColorSpace::Sycc; > > > > config->addConfiguration(cfg); > > }
On Mon, Jul 14, 2025 at 02:13:05PM +0300, Laurent Pinchart wrote: > On Thu, Jul 10, 2025 at 08:45:28AM +0100, Kieran Bingham wrote: > > Quoting Umang Jain (2025-07-01 12:29:21) > > > 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(). > > > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > > --- > > > src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > > index 049ebcba..841846b4 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > > adjusted = true; > > > } > > > > > > + if (!cfg.colorSpace) { > > > + cfg.colorSpace = ColorSpace::Sycc; > > > + status = Adjusted; > > > + adjusted = true; > > > + } > > > > It looks like virtual only uses NV12 so I think this is fine. > > Does the pipeline handler produce sYCC though ? This patch looks weird, > because it only sets a default colorspace if none is requested, and > keeps the application-selected colorspace otherwise. I don't see code in Perhaps you missed to note the validateColorSpace() below - which will correct any application-selected colorspace. > the pipeline handler that takes the colorspace into account to generate > frames. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + > > > + if (validateColorSpaces() == Adjusted) { > > > + status = Adjusted; > > > + adjusted = true; > > > + } > > > + > > > if (adjusted) > > > LOG(Virtual, Info) > > > << "Stream configuration adjusted to " << cfg.toString(); > > > @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > > cfg.pixelFormat = pixelFormat; > > > cfg.size = data->config_.maxResolutionSize; > > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > > > + cfg.colorSpace = ColorSpace::Sycc; > > > > > > config->addConfiguration(cfg); > > > } > > -- > Regards, > > Laurent Pinchart
Hi Umang, On Mon, Jul 14, 2025 at 06:38:25PM +0530, Umang Jain wrote: > On Mon, Jul 14, 2025 at 02:13:05PM +0300, Laurent Pinchart wrote: > > On Thu, Jul 10, 2025 at 08:45:28AM +0100, Kieran Bingham wrote: > > > Quoting Umang Jain (2025-07-01 12:29:21) > > > > 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(). > > > > > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > > > --- > > > > src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > > > index 049ebcba..841846b4 100644 > > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > > @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() > > > > adjusted = true; > > > > } > > > > > > > > + if (!cfg.colorSpace) { > > > > + cfg.colorSpace = ColorSpace::Sycc; > > > > + status = Adjusted; > > > > + adjusted = true; > > > > + } > > > > > > It looks like virtual only uses NV12 so I think this is fine. > > > > Does the pipeline handler produce sYCC though ? This patch looks weird, > > because it only sets a default colorspace if none is requested, and > > keeps the application-selected colorspace otherwise. I don't see code in > > Perhaps you missed to note the validateColorSpace() below - which will > correct any application-selected colorspace. Will it ? As far as I can tell, multiple colorspace values would be accepted by the pipeline handler, and the generate NV12 image will have the exact same YUV values for all of them. > > the pipeline handler that takes the colorspace into account to generate > > frames. > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + > > > > + if (validateColorSpaces() == Adjusted) { > > > > + status = Adjusted; > > > > + adjusted = true; > > > > + } > > > > + > > > > if (adjusted) > > > > LOG(Virtual, Info) > > > > << "Stream configuration adjusted to " << cfg.toString(); > > > > @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, > > > > cfg.pixelFormat = pixelFormat; > > > > cfg.size = data->config_.maxResolutionSize; > > > > cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; > > > > + cfg.colorSpace = ColorSpace::Sycc; > > > > > > > > config->addConfiguration(cfg); > > > > }
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 049ebcba..841846b4 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -180,6 +180,17 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate() adjusted = true; } + if (!cfg.colorSpace) { + cfg.colorSpace = ColorSpace::Sycc; + status = Adjusted; + adjusted = true; + } + + if (validateColorSpaces() == Adjusted) { + status = Adjusted; + adjusted = true; + } + if (adjusted) LOG(Virtual, Info) << "Stream configuration adjusted to " << cfg.toString(); @@ -244,6 +255,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera, cfg.pixelFormat = pixelFormat; cfg.size = data->config_.maxResolutionSize; cfg.bufferCount = VirtualCameraConfiguration::kBufferCount; + cfg.colorSpace = ColorSpace::Sycc; config->addConfiguration(cfg); }
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(). Signed-off-by: Umang Jain <uajain@igalia.com> --- src/libcamera/pipeline/virtual/virtual.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+)