| Message ID | 20250310110630.34857-1-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-03-10 11:06:30) > cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash > when it is accessed. If cfg.colorSpace is unset, simply return, the > same way as when YcbcrEncoding is set to None. I think this is something that we should ensure is trapped by lc-compliance in fact. I believe pipeline handlers /must/ always set the correct colorSpace after validate - so it's incorrect for applications to ever hit an undefined color space ... Of course crashing isn't nice either ... but is this occuring in SoftISP/simple pipeline handler ? > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/apps/cam/kms_sink.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > index 672c985a..aa9459cf 100644 > --- a/src/apps/cam/kms_sink.cpp > +++ b/src/apps/cam/kms_sink.cpp > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > colorEncoding_ = std::nullopt; > colorRange_ = std::nullopt; > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > + if (!cfg.colorSpace || > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > return 0; > > /* > -- > 2.48.1 >
Hi Kieran, Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-03-10 11:06:30) >> cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash >> when it is accessed. If cfg.colorSpace is unset, simply return, the >> same way as when YcbcrEncoding is set to None. > > I think this is something that we should ensure is trapped by > lc-compliance in fact. > > I believe pipeline handlers /must/ always set the correct colorSpace > after validate - so it's incorrect for applications to ever hit an > undefined color space ... I'm not sure whether all the pipelines do that; at least `simple' doesn't. I can fix `simple' pipeline but maybe some others have the problem too. > Of course crashing isn't nice either ... Let's have an assertion there then to still expose the problem while crashing in a civilized way? > but is this occuring in SoftISP/simple pipeline handler ? Yes. It started happening to me once I reinstalled my development system to Fedora 41. I can't see any obvious reason why it crashes now and not before (maybe some change in gcc? -- using 14.2.1). >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/cam/kms_sink.cpp | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp >> index 672c985a..aa9459cf 100644 >> --- a/src/apps/cam/kms_sink.cpp >> +++ b/src/apps/cam/kms_sink.cpp >> @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) >> colorEncoding_ = std::nullopt; >> colorRange_ = std::nullopt; >> >> - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >> + if (!cfg.colorSpace || >> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >> return 0; >> >> /* >> -- >> 2.48.1 >>
Quoting Milan Zamazal (2025-03-14 13:30:18) > Hi Kieran, > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2025-03-10 11:06:30) > >> cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash > >> when it is accessed. If cfg.colorSpace is unset, simply return, the > >> same way as when YcbcrEncoding is set to None. > > > > I think this is something that we should ensure is trapped by > > lc-compliance in fact. > > > > I believe pipeline handlers /must/ always set the correct colorSpace > > after validate - so it's incorrect for applications to ever hit an > > undefined color space ... > > I'm not sure whether all the pipelines do that; at least `simple' > doesn't. I can fix `simple' pipeline but maybe some others have the > problem too. That's why I think we need to do better on lc-complicance tests to ensure 'rules' are always validated. > > Of course crashing isn't nice either ... > > Let's have an assertion there then to still expose the problem while > crashing in a civilized way? In kmssink? I'm fine adding an assertion here - but what about the other sinks ? What about the other applciations. That's why 'this' should be validated for /all/ pipeline handlers. There should be a contract somewhere that states a pipeline handler /must/ fill in specific fields. I have an upcoming proposal that could let us add a validation layer to all runtimes too which could be another place to add runtime validations. Hopefully I'll get that out next week as an initial RFC. > > but is this occuring in SoftISP/simple pipeline handler ? > > Yes. It started happening to me once I reinstalled my development > system to Fedora 41. I can't see any obvious reason why it crashes now > and not before (maybe some change in gcc? -- using 14.2.1). > It will be curious if this only appears as a result of changing external tooling though ! > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/apps/cam/kms_sink.cpp | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > >> index 672c985a..aa9459cf 100644 > >> --- a/src/apps/cam/kms_sink.cpp > >> +++ b/src/apps/cam/kms_sink.cpp > >> @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > >> colorEncoding_ = std::nullopt; > >> colorRange_ = std::nullopt; > >> > >> - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > >> + if (!cfg.colorSpace || > >> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > >> return 0; > >> > >> /* > >> -- > >> 2.48.1 > >> >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-03-14 13:30:18) >> Hi Kieran, >> > >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >> > Quoting Milan Zamazal (2025-03-10 11:06:30) >> >> cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash >> >> when it is accessed. If cfg.colorSpace is unset, simply return, the >> >> same way as when YcbcrEncoding is set to None. >> > >> > I think this is something that we should ensure is trapped by >> > lc-compliance in fact. >> > >> > I believe pipeline handlers /must/ always set the correct colorSpace >> > after validate - so it's incorrect for applications to ever hit an >> > undefined color space ... >> >> I'm not sure whether all the pipelines do that; at least `simple' >> doesn't. I can fix `simple' pipeline but maybe some others have the >> problem too. > > That's why I think we need to do better on lc-complicance tests to > ensure 'rules' are always validated. > >> > Of course crashing isn't nice either ... >> >> Let's have an assertion there then to still expose the problem while >> crashing in a civilized way? > > In kmssink? I'm fine adding an assertion here - but what about the other > sinks ? What about the other applciations. That's why 'this' should be > validated for /all/ pipeline handlers. Well, I posted a patch that sets colorSpace in the simple pipeline. It fixes the crash for me. As for the assertion, I realized it wouldn't be a good idea to add it now. Since the crash seems to be environment dependent in some way, adding the assertion might cause previously unmet crashes with pipelines not setting colorSpace. Up to you how to proceed with the rest. > There should be a contract somewhere that states a pipeline handler > /must/ fill in specific fields. > > I have an upcoming proposal that could let us add a validation layer to > all runtimes too which could be another place to add runtime > validations. > > Hopefully I'll get that out next week as an initial RFC. > >> > but is this occuring in SoftISP/simple pipeline handler ? >> >> Yes. It started happening to me once I reinstalled my development >> system to Fedora 41. I can't see any obvious reason why it crashes now >> and not before (maybe some change in gcc? -- using 14.2.1). >> > > It will be curious if this only appears as a result of changing external > tooling though ! It looks compiler version dependent. It doesn't crash on my Debix but when I add an assertion there then it fails. So the bug was there but it got exposed to me only in current Fedora. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/apps/cam/kms_sink.cpp | 3 ++- >> >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp >> >> index 672c985a..aa9459cf 100644 >> >> --- a/src/apps/cam/kms_sink.cpp >> >> +++ b/src/apps/cam/kms_sink.cpp >> >> @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) >> >> colorEncoding_ = std::nullopt; >> >> colorRange_ = std::nullopt; >> >> >> >> - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >> >> + if (!cfg.colorSpace || >> >> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >> >> return 0; >> >> >> >> /* >> >> -- >> >> 2.48.1 >> >> >>
diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp index 672c985a..aa9459cf 100644 --- a/src/apps/cam/kms_sink.cpp +++ b/src/apps/cam/kms_sink.cpp @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) colorEncoding_ = std::nullopt; colorRange_ = std::nullopt; - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) + if (!cfg.colorSpace || + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) return 0; /*
cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash when it is accessed. If cfg.colorSpace is unset, simply return, the same way as when YcbcrEncoding is set to None. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/apps/cam/kms_sink.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)