pipeline: rpi: Fix crashes due to scoring unknown formats
diff mbox series

Message ID 20260923172119.1893511-1-jai.luthra@ideasonboard.com
State Accepted, archived
Headers show
Series
  • pipeline: rpi: Fix crashes due to scoring unknown formats
Related show

Commit Message

Jai Luthra Sept. 23, 2026, 5:21 p.m. UTC 
The mbusCodeToPixelFormat() helper is used at multiple places, including
for scoring the formats advertised by the sensor, which might include
formats unknown to libcamera. ASSERT(bayer.isValid()) leads to libcamera
crashing in such cases, so drop it.

Instead switch the return type to a std::optional<> so we can skip
scoring unknown or invalid formats.

Fixes: 6c71ee1f1530 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class")
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 44 ++++++++++++-------
 1 file changed, 27 insertions(+), 17 deletions(-)

--
2.55.0

Comments

Kieran Bingham Sept. 24, 2026, 12:21 p.m. UTC | #1 
Quoting Jai Luthra (2026-09-23 18:21:19)
> The mbusCodeToPixelFormat() helper is used at multiple places, including
> for scoring the formats advertised by the sensor, which might include
> formats unknown to libcamera. ASSERT(bayer.isValid()) leads to libcamera
> crashing in such cases, so drop it.
> 
> Instead switch the return type to a std::optional<> so we can skip
> scoring unknown or invalid formats.
> 
> Fixes: 6c71ee1f1530 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class")
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 44 ++++++++++++-------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 71f3f26a4ffe..a85b238ae14f 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -8,6 +8,7 @@
>  #include "pipeline_base.h"
> 
>  #include <chrono>
> +#include <optional>
> 
>  #include <linux/media-bus-format.h>
>  #include <linux/videodev2.h>
> @@ -43,12 +44,12 @@ namespace {
> 
>  constexpr unsigned int defaultRawBitDepth = 12;
> 
> -PixelFormat mbusCodeToPixelFormat(unsigned int code,
> -                                 BayerFormat::Packing packingReq)
> +std::optional<PixelFormat>
> +mbusCodeToPixelFormat(unsigned int code, BayerFormat::Packing packingReq)
>  {
>         BayerFormat bayer = BayerFormat::fromMbusCode(code);
> -
> -       ASSERT(bayer.isValid());
> +       if (!bayer.isValid())
> +               return std::nullopt;
> 
>         bayer.packing = packingReq;
>         PixelFormat pix = bayer.toPixelFormat();
> @@ -63,6 +64,9 @@ PixelFormat mbusCodeToPixelFormat(unsigned int code,
>                 pix = bayer.toPixelFormat();
>         }
> 
> +       if (!pix.isValid())
> +               return std::nullopt;
> +
>         return pix;
>  }
> 
> @@ -378,10 +382,12 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
>                                                          BayerFormat::Packing packingReq)
>  {
>         unsigned int code = format.code;
> -       const PixelFormat pix = mbusCodeToPixelFormat(code, packingReq);
>         V4L2DeviceFormat deviceFormat;
> 
> -       deviceFormat.fourcc = dev->toV4L2PixelFormat(pix);
> +       const std::optional<PixelFormat> pix = mbusCodeToPixelFormat(code, packingReq);
> +       ASSERT(pix.has_value());
> +
> +       deviceFormat.fourcc = dev->toV4L2PixelFormat(*pix);
>         deviceFormat.size = format.size;
>         deviceFormat.colorSpace = format.colorSpace;
>         return deviceFormat;
> @@ -395,7 +401,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
>                 std::make_unique<RPiCameraConfiguration>(data);
>         V4L2SubdeviceFormat sensorFormat;
>         unsigned int bufferCount;
> -       PixelFormat pixelFormat;
> +       std::optional<PixelFormat> pixelFormat;
>         V4L2VideoDevice::Formats fmts;
>         Size size;
>         std::optional<ColorSpace> colorSpace;
> @@ -411,7 +417,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
>                         sensorFormat = data->findBestFormat(size, defaultRawBitDepth);
>                         pixelFormat = mbusCodeToPixelFormat(sensorFormat.code,
>                                                             BayerFormat::Packing::CSI2);
> -                       ASSERT(pixelFormat.isValid());
> +                       ASSERT(pixelFormat.has_value());
>                         colorSpace = ColorSpace::Raw;
>                         bufferCount = 2;
>                         break;
> @@ -468,10 +474,10 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
>                 if (role == StreamRole::Raw) {
>                         /* Translate the MBUS codes to a PixelFormat. */
>                         for (const auto &format : data->sensorFormats_) {
> -                               PixelFormat pf = mbusCodeToPixelFormat(format.first,
> -                                                                      BayerFormat::Packing::CSI2);
> -                               if (pf.isValid())
> -                                       deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
> +                               std::optional<PixelFormat> pf = mbusCodeToPixelFormat(format.first,
> +                                                                                     BayerFormat::Packing::CSI2);
> +                               if (pf.has_value())
> +                                       deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(*pf),

As far as I can tell this looks sane, leaving only this path without the
assertion and keeping it in the other two paths, so we only add expected
formats.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                                                               std::forward_as_tuple(format.second.begin(), format.second.end()));
>                         }
>                 } else {
> @@ -498,7 +504,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
>                 StreamFormats formats(deviceFormats);
>                 StreamConfiguration cfg(formats);
>                 cfg.size = size;
> -               cfg.pixelFormat = pixelFormat;
> +               cfg.pixelFormat = *pixelFormat;
>                 cfg.colorSpace = colorSpace;
>                 cfg.bufferCount = bufferCount;
>                 config->addConfiguration(cfg);
> @@ -958,9 +964,13 @@ V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bit
> 
>         /* Calculate the closest/best mode from the user requested size. */
>         for (const auto &[mbusCode, sizes] : sensorFormats_) {
> -               const PixelFormat format = mbusCodeToPixelFormat(mbusCode,
> -                                                                BayerFormat::Packing::None);
> -               const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +               const std::optional<PixelFormat> format =
> +                       mbusCodeToPixelFormat(mbusCode, BayerFormat::Packing::None);
> +
> +               if (!format.has_value())
> +                       continue;
> +
> +               const PixelFormatInfo &info = PixelFormatInfo::info(*format);
> 
>                 for (const Size &size : sizes) {
>                         double reqAr = static_cast<double>(req.width) / req.height;
> @@ -981,7 +991,7 @@ V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bit
>                         }
> 
>                         LOG(RPI, Debug) << "Format: " << size
> -                                       << " fmt " << format
> +                                       << " fmt " << *format
>                                         << " Score: " << score
>                                         << " (best " << bestScore << ")";
>                 }
> --
> 2.55.0
>
Naushir Patuck Sept. 25, 2026, 8:58 a.m. UTC | #2 
Hi Jai,

Thank you for the patch.

On Wed, 23 Sept 2026 at 18:21, Jai Luthra <jai.luthra@ideasonboard.com>
wrote:

> The mbusCodeToPixelFormat() helper is used at multiple places, including
> for scoring the formats advertised by the sensor, which might include
> formats unknown to libcamera. ASSERT(bayer.isValid()) leads to libcamera
> crashing in such cases, so drop it.
>
> Instead switch the return type to a std::optional<> so we can skip
> scoring unknown or invalid formats.
>
> Fixes: 6c71ee1f1530 ("pipeline: raspberrypi: Introduce PipelineHandlerBase
> class")
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 44 ++++++++++++-------
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 71f3f26a4ffe..a85b238ae14f 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -8,6 +8,7 @@
>  #include "pipeline_base.h"
>
>  #include <chrono>
> +#include <optional>
>
>  #include <linux/media-bus-format.h>
>  #include <linux/videodev2.h>
> @@ -43,12 +44,12 @@ namespace {
>
>  constexpr unsigned int defaultRawBitDepth = 12;
>
> -PixelFormat mbusCodeToPixelFormat(unsigned int code,
> -                                 BayerFormat::Packing packingReq)
> +std::optional<PixelFormat>
> +mbusCodeToPixelFormat(unsigned int code, BayerFormat::Packing packingReq)
>  {
>         BayerFormat bayer = BayerFormat::fromMbusCode(code);
> -
> -       ASSERT(bayer.isValid());
> +       if (!bayer.isValid())
> +               return std::nullopt;
>
>         bayer.packing = packingReq;
>         PixelFormat pix = bayer.toPixelFormat();
> @@ -63,6 +64,9 @@ PixelFormat mbusCodeToPixelFormat(unsigned int code,
>                 pix = bayer.toPixelFormat();
>         }
>
> +       if (!pix.isValid())
> +               return std::nullopt;
> +
>         return pix;
>  }
>
> @@ -378,10 +382,12 @@ V4L2DeviceFormat
> PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
>
>  BayerFormat::Packing packingReq)
>  {
>         unsigned int code = format.code;
> -       const PixelFormat pix = mbusCodeToPixelFormat(code, packingReq);
>         V4L2DeviceFormat deviceFormat;
>
> -       deviceFormat.fourcc = dev->toV4L2PixelFormat(pix);
> +       const std::optional<PixelFormat> pix = mbusCodeToPixelFormat(code,
> packingReq);
> +       ASSERT(pix.has_value());
> +
> +       deviceFormat.fourcc = dev->toV4L2PixelFormat(*pix);
>         deviceFormat.size = format.size;
>         deviceFormat.colorSpace = format.colorSpace;
>         return deviceFormat;
> @@ -395,7 +401,7 @@ PipelineHandlerBase::generateConfiguration(Camera
> *camera, std::span<const Strea
>                 std::make_unique<RPiCameraConfiguration>(data);
>         V4L2SubdeviceFormat sensorFormat;
>         unsigned int bufferCount;
> -       PixelFormat pixelFormat;
> +       std::optional<PixelFormat> pixelFormat;
>         V4L2VideoDevice::Formats fmts;
>         Size size;
>         std::optional<ColorSpace> colorSpace;
> @@ -411,7 +417,7 @@ PipelineHandlerBase::generateConfiguration(Camera
> *camera, std::span<const Strea
>                         sensorFormat = data->findBestFormat(size,
> defaultRawBitDepth);
>                         pixelFormat =
> mbusCodeToPixelFormat(sensorFormat.code,
>
> BayerFormat::Packing::CSI2);
> -                       ASSERT(pixelFormat.isValid());
> +                       ASSERT(pixelFormat.has_value());
>                         colorSpace = ColorSpace::Raw;
>                         bufferCount = 2;
>                         break;
> @@ -468,10 +474,10 @@ PipelineHandlerBase::generateConfiguration(Camera
> *camera, std::span<const Strea
>                 if (role == StreamRole::Raw) {
>                         /* Translate the MBUS codes to a PixelFormat. */
>                         for (const auto &format : data->sensorFormats_) {
> -                               PixelFormat pf =
> mbusCodeToPixelFormat(format.first,
> -
> BayerFormat::Packing::CSI2);
> -                               if (pf.isValid())
> -
>  deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
> +                               std::optional<PixelFormat> pf =
> mbusCodeToPixelFormat(format.first,
> +
>            BayerFormat::Packing::CSI2);
> +                               if (pf.has_value())
> +
>  deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(*pf),
>
> std::forward_as_tuple(format.second.begin(), format.second.end()));
>                         }
>                 } else {
> @@ -498,7 +504,7 @@ PipelineHandlerBase::generateConfiguration(Camera
> *camera, std::span<const Strea
>                 StreamFormats formats(deviceFormats);
>                 StreamConfiguration cfg(formats);
>                 cfg.size = size;
> -               cfg.pixelFormat = pixelFormat;
> +               cfg.pixelFormat = *pixelFormat;
>                 cfg.colorSpace = colorSpace;
>                 cfg.bufferCount = bufferCount;
>                 config->addConfiguration(cfg);
> @@ -958,9 +964,13 @@ V4L2SubdeviceFormat CameraData::findBestFormat(const
> Size &req, unsigned int bit
>
>         /* Calculate the closest/best mode from the user requested size. */
>         for (const auto &[mbusCode, sizes] : sensorFormats_) {
> -               const PixelFormat format = mbusCodeToPixelFormat(mbusCode,
> -
> BayerFormat::Packing::None);
> -               const PixelFormatInfo &info =
> PixelFormatInfo::info(format);
> +               const std::optional<PixelFormat> format =
> +                       mbusCodeToPixelFormat(mbusCode,
> BayerFormat::Packing::None);
> +
> +               if (!format.has_value())
> +                       continue;
> +
> +               const PixelFormatInfo &info =
> PixelFormatInfo::info(*format);
>
>                 for (const Size &size : sizes) {
>                         double reqAr = static_cast<double>(req.width) /
> req.height;
> @@ -981,7 +991,7 @@ V4L2SubdeviceFormat CameraData::findBestFormat(const
> Size &req, unsigned int bit
>                         }
>
>                         LOG(RPI, Debug) << "Format: " << size
> -                                       << " fmt " << format
> +                                       << " fmt " << *format
>                                         << " Score: " << score
>                                         << " (best " << bestScore << ")";
>                 }
> --
> 2.55.0
>
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 71f3f26a4ffe..a85b238ae14f 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -8,6 +8,7 @@ 
 #include "pipeline_base.h"

 #include <chrono>
+#include <optional>

 #include <linux/media-bus-format.h>
 #include <linux/videodev2.h>
@@ -43,12 +44,12 @@  namespace {

 constexpr unsigned int defaultRawBitDepth = 12;

-PixelFormat mbusCodeToPixelFormat(unsigned int code,
-				  BayerFormat::Packing packingReq)
+std::optional<PixelFormat>
+mbusCodeToPixelFormat(unsigned int code, BayerFormat::Packing packingReq)
 {
 	BayerFormat bayer = BayerFormat::fromMbusCode(code);
-
-	ASSERT(bayer.isValid());
+	if (!bayer.isValid())
+		return std::nullopt;

 	bayer.packing = packingReq;
 	PixelFormat pix = bayer.toPixelFormat();
@@ -63,6 +64,9 @@  PixelFormat mbusCodeToPixelFormat(unsigned int code,
 		pix = bayer.toPixelFormat();
 	}

+	if (!pix.isValid())
+		return std::nullopt;
+
 	return pix;
 }

@@ -378,10 +382,12 @@  V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
 							 BayerFormat::Packing packingReq)
 {
 	unsigned int code = format.code;
-	const PixelFormat pix = mbusCodeToPixelFormat(code, packingReq);
 	V4L2DeviceFormat deviceFormat;

-	deviceFormat.fourcc = dev->toV4L2PixelFormat(pix);
+	const std::optional<PixelFormat> pix = mbusCodeToPixelFormat(code, packingReq);
+	ASSERT(pix.has_value());
+
+	deviceFormat.fourcc = dev->toV4L2PixelFormat(*pix);
 	deviceFormat.size = format.size;
 	deviceFormat.colorSpace = format.colorSpace;
 	return deviceFormat;
@@ -395,7 +401,7 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
 		std::make_unique<RPiCameraConfiguration>(data);
 	V4L2SubdeviceFormat sensorFormat;
 	unsigned int bufferCount;
-	PixelFormat pixelFormat;
+	std::optional<PixelFormat> pixelFormat;
 	V4L2VideoDevice::Formats fmts;
 	Size size;
 	std::optional<ColorSpace> colorSpace;
@@ -411,7 +417,7 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
 			sensorFormat = data->findBestFormat(size, defaultRawBitDepth);
 			pixelFormat = mbusCodeToPixelFormat(sensorFormat.code,
 							    BayerFormat::Packing::CSI2);
-			ASSERT(pixelFormat.isValid());
+			ASSERT(pixelFormat.has_value());
 			colorSpace = ColorSpace::Raw;
 			bufferCount = 2;
 			break;
@@ -468,10 +474,10 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
 		if (role == StreamRole::Raw) {
 			/* Translate the MBUS codes to a PixelFormat. */
 			for (const auto &format : data->sensorFormats_) {
-				PixelFormat pf = mbusCodeToPixelFormat(format.first,
-								       BayerFormat::Packing::CSI2);
-				if (pf.isValid())
-					deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
+				std::optional<PixelFormat> pf = mbusCodeToPixelFormat(format.first,
+										      BayerFormat::Packing::CSI2);
+				if (pf.has_value())
+					deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(*pf),
 							      std::forward_as_tuple(format.second.begin(), format.second.end()));
 			}
 		} else {
@@ -498,7 +504,7 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, std::span<const Strea
 		StreamFormats formats(deviceFormats);
 		StreamConfiguration cfg(formats);
 		cfg.size = size;
-		cfg.pixelFormat = pixelFormat;
+		cfg.pixelFormat = *pixelFormat;
 		cfg.colorSpace = colorSpace;
 		cfg.bufferCount = bufferCount;
 		config->addConfiguration(cfg);
@@ -958,9 +964,13 @@  V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bit

 	/* Calculate the closest/best mode from the user requested size. */
 	for (const auto &[mbusCode, sizes] : sensorFormats_) {
-		const PixelFormat format = mbusCodeToPixelFormat(mbusCode,
-								 BayerFormat::Packing::None);
-		const PixelFormatInfo &info = PixelFormatInfo::info(format);
+		const std::optional<PixelFormat> format =
+			mbusCodeToPixelFormat(mbusCode, BayerFormat::Packing::None);
+
+		if (!format.has_value())
+			continue;
+
+		const PixelFormatInfo &info = PixelFormatInfo::info(*format);

 		for (const Size &size : sizes) {
 			double reqAr = static_cast<double>(req.width) / req.height;
@@ -981,7 +991,7 @@  V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bit
 			}

 			LOG(RPI, Debug) << "Format: " << size
-					<< " fmt " << format
+					<< " fmt " << *format
 					<< " Score: " << score
 					<< " (best " << bestScore << ")";
 		}