| 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 >> >> >>
And now I'm working my way up from December to March I see both Milan and Antoine have posted a similar fix but for usage on different pipelines. It seems we need to do something to resolve these... See https://patchwork.libcamera.org/patch/22376/ Quoting Milan Zamazal (2025-03-20 17:18:50) > 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 > >> >> > >> >
Hi, Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > And now I'm working my way up from December to March I see both Milan > and Antoine have posted a similar fix but for usage on different > pipelines. > > It seems we need to do something to resolve these... The crash is quite annoying, I have to apply the patch in each branch I work on. IIRC Laurent asked me to fix the problem in `simple' pipeline so I added a (different) fix to my raw stream patches: https://patchwork.libcamera.org/patch/23432/. But those are stuck in review -- could we perhaps move on with at least that first patch? This could clarify what's the right thing to do in all the affected pipelines; or we can simply merge the Antoine's or my kms_sink.cpp one-liner to prevent the crash. > See https://patchwork.libcamera.org/patch/22376/ > > > Quoting Milan Zamazal (2025-03-20 17:18:50) >> 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(-)