[libcamera-devel,v1,2/3] libcamera: rpi: pipeline_base: Move findBestFormat to CameraData

18813 diff mbox series

From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

The findBestFormat() helper operates on the list of sensor formats,
which is owned by the CameraData class. Move the function to that class
as well to:

1) Avoid passing the list of formats to the function
2) Remove a static helper in favour of a class function
3) Allow subclasses with access to CameraData to call the function

Move to the CameraData class the scoreFormat helper as well.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 140 ++++++++++--------
 .../pipeline/rpi/common/pipeline_base.h       |   3 +
 2 files changed, 79 insertions(+), 64 deletions(-)

Hi Jacopo,

Thank you for your work!

On Wed, 12 Jul 2023 at 11:55, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> The findBestFormat() helper operates on the list of sensor formats,
> which is owned by the CameraData class. Move the function to that class
> as well to:
>
> 1) Avoid passing the list of formats to the function
> 2) Remove a static helper in favour of a class function
> 3) Allow subclasses with access to CameraData to call the function
>
> Move to the CameraData class the scoreFormat helper as well.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 140 ++++++++++--------
>  .../pipeline/rpi/common/pipeline_base.h       |   3 +
>  2 files changed, 79 insertions(+), 64 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index fb3756a47590..725c1db0d527 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -74,63 +74,6 @@ bool isMonoSensor(std::unique_ptr<CameraSensor> &sensor)
>         return bayer.order == BayerFormat::Order::MONO;
>  }
>
> -double scoreFormat(double desired, double actual)
> -{
> -       double score = desired - actual;
> -       /* Smaller desired dimensions are preferred. */
> -       if (score < 0.0)
> -               score = (-score) / 8;
> -       /* Penalise non-exact matches. */
> -       if (actual != desired)
> -               score *= 2;
> -
> -       return score;
> -}
> -
> -V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &req, unsigned int bitDepth)
> -{
> -       double bestScore = std::numeric_limits<double>::max(), score;
> -       V4L2SubdeviceFormat bestFormat;
> -       bestFormat.colorSpace = ColorSpace::Raw;
> -
> -       constexpr float penaltyAr = 1500.0;
> -       constexpr float penaltyBitDepth = 500.0;
> -
> -       /* Calculate the closest/best mode from the user requested size. */
> -       for (const auto &iter : formatsMap) {
> -               const unsigned int mbusCode = iter.first;
> -               const PixelFormat format = mbusCodeToPixelFormat(mbusCode,
> -                                                                BayerFormat::Packing::None);
> -               const PixelFormatInfo &info = PixelFormatInfo::info(format);
> -
> -               for (const Size &size : iter.second) {
> -                       double reqAr = static_cast<double>(req.width) / req.height;
> -                       double fmtAr = static_cast<double>(size.width) / size.height;
> -
> -                       /* Score the dimensions for closeness. */
> -                       score = scoreFormat(req.width, size.width);
> -                       score += scoreFormat(req.height, size.height);
> -                       score += penaltyAr * scoreFormat(reqAr, fmtAr);
> -
> -                       /* Add any penalties... this is not an exact science! */
> -                       score += utils::abs_diff(info.bitsPerPixel, bitDepth) * penaltyBitDepth;
> -
> -                       if (score <= bestScore) {
> -                               bestScore = score;
> -                               bestFormat.mbus_code = mbusCode;
> -                               bestFormat.size = size;
> -                       }
> -
> -                       LOG(RPI, Debug) << "Format: " << size
> -                                       << " fmt " << format
> -                                       << " Score: " << score
> -                                       << " (best " << bestScore << ")";
> -               }
> -       }
> -
> -       return bestFormat;
> -}
> -
>  const std::vector<ColorSpace> validColorSpaces = {
>         ColorSpace::Sycc,
>         ColorSpace::Smpte170m,
> @@ -265,6 +208,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>                   [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });
>
>         /* Do any platform specific fixups. */
> +       unsigned int bitDepth = defaultRawBitDepth;
> +       if (!rawStreams.empty()) {
> +               BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);
> +               bitDepth = bayerFormat.bitDepth;
> +       }
> +
> +       V4L2SubdeviceFormat sensorFormat =
> +               data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size
> +                                                        : rawStreams[0].cfg->size,
> +                                     bitDepth);
> +
>         status = data_->platformValidate(rawStreams, outStreams);
>         if (status == Invalid)
>                 return Invalid;
> @@ -275,8 +229,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>                 V4L2DeviceFormat rawFormat;
>
>                 const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> -               unsigned int bitDepth = info.isValid() ? info.bitsPerPixel : defaultRawBitDepth;
> -               V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size, bitDepth);
> +               bitDepth = info.isValid() ? info.bitsPerPixel : defaultRawBitDepth;
> +               sensorFormat = data_->findBestFormat(cfg.size, bitDepth);

