| Message ID | 20250723180815.82450-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Wed, Jul 23, 2025 at 08:08:06PM +0200, Milan Zamazal wrote: > StreamConfiguration's should have colorSpace set. This is not the case > in the simple pipeline. Let's set it there. This also fixes a crash in > `cam' due to accessing an unset colorSpace. > > The colour space is assigned as follows: > > - If raw role is requested, then the colour space must be raw, > regardless of the stream format. > - Otherwise, if software ISP is used, the default colour space is > set to Srgb (YcbcrEncoding::None, Range::Full). > - Otherwise, if a converter is used, the default colour space is left > unset. > - Then in configuration validation, if the pixel format is changed, the > colour space is set according to the new pixel format. > - If the colour space is still unset, it is set according to the > resulting pixel format. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index efb07051b..d45480fe7 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -25,6 +25,7 @@ > #include <libcamera/base/log.h> > > #include <libcamera/camera.h> > +#include <libcamera/color_space.h> > #include <libcamera/control_ids.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -35,6 +36,7 @@ > #include "libcamera/internal/converter.h" > #include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/software_isp/software_isp.h" > @@ -1206,6 +1208,28 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > if (cfg.pixelFormat != pixelFormat) { > LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > cfg.pixelFormat = pixelFormat; > + /* > + * Do not touch the colour space for raw requested roles. > + * Even if the pixel format is non-raw (whatever it means), we > + * shouldn't try to interpret the colour space of raw data. > + */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + cfg.colorSpace->adjust(pixelFormat); I'm trying to understand the rationale here. In validate() there's no such thing as a raw role anymore, as roles are used in generateConfiguration() only. Even if the configuration was generated with a raw role, applications can change the pixel format and get a processed stream. Could you please explain what you're trying to do ? > + status = Adjusted; > + } Please add a blank line here. > + if (!cfg.colorSpace) { > + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > + switch (info.colourEncoding) { > + case PixelFormatInfo::ColourEncodingRGB: > + cfg.colorSpace = ColorSpace::Srgb; > + break; > + case libcamera::PixelFormatInfo::ColourEncodingYUV: > + cfg.colorSpace = ColorSpace::Sycc; > + break; > + default: > + cfg.colorSpace = ColorSpace::Raw; > + } > + cfg.colorSpace->adjust(pixelFormat); > status = Adjusted; > } > > @@ -1314,11 +1338,23 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo > * > * \todo Implement a better way to pick the default format > */ > - for ([[maybe_unused]] StreamRole role : roles) { > + for (StreamRole role : roles) { > StreamConfiguration cfg{ StreamFormats{ formats } }; > cfg.pixelFormat = formats.begin()->first; > cfg.size = formats.begin()->second[0].max; > > + if (role == StreamRole::Raw) { > + /* Enforce raw colour space for raw roles. */ > + cfg.colorSpace = ColorSpace::Raw; > + } else if (data->swIsp_) { > + /* > + * This is what software ISP currently produces. It may be arguable > + * whether this applies also when CCM is not used but even then it's > + * still likely to be a reasonable setting. > + */ > + cfg.colorSpace = ColorSpace::Srgb; > + } > + The stream will still have no colorspace set for processed formats. The logic in validate() that picks a default colorspace based on the pixel format seems right for here too, you could move it to a separate function and use it in both places. > config->addConfiguration(cfg); > } >
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Wed, Jul 23, 2025 at 08:08:06PM +0200, Milan Zamazal wrote: >> StreamConfiguration's should have colorSpace set. This is not the case >> in the simple pipeline. Let's set it there. This also fixes a crash in >> `cam' due to accessing an unset colorSpace. >> >> The colour space is assigned as follows: >> >> - If raw role is requested, then the colour space must be raw, >> regardless of the stream format. >> - Otherwise, if software ISP is used, the default colour space is >> set to Srgb (YcbcrEncoding::None, Range::Full). >> - Otherwise, if a converter is used, the default colour space is left >> unset. >> - Then in configuration validation, if the pixel format is changed, the >> colour space is set according to the new pixel format. >> - If the colour space is still unset, it is set according to the >> resulting pixel format. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++++++++++++- >> 1 file changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index efb07051b..d45480fe7 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -25,6 +25,7 @@ >> #include <libcamera/base/log.h> >> >> #include <libcamera/camera.h> >> +#include <libcamera/color_space.h> >> #include <libcamera/control_ids.h> >> #include <libcamera/request.h> >> #include <libcamera/stream.h> >> @@ -35,6 +36,7 @@ >> #include "libcamera/internal/converter.h" >> #include "libcamera/internal/delayed_controls.h" >> #include "libcamera/internal/device_enumerator.h" >> +#include "libcamera/internal/formats.h" >> #include "libcamera/internal/media_device.h" >> #include "libcamera/internal/pipeline_handler.h" >> #include "libcamera/internal/software_isp/software_isp.h" >> @@ -1206,6 +1208,28 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> if (cfg.pixelFormat != pixelFormat) { >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> cfg.pixelFormat = pixelFormat; >> + /* >> + * Do not touch the colour space for raw requested roles. >> + * Even if the pixel format is non-raw (whatever it means), we >> + * shouldn't try to interpret the colour space of raw data. >> + */ >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + cfg.colorSpace->adjust(pixelFormat); > > I'm trying to understand the rationale here. In validate() there's no > such thing as a raw role anymore, as roles are used in > generateConfiguration() only. The first sentence in the comment should be changed to: "Do not touch raw colour spaces." > Even if the configuration was generated with a raw role, applications > can change the pixel format and get a processed stream. If an application changes the pixel format, shouldn't it also change the colour space to a non-raw one or unspecified? > Could you please explain what you're trying to do ? The rationale is that if a raw colour space is explicitly requested, there is probably a reason and we shouldn't try to change that. I don't say it's a completely correct argument but it looks to me like a safer choice in case we don't have more information. If you think otherwise, I can drop the condition. >> + status = Adjusted; >> + } > > Please add a blank line here. OK. >> + if (!cfg.colorSpace) { >> + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); >> + switch (info.colourEncoding) { >> + case PixelFormatInfo::ColourEncodingRGB: >> + cfg.colorSpace = ColorSpace::Srgb; >> + break; >> + case libcamera::PixelFormatInfo::ColourEncodingYUV: >> + cfg.colorSpace = ColorSpace::Sycc; >> + break; >> + default: >> + cfg.colorSpace = ColorSpace::Raw; >> + } >> + cfg.colorSpace->adjust(pixelFormat); >> status = Adjusted; >> } >> >> @@ -1314,11 +1338,23 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo >> * >> * \todo Implement a better way to pick the default format >> */ >> - for ([[maybe_unused]] StreamRole role : roles) { >> + for (StreamRole role : roles) { >> StreamConfiguration cfg{ StreamFormats{ formats } }; >> cfg.pixelFormat = formats.begin()->first; >> cfg.size = formats.begin()->second[0].max; >> >> + if (role == StreamRole::Raw) { >> + /* Enforce raw colour space for raw roles. */ >> + cfg.colorSpace = ColorSpace::Raw; >> + } else if (data->swIsp_) { >> + /* >> + * This is what software ISP currently produces. It may be arguable >> + * whether this applies also when CCM is not used but even then it's >> + * still likely to be a reasonable setting. >> + */ >> + cfg.colorSpace = ColorSpace::Srgb; >> + } >> + > > The stream will still have no colorspace set for processed formats. The > logic in validate() that picks a default colorspace based on the pixel > format seems right for here too, you could move it to a separate > function and use it in both places. Do you mean if (role == StreamRole::Raw) { /* Enforce raw colour space for raw roles. */ cfg.colorSpace = ColorSpace::Raw; } else { /* Select according to the pixel format. */ ... } or also for raw? Does "raw" mean, in the context of the simple pipeline, not interpreted in any way (including not trying to set the colour space) or just unprocessed? >> config->addConfiguration(cfg); >> } >>
Hi Milan, On Mon, Jul 28, 2025 at 11:23:58AM +0200, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Wed, Jul 23, 2025 at 08:08:06PM +0200, Milan Zamazal wrote: > >> StreamConfiguration's should have colorSpace set. This is not the case > >> in the simple pipeline. Let's set it there. This also fixes a crash in > >> `cam' due to accessing an unset colorSpace. > >> > >> The colour space is assigned as follows: > >> > >> - If raw role is requested, then the colour space must be raw, > >> regardless of the stream format. > >> - Otherwise, if software ISP is used, the default colour space is > >> set to Srgb (YcbcrEncoding::None, Range::Full). > >> - Otherwise, if a converter is used, the default colour space is left > >> unset. > >> - Then in configuration validation, if the pixel format is changed, the > >> colour space is set according to the new pixel format. > >> - If the colour space is still unset, it is set according to the > >> resulting pixel format. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++++++++++++- > >> 1 file changed, 37 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index efb07051b..d45480fe7 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -25,6 +25,7 @@ > >> #include <libcamera/base/log.h> > >> > >> #include <libcamera/camera.h> > >> +#include <libcamera/color_space.h> > >> #include <libcamera/control_ids.h> > >> #include <libcamera/request.h> > >> #include <libcamera/stream.h> > >> @@ -35,6 +36,7 @@ > >> #include "libcamera/internal/converter.h" > >> #include "libcamera/internal/delayed_controls.h" > >> #include "libcamera/internal/device_enumerator.h" > >> +#include "libcamera/internal/formats.h" > >> #include "libcamera/internal/media_device.h" > >> #include "libcamera/internal/pipeline_handler.h" > >> #include "libcamera/internal/software_isp/software_isp.h" > >> @@ -1206,6 +1208,28 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> if (cfg.pixelFormat != pixelFormat) { > >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > >> cfg.pixelFormat = pixelFormat; > >> + /* > >> + * Do not touch the colour space for raw requested roles. > >> + * Even if the pixel format is non-raw (whatever it means), we > >> + * shouldn't try to interpret the colour space of raw data. > >> + */ > >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > >> + cfg.colorSpace->adjust(pixelFormat); > > > > I'm trying to understand the rationale here. In validate() there's no > > such thing as a raw role anymore, as roles are used in > > generateConfiguration() only. > > The first sentence in the comment should be changed to: "Do not touch > raw colour spaces." > > > Even if the configuration was generated with a raw role, applications > > can change the pixel format and get a processed stream. > > If an application changes the pixel format, shouldn't it also change the > colour space to a non-raw one or unspecified? Probably, as YUV + ColorSpace::Raw doesn't make too much sense, but we need to be prepared for applications making mistakes. validate() should return a valid, sensible configuration. > > Could you please explain what you're trying to do ? > > The rationale is that if a raw colour space is explicitly requested, > there is probably a reason and we shouldn't try to change that. I don't > say it's a completely correct argument but it looks to me like a safer > choice in case we don't have more information. If you think otherwise, > I can drop the condition. I don't think we shouldn't consider ColorSpace::Raw is having any special meaning when set by an application. Or if we did, we would need to define what special meaning it has, and I don't know what that would be :-) > >> + status = Adjusted; > >> + } > > > > Please add a blank line here. > > OK. > > >> + if (!cfg.colorSpace) { > >> + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > >> + switch (info.colourEncoding) { > >> + case PixelFormatInfo::ColourEncodingRGB: > >> + cfg.colorSpace = ColorSpace::Srgb; > >> + break; > >> + case libcamera::PixelFormatInfo::ColourEncodingYUV: > >> + cfg.colorSpace = ColorSpace::Sycc; > >> + break; > >> + default: > >> + cfg.colorSpace = ColorSpace::Raw; > >> + } > >> + cfg.colorSpace->adjust(pixelFormat); > >> status = Adjusted; > >> } > >> > >> @@ -1314,11 +1338,23 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo > >> * > >> * \todo Implement a better way to pick the default format > >> */ > >> - for ([[maybe_unused]] StreamRole role : roles) { > >> + for (StreamRole role : roles) { > >> StreamConfiguration cfg{ StreamFormats{ formats } }; > >> cfg.pixelFormat = formats.begin()->first; > >> cfg.size = formats.begin()->second[0].max; > >> > >> + if (role == StreamRole::Raw) { > >> + /* Enforce raw colour space for raw roles. */ > >> + cfg.colorSpace = ColorSpace::Raw; > >> + } else if (data->swIsp_) { > >> + /* > >> + * This is what software ISP currently produces. It may be arguable > >> + * whether this applies also when CCM is not used but even then it's > >> + * still likely to be a reasonable setting. > >> + */ > >> + cfg.colorSpace = ColorSpace::Srgb; > >> + } > >> + > > > > The stream will still have no colorspace set for processed formats. The > > logic in validate() that picks a default colorspace based on the pixel > > format seems right for here too, you could move it to a separate > > function and use it in both places. > > Do you mean > > if (role == StreamRole::Raw) { > /* Enforce raw colour space for raw roles. */ > cfg.colorSpace = ColorSpace::Raw; > } else { > /* Select according to the pixel format. */ > ... > } > > or also for raw? I would first set the pixel format based on the role (and the supported pixel formats of course), and then the colorspace based on the pixel format. > Does "raw" mean, in the context of the simple > pipeline, not interpreted in any way (including not trying to set the > colour space) or just unprocessed? I think StreamRole::Raw should be considered as real raw images. Even if the camera pipeline in the SoC doesn't process the images, a YUV camera sensor doesn't produce raw images. > >> config->addConfiguration(cfg); > >> } > >>
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > On Mon, Jul 28, 2025 at 11:23:58AM +0200, Milan Zamazal wrote: >> Laurent Pinchart writes: >> > On Wed, Jul 23, 2025 at 08:08:06PM +0200, Milan Zamazal wrote: >> >> StreamConfiguration's should have colorSpace set. This is not the case >> >> in the simple pipeline. Let's set it there. This also fixes a crash in >> >> `cam' due to accessing an unset colorSpace. >> >> >> >> The colour space is assigned as follows: >> >> >> >> - If raw role is requested, then the colour space must be raw, >> >> regardless of the stream format. >> >> - Otherwise, if software ISP is used, the default colour space is >> >> set to Srgb (YcbcrEncoding::None, Range::Full). >> >> - Otherwise, if a converter is used, the default colour space is left >> >> unset. >> >> - Then in configuration validation, if the pixel format is changed, the >> >> colour space is set according to the new pixel format. >> >> - If the colour space is still unset, it is set according to the >> >> resulting pixel format. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++++++++++++- >> >> 1 file changed, 37 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> >> index efb07051b..d45480fe7 100644 >> >> --- a/src/libcamera/pipeline/simple/simple.cpp >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> >> @@ -25,6 +25,7 @@ >> >> #include <libcamera/base/log.h> >> >> >> >> #include <libcamera/camera.h> >> >> +#include <libcamera/color_space.h> >> >> #include <libcamera/control_ids.h> >> >> #include <libcamera/request.h> >> >> #include <libcamera/stream.h> >> >> @@ -35,6 +36,7 @@ >> >> #include "libcamera/internal/converter.h" >> >> #include "libcamera/internal/delayed_controls.h" >> >> #include "libcamera/internal/device_enumerator.h" >> >> +#include "libcamera/internal/formats.h" >> >> #include "libcamera/internal/media_device.h" >> >> #include "libcamera/internal/pipeline_handler.h" >> >> #include "libcamera/internal/software_isp/software_isp.h" >> >> @@ -1206,6 +1208,28 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> if (cfg.pixelFormat != pixelFormat) { >> >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> >> cfg.pixelFormat = pixelFormat; >> >> + /* >> >> + * Do not touch the colour space for raw requested roles. >> >> + * Even if the pixel format is non-raw (whatever it means), we >> >> + * shouldn't try to interpret the colour space of raw data. >> >> + */ >> >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> >> + cfg.colorSpace->adjust(pixelFormat); >> > >> > I'm trying to understand the rationale here. In validate() there's no >> > such thing as a raw role anymore, as roles are used in >> > generateConfiguration() only. >> >> The first sentence in the comment should be changed to: "Do not touch >> raw colour spaces." >> >> > Even if the configuration was generated with a raw role, applications >> > can change the pixel format and get a processed stream. >> >> If an application changes the pixel format, shouldn't it also change the >> colour space to a non-raw one or unspecified? > > Probably, as YUV + ColorSpace::Raw doesn't make too much sense, but we > need to be prepared for applications making mistakes. validate() should > return a valid, sensible configuration. Not completely sure about this particular combination, but I'd say that raw colour space is a more correct colour space assignment than e.g. sRGB for an RGB pixel format output without gamma & CCM applied. I can imagine that an application setting the pixel format to RGB and the colour space to raw is ready to get a debayered RGB without any corrections after debayering applied in the image processing pipeline. I admit I may be well beyond practical considerations here, expecting too much sophistication from both libcamera and applications, still much simplifying and ignoring the fact it's difficult to be 100% correct here in any case. But if we are going to derive the colour space from the pixel format unconditionally, despite those are actually two different while not completely independent things, we should have some answer to "why?". Taking this opportunity to clarify the things. >> > Could you please explain what you're trying to do ? >> >> The rationale is that if a raw colour space is explicitly requested, >> there is probably a reason and we shouldn't try to change that. I don't >> say it's a completely correct argument but it looks to me like a safer >> choice in case we don't have more information. If you think otherwise, >> I can drop the condition. > > I don't think we shouldn't consider ColorSpace::Raw is having any > special meaning when set by an application. Or if we did, we would need > to define what special meaning it has, and I don't know what that would > be :-) This may be one of the answers :-). An obvious objection could be that if we return e.g. sRGB colour space then the pixel values should represent, according to our & camera's best effort, the actual colours as defined by sRGB. Is it true for the current simple pipeline? And if not, does anybody care? >> >> + status = Adjusted; >> >> + } >> > >> > Please add a blank line here. >> >> OK. >> >> >> + if (!cfg.colorSpace) { >> >> + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); >> >> + switch (info.colourEncoding) { >> >> + case PixelFormatInfo::ColourEncodingRGB: >> >> + cfg.colorSpace = ColorSpace::Srgb; >> >> + break; >> >> + case libcamera::PixelFormatInfo::ColourEncodingYUV: >> >> + cfg.colorSpace = ColorSpace::Sycc; >> >> + break; >> >> + default: >> >> + cfg.colorSpace = ColorSpace::Raw; >> >> + } >> >> + cfg.colorSpace->adjust(pixelFormat); >> >> status = Adjusted; >> >> } >> >> >> >> @@ -1314,11 +1338,23 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo >> >> * >> >> * \todo Implement a better way to pick the default format >> >> */ >> >> - for ([[maybe_unused]] StreamRole role : roles) { >> >> + for (StreamRole role : roles) { >> >> StreamConfiguration cfg{ StreamFormats{ formats } }; >> >> cfg.pixelFormat = formats.begin()->first; >> >> cfg.size = formats.begin()->second[0].max; >> >> >> >> + if (role == StreamRole::Raw) { >> >> + /* Enforce raw colour space for raw roles. */ >> >> + cfg.colorSpace = ColorSpace::Raw; >> >> + } else if (data->swIsp_) { >> >> + /* >> >> + * This is what software ISP currently produces. It may be arguable >> >> + * whether this applies also when CCM is not used but even then it's >> >> + * still likely to be a reasonable setting. >> >> + */ >> >> + cfg.colorSpace = ColorSpace::Srgb; >> >> + } >> >> + >> > >> > The stream will still have no colorspace set for processed formats. The >> > logic in validate() that picks a default colorspace based on the pixel >> > format seems right for here too, you could move it to a separate >> > function and use it in both places. >> >> Do you mean >> >> if (role == StreamRole::Raw) { >> /* Enforce raw colour space for raw roles. */ >> cfg.colorSpace = ColorSpace::Raw; >> } else { >> /* Select according to the pixel format. */ >> ... >> } >> >> or also for raw? > > I would first set the pixel format based on the role (and the supported > pixel formats of course), and then the colorspace based on the pixel > format. > >> Does "raw" mean, in the context of the simple >> pipeline, not interpreted in any way (including not trying to set the >> colour space) or just unprocessed? > > I think StreamRole::Raw should be considered as real raw images. Even if > the camera pipeline in the SoC doesn't process the images, a YUV camera > sensor doesn't produce raw images. > >> >> config->addConfiguration(cfg); >> >> } >> >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index efb07051b..d45480fe7 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -25,6 +25,7 @@ #include <libcamera/base/log.h> #include <libcamera/camera.h> +#include <libcamera/color_space.h> #include <libcamera/control_ids.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -35,6 +36,7 @@ #include "libcamera/internal/converter.h" #include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" +#include "libcamera/internal/formats.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/software_isp/software_isp.h" @@ -1206,6 +1208,28 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() if (cfg.pixelFormat != pixelFormat) { LOG(SimplePipeline, Debug) << "Adjusting pixel format"; cfg.pixelFormat = pixelFormat; + /* + * Do not touch the colour space for raw requested roles. + * Even if the pixel format is non-raw (whatever it means), we + * shouldn't try to interpret the colour space of raw data. + */ + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) + cfg.colorSpace->adjust(pixelFormat); + status = Adjusted; + } + if (!cfg.colorSpace) { + const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); + switch (info.colourEncoding) { + case PixelFormatInfo::ColourEncodingRGB: + cfg.colorSpace = ColorSpace::Srgb; + break; + case libcamera::PixelFormatInfo::ColourEncodingYUV: + cfg.colorSpace = ColorSpace::Sycc; + break; + default: + cfg.colorSpace = ColorSpace::Raw; + } + cfg.colorSpace->adjust(pixelFormat); status = Adjusted; } @@ -1314,11 +1338,23 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo * * \todo Implement a better way to pick the default format */ - for ([[maybe_unused]] StreamRole role : roles) { + for (StreamRole role : roles) { StreamConfiguration cfg{ StreamFormats{ formats } }; cfg.pixelFormat = formats.begin()->first; cfg.size = formats.begin()->second[0].max; + if (role == StreamRole::Raw) { + /* Enforce raw colour space for raw roles. */ + cfg.colorSpace = ColorSpace::Raw; + } else if (data->swIsp_) { + /* + * This is what software ISP currently produces. It may be arguable + * whether this applies also when CCM is not used but even then it's + * still likely to be a reasonable setting. + */ + cfg.colorSpace = ColorSpace::Srgb; + } + config->addConfiguration(cfg); }
StreamConfiguration's should have colorSpace set. This is not the case in the simple pipeline. Let's set it there. This also fixes a crash in `cam' due to accessing an unset colorSpace. The colour space is assigned as follows: - If raw role is requested, then the colour space must be raw, regardless of the stream format. - Otherwise, if software ISP is used, the default colour space is set to Srgb (YcbcrEncoding::None, Range::Full). - Otherwise, if a converter is used, the default colour space is left unset. - Then in configuration validation, if the pixel format is changed, the colour space is set according to the new pixel format. - If the colour space is still unset, it is set according to the resulting pixel format. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-)