| Message ID | 20230118085953.7027-11-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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 >
Hi Kieran, Thank you for your feedback! On Fri, 20 Jan 2023 at 11:17, Kieran Bingham < kieran.bingham@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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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 > > >
Quoting Naushir Patuck (2023-01-20 11:55:56) > Hi Kieran, > > Thank you for your feedback! > > On Fri, 20 Jan 2023 at 11:17, Kieran Bingham < > kieran.bingham@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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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. 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@ideasonboard.com> > > 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 > > > > >
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@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@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@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@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";
On Fri, Jan 20, 2023 at 11:17:10AM +0000, Kieran Bingham via libcamera-devel 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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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... On this topic, we could also comment out the default parameters in this file, that's a common practice too. > > } > > } > > 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";
Hi Laurent, On Sun, 22 Jan 2023 at 22:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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. If this is to be removed, it would be nice to have a mechanism to flag frames that are not suitable for display or encoding so the application knows to drop it. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@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@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@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
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 } } 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); if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) { LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";
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@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/data/example.yaml | 6 +++++- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-)