[libcamera-devel] [PATCH v2 3/3] cam: Add optional argument to --capture to specify how many frames to capture

[Niklas Söderlund]

Hi Laurent,

Thanks for your comments.

On 2020-07-24 21:00:59 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jul 24, 2020 at 07:48:27PM +0200, Niklas Söderlund wrote:
> > Extend the '--capture' option with and optional numerical argument to be
> > able to specify how many frames to capture before exiting. If the
> > optional argument is not provided the old behavior of running until the
> > user interrupts with a SIGINT is retained.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > * Changes since v1
> > - Extend --capture option instead of adding a new --count option.
> > ---
> >  src/cam/capture.cpp | 17 +++++++++++++++--
> >  src/cam/capture.h   |  2 ++
> >  src/cam/main.cpp    |  5 +++--
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index aa53c407d7b71b44..2378c1c737e9f419 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[OptCapture].toInteger();
> >  
> >  	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;
> > +	}
> 
> Should this go a bit above, just before creating the new request ?
> Otherwise you'll leak it. Could you please test this series with
> valgrind ?

Woops, thanks for spotting this! You are correct valgrind warns for this 
fault while moving it as you suggest solves it.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > +
> >  	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 2009f11b6c336f2c..adbb1c657de0f053 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -167,8 +167,9 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  	parser.addOption(OptCamera, OptionString,
> >  			 "Specify which camera to operate on, by name or by index", "camera",
> >  			 ArgumentRequired, "camera");
> > -	parser.addOption(OptCapture, OptionNone,
> > -			 "Capture until interrupted by user", "capture");
> > +	parser.addOption(OptCapture, OptionInteger,
> > +			 "Capture until interrupted by user or until <count> frames captured",
> > +			 "capture", ArgumentOptional, "count");
> >  	parser.addOption(OptFile, OptionString,
> >  			 "Write captured frames to disk\n"
> >  			 "The first '#' character in the file name is expanded to the stream name and frame sequence number.\n"
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund