| Message ID | 20200519032505.17307-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 19/05/2020 04:24, Laurent Pinchart wrote: > The stream roles will be needed in the Capture class to verify the > configuration. Store they in the CamApp class and pass them to the s/they/them/ Other than that, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Capture class constructor. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/capture.cpp | 5 +++-- > src/cam/capture.h | 4 +++- > src/cam/main.cpp | 9 +++++---- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 55fa2dabcee9..b7e06bcc9463 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -16,8 +16,9 @@ > > using namespace libcamera; > > -Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) > - : camera_(camera), config_(config), writer_(nullptr) > +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config, > + const StreamRoles &roles) > + : camera_(camera), config_(config), roles_(roles), writer_(nullptr) > { > } > > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 9bca5661070e..c0e697b831fb 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -24,7 +24,8 @@ class Capture > { > public: > Capture(std::shared_ptr<libcamera::Camera> camera, > - libcamera::CameraConfiguration *config); > + libcamera::CameraConfiguration *config, > + const libcamera::StreamRoles &roles); > > int run(EventLoop *loop, const OptionsParser::Options &options); > private: > @@ -35,6 +36,7 @@ private: > > std::shared_ptr<libcamera::Camera> camera_; > libcamera::CameraConfiguration *config_; > + libcamera::StreamRoles roles_; > > std::map<libcamera::Stream *, std::string> streamName_; > BufferWriter *writer_; > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 2512fe9da782..cdd29d500202 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -47,6 +47,7 @@ private: > OptionsParser::Options options_; > CameraManager *cm_; > std::shared_ptr<Camera> camera_; > + StreamRoles roles_; > std::unique_ptr<libcamera::CameraConfiguration> config_; > EventLoop *loop_; > }; > @@ -194,10 +195,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > int CamApp::prepareConfig() > { > - StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]); > + roles_ = StreamKeyValueParser::roles(options_[OptStream]); > > - config_ = camera_->generateConfiguration(roles); > - if (!config_ || config_->size() != roles.size()) { > + config_ = camera_->generateConfiguration(roles_); > + if (!config_ || config_->size() != roles_.size()) { > std::cerr << "Failed to get default stream configuration" > << std::endl; > return -EINVAL; > @@ -326,7 +327,7 @@ int CamApp::run() > } > > if (options_.isSet(OptCapture)) { > - Capture capture(camera_, config_.get()); > + Capture capture(camera_, config_.get(), roles_); > return capture.run(loop_, options_); > } > >
Hi Laurent, Thanks for your work. On 2020-05-19 06:24:58 +0300, Laurent Pinchart wrote: > The stream roles will be needed in the Capture class to verify the > configuration. Store they in the CamApp class and pass them to the > Capture class constructor. The patch itself is fine but I wonder if this is the right way. We have previously endeavored to limit the spread of the roles past the generateConfiguration() as they somewhat lose there meaning after that, right? Even if the user requested a viewfinder and video stream they are only hints to the pipeline hander is "free" to return any type of camera configuration, even one with only 1 stream even if more where requested in the role hints. For this reason I wonder if instead the check for the combination of the new option -D and that the stream role hint should be set to viewfinder should be moved and be validated right after we have parsed the command line options? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/capture.cpp | 5 +++-- > src/cam/capture.h | 4 +++- > src/cam/main.cpp | 9 +++++---- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 55fa2dabcee9..b7e06bcc9463 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -16,8 +16,9 @@ > > using namespace libcamera; > > -Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) > - : camera_(camera), config_(config), writer_(nullptr) > +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config, > + const StreamRoles &roles) > + : camera_(camera), config_(config), roles_(roles), writer_(nullptr) > { > } > > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 9bca5661070e..c0e697b831fb 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -24,7 +24,8 @@ class Capture > { > public: > Capture(std::shared_ptr<libcamera::Camera> camera, > - libcamera::CameraConfiguration *config); > + libcamera::CameraConfiguration *config, > + const libcamera::StreamRoles &roles); > > int run(EventLoop *loop, const OptionsParser::Options &options); > private: > @@ -35,6 +36,7 @@ private: > > std::shared_ptr<libcamera::Camera> camera_; > libcamera::CameraConfiguration *config_; > + libcamera::StreamRoles roles_; > > std::map<libcamera::Stream *, std::string> streamName_; > BufferWriter *writer_; > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index 2512fe9da782..cdd29d500202 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -47,6 +47,7 @@ private: > OptionsParser::Options options_; > CameraManager *cm_; > std::shared_ptr<Camera> camera_; > + StreamRoles roles_; > std::unique_ptr<libcamera::CameraConfiguration> config_; > EventLoop *loop_; > }; > @@ -194,10 +195,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > int CamApp::prepareConfig() > { > - StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]); > + roles_ = StreamKeyValueParser::roles(options_[OptStream]); > > - config_ = camera_->generateConfiguration(roles); > - if (!config_ || config_->size() != roles.size()) { > + config_ = camera_->generateConfiguration(roles_); > + if (!config_ || config_->size() != roles_.size()) { > std::cerr << "Failed to get default stream configuration" > << std::endl; > return -EINVAL; > @@ -326,7 +327,7 @@ int CamApp::run() > } > > if (options_.isSet(OptCapture)) { > - Capture capture(camera_, config_.get()); > + Capture capture(camera_, config_.get(), roles_); > return capture.run(loop_, options_); > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, May 19, 2020 at 04:27:50PM +0200, Niklas Söderlund wrote: > On 2020-05-19 06:24:58 +0300, Laurent Pinchart wrote: > > The stream roles will be needed in the Capture class to verify the > > configuration. Store they in the CamApp class and pass them to the > > Capture class constructor. > > The patch itself is fine but I wonder if this is the right way. We have > previously endeavored to limit the spread of the roles past the > generateConfiguration() as they somewhat lose there meaning after that, > right? > > Even if the user requested a viewfinder and video stream they are only > hints to the pipeline hander is "free" to return any type of camera > configuration, even one with only 1 stream even if more where requested > in the role hints. > > For this reason I wonder if instead the check for the combination of the > new option -D and that the stream role hint should be set to viewfinder > should be moved and be validated right after we have parsed the command > line options? I'll do so. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/capture.cpp | 5 +++-- > > src/cam/capture.h | 4 +++- > > src/cam/main.cpp | 9 +++++---- > > 3 files changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > index 55fa2dabcee9..b7e06bcc9463 100644 > > --- a/src/cam/capture.cpp > > +++ b/src/cam/capture.cpp > > @@ -16,8 +16,9 @@ > > > > using namespace libcamera; > > > > -Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) > > - : camera_(camera), config_(config), writer_(nullptr) > > +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config, > > + const StreamRoles &roles) > > + : camera_(camera), config_(config), roles_(roles), writer_(nullptr) > > { > > } > > > > diff --git a/src/cam/capture.h b/src/cam/capture.h > > index 9bca5661070e..c0e697b831fb 100644 > > --- a/src/cam/capture.h > > +++ b/src/cam/capture.h > > @@ -24,7 +24,8 @@ class Capture > > { > > public: > > Capture(std::shared_ptr<libcamera::Camera> camera, > > - libcamera::CameraConfiguration *config); > > + libcamera::CameraConfiguration *config, > > + const libcamera::StreamRoles &roles); > > > > int run(EventLoop *loop, const OptionsParser::Options &options); > > private: > > @@ -35,6 +36,7 @@ private: > > > > std::shared_ptr<libcamera::Camera> camera_; > > libcamera::CameraConfiguration *config_; > > + libcamera::StreamRoles roles_; > > > > std::map<libcamera::Stream *, std::string> streamName_; > > BufferWriter *writer_; > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index 2512fe9da782..cdd29d500202 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -47,6 +47,7 @@ private: > > OptionsParser::Options options_; > > CameraManager *cm_; > > std::shared_ptr<Camera> camera_; > > + StreamRoles roles_; > > std::unique_ptr<libcamera::CameraConfiguration> config_; > > EventLoop *loop_; > > }; > > @@ -194,10 +195,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > > int CamApp::prepareConfig() > > { > > - StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]); > > + roles_ = StreamKeyValueParser::roles(options_[OptStream]); > > > > - config_ = camera_->generateConfiguration(roles); > > - if (!config_ || config_->size() != roles.size()) { > > + config_ = camera_->generateConfiguration(roles_); > > + if (!config_ || config_->size() != roles_.size()) { > > std::cerr << "Failed to get default stream configuration" > > << std::endl; > > return -EINVAL; > > @@ -326,7 +327,7 @@ int CamApp::run() > > } > > > > if (options_.isSet(OptCapture)) { > > - Capture capture(camera_, config_.get()); > > + Capture capture(camera_, config_.get(), roles_); > > return capture.run(loop_, options_); > > } > >
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index 55fa2dabcee9..b7e06bcc9463 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -16,8 +16,9 @@ using namespace libcamera; -Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) - : camera_(camera), config_(config), writer_(nullptr) +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config, + const StreamRoles &roles) + : camera_(camera), config_(config), roles_(roles), writer_(nullptr) { } diff --git a/src/cam/capture.h b/src/cam/capture.h index 9bca5661070e..c0e697b831fb 100644 --- a/src/cam/capture.h +++ b/src/cam/capture.h @@ -24,7 +24,8 @@ class Capture { public: Capture(std::shared_ptr<libcamera::Camera> camera, - libcamera::CameraConfiguration *config); + libcamera::CameraConfiguration *config, + const libcamera::StreamRoles &roles); int run(EventLoop *loop, const OptionsParser::Options &options); private: @@ -35,6 +36,7 @@ private: std::shared_ptr<libcamera::Camera> camera_; libcamera::CameraConfiguration *config_; + libcamera::StreamRoles roles_; std::map<libcamera::Stream *, std::string> streamName_; BufferWriter *writer_; diff --git a/src/cam/main.cpp b/src/cam/main.cpp index 2512fe9da782..cdd29d500202 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -47,6 +47,7 @@ private: OptionsParser::Options options_; CameraManager *cm_; std::shared_ptr<Camera> camera_; + StreamRoles roles_; std::unique_ptr<libcamera::CameraConfiguration> config_; EventLoop *loop_; }; @@ -194,10 +195,10 @@ int CamApp::parseOptions(int argc, char *argv[]) int CamApp::prepareConfig() { - StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]); + roles_ = StreamKeyValueParser::roles(options_[OptStream]); - config_ = camera_->generateConfiguration(roles); - if (!config_ || config_->size() != roles.size()) { + config_ = camera_->generateConfiguration(roles_); + if (!config_ || config_->size() != roles_.size()) { std::cerr << "Failed to get default stream configuration" << std::endl; return -EINVAL; @@ -326,7 +327,7 @@ int CamApp::run() } if (options_.isSet(OptCapture)) { - Capture capture(camera_, config_.get()); + Capture capture(camera_, config_.get(), roles_); return capture.run(loop_, options_); }
The stream roles will be needed in the Capture class to verify the configuration. Store they in the CamApp class and pass them to the Capture class constructor. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/capture.cpp | 5 +++-- src/cam/capture.h | 4 +++- src/cam/main.cpp | 9 +++++---- 3 files changed, 11 insertions(+), 7 deletions(-)