[libcamera-devel] [PATCH v5 10/12] pipeline: raspberrypi: Add a parameter to disable startup drop frames
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 22 23:56:37 CET 2023
Hello,
On Fri, Jan 20, 2023 at 02:05:57PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck (2023-01-20 11:55:56)
> > On Fri, 20 Jan 2023 at 11:17, Kieran Bingham 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").
At some point I'm sure I'll be tempted to propose dropping the startup
frame drops mechanism altogether. For now, this sounds good.
> > > > 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;
data->dropFrameCount_ = data->config_.disableStartupFrameDrops
? 0 : startConfig.dropFrameCount;
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > 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.
>
> Ahh - yes - I'm completely blind! Of course it is!
>
> Sorry for the distraction.
>
> Aha, which also means I should add this:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > > 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";
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list