[libcamera-devel,v2,3/3] cam: Add optional argument to --capture to specify how many frames to capture
diff mbox series

Message ID 20200724174827.757493-4-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • cam: Add options to make batch testing with cam easier
Related show

Commit Message

Niklas Söderlund July 24, 2020, 5:48 p.m. UTC 
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@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(-)

Comments

Laurent Pinchart July 24, 2020, 6 p.m. UTC | #1 
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@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 ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@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"
Niklas Söderlund July 24, 2020, 7:15 p.m. UTC | #2 
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@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@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

Patch
diff mbox series 

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;
+	}
+
 	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"