[libcamera-devel] [PATCH 9/9] libcamera: framebuffer_allocator: Lift camera restrictions on allocator
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Mar 16 16:50:00 CET 2020
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 at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list