<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 20 Jan 2023 at 11:17, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.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">Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:51)<br>
> Add a new pipeline config parameter "disable_startup_frame_drops" to disable<br>
> any startup drop frames, overriding the IPA request.<br>
> <br>
> When this parameter is set, it allows the pipeline handler to run with no<br>
> internally allocated Unicam buffers ("min_unicam_buffers").<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  src/libcamera/pipeline/raspberrypi/data/example.yaml |  6 +++++-<br>
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp   | 10 +++++++++-<br>
>  2 files changed, 14 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> index 001a906af528..421f30e62aa3 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> @@ -15,6 +15,10 @@<br>
>                  #<br>
>                  # internal buffer count = max(min_unicam_buffers,<br>
>                  #         min_total_unicam_buffers - external buffer count)<br>
> -                "min_total_unicam_buffers": 4<br>
> +                "min_total_unicam_buffers": 4,<br>
> +<br>
> +                # Override any request from the IPA to drop a number of startup<br>
> +                # frames.<br>
> +                "disable_startup_frame_drops": false<br>
<br>
Many configuration files I've seen and used specify options as ...<br>
optional. While the documentation describes the default value...<br>
<br>
>          }<br>
>  }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 39f48e0a57fb..3529d331deb6 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -309,6 +309,11 @@ public:<br>
>                  * the Unicam Image stream.<br>
>                  */<br>
>                 unsigned int minTotalUnicamBuffers;<br>
> +               /*<br>
> +                * Override any request from the IPA to drop a number of startup<br>
> +                * frames.<br>
> +                */<br>
> +               bool disableStartupFrameDrops;<br>
>         };<br>
>  <br>
>         Config config_;<br>
> @@ -1053,7 +1058,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
>                 data->setSensorControls(startConfig.controls);<br>
>  <br>
>         /* Configure the number of dropped frames required on startup. */<br>
> -       data->dropFrameCount_ = startConfig.dropFrameCount;<br>
> +       data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? 0 : startConfig.dropFrameCount;<br>
>  <br>
>         for (auto const stream : data->streams_)<br>
>                 stream->resetBuffers();<br>
> @@ -1706,6 +1711,7 @@ int RPiCameraData::configurePipeline()<br>
>         config_ = {<br>
>                 .minUnicamBuffers = 2,<br>
>                 .minTotalUnicamBuffers = 4,<br>
> +               .disableStartupFrameDrops = false,<br>
>         };<br>
>  <br>
>         char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> @@ -1739,6 +1745,8 @@ int RPiCameraData::configurePipeline()<br>
>                 phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);<br>
>         config_.minTotalUnicamBuffers =<br>
>                 phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);<br>
> +       config_.disableStartupFrameDrops =<br>
> +               phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);<br>
<br>
That leads me to believe the parser should be optional here with<br>
value_or() ? or such? (applies to the other options too?)<br></blockquote><div><br></div><div>I think the above code does exactly that right?</div><div>We are using the T YamlObject::get(const T &defaultValue) signature here that returns defaultValue if the key is not present.</div><div> </div><div>Regards,</div><div>Naush</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>
I guess it could be value_or(config_.disableStartupFrameDrops); to keep<br>
it the same... Those lines might get long though, but it could be<br>
cleaned up otherwise with a helper/macro maybe:<br>
<br>
        CONFIG_OPTION(bool, disableStartupFrameDrops, disable_startup_frame_drops);<br>
<br>
as I expect this to grow with more options so keeping it readable may be<br>
helpful.<br>
<br>
<br>
>         if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {<br>
>                 LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>