| Message ID | 20200724134320.637696-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas,
This is a very nice addition to cam!
Just could it be easier to add a numerical argument to the "capture" option ('cam -C <N>' - similar
to how yavta works) rather creating a new option?
Thanks,
Andrey
On 24.07.2020 16:43, Niklas Söderlund wrote:
> Add an option '--count' to specify how many frames to capture before
> terminating the capture loop.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> src/cam/capture.cpp | 17 +++++++++++++++--
> src/cam/capture.h | 2 ++
> src/cam/main.cpp | 2 ++
> src/cam/main.h | 1 +
> 4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index aa53c407d7b71b44..3ca70d9c6a8c7b7e 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -17,7 +17,8 @@
> using namespace libcamera;
>
> Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
> - : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr)
> + : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr),
> + captureCount_(0), captureLimit_(0)
> {
> }
>
> @@ -26,6 +27,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> int ret;
>
> loop_ = loop;
> + captureCount_ = 0;
> + captureLimit_ = options.isSet(OptCount) ? options[OptCount].toInteger() : 0;
>
> if (!camera_) {
> std::cout << "Can't capture without a camera" << std::endl;
> @@ -133,7 +136,11 @@ int Capture::capture(FrameBufferAllocator *allocator)
> }
> }
>
> - std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> + if (captureLimit_)
> + std::cout << "Capture " << captureLimit_ << " frames" << std::endl;
> + else
> + std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> +
> ret = loop_->exec();
> if (ret)
> std::cout << "Failed to run capture loop" << std::endl;
> @@ -202,5 +209,11 @@ void Capture::requestComplete(Request *request)
> request->addBuffer(stream, buffer);
> }
>
> + captureCount_++;
> + if (captureLimit_ && captureCount_ >= captureLimit_) {
> + loop_->exit(0);
> + return;
> + }
> +
> camera_->queueRequest(request);
> }
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index 67c6260bfe24aefc..0a1cbc519f2a90c5 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -40,6 +40,8 @@ private:
> std::chrono::steady_clock::time_point last_;
>
> EventLoop *loop_;
> + unsigned int captureCount_;
> + unsigned int captureLimit_;
> };
>
> #endif /* __CAM_CAPTURE_H__ */
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index a223563ad37e9fe5..f3bacb4a895600ee 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -188,6 +188,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> parser.addOption(OptStrictFormats, OptionNone,
> "Do not allow requested stream format(s) to be adjusted",
> "strict-formats");
> + parser.addOption(OptCount, OptionInteger, "Number of frames to capture",
> + "count", ArgumentRequired, "count");
>
> options_ = parser.parse(argc, argv);
> if (!options_.valid())
> diff --git a/src/cam/main.h b/src/cam/main.h
> index 6f95add31a6341cf..d4c93fee35681e86 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -18,6 +18,7 @@ enum {
> OptStream = 's',
> OptListControls = 256,
> OptStrictFormats = 257,
> + OptCount = 258,
> };
>
> #endif /* __CAM_MAIN_H__ */
>
Hi Niklas, Andrey beat me to the option value point, but still a Q below: On 24/07/2020 14:43, Niklas Söderlund wrote: > Add an option '--count' to specify how many frames to capture before > terminating the capture loop. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> <edit, actually there isn't anything but minors here, so addressed how you feel best:> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/cam/capture.cpp | 17 +++++++++++++++-- > src/cam/capture.h | 2 ++ > src/cam/main.cpp | 2 ++ > src/cam/main.h | 1 + > 4 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index aa53c407d7b71b44..3ca70d9c6a8c7b7e 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -17,7 +17,8 @@ > using namespace libcamera; > > Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) > - : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr) > + : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr), > + captureCount_(0), captureLimit_(0) > { > } > > @@ -26,6 +27,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options) > int ret; > > loop_ = loop; > + captureCount_ = 0; > + captureLimit_ = options.isSet(OptCount) ? options[OptCount].toInteger() : 0; This prevents setting a limit of 0. I don't know if that's useful, except for testing stream_on, followed by immediate stream_off ... Actually that wouldn't work anyway, as the check would only happen in request complete, so we know it's always going to do at least one capture... > if (!camera_) { > std::cout << "Can't capture without a camera" << std::endl; > @@ -133,7 +136,11 @@ int Capture::capture(FrameBufferAllocator *allocator) > } > } > > - std::cout << "Capture until user interrupts by SIGINT" << std::endl; > + if (captureLimit_) > + std::cout << "Capture " << captureLimit_ << " frames" << std::endl; > + else > + std::cout << "Capture until user interrupts by SIGINT" << std::endl; > + > ret = loop_->exec(); > if (ret) > std::cout << "Failed to run capture loop" << std::endl; > @@ -202,5 +209,11 @@ void Capture::requestComplete(Request *request) > request->addBuffer(stream, buffer); > } > > + captureCount_++; > + if (captureLimit_ && captureCount_ >= captureLimit_) { > + loop_->exit(0); > + return; > + } > + > camera_->queueRequest(request); > } > diff --git a/src/cam/capture.h b/src/cam/capture.h > index 67c6260bfe24aefc..0a1cbc519f2a90c5 100644 > --- a/src/cam/capture.h > +++ b/src/cam/capture.h > @@ -40,6 +40,8 @@ private: > std::chrono::steady_clock::time_point last_; > > EventLoop *loop_; > + unsigned int captureCount_; > + unsigned int captureLimit_; As these are already in the capture class, I guess they could just be count, and limit ... but that's just some bikeshed colour ... > }; > > #endif /* __CAM_CAPTURE_H__ */ > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index a223563ad37e9fe5..f3bacb4a895600ee 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -188,6 +188,8 @@ int CamApp::parseOptions(int argc, char *argv[]) > parser.addOption(OptStrictFormats, OptionNone, > "Do not allow requested stream format(s) to be adjusted", > "strict-formats"); > + parser.addOption(OptCount, OptionInteger, "Number of frames to capture", > + "count", ArgumentRequired, "count"); Would reusing the capture option simplify the args? > > options_ = parser.parse(argc, argv); > if (!options_.valid()) > diff --git a/src/cam/main.h b/src/cam/main.h > index 6f95add31a6341cf..d4c93fee35681e86 100644 > --- a/src/cam/main.h > +++ b/src/cam/main.h > @@ -18,6 +18,7 @@ enum { > OptStream = 's', > OptListControls = 256, > OptStrictFormats = 257, > + OptCount = 258, > }; > > #endif /* __CAM_MAIN_H__ */ >
Hi Andrey and Kieran, On 2020-07-24 16:58:42 +0300, Andrey Konovalov wrote: > Hi Niklas, > > This is a very nice addition to cam! > > Just could it be easier to add a numerical argument to the "capture" option ('cam -C <N>' - similar > to how yavta works) rather creating a new option? This is a great suggestion pointed out by you and Kieran and I will do so for v2. > > Thanks, > Andrey > > On 24.07.2020 16:43, Niklas Söderlund wrote: > > Add an option '--count' to specify how many frames to capture before > > terminating the capture loop. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/capture.cpp | 17 +++++++++++++++-- > > src/cam/capture.h | 2 ++ > > src/cam/main.cpp | 2 ++ > > src/cam/main.h | 1 + > > 4 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > index aa53c407d7b71b44..3ca70d9c6a8c7b7e 100644 > > --- a/src/cam/capture.cpp > > +++ b/src/cam/capture.cpp > > @@ -17,7 +17,8 @@ > > using namespace libcamera; > > Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) > > - : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr) > > + : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr), > > + captureCount_(0), captureLimit_(0) > > { > > } > > @@ -26,6 +27,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options) > > int ret; > > loop_ = loop; > > + captureCount_ = 0; > > + captureLimit_ = options.isSet(OptCount) ? options[OptCount].toInteger() : 0; > > if (!camera_) { > > std::cout << "Can't capture without a camera" << std::endl; > > @@ -133,7 +136,11 @@ int Capture::capture(FrameBufferAllocator *allocator) > > } > > } > > - std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > + if (captureLimit_) > > + std::cout << "Capture " << captureLimit_ << " frames" << std::endl; > > + else > > + std::cout << "Capture until user interrupts by SIGINT" << std::endl; > > + > > ret = loop_->exec(); > > if (ret) > > std::cout << "Failed to run capture loop" << std::endl; > > @@ -202,5 +209,11 @@ void Capture::requestComplete(Request *request) > > request->addBuffer(stream, buffer); > > } > > + captureCount_++; > > + if (captureLimit_ && captureCount_ >= captureLimit_) { > > + loop_->exit(0); > > + return; > > + } > > + > > camera_->queueRequest(request); > > } > > diff --git a/src/cam/capture.h b/src/cam/capture.h > > index 67c6260bfe24aefc..0a1cbc519f2a90c5 100644 > > --- a/src/cam/capture.h > > +++ b/src/cam/capture.h > > @@ -40,6 +40,8 @@ private: > > std::chrono::steady_clock::time_point last_; > > EventLoop *loop_; > > + unsigned int captureCount_; > > + unsigned int captureLimit_; > > }; > > #endif /* __CAM_CAPTURE_H__ */ > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index a223563ad37e9fe5..f3bacb4a895600ee 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -188,6 +188,8 @@ int CamApp::parseOptions(int argc, char *argv[]) > > parser.addOption(OptStrictFormats, OptionNone, > > "Do not allow requested stream format(s) to be adjusted", > > "strict-formats"); > > + parser.addOption(OptCount, OptionInteger, "Number of frames to capture", > > + "count", ArgumentRequired, "count"); > > options_ = parser.parse(argc, argv); > > if (!options_.valid()) > > diff --git a/src/cam/main.h b/src/cam/main.h > > index 6f95add31a6341cf..d4c93fee35681e86 100644 > > --- a/src/cam/main.h > > +++ b/src/cam/main.h > > @@ -18,6 +18,7 @@ enum { > > OptStream = 's', > > OptListControls = 256, > > OptStrictFormats = 257, > > + OptCount = 258, > > }; > > #endif /* __CAM_MAIN_H__ */ > >
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index aa53c407d7b71b44..3ca70d9c6a8c7b7e 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -17,7 +17,8 @@ using namespace libcamera; Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config) - : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr) + : camera_(camera), config_(config), writer_(nullptr), loop_(nullptr), + captureCount_(0), captureLimit_(0) { } @@ -26,6 +27,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options) int ret; loop_ = loop; + captureCount_ = 0; + captureLimit_ = options.isSet(OptCount) ? options[OptCount].toInteger() : 0; if (!camera_) { std::cout << "Can't capture without a camera" << std::endl; @@ -133,7 +136,11 @@ int Capture::capture(FrameBufferAllocator *allocator) } } - std::cout << "Capture until user interrupts by SIGINT" << std::endl; + if (captureLimit_) + std::cout << "Capture " << captureLimit_ << " frames" << std::endl; + else + std::cout << "Capture until user interrupts by SIGINT" << std::endl; + ret = loop_->exec(); if (ret) std::cout << "Failed to run capture loop" << std::endl; @@ -202,5 +209,11 @@ void Capture::requestComplete(Request *request) request->addBuffer(stream, buffer); } + captureCount_++; + if (captureLimit_ && captureCount_ >= captureLimit_) { + loop_->exit(0); + return; + } + camera_->queueRequest(request); } diff --git a/src/cam/capture.h b/src/cam/capture.h index 67c6260bfe24aefc..0a1cbc519f2a90c5 100644 --- a/src/cam/capture.h +++ b/src/cam/capture.h @@ -40,6 +40,8 @@ private: std::chrono::steady_clock::time_point last_; EventLoop *loop_; + unsigned int captureCount_; + unsigned int captureLimit_; }; #endif /* __CAM_CAPTURE_H__ */ diff --git a/src/cam/main.cpp b/src/cam/main.cpp index a223563ad37e9fe5..f3bacb4a895600ee 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -188,6 +188,8 @@ int CamApp::parseOptions(int argc, char *argv[]) parser.addOption(OptStrictFormats, OptionNone, "Do not allow requested stream format(s) to be adjusted", "strict-formats"); + parser.addOption(OptCount, OptionInteger, "Number of frames to capture", + "count", ArgumentRequired, "count"); options_ = parser.parse(argc, argv); if (!options_.valid()) diff --git a/src/cam/main.h b/src/cam/main.h index 6f95add31a6341cf..d4c93fee35681e86 100644 --- a/src/cam/main.h +++ b/src/cam/main.h @@ -18,6 +18,7 @@ enum { OptStream = 's', OptListControls = 256, OptStrictFormats = 257, + OptCount = 258, }; #endif /* __CAM_MAIN_H__ */
Add an option '--count' to specify how many frames to capture before terminating the capture loop. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/capture.cpp | 17 +++++++++++++++-- src/cam/capture.h | 2 ++ src/cam/main.cpp | 2 ++ src/cam/main.h | 1 + 4 files changed, 20 insertions(+), 2 deletions(-)