<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body>
    <pre>
</pre>
    <div class="moz-cite-prefix">
      <pre>On 17/12/2024 17:46, Laurent Pinchart wrote:</pre>
    </div>
    <blockquote type="cite" cite="mid:20241217164650.GH20432@pendragon.ideasonboard.com">
      <pre class="moz-quote-pre" wrap="">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:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Quoting Antoine Bouyer (2024-12-17 14:02:39)
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">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:
 (<a class="moz-txt-link-freetext" href="https://en.cppreference.com/w/cpp/utility/optional/operator*">https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.cppreference.com%2Fw%2Fcpp%2Futility%2Foptional%2Foperator*&data=05%7C02%7Cantoine.bouyer%40nxp.com%7Cb85096fb16c94ded510c08dd1eba68c8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638700508158613641%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=6RHk9aC4cBktiGYz%2FU1SI9kdKOcEi9dqdrR9MyWqgfM%3D&reserved=0</a>)

"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 <a class="moz-txt-link-rfc2396E" href="mailto:antoine.bouyer@nxp.com"><antoine.bouyer@nxp.com></a>
Reviewed-by: Kieran Bingham <a class="moz-txt-link-rfc2396E" href="mailto:kieran.bingham@ideasonboard.com"><kieran.bingham@ideasonboard.com></a>
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
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...
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Let's see first, Antoine, which pipeline handler have you encountered
this issue with ?</pre>
    </blockquote>
    <pre>This is with imx8-isi pipeline.
Actually I was thinking about such approach initially, then changed to has_value() fix
because of "optional" flag.
</pre>
    <blockquote type="cite" cite="mid:20241217164650.GH20432@pendragon.ideasonboard.com">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">---
 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;

        /*
</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
--
Regards,

Laurent Pinchart</pre>
    </blockquote>
    <pre>Best regards</pre>
    <pre>Antoine</pre>
  </body>
</html>