[libcamera-devel,v1,3/3] libcamera: rpi: pipeline_base: Cache sensor format

18814 diff mbox series

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

The format to be applied on the sensor is selected by two criteria: the
desired output size and the bit depth. As the selection depends on the
presence of a RAW stream and the streams configuration is handled in
validate() there is no need to re-compute the format in configure().

Centralize the computation of the sensor format in validate() and remove
it from configure().

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 46 +++++++------------
 .../pipeline/rpi/common/pipeline_base.h       |  2 +
 2 files changed, 19 insertions(+), 29 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 format to be applied on the sensor is selected by two criteria: the
> desired output size and the bit depth. As the selection depends on the
> presence of a RAW stream and the streams configuration is handled in
> validate() there is no need to re-compute the format in configure().
>
> Centralize the computation of the sensor format in validate() and remove
> it from configure().
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 46 +++++++------------
>  .../pipeline/rpi/common/pipeline_base.h       |  2 +
>  2 files changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 725c1db0d527..d96277a2794e 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -214,10 +214,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>                 bitDepth = bayerFormat.bitDepth;
>         }
>
> -       V4L2SubdeviceFormat sensorFormat =
> -               data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size
> -                                                        : rawStreams[0].cfg->size,
> -                                     bitDepth);
> +       sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size
> +                                                                : rawStreams[0].cfg->size,
> +                                             bitDepth);
>
>         status = data_->platformValidate(rawStreams, outStreams);
>         if (status == Invalid)
> @@ -230,10 +229,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>
>                 const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>                 bitDepth = info.isValid() ? info.bitsPerPixel : defaultRawBitDepth;
> -               sensorFormat = data_->findBestFormat(cfg.size, bitDepth);
> +               V4L2SubdeviceFormat subdevFormat = data_->findBestFormat(cfg.size, bitDepth);
>
>                 BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
> -               rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat, packing);
> +               rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, subdevFormat, packing);

As per the comments on the previous patch, if we agree that subdevFormat can be
removed, we can replace it with sensorFormat_ in the above call to
PipelineHandlerBase::toV4L2DeviceFormat().

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

Regards,
Naush

>
>                 int ret = raw.dev->tryFormat(&rawFormat);
>                 if (ret)
> @@ -453,8 +452,6 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>                 stream->clearFlags(StreamFlag::External);
>
>         std::vector<CameraData::StreamParams> rawStreams, ispStreams;
> -       std::optional<BayerFormat::Packing> packing;
> -       unsigned int bitDepth = defaultRawBitDepth;
>
>         for (unsigned i = 0; i < config->size(); i++) {
>                 StreamConfiguration *cfg = &config->at(i);
> @@ -472,32 +469,23 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>         std::sort(ispStreams.begin(), ispStreams.end(),
>                   [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });
>
> -       /*
> -        * Calculate the best sensor mode we can use based on the user's request,
> -        * and apply it to the sensor with the cached tranform, if any.
> -        *
> -        * If we have been given a RAW stream, use that size for setting up the sensor.
> -        */
> -       if (!rawStreams.empty()) {
> -               BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);
> -               /* Replace the user requested packing/bit-depth. */
> -               packing = bayerFormat.packing;
> -               bitDepth = bayerFormat.bitDepth;
> -       }
> -
> -       V4L2SubdeviceFormat sensorFormat =
> -               data->findBestFormat(rawStreams.empty() ? ispStreams[0].cfg->size
> -                                                       : rawStreams[0].cfg->size,
> -                                    bitDepth);
> +       /* Apply the format on the sensor with any cached transform. */
> +       const RPiCameraConfiguration *rpiConfig =
> +                               static_cast<const RPiCameraConfiguration *>(config);
> +       V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;
>
> -       /* Apply any cached transform. */
> -       const RPiCameraConfiguration *rpiConfig = static_cast<const RPiCameraConfiguration *>(config);
> -
> -       /* Then apply the format on the sensor. */
>         ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);
>         if (ret)
>                 return ret;
>
> +       /* Use the user requested packing/bit-depth. */
> +       std::optional<BayerFormat::Packing> packing;
> +       if (!rawStreams.empty()) {
> +               BayerFormat bayerFormat =
> +                       BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);
> +               packing = bayerFormat.packing;
> +       }
> +
>         /*
>          * Platform specific internal stream configuration. This also assigns
>          * external streams which get configured below.
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 2eda3cd89812..a139c98a5a2b 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -262,6 +262,8 @@ public:
>
>         /* Cache the combinedTransform_ that will be applied to the sensor */
>         Transform combinedTransform_;
> +       /* The sensor format computed in validate() */
> +       V4L2SubdeviceFormat sensorFormat_;
>
>  private:
>         const CameraData *data_;
> --
> 2.34.1
>