| Message ID | 20251104153501.34362-2-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Milan Zamazal <mzamazal@redhat.com> writes: > 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. This is the most valuable patch for me from the whole series because it fixes the crash. Is there any chance to get this individual patch or another fix of the problem included in the upcoming release? > We set the colour spaces according to the pixel format. This is not > completely correct because pixel formats and colour spaces are > different, although not completely independent, things. But for the > lack of a better practical option to determine the colour space, we use > this. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 91715b7f8..04446a3ef 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> > @@ -36,6 +37,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/global_configuration.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > @@ -1222,6 +1224,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > status = Adjusted; > } > > + /* > + * Best effort to fix the color space. If the color space is not set, > + * set it according to the pixel format, which may not be correct (pixel > + * formats and color spaces are different things, although somewhat > + * related) but we don't have a better option at the moment. Then in any > + * case, perform the standard pixel format based color space adjustment. > + */ > + 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; > + } > + LOG(SimplePipeline, Debug) > + << "Unspecified color space set to " > + << cfg.colorSpace.value().toString(); > + status = Adjusted; > + } > + if (cfg.colorSpace->adjust(pixelFormat)) { > + LOG(SimplePipeline, Debug) > + << "Color space adjusted to " > + << cfg.colorSpace.value().toString(); > + status = Adjusted; > + } > + > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > Size adjustedSize = pipeConfig_->captureSize; > /*
Quoting Milan Zamazal (2025-11-21 11:01:15) > Milan Zamazal <mzamazal@redhat.com> writes: > > > 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. > > This is the most valuable patch for me from the whole series because it > fixes the crash. Is there any chance to get this individual patch or > another fix of the problem included in the upcoming release? If this fixes a crash, is there a related bug report to add with: Closes: <url> of a Fixes: <sha1> ? Otherwise I will not note this as a bug fix in the release notes. > > > We set the colour spaces according to the pixel format. This is not > > completely correct because pixel formats and colour spaces are > > different, although not completely independent, things. But for the > > lack of a better practical option to determine the colour space, we use > > this. > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 91715b7f8..04446a3ef 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> > > @@ -36,6 +37,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/global_configuration.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/pipeline_handler.h" > > @@ -1222,6 +1224,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > status = Adjusted; > > } > > > > + /* > > + * Best effort to fix the color space. If the color space is not set, > > + * set it according to the pixel format, which may not be correct (pixel > > + * formats and color spaces are different things, although somewhat > > + * related) but we don't have a better option at the moment. Then in any > > + * case, perform the standard pixel format based color space adjustment. > > + */ > > + 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; > > + } > > + LOG(SimplePipeline, Debug) > > + << "Unspecified color space set to " > > + << cfg.colorSpace.value().toString(); > > + status = Adjusted; I 'think' we don't have to set status = Adjusted here... because it wasn't 'adjusted' from what the application requested (they didn't request anything) so we're just populating it to report. So I think this scope should also then call cfg.colorSpace->adjust(pixelFormat) to make sure all of those checks and updates are performed, and updated based on the pixelformat also without affecting the Adjusted status. } > > + } > > + if (cfg.colorSpace->adjust(pixelFormat)) { > > + LOG(SimplePipeline, Debug) > > + << "Color space adjusted to " > > + << cfg.colorSpace.value().toString(); > > + status = Adjusted; > > + } > > + > > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > > Size adjustedSize = pipeConfig_->captureSize; > > /* >
Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-11-21 11:01:15) >> Milan Zamazal <mzamazal@redhat.com> writes: >> > >> > 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. >> >> This is the most valuable patch for me from the whole series because it >> fixes the crash. Is there any chance to get this individual patch or >> another fix of the problem included in the upcoming release? > > If this fixes a crash, is there a related bug report to add with: I don't think so, is it worth to create one? > Closes: <url> > > of a > Fixes: <sha1> > > ? Otherwise I will not note this as a bug fix in the release notes. > >> >> > We set the colour spaces according to the pixel format. This is not >> > completely correct because pixel formats and colour spaces are >> > different, although not completely independent, things. But for the >> > lack of a better practical option to determine the colour space, we use >> > this. >> > >> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> > --- >> > src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++++++++ >> > 1 file changed, 33 insertions(+) >> > >> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> > index 91715b7f8..04446a3ef 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> >> > @@ -36,6 +37,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/global_configuration.h" >> > #include "libcamera/internal/media_device.h" >> > #include "libcamera/internal/pipeline_handler.h" >> > @@ -1222,6 +1224,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> > status = Adjusted; >> > } >> > >> > + /* >> > + * Best effort to fix the color space. If the color space is not set, >> > + * set it according to the pixel format, which may not be correct (pixel >> > + * formats and color spaces are different things, although somewhat >> > + * related) but we don't have a better option at the moment. Then in any >> > + * case, perform the standard pixel format based color space adjustment. >> > + */ >> > + 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; >> > + } >> > + LOG(SimplePipeline, Debug) >> > + << "Unspecified color space set to " >> > + << cfg.colorSpace.value().toString(); >> > + status = Adjusted; > > I 'think' we don't have to set status = Adjusted here... because it > wasn't 'adjusted' from what the application requested (they didn't > request anything) so we're just populating it to report. > > So I think this scope should also then call cfg.colorSpace->adjust(pixelFormat) > to make sure all of those checks and updates are performed, and updated > based on the pixelformat also without affecting the Adjusted status. OK. Should I pick the updated patch and post it separately? > > > } > >> > + } >> > + if (cfg.colorSpace->adjust(pixelFormat)) { >> > + LOG(SimplePipeline, Debug) >> > + << "Color space adjusted to " >> > + << cfg.colorSpace.value().toString(); >> > + status = Adjusted; >> > + } >> > + >> > if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> > Size adjustedSize = pipeConfig_->captureSize; >> > /* >>
Quoting Milan Zamazal (2025-11-21 13:11:16) > Hi Kieran, > > thank you for review. > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2025-11-21 11:01:15) > >> Milan Zamazal <mzamazal@redhat.com> writes: > >> > > > >> > 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. > >> > >> This is the most valuable patch for me from the whole series because it > >> fixes the crash. Is there any chance to get this individual patch or > >> another fix of the problem included in the upcoming release? > > > > If this fixes a crash, is there a related bug report to add with: > > I don't think so, is it worth to create one? I just need something that will highlight this is a bug fix for the logs and release notes. If you can't find a Fixes: <sha1> then yes lets open an issue just to close it (but it gives the process something to reference, and a way to discuss the issue if it recurs in the future or such). > > Closes: <url> > > > > of a > > Fixes: <sha1> > > > > ? Otherwise I will not note this as a bug fix in the release notes. > > > >> > >> > We set the colour spaces according to the pixel format. This is not > >> > completely correct because pixel formats and colour spaces are > >> > different, although not completely independent, things. But for the > >> > lack of a better practical option to determine the colour space, we use > >> > this. > >> > > >> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> > --- > >> > src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++++++++ > >> > 1 file changed, 33 insertions(+) > >> > > >> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> > index 91715b7f8..04446a3ef 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> > >> > @@ -36,6 +37,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/global_configuration.h" > >> > #include "libcamera/internal/media_device.h" > >> > #include "libcamera/internal/pipeline_handler.h" > >> > @@ -1222,6 +1224,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> > status = Adjusted; > >> > } > >> > > >> > + /* > >> > + * Best effort to fix the color space. If the color space is not set, > >> > + * set it according to the pixel format, which may not be correct (pixel > >> > + * formats and color spaces are different things, although somewhat > >> > + * related) but we don't have a better option at the moment. Then in any > >> > + * case, perform the standard pixel format based color space adjustment. > >> > + */ > >> > + 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; > >> > + } > >> > + LOG(SimplePipeline, Debug) > >> > + << "Unspecified color space set to " > >> > + << cfg.colorSpace.value().toString(); > >> > + status = Adjusted; > > > > I 'think' we don't have to set status = Adjusted here... because it > > wasn't 'adjusted' from what the application requested (they didn't > > request anything) so we're just populating it to report. > > > > So I think this scope should also then call cfg.colorSpace->adjust(pixelFormat) > > to make sure all of those checks and updates are performed, and updated > > based on the pixelformat also without affecting the Adjusted status. > > OK. > > Should I pick the updated patch and post it separately? Sure, if this one is 'urgent' split it out and we can fast track it. Which means all the more reason to create an issue on the tracker ;-) I like that they automatically close when the patch is merged now - so there's no worry about opening one :D -- Kieran > > > > > > > } > > > >> > + } > >> > + if (cfg.colorSpace->adjust(pixelFormat)) { > >> > + LOG(SimplePipeline, Debug) > >> > + << "Color space adjusted to " > >> > + << cfg.colorSpace.value().toString(); > >> > + status = Adjusted; > >> > + } > >> > + > >> > if (!pipeConfig_->outputSizes.contains(cfg.size)) { > >> > Size adjustedSize = pipeConfig_->captureSize; > >> > /* > >> >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 91715b7f8..04446a3ef 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> @@ -36,6 +37,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/global_configuration.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" @@ -1222,6 +1224,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() status = Adjusted; } + /* + * Best effort to fix the color space. If the color space is not set, + * set it according to the pixel format, which may not be correct (pixel + * formats and color spaces are different things, although somewhat + * related) but we don't have a better option at the moment. Then in any + * case, perform the standard pixel format based color space adjustment. + */ + 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; + } + LOG(SimplePipeline, Debug) + << "Unspecified color space set to " + << cfg.colorSpace.value().toString(); + status = Adjusted; + } + if (cfg.colorSpace->adjust(pixelFormat)) { + LOG(SimplePipeline, Debug) + << "Color space adjusted to " + << cfg.colorSpace.value().toString(); + status = Adjusted; + } + if (!pipeConfig_->outputSizes.contains(cfg.size)) { Size adjustedSize = pipeConfig_->captureSize; /*
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. We set the colour spaces according to the pixel format. This is not completely correct because pixel formats and colour spaces are different, although not completely independent, things. But for the lack of a better practical option to determine the colour space, we use this. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++++++++ 1 file changed, 33 insertions(+)