| Message ID | 20241217140238.215572-2-antoine.bouyer@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Antoine Bouyer (2024-12-17 14:02:39) > During configuration stage, kms_sink doesn't check optional colorSpace > parameter is valid while dereferencing it. Then it leads to a crash > with below signature if parameter is not defined: > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > led. > Aborted (core dumped) > > This behavior is confirmed in the "operator->" page: > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > "This operator does not check whether the optional contains a value!" > > This crash is observed with styhead's gcc compiler (version 14.2.0), > while it was not with previous scarthgap's gcc compiler (version 13.2.0). > > Use has_value() as a fix to make sure this property exists and prevent > crash. > > Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks for the updates that I suggested, however I'm weary that perhaps Laurent was suggesting this /shouldn't/ be possible to happen and should be fixed in the pipeline handler? (and perhaps enforced?, and maybe guaranteed to be set to RAW if unknown?) We could perhaps have a common path in the configuration that sets RAW when the pipeline handler didn't set it ? Or maybe we enforce it must be set by lc-compliance or such... > --- > 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 672c985..8f3b867 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.has_value() || > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > return 0; > > /* > -- > 2.34.1 >
On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote: > Quoting Antoine Bouyer (2024-12-17 14:02:39) > > During configuration stage, kms_sink doesn't check optional colorSpace > > parameter is valid while dereferencing it. Then it leads to a crash > > with below signature if parameter is not defined: > > > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > > led. > > Aborted (core dumped) > > > > This behavior is confirmed in the "operator->" page: > > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > > > "This operator does not check whether the optional contains a value!" > > > > This crash is observed with styhead's gcc compiler (version 14.2.0), > > while it was not with previous scarthgap's gcc compiler (version 13.2.0). > > > > Use has_value() as a fix to make sure this property exists and prevent > > crash. > > > > Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Thanks for the updates that I suggested, however I'm weary that perhaps > Laurent was suggesting this /shouldn't/ be possible to happen and should > be fixed in the pipeline handler? (and perhaps enforced?, and maybe > guaranteed to be set to RAW if unknown?) > > We could perhaps have a common path in the configuration that sets RAW > when the pipeline handler didn't set it ? Or maybe we enforce it must be > set by lc-compliance or such... Let's see first, Antoine, which pipeline handler have you encountered this issue with ? > > --- > > 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 672c985..8f3b867 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.has_value() || > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > return 0; > > > > /*
On 17/12/2024 17:46, Laurent Pinchart wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote: >> Quoting Antoine Bouyer (2024-12-17 14:02:39) >>> During configuration stage, kms_sink doesn't check optional colorSpace >>> parameter is valid while dereferencing it. Then it leads to a crash >>> with below signature if parameter is not defined: >>> >>> /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 >>> 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp >>> ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai >>> led. >>> Aborted (core dumped) >>> >>> This behavior is confirmed in the "operator->" page: >>> (https://en.cppreference.com/w/cpp/utility/optional/operator*) >>> >>> "This operator does not check whether the optional contains a value!" >>> >>> This crash is observed with styhead's gcc compiler (version 14.2.0), >>> while it was not with previous scarthgap's gcc compiler (version 13.2.0). >>> >>> Use has_value() as a fix to make sure this property exists and prevent >>> crash. >>> >>> Fixes: 6d7ddace5278 ("cam: Add KMS sink class") >>> Signed-off-by: Antoine Bouyer<antoine.bouyer@nxp.com> >>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com> >> Thanks for the updates that I suggested, however I'm weary that perhaps >> Laurent was suggesting this /shouldn't/ be possible to happen and should >> be fixed in the pipeline handler? (and perhaps enforced?, and maybe >> guaranteed to be set to RAW if unknown?) >> >> We could perhaps have a common path in the configuration that sets RAW >> when the pipeline handler didn't set it ? Or maybe we enforce it must be >> set by lc-compliance or such... > Let's see first, Antoine, which pipeline handler have you encountered > this issue with ? This is with imx8-isi pipeline. Actually I was thinking about such approach initially, then changed to has_value() fix because of "optional" flag. > >>> --- >>> 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 672c985..8f3b867 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.has_value() || >>> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >>> return 0; >>> >>> /* > -- > Regards, > > Laurent Pinchart Best regards Antoine
Quoting Antoine Bouyer (2024-12-17 16:59:06) > On 17/12/2024 17:46, Laurent Pinchart wrote: > > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > > > > On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote: > >> Quoting Antoine Bouyer (2024-12-17 14:02:39) > >>> During configuration stage, kms_sink doesn't check optional colorSpace > >>> parameter is valid while dereferencing it. Then it leads to a crash > >>> with below signature if parameter is not defined: > >>> > >>> /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > >>> 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > >>> ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > >>> led. > >>> Aborted (core dumped) > >>> > >>> This behavior is confirmed in the "operator->" page: > >>> (https://en.cppreference.com/w/cpp/utility/optional/operator*) > >>> > >>> "This operator does not check whether the optional contains a value!" > >>> > >>> This crash is observed with styhead's gcc compiler (version 14.2.0), > >>> while it was not with previous scarthgap's gcc compiler (version 13.2.0). > >>> > >>> Use has_value() as a fix to make sure this property exists and prevent > >>> crash. > >>> > >>> Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > >>> Signed-off-by: Antoine Bouyer<antoine.bouyer@nxp.com> > >>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com> > >> Thanks for the updates that I suggested, however I'm weary that perhaps > >> Laurent was suggesting this /shouldn't/ be possible to happen and should > >> be fixed in the pipeline handler? (and perhaps enforced?, and maybe > >> guaranteed to be set to RAW if unknown?) > >> > >> We could perhaps have a common path in the configuration that sets RAW > >> when the pipeline handler didn't set it ? Or maybe we enforce it must be > >> set by lc-compliance or such... > > Let's see first, Antoine, which pipeline handler have you encountered > > this issue with ? > > This is with imx8-isi pipeline. > Actually I was thinking about such approach initially, then changed to has_value() fix > because of "optional" flag. Moving my questions from v1 to here: (I hope I haven't now missed a v3..) Antoine, do you plan to provide a fix for the ISI pipeline handler to always provide a colorSpace ? I don't see such a change currently in tree ? -- Kieran > > > > >>> --- > >>> 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 672c985..8f3b867 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.has_value() || > >>> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > >>> return 0; > >>> > >>> /* > > -- > > Regards, > > > > Laurent Pinchart > > Best regards > > Antoine
On 27/06/2025 10:30, Kieran Bingham wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Quoting Antoine Bouyer (2024-12-17 16:59:06) >> On 17/12/2024 17:46, Laurent Pinchart wrote: >> >>> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button >>> >>> >>> On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote: >>>> Quoting Antoine Bouyer (2024-12-17 14:02:39) >>>>> During configuration stage, kms_sink doesn't check optional colorSpace >>>>> parameter is valid while dereferencing it. Then it leads to a crash >>>>> with below signature if parameter is not defined: >>>>> >>>>> /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 >>>>> 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp >>>>> ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai >>>>> led. >>>>> Aborted (core dumped) >>>>> >>>>> This behavior is confirmed in the "operator->" page: >>>>> (https://en.cppreference.com/w/cpp/utility/optional/operator*) >>>>> >>>>> "This operator does not check whether the optional contains a value!" >>>>> >>>>> This crash is observed with styhead's gcc compiler (version 14.2.0), >>>>> while it was not with previous scarthgap's gcc compiler (version 13.2.0). >>>>> >>>>> Use has_value() as a fix to make sure this property exists and prevent >>>>> crash. >>>>> >>>>> Fixes: 6d7ddace5278 ("cam: Add KMS sink class") >>>>> Signed-off-by: Antoine Bouyer<antoine.bouyer@nxp.com> >>>>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com> >>>> Thanks for the updates that I suggested, however I'm weary that perhaps >>>> Laurent was suggesting this /shouldn't/ be possible to happen and should >>>> be fixed in the pipeline handler? (and perhaps enforced?, and maybe >>>> guaranteed to be set to RAW if unknown?) >>>> >>>> We could perhaps have a common path in the configuration that sets RAW >>>> when the pipeline handler didn't set it ? Or maybe we enforce it must be >>>> set by lc-compliance or such... >>> Let's see first, Antoine, which pipeline handler have you encountered >>> this issue with ? >> >> This is with imx8-isi pipeline. >> Actually I was thinking about such approach initially, then changed to has_value() fix >> because of "optional" flag. > > Moving my questions from v1 to here: (I hope I haven't now missed a > v3..) > > Antoine, do you plan to provide a fix for the ISI pipeline handler to > always provide a colorSpace ? I don't see such a change currently in > tree ? > > -- > Kieran Hi Kieran, Good to see old patchset are still alive ;) I did not push any V3 (yet). Unfortunately no, I did not plan to force colorSpace in imx8 pipeline handler. I assume that if the parameter must be present, it becomes "mandatory" and not optional anymore. So optional property should be removed as well I guess. I don't know what would be the impact on other pipelines then ... so I did not explore that option on my side. Best regards Antoine > >> >>> >>>>> --- >>>>> 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 672c985..8f3b867 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.has_value() || >>>>> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) >>>>> return 0; >>>>> >>>>> /* >>> -- >>> Regards, >>> >>> Laurent Pinchart >> >> Best regards >> >> Antoine
Quoting Antoine Bouyer (2025-06-27 10:50:01) > > > On 27/06/2025 10:30, Kieran Bingham wrote: > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > > > > Quoting Antoine Bouyer (2024-12-17 16:59:06) > >> On 17/12/2024 17:46, Laurent Pinchart wrote: > >> > >>> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > >>> > >>> > >>> On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote: > >>>> Quoting Antoine Bouyer (2024-12-17 14:02:39) > >>>>> During configuration stage, kms_sink doesn't check optional colorSpace > >>>>> parameter is valid while dereferencing it. Then it leads to a crash > >>>>> with below signature if parameter is not defined: > >>>>> > >>>>> /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > >>>>> 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > >>>>> ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > >>>>> led. > >>>>> Aborted (core dumped) > >>>>> > >>>>> This behavior is confirmed in the "operator->" page: > >>>>> (https://en.cppreference.com/w/cpp/utility/optional/operator*) > >>>>> > >>>>> "This operator does not check whether the optional contains a value!" > >>>>> > >>>>> This crash is observed with styhead's gcc compiler (version 14.2.0), > >>>>> while it was not with previous scarthgap's gcc compiler (version 13.2.0). > >>>>> > >>>>> Use has_value() as a fix to make sure this property exists and prevent > >>>>> crash. > >>>>> > >>>>> Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > >>>>> Signed-off-by: Antoine Bouyer<antoine.bouyer@nxp.com> > >>>>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com> > >>>> Thanks for the updates that I suggested, however I'm weary that perhaps > >>>> Laurent was suggesting this /shouldn't/ be possible to happen and should > >>>> be fixed in the pipeline handler? (and perhaps enforced?, and maybe > >>>> guaranteed to be set to RAW if unknown?) > >>>> > >>>> We could perhaps have a common path in the configuration that sets RAW > >>>> when the pipeline handler didn't set it ? Or maybe we enforce it must be > >>>> set by lc-compliance or such... > >>> Let's see first, Antoine, which pipeline handler have you encountered > >>> this issue with ? > >> > >> This is with imx8-isi pipeline. > >> Actually I was thinking about such approach initially, then changed to has_value() fix > >> because of "optional" flag. > > > > Moving my questions from v1 to here: (I hope I haven't now missed a > > v3..) > > > > Antoine, do you plan to provide a fix for the ISI pipeline handler to > > always provide a colorSpace ? I don't see such a change currently in > > tree ? > > > > -- > > Kieran > > Hi Kieran, > Good to see old patchset are still alive ;) I did not push any V3 (yet). > > Unfortunately no, I did not plan to force colorSpace in imx8 pipeline > handler. I assume that if the parameter must be present, it becomes > "mandatory" and not optional anymore. So optional property should be > removed as well I guess. Laurent replied to that point in a previous mail I think... https://patchwork.libcamera.org/patch/22373/#32812 >>> The member is optional primarily to allow applications to get the >>> default color space. We also allow pipeline handlers to leave it >>> unset when they don't know what color space is produced, but that >>> shouldn't be the rule. Actually, except for the simple pipeline >>> handler (and possibly for UVC, I haven't checked), I would make it >>> mandatory for pipeline handlers to return a valid color space. We >>> could also require them to report ColorSpace::Raw when they don't >>> know. > > I don't know what would be the impact on other pipelines then ... so I > did not explore that option on my side. > > Best regards > Antoine
diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp index 672c985..8f3b867 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.has_value() || + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) return 0; /*
During configuration stage, kms_sink doesn't check optional colorSpace parameter is valid while dereferencing it. Then it leads to a crash with below signature if parameter is not defined: /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai led. Aborted (core dumped) This behavior is confirmed in the "operator->" page: (https://en.cppreference.com/w/cpp/utility/optional/operator*) "This operator does not check whether the optional contains a value!" This crash is observed with styhead's gcc compiler (version 14.2.0), while it was not with previous scarthgap's gcc compiler (version 13.2.0). Use has_value() as a fix to make sure this property exists and prevent crash. Fixes: 6d7ddace5278 ("cam: Add KMS sink class") Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/apps/cam/kms_sink.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)