| Message ID | 20250627113449.23106-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Fri, Jun 27, 2025 at 01:34:36PM +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. > - 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 | 25 +++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index efb07051b..a008b13d1 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,8 +1208,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > if (cfg.pixelFormat != pixelFormat) { > LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > cfg.pixelFormat = pixelFormat; > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + cfg.colorSpace->adjust(pixelFormat); Can this possibly can be avoided with a call to validateColorSpaces() ? See below. > 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; > + } Add: status = Adjusted; > + cfg.colorSpace->adjust(pixelFormat); Instead of doing this here, possibly call validateColorSpaces() outside this `if` block ? > + } validateColorSpaces(); and set status accordingly? > > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > Size adjustedSize = pipeConfig_->captureSize; > @@ -1314,11 +1332,16 @@ 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) > + cfg.colorSpace = ColorSpace::Raw; The benefit of validateColorSpaces(), it will set and ensure Raw Colorspace is set for Raw streams and then this can be omitted here. However, if you prefer keeping it here, that's should be fine as well. > + else if (data->swIsp_) > + cfg.colorSpace = ColorSpace::Srgb; > + > config->addConfiguration(cfg); > } > > -- > 2.50.0 >
Hi Umang, thank you for review. Umang Jain <uajain@igalia.com> writes: > On Fri, Jun 27, 2025 at 01:34:36PM +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. >> - 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 | 25 +++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index efb07051b..a008b13d1 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,8 +1208,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> if (cfg.pixelFormat != pixelFormat) { >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> cfg.pixelFormat = pixelFormat; >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + cfg.colorSpace->adjust(pixelFormat); > > Can this possibly can be avoided with a call to validateColorSpaces() ? I don't think so, see below. > See below. > >> 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; >> + } > > Add: > status = Adjusted; OK. >> + cfg.colorSpace->adjust(pixelFormat); > > Instead of doing this here, possibly call validateColorSpaces() outside > this `if` block ? > >> + } > > validateColorSpaces(); > > and set status accordingly? Shouldn't validateColorSpaces be called outside the whole `for' cycle? >> if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> Size adjustedSize = pipeConfig_->captureSize; >> @@ -1314,11 +1332,16 @@ 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) >> + cfg.colorSpace = ColorSpace::Raw; > > The benefit of validateColorSpaces(), it will set and ensure Raw > Colorspace is set for Raw streams and then this can be omitted here. > However, if you prefer keeping it here, that's should be fine as well. The idea here is that if a raw stream role is requested then the stream must be handled as raw, regardless of its format. Setting colorSpace to Raw here and avoiding adjusting it in validation above (see the first comment) ensures the requirement. I don't say this idea is necessarily meaningful or correct but if it is then validateColorSpaces doesn't have this knowledge and could do something different than required. >> + else if (data->swIsp_) >> + cfg.colorSpace = ColorSpace::Srgb; >> + >> config->addConfiguration(cfg); >> } >> >> -- >> 2.50.0 >>
Hi On Tue, Jul 01, 2025 at 03:40:06PM +0200, Milan Zamazal wrote: > Hi Umang, > > thank you for review. > > Umang Jain <uajain@igalia.com> writes: > > > On Fri, Jun 27, 2025 at 01:34:36PM +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. > >> - 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 | 25 +++++++++++++++++++++++- > >> 1 file changed, 24 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index efb07051b..a008b13d1 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,8 +1208,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> if (cfg.pixelFormat != pixelFormat) { > >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > >> cfg.pixelFormat = pixelFormat; > >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > >> + cfg.colorSpace->adjust(pixelFormat); > > > > Can this possibly can be avoided with a call to validateColorSpaces() ? > > I don't think so, see below. > > > See below. > > > >> 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; > >> + } > > > > Add: > > status = Adjusted; > > OK. > > >> + cfg.colorSpace->adjust(pixelFormat); > > > > Instead of doing this here, possibly call validateColorSpaces() outside > > this `if` block ? > > > >> + } > > > > validateColorSpaces(); > > > > and set status accordingly? > > Shouldn't validateColorSpaces be called outside the whole `for' cycle? Oh yes, I missed the loop. It should be called outside. > > >> if (!pipeConfig_->outputSizes.contains(cfg.size)) { > >> Size adjustedSize = pipeConfig_->captureSize; > >> @@ -1314,11 +1332,16 @@ 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) > >> + cfg.colorSpace = ColorSpace::Raw; > > > > The benefit of validateColorSpaces(), it will set and ensure Raw > > Colorspace is set for Raw streams and then this can be omitted here. > > However, if you prefer keeping it here, that's should be fine as well. > > The idea here is that if a raw stream role is requested then the stream > must be handled as raw, regardless of its format. Setting colorSpace to > Raw here and avoiding adjusting it in validation above (see the first > comment) ensures the requirement. Do you mean this comment? - If raw role is requested, then the colour space must be raw. Given your explanation above that colorspace should must be raw, for role=raw *regardless of its format* - is something I didn't consider or inferred. The comment in the commit message should be expanded: - If raw role is requested, then the colour space must be raw, regardless of the stream format. > > I don't say this idea is necessarily meaningful or correct but if it is > then validateColorSpaces doesn't have this knowledge and could do > something different than required. This idea is certainly not evident from the patch (atleast to me). I would prefer to document it in a comment, possibly justifying the rationale. > > >> + else if (data->swIsp_) > >> + cfg.colorSpace = ColorSpace::Srgb; > >> + > >> config->addConfiguration(cfg); > >> } > >> > >> -- > >> 2.50.0 > >> >
Umang Jain <uajain@igalia.com> writes: > Hi > > On Tue, Jul 01, 2025 at 03:40:06PM +0200, Milan Zamazal wrote: >> Hi Umang, >> >> thank you for review. >> >> Umang Jain <uajain@igalia.com> writes: >> >> > On Fri, Jun 27, 2025 at 01:34:36PM +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. >> >> - 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 | 25 +++++++++++++++++++++++- >> >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> >> index efb07051b..a008b13d1 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,8 +1208,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> if (cfg.pixelFormat != pixelFormat) { >> >> LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> >> cfg.pixelFormat = pixelFormat; >> >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> >> + cfg.colorSpace->adjust(pixelFormat); >> > >> > Can this possibly can be avoided with a call to validateColorSpaces() ? >> >> I don't think so, see below. >> >> > See below. >> > >> >> 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; >> >> + } >> > >> > Add: >> > status = Adjusted; >> >> OK. >> >> >> + cfg.colorSpace->adjust(pixelFormat); >> > >> > Instead of doing this here, possibly call validateColorSpaces() outside >> > this `if` block ? >> > >> >> + } >> > >> > validateColorSpaces(); >> > >> > and set status accordingly? >> >> Shouldn't validateColorSpaces be called outside the whole `for' cycle? > > Oh yes, I missed the loop. It should be called outside. > >> >> >> if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> >> Size adjustedSize = pipeConfig_->captureSize; >> >> @@ -1314,11 +1332,16 @@ 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) >> >> + cfg.colorSpace = ColorSpace::Raw; >> > >> > The benefit of validateColorSpaces(), it will set and ensure Raw >> > Colorspace is set for Raw streams and then this can be omitted here. >> > However, if you prefer keeping it here, that's should be fine as well. >> >> The idea here is that if a raw stream role is requested then the stream >> must be handled as raw, regardless of its format. Setting colorSpace to >> Raw here and avoiding adjusting it in validation above (see the first >> comment) ensures the requirement. > > Do you mean this comment? > - If raw role is requested, then the colour space must be raw. Yes. > Given your explanation above that colorspace should must be raw, for > role=raw *regardless of its format* - is something I didn't consider > or inferred. The comment in the commit message should be expanded: > > - If raw role is requested, then the colour space must be raw, > regardless of the stream format. OK. >> I don't say this idea is necessarily meaningful or correct but if it is >> then validateColorSpaces doesn't have this knowledge and could do >> something different than required. > > This idea is certainly not evident from the patch (atleast to me). > I would prefer to document it in a comment, possibly justifying > the rationale. I'll add a comment. >> >> + else if (data->swIsp_) >> >> + cfg.colorSpace = ColorSpace::Srgb; >> >> + >> >> config->addConfiguration(cfg); >> >> } >> >> >> >> -- >> >> 2.50.0 >> >> >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index efb07051b..a008b13d1 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,8 +1208,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() if (cfg.pixelFormat != pixelFormat) { LOG(SimplePipeline, Debug) << "Adjusting pixel format"; cfg.pixelFormat = pixelFormat; + 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); + } if (!pipeConfig_->outputSizes.contains(cfg.size)) { Size adjustedSize = pipeConfig_->captureSize; @@ -1314,11 +1332,16 @@ 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) + cfg.colorSpace = ColorSpace::Raw; + else if (data->swIsp_) + 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. - 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 | 25 +++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)