| Message ID | 20250320162624.121616-1-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Thu, Mar 20, 2025 at 05:26:24PM +0100, Milan Zamazal wrote: > StreamConfiguration's should have colorSpace set. This is not the case > in the simple pipeline. Let's set it there, basically mimicking what > rpi and rkisp1 pipelines do (having a common helper may be a > consideration). > > This also fixes a crash in `cam' due to accessing an unset colorSpace. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 6e039bf3..62db96ca 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> > @@ -1196,11 +1197,24 @@ 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; > > + switch (role) { > + case StreamRole::Raw: > + cfg.colorSpace = ColorSpace::Raw; > + break; > + case StreamRole::StillCapture: > + case StreamRole::Viewfinder: > + cfg.colorSpace = ColorSpace::Sycc; > + break; > + case StreamRole::VideoRecording: > + cfg.colorSpace = ColorSpace::Rec709; > + break; > + } > + Doesn't this need to depend on the hardware ? For instance ColorSpace::Rec709 uses limited range, so it only makes sense with YUV formats. When using a YUV sensor you may be able to produce that colour space, but the software ISP doesn't support outputting YUV. You also need colour space handling in SimpleCameraConfiguration::validate(). > config->addConfiguration(cfg); > } >
Quoting Milan Zamazal (2025-03-20 16:26:24) > StreamConfiguration's should have colorSpace set. This is not the case > in the simple pipeline. Let's set it there, basically mimicking what > rpi and rkisp1 pipelines do (having a common helper may be a > consideration). > > This also fixes a crash in `cam' due to accessing an unset colorSpace. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 6e039bf3..62db96ca 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> > @@ -1196,11 +1197,24 @@ 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; > > + switch (role) { > + case StreamRole::Raw: > + cfg.colorSpace = ColorSpace::Raw; > + break; > + case StreamRole::StillCapture: > + case StreamRole::Viewfinder: > + cfg.colorSpace = ColorSpace::Sycc; > + break; > + case StreamRole::VideoRecording: > + cfg.colorSpace = ColorSpace::Rec709; > + break; > + } > + I'm not convinced we can set these like this yet. I don't think it's correct/accurate yet - The debayering isn't performing any CCM or color space conversion that I can see - so I 'suspect' they are always in ColorSpace::Raw... but I'm not sure if that's actually valid for a debayered stream ? But until there's any form of CSC ... I don't think we can report that we're in Rec709 for instance... So maybe ColorSpace::Raw is the correct thing to report for all cases now. And crucially, it needs to be set in validate() so that if an application 'requests' a specific colorspace, the validation specifies what will actually be provided. (so in the future if there is any CSC, it would ensure that the correct colorspace is reported back)... -- Kieran > config->addConfiguration(cfg); > } > > -- > 2.49.0 >
On Thu, Mar 20, 2025 at 05:18:39PM +0000, Kieran Bingham wrote: > Quoting Milan Zamazal (2025-03-20 16:26:24) > > StreamConfiguration's should have colorSpace set. This is not the case > > in the simple pipeline. Let's set it there, basically mimicking what > > rpi and rkisp1 pipelines do (having a common helper may be a > > consideration). > > > > This also fixes a crash in `cam' due to accessing an unset colorSpace. > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 6e039bf3..62db96ca 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> > > @@ -1196,11 +1197,24 @@ 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; > > > > + switch (role) { > > + case StreamRole::Raw: > > + cfg.colorSpace = ColorSpace::Raw; > > + break; > > + case StreamRole::StillCapture: > > + case StreamRole::Viewfinder: > > + cfg.colorSpace = ColorSpace::Sycc; > > + break; > > + case StreamRole::VideoRecording: > > + cfg.colorSpace = ColorSpace::Rec709; > > + break; > > + } > > + > > I'm not convinced we can set these like this yet. I don't think it's > correct/accurate yet - The debayering isn't performing any CCM or color > space conversion that I can see - so I 'suspect' they are always in > ColorSpace::Raw... but I'm not sure if that's actually valid for a > debayered stream ? > > But until there's any form of CSC ... I don't think we can report that > we're in Rec709 for instance... The soft ISP outputs RGB only, so ycbcrEncoding must be YcbcrEncoding::None and range must be Range::Full. > So maybe ColorSpace::Raw is the correct thing to report for all cases > now. Don't forget that the simpler pipeline handler also works with RGB/YUV sensors, without the soft ISP. This needs to be taken into account too. > And crucially, it needs to be set in validate() so that if an > application 'requests' a specific colorspace, the validation specifies > what will actually be provided. (so in the future if there is any CSC, > it would ensure that the correct colorspace is reported back)... > > > config->addConfiguration(cfg); > > } > >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 6e039bf3..62db96ca 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> @@ -1196,11 +1197,24 @@ 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; + switch (role) { + case StreamRole::Raw: + cfg.colorSpace = ColorSpace::Raw; + break; + case StreamRole::StillCapture: + case StreamRole::Viewfinder: + cfg.colorSpace = ColorSpace::Sycc; + break; + case StreamRole::VideoRecording: + cfg.colorSpace = ColorSpace::Rec709; + break; + } + config->addConfiguration(cfg); }
StreamConfiguration's should have colorSpace set. This is not the case in the simple pipeline. Let's set it there, basically mimicking what rpi and rkisp1 pipelines do (having a common helper may be a consideration). This also fixes a crash in `cam' due to accessing an unset colorSpace. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)