[libcamera-devel,v2,2/3] cam: capture: Cache the EventLoop handler
diff mbox series

Message ID 20200724174827.757493-3-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 
Prepare for the ability to exit the event loop based on conditions in
the request complete handler by caching the pointer instead of passing
it as an argument.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/cam/capture.cpp | 10 ++++++----
 src/cam/capture.h   |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart July 24, 2020, 5:57 p.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Fri, Jul 24, 2020 at 07:48:26PM +0200, Niklas Söderlund wrote:
> Prepare for the ability to exit the event loop based on conditions in
> the request complete handler by caching the pointer instead of passing
> it as an argument.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/cam/capture.cpp | 10 ++++++----
>  src/cam/capture.h   |  5 +++--
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 55fa2dabcee97f21..aa53c407d7b71b44 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -17,7 +17,7 @@
>  using namespace libcamera;
>  
>  Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
> -	: camera_(camera), config_(config), writer_(nullptr)
> +	: camera_(camera), config_(config), writer_(nullptr), loop_(nullptr)

Would it make sense to pass the event loop to the constructor ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  {
>  }
>  
> @@ -25,6 +25,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
>  {
>  	int ret;
>  
> +	loop_ = loop;
> +
>  	if (!camera_) {
>  		std::cout << "Can't capture without a camera" << std::endl;
>  		return -ENODEV;
> @@ -54,7 +56,7 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
>  
>  	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
>  
> -	ret = capture(loop, allocator);
> +	ret = capture(allocator);
>  
>  	if (options.isSet(OptFile)) {
>  		delete writer_;
> @@ -66,7 +68,7 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
>  	return ret;
>  }
>  
> -int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> +int Capture::capture(FrameBufferAllocator *allocator)
>  {
>  	int ret;
>  
> @@ -132,7 +134,7 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
>  	}
>  
>  	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> -	ret = loop->exec();
> +	ret = loop_->exec();
>  	if (ret)
>  		std::cout << "Failed to run capture loop" << std::endl;
>  
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index 9bca5661070efcf5..67c6260bfe24aefc 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -28,8 +28,7 @@ public:
>  
>  	int run(EventLoop *loop, const OptionsParser::Options &options);
>  private:
> -	int capture(EventLoop *loop,
> -		    libcamera::FrameBufferAllocator *allocator);
> +	int capture(libcamera::FrameBufferAllocator *allocator);
>  
>  	void requestComplete(libcamera::Request *request);
>  
> @@ -39,6 +38,8 @@ private:
>  	std::map<libcamera::Stream *, std::string> streamName_;
>  	BufferWriter *writer_;
>  	std::chrono::steady_clock::time_point last_;
> +
> +	EventLoop *loop_;
>  };
>  
>  #endif /* __CAM_CAPTURE_H__ */
Niklas Söderlund July 24, 2020, 7:10 p.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-07-24 20:57:50 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jul 24, 2020 at 07:48:26PM +0200, Niklas Söderlund wrote:
> > Prepare for the ability to exit the event loop based on conditions in
> > the request complete handler by caching the pointer instead of passing
> > it as an argument.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/cam/capture.cpp | 10 ++++++----
> >  src/cam/capture.h   |  5 +++--
> >  2 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 55fa2dabcee97f21..aa53c407d7b71b44 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -17,7 +17,7 @@
> >  using namespace libcamera;
> >  
> >  Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
> > -	: camera_(camera), config_(config), writer_(nullptr)
> > +	: camera_(camera), config_(config), writer_(nullptr), loop_(nullptr)
> 
> Would it make sense to pass the event loop to the constructor ?

That might be nicer, will do so in v3.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  {
> >  }
> >  
> > @@ -25,6 +25,8 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> >  {
> >  	int ret;
> >  
> > +	loop_ = loop;
> > +
> >  	if (!camera_) {
> >  		std::cout << "Can't capture without a camera" << std::endl;
> >  		return -ENODEV;
> > @@ -54,7 +56,7 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> >  
> >  	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
> >  
> > -	ret = capture(loop, allocator);
> > +	ret = capture(allocator);
> >  
> >  	if (options.isSet(OptFile)) {
> >  		delete writer_;
> > @@ -66,7 +68,7 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> >  	return ret;
> >  }
> >  
> > -int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> > +int Capture::capture(FrameBufferAllocator *allocator)
> >  {
> >  	int ret;
> >  
> > @@ -132,7 +134,7 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> >  	}
> >  
> >  	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> > -	ret = loop->exec();
> > +	ret = loop_->exec();
> >  	if (ret)
> >  		std::cout << "Failed to run capture loop" << std::endl;
> >  
> > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > index 9bca5661070efcf5..67c6260bfe24aefc 100644
> > --- a/src/cam/capture.h
> > +++ b/src/cam/capture.h
> > @@ -28,8 +28,7 @@ public:
> >  
> >  	int run(EventLoop *loop, const OptionsParser::Options &options);
> >  private:
> > -	int capture(EventLoop *loop,
> > -		    libcamera::FrameBufferAllocator *allocator);
> > +	int capture(libcamera::FrameBufferAllocator *allocator);
> >  
> >  	void requestComplete(libcamera::Request *request);
> >  
> > @@ -39,6 +38,8 @@ private:
> >  	std::map<libcamera::Stream *, std::string> streamName_;
> >  	BufferWriter *writer_;
> >  	std::chrono::steady_clock::time_point last_;
> > +
> > +	EventLoop *loop_;
> >  };
> >  
> >  #endif /* __CAM_CAPTURE_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
index 55fa2dabcee97f21..aa53c407d7b71b44 100644
--- a/src/cam/capture.cpp
+++ b/src/cam/capture.cpp
@@ -17,7 +17,7 @@ 
 using namespace libcamera;
 
 Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
-	: camera_(camera), config_(config), writer_(nullptr)
+	: camera_(camera), config_(config), writer_(nullptr), loop_(nullptr)
 {
 }
 
@@ -25,6 +25,8 @@  int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
 {
 	int ret;
 
+	loop_ = loop;
+
 	if (!camera_) {
 		std::cout << "Can't capture without a camera" << std::endl;
 		return -ENODEV;
@@ -54,7 +56,7 @@  int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
 
 	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
 
-	ret = capture(loop, allocator);
+	ret = capture(allocator);
 
 	if (options.isSet(OptFile)) {
 		delete writer_;
@@ -66,7 +68,7 @@  int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
 	return ret;
 }
 
-int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
+int Capture::capture(FrameBufferAllocator *allocator)
 {
 	int ret;
 
@@ -132,7 +134,7 @@  int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
 	}
 
 	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
-	ret = loop->exec();
+	ret = loop_->exec();
 	if (ret)
 		std::cout << "Failed to run capture loop" << std::endl;
 
diff --git a/src/cam/capture.h b/src/cam/capture.h
index 9bca5661070efcf5..67c6260bfe24aefc 100644
--- a/src/cam/capture.h
+++ b/src/cam/capture.h
@@ -28,8 +28,7 @@  public:
 
 	int run(EventLoop *loop, const OptionsParser::Options &options);
 private:
-	int capture(EventLoop *loop,
-		    libcamera::FrameBufferAllocator *allocator);
+	int capture(libcamera::FrameBufferAllocator *allocator);
 
 	void requestComplete(libcamera::Request *request);
 
@@ -39,6 +38,8 @@  private:
 	std::map<libcamera::Stream *, std::string> streamName_;
 	BufferWriter *writer_;
 	std::chrono::steady_clock::time_point last_;
+
+	EventLoop *loop_;
 };
 
 #endif /* __CAM_CAPTURE_H__ */