[libcamera-devel] [PATCH v5 10/12] pipeline: raspberrypi: Add a parameter to disable startup drop frames

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:51)
> Add a new pipeline config parameter "disable_startup_frame_drops" to disable
> any startup drop frames, overriding the IPA request.
> 
> When this parameter is set, it allows the pipeline handler to run with no
> internally allocated Unicam buffers ("min_unicam_buffers").
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/data/example.yaml |  6 +++++-
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp   | 10 +++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> index 001a906af528..421f30e62aa3 100644
> --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> @@ -15,6 +15,10 @@
>                  #
>                  # internal buffer count = max(min_unicam_buffers,
>                  #         min_total_unicam_buffers - external buffer count)
> -                "min_total_unicam_buffers": 4
> +                "min_total_unicam_buffers": 4,
> +
> +                # Override any request from the IPA to drop a number of startup
> +                # frames.
> +                "disable_startup_frame_drops": false

Many configuration files I've seen and used specify options as ...
optional. While the documentation describes the default value...

>          }
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 39f48e0a57fb..3529d331deb6 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -309,6 +309,11 @@ public:
>                  * the Unicam Image stream.
>                  */
>                 unsigned int minTotalUnicamBuffers;
> +               /*
> +                * Override any request from the IPA to drop a number of startup
> +                * frames.
> +                */
> +               bool disableStartupFrameDrops;
>         };
>  
>         Config config_;
> @@ -1053,7 +1058,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>                 data->setSensorControls(startConfig.controls);
>  
>         /* Configure the number of dropped frames required on startup. */
> -       data->dropFrameCount_ = startConfig.dropFrameCount;
> +       data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? 0 : startConfig.dropFrameCount;
>  
>         for (auto const stream : data->streams_)
>                 stream->resetBuffers();
> @@ -1706,6 +1711,7 @@ int RPiCameraData::configurePipeline()
>         config_ = {
>                 .minUnicamBuffers = 2,
>                 .minTotalUnicamBuffers = 4,
> +               .disableStartupFrameDrops = false,
>         };
>  
>         char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> @@ -1739,6 +1745,8 @@ int RPiCameraData::configurePipeline()
>                 phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);
>         config_.minTotalUnicamBuffers =
>                 phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);
> +       config_.disableStartupFrameDrops =
> +               phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);

That leads me to believe the parser should be optional here with
value_or() ? or such? (applies to the other options too?)

I guess it could be value_or(config_.disableStartupFrameDrops); to keep
it the same... Those lines might get long though, but it could be
cleaned up otherwise with a helper/macro maybe:

	CONFIG_OPTION(bool, disableStartupFrameDrops, disable_startup_frame_drops);

as I expect this to grow with more options so keeping it readable may be
helpful.


>         if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {
>                 LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";
> -- 
> 2.25.1
>