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

[Naushir Patuck]

Hi Kieran,

Thank you for your feedback!

On Fri, 20 Jan 2023 at 11:17, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> 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 think the above code does exactly that right?
We are using the T YamlObject::get(const T &defaultValue) signature here
that returns defaultValue if the key is not present.

Regards,
Naush


> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230120/0cd7874e/attachment.htm>