<div dir="ltr"><div dir="ltr">Hi David, <div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 30 Nov 2021 at 11:23, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">When a raw stream is specified, the bit depth and packing requested<br>
should influence our choice of camera mode to match (if possible).<br>
<br>
Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
---<br>
 .../pipeline/raspberrypi/raspberrypi.cpp  Â  Â  | 33 ++++++++++---------<br>
 1 file changed, 18 insertions(+), 15 deletions(-)<br>
<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index 045725dd..03012e89 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -49,6 +49,8 @@ LOG_DEFINE_CATEGORY(RPI)<br>
<br>
 namespace {<br>
<br>
+unsigned int DEFAULT_RAW_BIT_DEPTH = 12;<br>
+<br></blockquote><div><br></div><div>Perhaps a const, or constexpr for this one?</div><div>I would switch to camelCase as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 /* Map of mbus codes to supported sizes reported by the sensor. */<br>
 using SensorFormats = std::map<unsigned int, std::vector<Size>>;<br>
<br>
@@ -125,15 +127,13 @@ double scoreFormat(double desired, double actual)<br>
  Â  Â  Â  return score;<br>
 }<br>
<br>
-V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req)<br>
+V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req, unsigned int bitDepth)<br>
 {<br>
  Â  Â  Â  double bestScore = std::numeric_limits<double>::max(), score;<br>
  Â  Â  Â  V4L2SubdeviceFormat bestFormat;<br>
<br>
 #define PENALTY_AR  Â  Â  Â  Â  Â  Â 1500.0<br>
-#define PENALTY_8BIT  Â  Â  Â  Â  Â 2000.0<br>
-#define PENALTY_10BIT  Â  Â  Â  Â  1000.0<br>
-#define PENALTY_12BIT  Â  Â  Â  Â  Â  Â 0.0<br>
+#define PENALTY_BIT_DEPTH  Â  Â  500.0<br></blockquote><div><br></div><div>Should we just switch to constexpr as well and do away with the #define?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  Â  Â  Â  /* Calculate the closest/best mode from the user requested size. */<br>
  Â  Â  Â  for (const auto &iter : formatsMap) {<br>
@@ -152,12 +152,7 @@ V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  score += PENALTY_AR * scoreFormat(reqAr, fmtAr);<br>
<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  /* Add any penalties... this is not an exact science! */<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (info.bitsPerPixel == 12)<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â score += PENALTY_12BIT;<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â else if (info.bitsPerPixel == 10)<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â score += PENALTY_10BIT;<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â else if (info.bitsPerPixel == 8)<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â score += PENALTY_8BIT;<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â score += abs(info.bitsPerPixel - bitDepth) * PENALTY_BIT_DEPTH;<br></blockquote><div><br></div><div>Should we use the scoreFormat() call for this? That way, we can weight the scoring to a higher bit depth if an</div><div>exact match is not found.  We may need to reword scoreFormat() appropriately.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  if (score <= bestScore) {<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  bestScore = score;<br>
@@ -397,9 +392,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â * Calculate the best sensor mode we can use based on<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â * the user request.<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â */<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unsigned int bitDepth = info.isValid() ? info.bitsPerPixel : DEFAULT_RAW_BIT_DEPTH;<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size, bitDepth);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â BayerFormat::Packing packing = BayerFormat::Packing::CSI2;<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (info.isValid() && !info.packed)<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â packing = BayerFormat::Packing::None;<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  BayerFormat::Packing::CSI2);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  packing);<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  if (ret)<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  return Invalid;<br>
@@ -533,7 +533,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
  Â  Â  Â  Â  Â  Â  Â  switch (role) {<br>
  Â  Â  Â  Â  Â  Â  Â  case StreamRole::Raw:<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  size = data->sensor_->resolution();<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sensorFormat = findBestFormat(data->sensorFormats_, size);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sensorFormat = findBestFormat(data->sensorFormats_, size, DEFAULT_RAW_BIT_DEPTH);<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  pixelFormat = mbusCodeToPixelFormat(sensorFormat.mbus_code,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  BayerFormat::Packing::CSI2);<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ASSERT(pixelFormat.isValid());<br>
@@ -622,6 +622,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
  Â  Â  Â  Size maxSize, sensorSize;<br>
  Â  Â  Â  unsigned int maxIndex = 0;<br>
  Â  Â  Â  bool rawStream = false;<br>
+  Â  Â  Â unsigned int bitDepth = DEFAULT_RAW_BIT_DEPTH;<br>
<br>
  Â  Â  Â  /*<br>
  Â  Â  Â  Â * Look for the RAW stream (if given) size as well as the largest<br>
@@ -638,7 +639,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  sensorSize = cfg.size;<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  rawStream = true;<br>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â Â <br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â BayerFormat bayerFormat = BayerFormat::fromPixelFormat(cfg.pixelFormat);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â packing = bayerFormat.packing;<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â bitDepth = bayerFormat.bitDepth;<br>
  Â  Â  Â  Â  Â  Â  Â  } else {<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  if (cfg.size > maxSize) {<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  maxSize = config->at(i).size;<br>
@@ -664,7 +667,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
  Â  Â  Â  }<br>
<br>
  Â  Â  Â  /* First calculate the best sensor mode we can use based on the user request. */<br>
-  Â  Â  Â V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);<br>
+  Â  Â  Â V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize, bitDepth);<br>
  Â  Â  Â  ret = data->sensor_->setFormat(&sensorFormat);<br>
  Â  Â  Â  if (ret)<br>
  Â  Â  Â  Â  Â  Â  Â  return ret;<br></blockquote><div><br></div><div>With or without the suggestions:</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-- <br>
2.30.2<br>
<br>
</blockquote></div></div>