| Message ID | 20250624122830.726987-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Tue, Jun 24, 2025 at 02:28:29PM +0200, Barnabás Pőcze wrote: > Save the result of `Camera::generateConfiguration()` in a local variable, > and only commit it to the member variable `config_` if it passes all checks. > This removes the need for explicit `config_.reset()` calls in error paths, > hence making things simpler. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> I think that you can push this patch > --- > src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 2a3fa3b68..d076e964a 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > { > assert(!roles.empty()); > > - config_ = camera_->generateConfiguration(roles); > - if (!config_) > + auto config = camera_->generateConfiguration(roles); > + if (!config) > GTEST_SKIP() << "Roles not supported by camera"; > > - ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration"; > + ASSERT_EQ(config->size(), roles.size()) << "Unexpected number of streams in configuration"; > > /* > * Set the buffers count to the largest value across all streams. > * \todo: Should all streams from a Camera have the same buffer count ? > */ > auto largest = > - std::max_element(config_->begin(), config_->end(), > + std::max_element(config->begin(), config->end(), > [](const StreamConfiguration &l, const StreamConfiguration &r) > { return l.bufferCount < r.bufferCount; }); > > - assert(largest != config_->end()); > + assert(largest != config->end()); > > - for (auto &cfg : *config_) > + for (auto &cfg : *config) > cfg.bufferCount = largest->bufferCount; > > - if (config_->validate() != CameraConfiguration::Valid) { > - config_.reset(); > - FAIL() << "Configuration not valid"; > - } > + ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > + ASSERT_EQ(camera_->configure(config.get()), 0); > > - if (camera_->configure(config_.get())) { > - config_.reset(); > - FAIL() << "Failed to configure camera"; > - } > + config_ = std::move(config); > } > > void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit) > -- > 2.50.0 >
Hi Barnabás, Thank you for the patch. Quoting Barnabás Pőcze (2025-06-24 14:28:29) > Save the result of `Camera::generateConfiguration()` in a local variable, > and only commit it to the member variable `config_` if it passes all checks. > This removes the need for explicit `config_.reset()` calls in error paths, > hence making things simpler. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 2a3fa3b68..d076e964a 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > { > assert(!roles.empty()); > > - config_ = camera_->generateConfiguration(roles); > - if (!config_) > + auto config = camera_->generateConfiguration(roles); > + if (!config) > GTEST_SKIP() << "Roles not supported by camera"; > > - ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration"; > + ASSERT_EQ(config->size(), roles.size()) << "Unexpected number of streams in configuration"; > > /* > * Set the buffers count to the largest value across all streams. > * \todo: Should all streams from a Camera have the same buffer count ? > */ > auto largest = > - std::max_element(config_->begin(), config_->end(), > + std::max_element(config->begin(), config->end(), > [](const StreamConfiguration &l, const StreamConfiguration &r) > { return l.bufferCount < r.bufferCount; }); > > - assert(largest != config_->end()); > + assert(largest != config->end()); > > - for (auto &cfg : *config_) > + for (auto &cfg : *config) > cfg.bufferCount = largest->bufferCount; > > - if (config_->validate() != CameraConfiguration::Valid) { > - config_.reset(); > - FAIL() << "Configuration not valid"; > - } > + ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > + ASSERT_EQ(camera_->configure(config.get()), 0); Actually I don't like this as it moves logic into an ASSERT. My assumptions was (maybe others see that differently) that ASSERTs could (in theory) be defined as noop for release builds and therefore must not contain anything that needs to be called. Is my assumption wrong? Best regards, Stefan > > - if (camera_->configure(config_.get())) { > - config_.reset(); > - FAIL() << "Failed to configure camera"; > - } > + config_ = std::move(config); > } > > void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit) > -- > 2.50.0 >
Hi 2025. 11. 03. 13:53 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Thank you for the patch. > > Quoting Barnabás Pőcze (2025-06-24 14:28:29) >> Save the result of `Camera::generateConfiguration()` in a local variable, >> and only commit it to the member variable `config_` if it passes all checks. >> This removes the need for explicit `config_.reset()` calls in error paths, >> hence making things simpler. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++------------- >> 1 file changed, 9 insertions(+), 14 deletions(-) >> >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp >> index 2a3fa3b68..d076e964a 100644 >> --- a/src/apps/lc-compliance/helpers/capture.cpp >> +++ b/src/apps/lc-compliance/helpers/capture.cpp >> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >> { >> assert(!roles.empty()); >> >> - config_ = camera_->generateConfiguration(roles); >> - if (!config_) >> + auto config = camera_->generateConfiguration(roles); >> + if (!config) >> GTEST_SKIP() << "Roles not supported by camera"; >> >> - ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration"; >> + ASSERT_EQ(config->size(), roles.size()) << "Unexpected number of streams in configuration"; >> >> /* >> * Set the buffers count to the largest value across all streams. >> * \todo: Should all streams from a Camera have the same buffer count ? >> */ >> auto largest = >> - std::max_element(config_->begin(), config_->end(), >> + std::max_element(config->begin(), config->end(), >> [](const StreamConfiguration &l, const StreamConfiguration &r) >> { return l.bufferCount < r.bufferCount; }); >> >> - assert(largest != config_->end()); >> + assert(largest != config->end()); >> >> - for (auto &cfg : *config_) >> + for (auto &cfg : *config) >> cfg.bufferCount = largest->bufferCount; >> >> - if (config_->validate() != CameraConfiguration::Valid) { >> - config_.reset(); >> - FAIL() << "Configuration not valid"; >> - } >> + ASSERT_EQ(config->validate(), CameraConfiguration::Valid); >> + ASSERT_EQ(camera_->configure(config.get()), 0); > > Actually I don't like this as it moves logic into an ASSERT. My > assumptions was (maybe others see that differently) that ASSERTs could > (in theory) be defined as noop for release builds and therefore must not > contain anything that needs to be called. Is my assumption wrong? As far as I am aware gtest never defines its ASSERT/EXPECT macros as no-ops. In fact, I would be very surprised if it did. For `assert()` from `<cassert>`, etc. I agree completely. > > Best regards, > Stefan > >> >> - if (camera_->configure(config_.get())) { >> - config_.reset(); >> - FAIL() << "Failed to configure camera"; >> - } >> + config_ = std::move(config); >> } >> >> void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit) >> -- >> 2.50.0 >>
Hi, Quoting Barnabás Pőcze (2025-11-03 14:00:57) > Hi > > 2025. 11. 03. 13:53 keltezéssel, Stefan Klug írta: > > Hi Barnabás, > > > > Thank you for the patch. > > > > Quoting Barnabás Pőcze (2025-06-24 14:28:29) > >> Save the result of `Camera::generateConfiguration()` in a local variable, > >> and only commit it to the member variable `config_` if it passes all checks. > >> This removes the need for explicit `config_.reset()` calls in error paths, > >> hence making things simpler. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++------------- > >> 1 file changed, 9 insertions(+), 14 deletions(-) > >> > >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > >> index 2a3fa3b68..d076e964a 100644 > >> --- a/src/apps/lc-compliance/helpers/capture.cpp > >> +++ b/src/apps/lc-compliance/helpers/capture.cpp > >> @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > >> { > >> assert(!roles.empty()); > >> > >> - config_ = camera_->generateConfiguration(roles); > >> - if (!config_) > >> + auto config = camera_->generateConfiguration(roles); > >> + if (!config) > >> GTEST_SKIP() << "Roles not supported by camera"; > >> > >> - ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration"; > >> + ASSERT_EQ(config->size(), roles.size()) << "Unexpected number of streams in configuration"; > >> > >> /* > >> * Set the buffers count to the largest value across all streams. > >> * \todo: Should all streams from a Camera have the same buffer count ? > >> */ > >> auto largest = > >> - std::max_element(config_->begin(), config_->end(), > >> + std::max_element(config->begin(), config->end(), > >> [](const StreamConfiguration &l, const StreamConfiguration &r) > >> { return l.bufferCount < r.bufferCount; }); > >> > >> - assert(largest != config_->end()); > >> + assert(largest != config->end()); > >> > >> - for (auto &cfg : *config_) > >> + for (auto &cfg : *config) > >> cfg.bufferCount = largest->bufferCount; > >> > >> - if (config_->validate() != CameraConfiguration::Valid) { > >> - config_.reset(); > >> - FAIL() << "Configuration not valid"; > >> - } > >> + ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > >> + ASSERT_EQ(camera_->configure(config.get()), 0); > > > > Actually I don't like this as it moves logic into an ASSERT. My > > assumptions was (maybe others see that differently) that ASSERTs could > > (in theory) be defined as noop for release builds and therefore must not > > contain anything that needs to be called. Is my assumption wrong? > > As far as I am aware gtest never defines its ASSERT/EXPECT macros as no-ops. > In fact, I would be very surprised if it did. > > For `assert()` from `<cassert>`, etc. I agree completely. Oops, I completely missed that this is gtest and my brain just jumped on upper case letters. In that case Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > > > > > > Best regards, > > Stefan > > > >> > >> - if (camera_->configure(config_.get())) { > >> - config_.reset(); > >> - FAIL() << "Failed to configure camera"; > >> - } > >> + config_ = std::move(config); > >> } > >> > >> void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit) > >> -- > >> 2.50.0 > >> >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index 2a3fa3b68..d076e964a 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -27,35 +27,30 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) { assert(!roles.empty()); - config_ = camera_->generateConfiguration(roles); - if (!config_) + auto config = camera_->generateConfiguration(roles); + if (!config) GTEST_SKIP() << "Roles not supported by camera"; - ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration"; + ASSERT_EQ(config->size(), roles.size()) << "Unexpected number of streams in configuration"; /* * Set the buffers count to the largest value across all streams. * \todo: Should all streams from a Camera have the same buffer count ? */ auto largest = - std::max_element(config_->begin(), config_->end(), + std::max_element(config->begin(), config->end(), [](const StreamConfiguration &l, const StreamConfiguration &r) { return l.bufferCount < r.bufferCount; }); - assert(largest != config_->end()); + assert(largest != config->end()); - for (auto &cfg : *config_) + for (auto &cfg : *config) cfg.bufferCount = largest->bufferCount; - if (config_->validate() != CameraConfiguration::Valid) { - config_.reset(); - FAIL() << "Configuration not valid"; - } + ASSERT_EQ(config->validate(), CameraConfiguration::Valid); + ASSERT_EQ(camera_->configure(config.get()), 0); - if (camera_->configure(config_.get())) { - config_.reset(); - FAIL() << "Failed to configure camera"; - } + config_ = std::move(config); } void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)
Save the result of `Camera::generateConfiguration()` in a local variable, and only commit it to the member variable `config_` if it passes all checks. This removes the need for explicit `config_.reset()` calls in error paths, hence making things simpler. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/lc-compliance/helpers/capture.cpp | 23 +++++++++------------- 1 file changed, 9 insertions(+), 14 deletions(-)