[libcamera-devel,9/9] libcamera: framebuffer_allocator: Lift camera restrictions on allocator
diff mbox series

Message ID 20200314235728.15495-10-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Simplify buffer management with V4L2 buffer orphaning
Related show

Commit Message

Laurent Pinchart March 14, 2020, 11:57 p.m. UTC 
The Camera class currently requires the allocator to have no allocated
buffer before the camera is reconfigured, and the allocator to be
destroyed before the camera is released. There's no basis for these
restrictions anymore, remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/camera.h                |  2 --
 include/libcamera/framebuffer_allocator.h |  5 +----
 src/cam/capture.cpp                       |  2 +-
 src/gstreamer/gstlibcameraallocator.cpp   |  2 +-
 src/libcamera/camera.cpp                  | 18 +---------------
 src/libcamera/framebuffer_allocator.cpp   | 25 -----------------------
 src/qcam/main_window.cpp                  |  2 +-
 src/v4l2/v4l2_camera.cpp                  |  2 +-
 test/camera/capture.cpp                   |  2 +-
 test/camera/statemachine.cpp              |  2 +-
 10 files changed, 8 insertions(+), 54 deletions(-)

Comments

Niklas Söderlund March 16, 2020, 3:50 p.m. UTC | #1 
Hi Laurent,

Thanks for this nice cleanup.

