<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Oct 2021 at 14:13, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">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">Hi Naush<br>
<br>
Thanks for this patch!<br>
<br>
On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> It is more convenient to return a PixelFormat from findBestMode(), as the<br>
> conversions from PixelFormat to V4L2SubdeviceFormat and V4L2DeviceFormat are<br>
> simpler.<br>
<br>
Yes indeed, though the PixelFormat still includes packing in an<br>
"unhelpful" way, so there might be even better alternatives?<br></blockquote><div><br></div><div>Yes, as per the comments, on the earlier patch PixelFormat could return a</div><div>V4L2SubdeviceFormat struct to make things more convenient in the code.<br></div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Best regards<br>
<br>
David<br>
<br>
><br>
> Add some internal helpers to perform these conversions.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 73 +++++++++++++------<br>
>  1 file changed, 52 insertions(+), 21 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 730f1575095c..0f13127a7748 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -50,6 +50,7 @@ namespace {<br>
><br>
>  /* Map of mbus codes to supported sizes reported by the sensor. */<br>
>  using SensorFormats = std::map<unsigned int, std::vector<Size>>;<br>
> +using SensorMode = std::pair<PixelFormat, Size>;<br>
><br>
>  SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)<br>
>  {<br>
> @@ -61,6 +62,34 @@ SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)<br>
>         return formats;<br>
>  }<br>
><br>
> +inline V4L2DeviceFormat toV4L2DeviceFormat(SensorMode &mode)<br>
> +{<br>
> +       V4L2DeviceFormat deviceFormat;<br>
> +<br>
> +       deviceFormat.fourcc = V4L2PixelFormat::fromPixelFormat(mode.first);<br>
> +       deviceFormat.size = mode.second;<br>
> +       return deviceFormat;<br>
> +}<br>
> +<br>
> +inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format)<br>
> +{<br>
> +       V4L2DeviceFormat deviceFormat;<br>
> +<br>
> +       deviceFormat.fourcc = BayerFormat::fromMbusCode(format.mbus_code).toV4L2PixelFormat();<br>
> +       deviceFormat.size = format.size;<br>
> +       return deviceFormat;<br>
> +}<br>
> +<br>
> +inline V4L2SubdeviceFormat toV4L2SubdeviceFormat(SensorMode &mode)<br>
> +{<br>
> +       V4L2SubdeviceFormat subdeviceFormat;<br>
> +       V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(mode.first);<br>
> +<br>
> +       subdeviceFormat.mbus_code = BayerFormat::fromV4L2PixelFormat(fourcc).toMbusCode();<br>
> +       subdeviceFormat.size = mode.second;<br>
> +       return subdeviceFormat;<br>
> +}<br>
> +<br>
>  bool isRaw(PixelFormat &pixFmt)<br>
>  {<br>
>         /*<br>
> @@ -87,10 +116,10 @@ double scoreFormat(double desired, double actual)<br>
>         return score;<br>
>  }<br>
><br>
> -V4L2DeviceFormat findBestMode(const SensorFormats &formatsMap, const Size &req)<br>
> +SensorMode findBestMode(const SensorFormats &formatsMap, const Size &req)<br>
>  {<br>
>         double bestScore = std::numeric_limits<double>::max(), score;<br>
> -       V4L2DeviceFormat bestMode;<br>
> +       SensorMode bestMode;<br>
><br>
>  #define PENALTY_AR             1500.0<br>
>  #define PENALTY_8BIT           2000.0<br>
> @@ -101,8 +130,8 @@ V4L2DeviceFormat findBestMode(const SensorFormats &formatsMap, const Size &req)<br>
>         /* Calculate the closest/best mode from the user requested size. */<br>
>         for (const auto &iter : formatsMap) {<br>
>                 const unsigned int mbus_code = iter.first;<br>
> -               const V4L2PixelFormat v4l2Format = BayerFormat::fromMbusCode(mbus_code).toV4L2PixelFormat();<br>
> -               const PixelFormatInfo &info = PixelFormatInfo::info(v4l2Format);<br>
> +               const PixelFormat format = BayerFormat::fromMbusCode(mbus_code).toPixelFormat();<br>
> +               const PixelFormatInfo &info = PixelFormatInfo::info(format);<br>
><br>
>                 for (const Size &sz : iter.second) {<br>
>                         double reqAr = static_cast<double>(req.width) / req.height;<br>
> @@ -126,12 +155,12 @@ V4L2DeviceFormat findBestMode(const SensorFormats &formatsMap, const Size &req)<br>
><br>
>                         if (score <= bestScore) {<br>
>                                 bestScore = score;<br>
> -                               bestMode.fourcc = v4l2Format;<br>
> -                               bestMode.size = sz;<br>
> +                               bestMode.first = format;<br>
> +                               bestMode.second = sz;<br>
>                         }<br>
><br>
>                         LOG(RPI, Info) << "Mode: " << sz.width << "x" << sz.height<br>
> -                                      << " fmt " << v4l2Format.toString()<br>
> +                                      << " fmt " << format.toString()<br>
>                                        << " Score: " << score<br>
>                                        << " (best " << bestScore << ")";<br>
>                 }<br>
> @@ -364,8 +393,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
>                          * Calculate the best sensor mode we can use based on<br>
>                          * the user request.<br>
>                          */<br>
> -                       V4L2DeviceFormat sensorFormat = findBestMode(data_->sensorFormats_, cfg.size);<br>
> -                       int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);<br>
> +                       SensorMode sensorMode = findBestMode(data_->sensorFormats_, cfg.size);<br>
> +                       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorMode);<br>
> +                       int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat);<br>
>                         if (ret)<br>
>                                 return Invalid;<br>
><br>
> @@ -377,7 +407,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
>                          * fetch the "native" (i.e. untransformed) Bayer order,<br>
>                          * because the sensor may currently be flipped!<br>
>                          */<br>
> -                       V4L2PixelFormat fourcc = sensorFormat.fourcc;<br>
> +                       V4L2PixelFormat fourcc = unicamFormat.fourcc;<br>
>                         if (data_->flipsAlterBayerOrder_) {<br>
>                                 BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);<br>
>                                 bayer.order = data_->nativeBayerOrder_;<br>
> @@ -386,15 +416,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
>                         }<br>
><br>
>                         PixelFormat sensorPixFormat = fourcc.toPixelFormat();<br>
> -                       if (cfg.size != sensorFormat.size ||<br>
> +                       if (cfg.size != unicamFormat.size ||<br>
>                             cfg.pixelFormat != sensorPixFormat) {<br>
> -                               cfg.size = sensorFormat.size;<br>
> +                               cfg.size = unicamFormat.size;<br>
>                                 cfg.pixelFormat = sensorPixFormat;<br>
>                                 status = Adjusted;<br>
>                         }<br>
><br>
> -                       cfg.stride = sensorFormat.planes[0].bpl;<br>
> -                       cfg.frameSize = sensorFormat.planes[0].size;<br>
> +                       cfg.stride = unicamFormat.planes[0].bpl;<br>
> +                       cfg.frameSize = unicamFormat.planes[0].size;<br>
><br>
>                         rawCount++;<br>
>                 } else {<br>
> @@ -483,7 +513,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
>  {<br>
>         RPiCameraData *data = cameraData(camera);<br>
>         CameraConfiguration *config = new RPiCameraConfiguration(data);<br>
> -       V4L2DeviceFormat sensorFormat;<br>
> +       V4L2DeviceFormat unicamFormat;<br>
> +       SensorMode sensorMode;<br>
>         unsigned int bufferCount;<br>
>         PixelFormat pixelFormat;<br>
>         V4L2VideoDevice::Formats fmts;<br>
> @@ -498,8 +529,9 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
>                 switch (role) {<br>
>                 case StreamRole::Raw:<br>
>                         size = data->sensor_->resolution();<br>
> -                       sensorFormat = findBestMode(data->sensorFormats_, size);<br>
> -                       pixelFormat = sensorFormat.fourcc.toPixelFormat();<br>
> +                       sensorMode = findBestMode(data->sensorFormats_, size);<br>
> +                       unicamFormat = toV4L2DeviceFormat(sensorMode);<br>
> +                       pixelFormat = sensorMode.first;<br>
>                         ASSERT(pixelFormat.isValid());<br>
>                         bufferCount = 2;<br>
>                         rawCount++;<br>
> @@ -609,10 +641,9 @@ 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>
> -       V4L2DeviceFormat unicamFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);<br>
> -<br>
> -       unsigned int mbus_code = BayerFormat::fromV4L2PixelFormat(unicamFormat.fourcc).toMbusCode();<br>
> -       V4L2SubdeviceFormat sensorFormat { .mbus_code = mbus_code, .size = unicamFormat.size };<br>
> +       SensorMode sensorMode = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);<br>
> +       V4L2SubdeviceFormat sensorFormat = toV4L2SubdeviceFormat(sensorMode);<br>
> +       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorMode);<br>
><br>
>         ret = data->sensor_->setFormat(&sensorFormat);<br>
>         if (ret)<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>