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

[Naushir Patuck]

Hi David,

Thank you for your feedback.

On Tue, 1 Nov 2022 at 12:12, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the patch!
>
> On Fri, 14 Oct 2022 at 14:19, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > 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").
> >
> > Add a validation to ensure if "disable_startup_frame_drops" is false, at
> least
> > one internal Unicam buffer is allocated, possibly overriding the
> > "min_unicam_buffers" parameter.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/data/default.json        |  7 +++++--
> >  .../pipeline/raspberrypi/raspberrypi.cpp          | 15 ++++++++++++++-
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json
> b/src/libcamera/pipeline/raspberrypi/data/default.json
> > index a7ea735c87f4..707414bcc5c5 100644
> > --- a/src/libcamera/pipeline/raspberrypi/data/default.json
> > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json
> > @@ -5,7 +5,7 @@
> >          "pipeline_handler":
> >          {
> >                  # The minimum number of internal buffers to be
> allocated for Unicam.
> > -                # This value must be greater than 0, but less than or
> equal to min_total_unicam_buffers.
> > +                # This value must less than or equal to
> min_total_unicam_buffers.
> >                  "min_unicam_buffers": 2,
> >
> >                  # The minimum total (internal + external) buffer count
> used for Unicam.
> > @@ -15,6 +15,9 @@
> >                  "min_total_unicam_buffers": 4,
> >
> >                  # The number of internal buffers used for ISP Output0.
> > -                "num_output0_buffers": 1
> > +                "num_output0_buffers": 1,
> > +
> > +                # Override any request from the IPA to drop a number of
> startup frames.
> > +                "disable_startup_frame_drops": false
> >          }
> >  }
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 2aba0430c02e..135948d82f41 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -300,6 +300,7 @@ public:
> >                 unsigned int minUnicamBuffers;
> >                 unsigned int minTotalUnicamBuffers;
> >                 unsigned int numOutput0Buffers;
> > +               bool disableStartupFrameDrops;
> >         };
> >
> >         Config config_;
> > @@ -1044,7 +1045,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;
>
> Might this warrant an INFO message, just in case folks are confused by
> the changing AGC/AWB when things start? Or maybe that would be
> annoying, for example if an application wants to run like this and
> handles the startup frames for itself?
>

There are already DEBUG level messages for dropping convergence frames,
do you think that is enough?

Regards,
Naush



>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> >
> >         for (auto const stream : data->streams_)
> >                 stream->resetBuffers();
> > @@ -1430,6 +1431,7 @@ int
> PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> >                 .minUnicamBuffers = 2,
> >                 .minTotalUnicamBuffers = 4,
> >                 .numOutput0Buffers = 1,
> > +               .disableStartupFrameDrops = false,
> >         };
> >
> >         char const *configFromEnv =
> utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> > @@ -1464,6 +1466,8 @@ int
> PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> >                 phConfig["min_total_unicam_buffers"].get<unsigned
> int>(config.minTotalUnicamBuffers);
> >         config.numOutput0Buffers =
> >                 phConfig["num_output0_buffers"].get<unsigned
> int>(config.numOutput0Buffers);
> > +       config.disableStartupFrameDrops =
> > +
>  phConfig["disable_startup_frame_drops"].get<bool>(config.disableStartupFrameDrops);
> >
> >         if (config.minTotalUnicamBuffers < config.minUnicamBuffers ||
> config.minTotalUnicamBuffers < 1) {
> >                 LOG(RPI, Error) << "Invalid Unicam buffer configuration
> used!";
> > @@ -1543,6 +1547,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera
> *camera)
> >                          */
> >                         numBuffers =
> std::max<int>(data->config_.minUnicamBuffers,
> >                                                    minBuffers -
> numRawBuffers);
> > +
> > +                       if (numBuffers == 0 && data->dropFrameCount_) {
> > +                               LOG(RPI, Warning)
> > +                                       << "Configured with no Unicam
> buffers,"
> > +                                          " but the IPA requested
> startup frame drops."
> > +                                          " Increasing to one buffer.";
> > +                               numBuffers = 1;
> > +                       }
> > +
> >                         data->numUnicamBuffers = numBuffers;
> >                 } else if (stream == &data->isp_[Isp::Input]) {
> >                         /*
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221129/3d33e08e/attachment.htm>