On 2020-03-15 01:57:28 +0200, Laurent Pinchart wrote:
> The Camera class currently requires the allocator to have no allocated
> buffer before the camera is reconfigured, and the allocator to be
> destroyed before the camera is released. There's no basis for these
> restrictions anymore, remove them.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/camera.h                |  2 --
>  include/libcamera/framebuffer_allocator.h |  5 +----
>  src/cam/capture.cpp                       |  2 +-
>  src/gstreamer/gstlibcameraallocator.cpp   |  2 +-
>  src/libcamera/camera.cpp                  | 18 +---------------
>  src/libcamera/framebuffer_allocator.cpp   | 25 -----------------------
>  src/qcam/main_window.cpp                  |  2 +-
>  src/v4l2/v4l2_camera.cpp                  |  2 +-
>  test/camera/capture.cpp                   |  2 +-
>  test/camera/statemachine.cpp              |  2 +-
>  10 files changed, 8 insertions(+), 54 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a5e2b49f0f25..9c0e58f7864b 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -113,8 +113,6 @@ private:
>  	friend class FrameBufferAllocator;
>  	int exportFrameBuffers(Stream *stream,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> -	/* \todo Remove allocator_ from the exposed API */
> -	FrameBufferAllocator *allocator_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> index 42812253ada9..78f1353964eb 100644
> --- a/include/libcamera/framebuffer_allocator.h
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -20,8 +20,7 @@ class Stream;
>  class FrameBufferAllocator
>  {
>  public:
> -	static FrameBufferAllocator *create(std::shared_ptr<Camera> camera);
> -
> +	FrameBufferAllocator(std::shared_ptr<Camera> camera);
>  	FrameBufferAllocator(const Camera &) = delete;
>  	FrameBufferAllocator &operator=(const Camera &) = delete;
>  
> @@ -34,8 +33,6 @@ public:
>  	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
>  
>  private:
> -	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> -
>  	std::shared_ptr<Camera> camera_;
>  	std::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;
>  };
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 7d970f991d3a..b62a9b24b216 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -52,7 +52,7 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
>  	}
>  
>  
> -	FrameBufferAllocator *allocator = FrameBufferAllocator::create(camera_);
> +	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
>  
>  	ret = capture(loop, allocator);
>  
> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> index d0b90ecaa873..1d5959c076cb 100644
> --- a/src/gstreamer/gstlibcameraallocator.cpp
> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> @@ -188,7 +188,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
>  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
>  							  nullptr));
>  
> -	self->fb_allocator = FrameBufferAllocator::create(camera);
> +	self->fb_allocator = new FrameBufferAllocator(camera);
>  	for (Stream *stream : camera->streams()) {
>  		gint ret;
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 5593c1b317a0..8c3bb2c2a01f 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -508,7 +508,7 @@ const std::string &Camera::name() const
>  
>  Camera::Camera(PipelineHandler *pipe, const std::string &name,
>  	       const std::set<Stream *> &streams)
> -	: p_(new Private(pipe, name, streams)), allocator_(nullptr)
> +	: p_(new Private(pipe, name, streams))
>  {
>  }
>  
> @@ -620,16 +620,6 @@ int Camera::release()
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> -	if (allocator_) {
> -		/*
> -		 * \todo Try to find a better API that would make this error
> -		 * impossible.
> -		 */
> -		LOG(Camera, Error)
> -			<< "Buffers must be freed before the camera can be reconfigured";
> -		return -EBUSY;
> -	}
> -
>  	p_->pipe_->unlock();
>  
>  	p_->setState(Private::CameraAvailable);
> @@ -763,12 +753,6 @@ int Camera::configure(CameraConfiguration *config)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (allocator_ && allocator_->allocated()) {
> -		LOG(Camera, Error)
> -			<< "Allocator must be deleted before camera can be reconfigured";
> -		return -EBUSY;
> -	}
> -
>  	if (config->validate() != CameraConfiguration::Valid) {
>  		LOG(Camera, Error)
>  			<< "Can't configure camera with invalid configuration";
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 6f7a2e90b08a..a37b564c6701 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -53,29 +53,6 @@ LOG_DEFINE_CATEGORY(Allocator)
>   * are provided externally applications shall not use this class.
>   */
>  
> -/**
> - * \brief Create a FrameBuffer allocator
> - * \param[in] camera The camera the allocator serves
> - *
> - * A single allocator may be created for a Camera instance.
> - *
> - * The caller is responsible for deleting the allocator before the camera is
> - * released.
> - *
> - * \return A pointer to the newly created allocator object or nullptr on error
> - */
> -FrameBufferAllocator *
> -FrameBufferAllocator::create(std::shared_ptr<Camera> camera)
> -{
> -	if (camera->allocator_) {
> -		LOG(Allocator, Error) << "Camera already has an allocator";
> -		return nullptr;
> -	}
> -
> -	camera->allocator_ = new FrameBufferAllocator(camera);
> -	return camera->allocator_;
> -}
> -
>  /**
>   * \brief Construct a FrameBufferAllocator serving a camera
>   * \param[in] camera The camera
> @@ -88,8 +65,6 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
>  FrameBufferAllocator::~FrameBufferAllocator()
>  {
>  	buffers_.clear();
> -
> -	camera_->allocator_ = nullptr;
>  }
>  
>  /**
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index ae1760dfd647..47d37c3e62ce 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -240,7 +240,7 @@ int MainWindow::startCapture()
>  
>  	adjustSize();
>  
> -	allocator_ = FrameBufferAllocator::create(camera_);
> +	allocator_ = new FrameBufferAllocator(camera_);
>  	ret = allocator_->allocate(stream);
>  	if (ret < 0) {
>  		std::cerr << "Failed to allocate capture buffers" << std::endl;
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index e7018b566475..f0b9f1804c94 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -40,7 +40,7 @@ int V4L2Camera::open()
>  		return -EINVAL;
>  	}
>  
> -	bufferAllocator_ = FrameBufferAllocator::create(camera_);
> +	bufferAllocator_ = new FrameBufferAllocator(camera_);
>  
>  	return 0;
>  }
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index b304d59c1c2a..f6b2f348bda5 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -63,7 +63,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		allocator_ = FrameBufferAllocator::create(camera_);
> +		allocator_ = new FrameBufferAllocator(camera_);
>  
>  		return TestPass;
>  	}
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index 20541b3e4752..325b4674bcc9 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -117,7 +117,7 @@ protected:
>  			return TestFail;
>  
>  		/* Use internally allocated buffers. */
> -		allocator_ = FrameBufferAllocator::create(camera_);
> +		allocator_ = new FrameBufferAllocator(camera_);
>  		Stream *stream = *camera_->streams().begin();
>  		if (allocator_->allocate(stream) < 0)
>  			return TestFail;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
index a5e2b49f0f25..9c0e58f7864b 100644
--- a/include/libcamera/camera.h
+++ b/include/libcamera/camera.h
@@ -113,8 +113,6 @@  private:
 	friend class FrameBufferAllocator;
 	int exportFrameBuffers(Stream *stream,
 			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
-	/* \todo Remove allocator_ from the exposed API */
-	FrameBufferAllocator *allocator_;
 };
 
 } /* namespace libcamera */
diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
index 42812253ada9..78f1353964eb 100644
--- a/include/libcamera/framebuffer_allocator.h
+++ b/include/libcamera/framebuffer_allocator.h
@@ -20,8 +20,7 @@  class Stream;
 class FrameBufferAllocator
 {
 public:
-	static FrameBufferAllocator *create(std::shared_ptr<Camera> camera);
-
+	FrameBufferAllocator(std::shared_ptr<Camera> camera);
 	FrameBufferAllocator(const Camera &) = delete;
 	FrameBufferAllocator &operator=(const Camera &) = delete;
 
@@ -34,8 +33,6 @@  public:
 	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
 
 private:
-	FrameBufferAllocator(std::shared_ptr<Camera> camera);
-
 	std::shared_ptr<Camera> camera_;
 	std::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;
 };
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
index 7d970f991d3a..b62a9b24b216 100644
--- a/src/cam/capture.cpp
+++ b/src/cam/capture.cpp
@@ -52,7 +52,7 @@  int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
 	}
 
 
-	FrameBufferAllocator *allocator = FrameBufferAllocator::create(camera_);
+	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
 
 	ret = capture(loop, allocator);
 
diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
index d0b90ecaa873..1d5959c076cb 100644
--- a/src/gstreamer/gstlibcameraallocator.cpp
+++ b/src/gstreamer/gstlibcameraallocator.cpp
@@ -188,7 +188,7 @@  gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
 	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
 							  nullptr));
 
-	self->fb_allocator = FrameBufferAllocator::create(camera);
+	self->fb_allocator = new FrameBufferAllocator(camera);
 	for (Stream *stream : camera->streams()) {
 		gint ret;
 
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 5593c1b317a0..8c3bb2c2a01f 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -508,7 +508,7 @@  const std::string &Camera::name() const
 
 Camera::Camera(PipelineHandler *pipe, const std::string &name,
 	       const std::set<Stream *> &streams)
-	: p_(new Private(pipe, name, streams)), allocator_(nullptr)
+	: p_(new Private(pipe, name, streams))
 {
 }
 
@@ -620,16 +620,6 @@  int Camera::release()
 	if (ret < 0)
 		return ret == -EACCES ? -EBUSY : ret;
 
-	if (allocator_) {
-		/*
-		 * \todo Try to find a better API that would make this error
-		 * impossible.
-		 */
-		LOG(Camera, Error)
-			<< "Buffers must be freed before the camera can be reconfigured";
-		return -EBUSY;
-	}
-
 	p_->pipe_->unlock();
 
 	p_->setState(Private::CameraAvailable);
@@ -763,12 +753,6 @@  int Camera::configure(CameraConfiguration *config)
 	if (ret < 0)
 		return ret;
 
-	if (allocator_ && allocator_->allocated()) {
-		LOG(Camera, Error)
-			<< "Allocator must be deleted before camera can be reconfigured";
-		return -EBUSY;
-	}
-
 	if (config->validate() != CameraConfiguration::Valid) {
 		LOG(Camera, Error)
 			<< "Can't configure camera with invalid configuration";
diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
index 6f7a2e90b08a..a37b564c6701 100644
--- a/src/libcamera/framebuffer_allocator.cpp
+++ b/src/libcamera/framebuffer_allocator.cpp
@@ -53,29 +53,6 @@  LOG_DEFINE_CATEGORY(Allocator)
  * are provided externally applications shall not use this class.
  */
 
-/**
- * \brief Create a FrameBuffer allocator
- * \param[in] camera The camera the allocator serves
- *
- * A single allocator may be created for a Camera instance.
- *
- * The caller is responsible for deleting the allocator before the camera is
- * released.
- *
- * \return A pointer to the newly created allocator object or nullptr on error
- */
-FrameBufferAllocator *
-FrameBufferAllocator::create(std::shared_ptr<Camera> camera)
-{
-	if (camera->allocator_) {
-		LOG(Allocator, Error) << "Camera already has an allocator";
-		return nullptr;
-	}
-
-	camera->allocator_ = new FrameBufferAllocator(camera);
-	return camera->allocator_;
-}
-
 /**
  * \brief Construct a FrameBufferAllocator serving a camera
  * \param[in] camera The camera
@@ -88,8 +65,6 @@  FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
 FrameBufferAllocator::~FrameBufferAllocator()
 {
 	buffers_.clear();
-
-	camera_->allocator_ = nullptr;
 }
 
 /**
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index ae1760dfd647..47d37c3e62ce 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -240,7 +240,7 @@  int MainWindow::startCapture()
 
 	adjustSize();
 
-	allocator_ = FrameBufferAllocator::create(camera_);
+	allocator_ = new FrameBufferAllocator(camera_);
 	ret = allocator_->allocate(stream);
 	if (ret < 0) {
 		std::cerr << "Failed to allocate capture buffers" << std::endl;
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
index e7018b566475..f0b9f1804c94 100644
--- a/src/v4l2/v4l2_camera.cpp
+++ b/src/v4l2/v4l2_camera.cpp
@@ -40,7 +40,7 @@  int V4L2Camera::open()
 		return -EINVAL;
 	}
 
-	bufferAllocator_ = FrameBufferAllocator::create(camera_);
+	bufferAllocator_ = new FrameBufferAllocator(camera_);
 
 	return 0;
 }
diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
index b304d59c1c2a..f6b2f348bda5 100644
--- a/test/camera/capture.cpp
+++ b/test/camera/capture.cpp
@@ -63,7 +63,7 @@  protected:
 			return TestFail;
 		}
 
-		allocator_ = FrameBufferAllocator::create(camera_);
+		allocator_ = new FrameBufferAllocator(camera_);
 
 		return TestPass;
 	}
diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
index 20541b3e4752..325b4674bcc9 100644
--- a/test/camera/statemachine.cpp
+++ b/test/camera/statemachine.cpp
@@ -117,7 +117,7 @@  protected:
 			return TestFail;
 
 		/* Use internally allocated buffers. */
-		allocator_ = FrameBufferAllocator::create(camera_);
+		allocator_ = new FrameBufferAllocator(camera_);
 		Stream *stream = *camera_->streams().begin();
 		if (allocator_->allocate(stream) < 0)
 			return TestFail;