Message ID | 20200724134320.637696-2-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, I can see some definite value in this option! On 24/07/2020 14:43, Niklas Söderlund wrote: > Add an '--strict-formats' option which fails the camera configuration s/an/a/ > step if the format is adjusted, > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 17 ++++++++++++++++- > src/cam/main.h | 1 + > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 2512fe9da782165b..a223563ad37e9fe5 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -49,12 +49,15 @@ private: > std::shared_ptr<Camera> camera_; > std::unique_ptr<libcamera::CameraConfiguration> config_; > EventLoop *loop_; > + > + bool strictFormats_; > }; > > CamApp *CamApp::app_ = nullptr; > > CamApp::CamApp() > - : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr) > + : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr), > + strictFormats_(false) > { > CamApp::app_ = this; > } > @@ -77,6 +80,9 @@ int CamApp::init(int argc, char **argv) > if (ret < 0) > return ret; > > + if (options_.isSet(OptStrictFormats)) > + strictFormats_ = true; > + > cm_ = new CameraManager(); > > ret = cm_->start(); > @@ -179,6 +185,9 @@ int CamApp::parseOptions(int argc, char *argv[]) > "list-controls"); > parser.addOption(OptListProperties, OptionNone, "List cameras properties", > "list-properties"); > + parser.addOption(OptStrictFormats, OptionNone, > + "Do not allow requested stream format(s) to be adjusted", > + "strict-formats"); > > options_ = parser.parse(argc, argv); > if (!options_.valid()) > @@ -214,6 +223,12 @@ int CamApp::prepareConfig() > case CameraConfiguration::Valid: > break; > case CameraConfiguration::Adjusted: > + if (strictFormats_) { > + std::cout << "Adjustng camera configuration not allowed" s/Adjustng/Adjusting/ Could/should we print here what the stream configurations were adjusted /to/ to help debugging? Otherwise, > + << std::endl; > + config_.reset(); Is this (reset) essential? Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + return -EINVAL; > + } > std::cout << "Camera configuration adjusted" << std::endl; > break; > case CameraConfiguration::Invalid: > diff --git a/src/cam/main.h b/src/cam/main.h > index 4a130d8dd2906ad4..6f95add31a6341cf 100644 > --- a/src/cam/main.h > +++ b/src/cam/main.h > @@ -17,6 +17,7 @@ enum { > OptListProperties = 'p', > OptStream = 's', > OptListControls = 256, > + OptStrictFormats = 257, > }; > > #endif /* __CAM_MAIN_H__ */ >
Hi Kieran, Thanks for your feedback. On 2020-07-24 14:57:28 +0100, Kieran Bingham wrote: > Hi Niklas, > > I can see some definite value in this option! > > On 24/07/2020 14:43, Niklas Söderlund wrote: > > Add an '--strict-formats' option which fails the camera configuration > > s/an/a/ > > > step if the format is adjusted, > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 17 ++++++++++++++++- > > src/cam/main.h | 1 + > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index 2512fe9da782165b..a223563ad37e9fe5 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -49,12 +49,15 @@ private: > > std::shared_ptr<Camera> camera_; > > std::unique_ptr<libcamera::CameraConfiguration> config_; > > EventLoop *loop_; > > + > > + bool strictFormats_; > > }; > > > > CamApp *CamApp::app_ = nullptr; > > > > CamApp::CamApp() > > - : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr) > > + : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr), > > + strictFormats_(false) > > { > > CamApp::app_ = this; > > } > > @@ -77,6 +80,9 @@ int CamApp::init(int argc, char **argv) > > if (ret < 0) > > return ret; > > > > + if (options_.isSet(OptStrictFormats)) > > + strictFormats_ = true; > > + > > cm_ = new CameraManager(); > > > > ret = cm_->start(); > > @@ -179,6 +185,9 @@ int CamApp::parseOptions(int argc, char *argv[]) > > "list-controls"); > > parser.addOption(OptListProperties, OptionNone, "List cameras properties", > > "list-properties"); > > + parser.addOption(OptStrictFormats, OptionNone, > > + "Do not allow requested stream format(s) to be adjusted", > > + "strict-formats"); > > > > options_ = parser.parse(argc, argv); > > if (!options_.valid()) > > @@ -214,6 +223,12 @@ int CamApp::prepareConfig() > > case CameraConfiguration::Valid: > > break; > > case CameraConfiguration::Adjusted: > > + if (strictFormats_) { > > + std::cout << "Adjustng camera configuration not allowed" > > s/Adjustng/Adjusting/ > > > Could/should we print here what the stream configurations were adjusted > /to/ to help debugging? We could, but there are other places we could do this and don't. I think we should add this to core at the Debug level so no matter the application we could get this information out. I know you have added something like this in your HAL JPEG work so I think we should address this at a common point. > > > Otherwise, > > > + << std::endl; > > + config_.reset(); > > Is this (reset) essential? Yes, we need to reset the config_ variable if we don't want to proceed. We do the same here for CameraConfiguration::Invalid (that is not shown in the context for this patch). > > Otherwise, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks! > > > > > + return -EINVAL; > > + } > > std::cout << "Camera configuration adjusted" << std::endl; > > break; > > case CameraConfiguration::Invalid: > > diff --git a/src/cam/main.h b/src/cam/main.h > > index 4a130d8dd2906ad4..6f95add31a6341cf 100644 > > --- a/src/cam/main.h > > +++ b/src/cam/main.h > > @@ -17,6 +17,7 @@ enum { > > OptListProperties = 'p', > > OptStream = 's', > > OptListControls = 256, > > + OptStrictFormats = 257, > > }; > > > > #endif /* __CAM_MAIN_H__ */ > > > > -- > Regards > -- > Kieran
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index 2512fe9da782165b..a223563ad37e9fe5 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -49,12 +49,15 @@ private: std::shared_ptr<Camera> camera_; std::unique_ptr<libcamera::CameraConfiguration> config_; EventLoop *loop_; + + bool strictFormats_; }; CamApp *CamApp::app_ = nullptr; CamApp::CamApp() - : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr) + : cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr), + strictFormats_(false) { CamApp::app_ = this; } @@ -77,6 +80,9 @@ int CamApp::init(int argc, char **argv) if (ret < 0) return ret; + if (options_.isSet(OptStrictFormats)) + strictFormats_ = true; + cm_ = new CameraManager(); ret = cm_->start(); @@ -179,6 +185,9 @@ int CamApp::parseOptions(int argc, char *argv[]) "list-controls"); parser.addOption(OptListProperties, OptionNone, "List cameras properties", "list-properties"); + parser.addOption(OptStrictFormats, OptionNone, + "Do not allow requested stream format(s) to be adjusted", + "strict-formats"); options_ = parser.parse(argc, argv); if (!options_.valid()) @@ -214,6 +223,12 @@ int CamApp::prepareConfig() case CameraConfiguration::Valid: break; case CameraConfiguration::Adjusted: + if (strictFormats_) { + std::cout << "Adjustng camera configuration not allowed" + << std::endl; + config_.reset(); + return -EINVAL; + } std::cout << "Camera configuration adjusted" << std::endl; break; case CameraConfiguration::Invalid: diff --git a/src/cam/main.h b/src/cam/main.h index 4a130d8dd2906ad4..6f95add31a6341cf 100644 --- a/src/cam/main.h +++ b/src/cam/main.h @@ -17,6 +17,7 @@ enum { OptListProperties = 'p', OptStream = 's', OptListControls = 256, + OptStrictFormats = 257, }; #endif /* __CAM_MAIN_H__ */
Add an '--strict-formats' option which fails the camera configuration step if the format is adjusted, Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 17 ++++++++++++++++- src/cam/main.h | 1 + 2 files changed, 17 insertions(+), 1 deletion(-)