I think the above lines can now be removed as sensorFormat is generated above,
and ought to be the same as if it were computed in this loop.  A brief test with
this removed seems to confirm this.

>
>                 BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
>                 rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat, packing);
> @@ -391,7 +345,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
>                 switch (role) {
>                 case StreamRole::Raw:
>                         size = sensorSize;
> -                       sensorFormat = findBestFormat(data->sensorFormats_, size, defaultRawBitDepth);
> +                       sensorFormat = data->findBestFormat(size, defaultRawBitDepth);
>                         pixelFormat = mbusCodeToPixelFormat(sensorFormat.mbus_code,
>                                                             BayerFormat::Packing::CSI2);
>                         ASSERT(pixelFormat.isValid());
> @@ -531,10 +485,11 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>                 bitDepth = bayerFormat.bitDepth;
>         }
>
> -       V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_,
> -                                                         rawStreams.empty() ? ispStreams[0].cfg->size
> -                                                                            : rawStreams[0].cfg->size,
> -                                                         bitDepth);
> +       V4L2SubdeviceFormat sensorFormat =
> +               data->findBestFormat(rawStreams.empty() ? ispStreams[0].cfg->size
> +                                                       : rawStreams[0].cfg->size,
> +                                    bitDepth);
> +
>         /* Apply any cached transform. */
>         const RPiCameraConfiguration *rpiConfig = static_cast<const RPiCameraConfiguration *>(config);
>
> @@ -954,6 +909,63 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)
>         return 0;
>  }
>
> +double CameraData::scoreFormat(double desired, double actual) const
> +{
> +       double score = desired - actual;
> +       /* Smaller desired dimensions are preferred. */
> +       if (score < 0.0)
> +               score = (-score) / 8;
> +       /* Penalise non-exact matches. */
> +       if (actual != desired)
> +               score *= 2;
> +
> +       return score;
> +}

This does not need to be a member function, but I'm not too bothered either way.

Apart from clarification of the sensorFormat bit above,

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

Regards,
Naush

> +
> +V4L2SubdeviceFormat CameraData::findBestFormat(const Size &req, unsigned int bitDepth) const
> +{
> +       double bestScore = std::numeric_limits<double>::max(), score;
> +       V4L2SubdeviceFormat bestFormat;
> +       bestFormat.colorSpace = ColorSpace::Raw;
> +
> +       constexpr float penaltyAr = 1500.0;
> +       constexpr float penaltyBitDepth = 500.0;
> +
> +       /* Calculate the closest/best mode from the user requested size. */
> +       for (const auto &iter : sensorFormats_) {
> +               const unsigned int mbusCode = iter.first;
> +               const PixelFormat format = mbusCodeToPixelFormat(mbusCode,
> +                                                                BayerFormat::Packing::None);
> +               const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +               for (const Size &size : iter.second) {
> +                       double reqAr = static_cast<double>(req.width) / req.height;
> +                       double fmtAr = static_cast<double>(size.width) / size.height;
> +
> +                       /* Score the dimensions for closeness. */
> +                       score = scoreFormat(req.width, size.width);
> +                       score += scoreFormat(req.height, size.height);
> +                       score += penaltyAr * scoreFormat(reqAr, fmtAr);
> +
> +                       /* Add any penalties... this is not an exact science! */
> +                       score += utils::abs_diff(info.bitsPerPixel, bitDepth) * penaltyBitDepth;
> +
> +                       if (score <= bestScore) {
> +                               bestScore = score;
> +                               bestFormat.mbus_code = mbusCode;
> +                               bestFormat.size = size;
> +                       }
> +
> +                       LOG(RPI, Debug) << "Format: " << size
> +                                       << " fmt " << format
> +                                       << " Score: " << score
> +                                       << " (best " << bestScore << ")";
> +               }
> +       }
> +
> +       return bestFormat;
> +}
> +
>  void CameraData::freeBuffers()
>  {
>         if (ipa_) {
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index f648e81054bb..2eda3cd89812 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -81,6 +81,9 @@ public:
>         virtual void platformStart() = 0;
>         virtual void platformStop() = 0;
>
> +       double scoreFormat(double desired, double actual) const;
> +       V4L2SubdeviceFormat findBestFormat(const Size &req, unsigned int bitDepth) const;
> +
>         void freeBuffers();
>         virtual void platformFreeBuffers() = 0;
>
> --
> 2.34.1
>