[libcamera-devel] [PATCH] libcamera: request: Make Request class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 18 13:54:31 CEST 2021
Implement the D-Pointer design pattern in the Request class to allow
changing internal data without affecting the public ABI.
Move a few internal fields that are not needed to implement the public
API to the Request::Private class already. More fields may be moved
later.
Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
I've had this patch in my tree for some time now, and in a recent
conversation with Jacopo I realized it could be useful to address fence
handling correctly. It's a bit of a work in progress in the sense that
it doesn't go through all the members of the Request class to decide
which ones to move to the private class, but I think it can be built
upon.
---
include/libcamera/internal/meson.build | 1 +
include/libcamera/internal/request.h | 36 ++++++++++++++++
include/libcamera/request.h | 8 ++--
src/libcamera/pipeline_handler.cpp | 7 +--
src/libcamera/request.cpp | 60 +++++++++++++++++++-------
5 files changed, 89 insertions(+), 23 deletions(-)
create mode 100644 include/libcamera/internal/request.h
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
index 665fd6de4ed3..cae65b0604ff 100644
--- a/include/libcamera/internal/meson.build
+++ b/include/libcamera/internal/meson.build
@@ -34,6 +34,7 @@ libcamera_internal_headers = files([
'pipeline_handler.h',
'process.h',
'pub_key.h',
+ 'request.h',
'source_paths.h',
'sysfs.h',
'v4l2_device.h',
diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
new file mode 100644
index 000000000000..ca954f06f497
--- /dev/null
+++ b/include/libcamera/internal/request.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * request.h - Request class private data
+ */
+#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
+#define __LIBCAMERA_INTERNAL_REQUEST_H__
+
+#include <libcamera/request.h>
+
+namespace libcamera {
+
+class Camera;
+class CameraControlValidator;
+class FrameBuffer;
+
+class Request::Private : public Extensible::Private
+{
+ LIBCAMERA_DECLARE_PUBLIC(Request)
+
+public:
+ Private(Camera *camera);
+ ~Private();
+
+ Camera *camera() const { return camera_; }
+
+private:
+ Camera *camera_;
+ CameraControlValidator *validator_;
+ bool cancelled_;
+};
+
+} /* namespace libcamera */
+
+#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index 2d361c9d97dc..e378355f18fc 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -21,12 +21,13 @@
namespace libcamera {
class Camera;
-class CameraControlValidator;
class FrameBuffer;
class Stream;
-class Request
+class Request : public Extensible
{
+ LIBCAMERA_DECLARE_PRIVATE()
+
public:
enum Status {
RequestPending,
@@ -70,8 +71,6 @@ private:
bool completeBuffer(FrameBuffer *buffer);
- Camera *camera_;
- CameraControlValidator *validator_;
ControlList *controls_;
ControlList *metadata_;
BufferMap bufferMap_;
@@ -80,7 +79,6 @@ private:
uint32_t sequence_;
const uint64_t cookie_;
Status status_;
- bool cancelled_;
};
} /* namespace libcamera */
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index f69c4f03b80f..cada864687ff 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -19,6 +19,7 @@
#include "libcamera/internal/camera.h"
#include "libcamera/internal/device_enumerator.h"
#include "libcamera/internal/media_device.h"
+#include "libcamera/internal/request.h"
#include "libcamera/internal/tracepoints.h"
/**
@@ -311,7 +312,7 @@ void PipelineHandler::queueRequest(Request *request)
{
LIBCAMERA_TRACEPOINT(request_queue, request);
- Camera *camera = request->camera_;
+ Camera *camera = request->_d()->camera();
Camera::Private *data = camera->_d();
data->queuedRequests_.push_back(request);
@@ -360,7 +361,7 @@ void PipelineHandler::queueRequest(Request *request)
*/
bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
{
- Camera *camera = request->camera_;
+ Camera *camera = request->_d()->camera();
camera->bufferCompleted.emit(request, buffer);
return request->completeBuffer(buffer);
}
@@ -381,7 +382,7 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
*/
void PipelineHandler::completeRequest(Request *request)
{
- Camera *camera = request->camera_;
+ Camera *camera = request->_d()->camera();
request->complete();
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index f95ce4db5eaa..7863269f3139 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -19,10 +19,11 @@
#include "libcamera/internal/camera_controls.h"
#include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/request.h"
#include "libcamera/internal/tracepoints.h"
/**
- * \file request.h
+ * \file libcamera/request.h
* \brief Describes a frame capture request to be processed by a camera
*/
@@ -30,6 +31,39 @@ namespace libcamera {
LOG_DEFINE_CATEGORY(Request)
+/**
+ * \class Request::Private
+ * \brief Request private data
+ *
+ * The Request::Private class stores all private data associated with a
+ * request. It implements the d-pointer design pattern to hide core
+ * Request data from the public API, and exposes utility functions to
+ * internal users of the request (namely the PipelineHandler class and its
+ * subclasses).
+ */
+
+Request::Private::Private(Camera *camera)
+ : camera_(camera), cancelled_(false)
+{
+ /**
+ * \todo Should the Camera expose a validator instance, to avoid
+ * creating a new instance for each request?
+ */
+ validator_ = new CameraControlValidator(camera);
+}
+
+Request::Private::~Private()
+{
+ delete validator_;
+}
+
+/**
+ * \fn Request::Private::camera()
+ * \brief Retrieve the camera this request has been queued to
+ * \return The Camera this request has been queued to, or nullptr if the
+ * request hasn't been queued
+ */
+
/**
* \enum Request::Status
* Request completion status
@@ -74,15 +108,10 @@ LOG_DEFINE_CATEGORY(Request)
* completely opaque to libcamera.
*/
Request::Request(Camera *camera, uint64_t cookie)
- : camera_(camera), sequence_(0), cookie_(cookie),
- status_(RequestPending), cancelled_(false)
+ : Extensible(std::make_unique<Private>(camera)), sequence_(0),
+ cookie_(cookie), status_(RequestPending)
{
- /**
- * \todo Should the Camera expose a validator instance, to avoid
- * creating a new instance for each request?
- */
- validator_ = new CameraControlValidator(camera);
- controls_ = new ControlList(controls::controls, validator_);
+ controls_ = new ControlList(controls::controls, _d()->validator_);
/**
* \todo: Add a validator for metadata controls.
@@ -100,7 +129,6 @@ Request::~Request()
delete metadata_;
delete controls_;
- delete validator_;
}
/**
@@ -130,7 +158,7 @@ void Request::reuse(ReuseFlag flags)
sequence_ = 0;
status_ = RequestPending;
- cancelled_ = false;
+ _d()->cancelled_ = false;
controls_->clear();
metadata_->clear();
@@ -286,7 +314,7 @@ void Request::complete()
ASSERT(status_ == RequestPending);
ASSERT(!hasPendingBuffers());
- status_ = cancelled_ ? RequestCancelled : RequestComplete;
+ status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;
LOG(Request, Debug) << toString();
@@ -303,17 +331,19 @@ void Request::complete()
*/
void Request::cancel()
{
+ Private *const d = _d();
+
LIBCAMERA_TRACEPOINT(request_cancel, this);
ASSERT(status_ == RequestPending);
for (FrameBuffer *buffer : pending_) {
buffer->cancel();
- camera_->bufferCompleted.emit(this, buffer);
+ d->camera_->bufferCompleted.emit(this, buffer);
}
pending_.clear();
- cancelled_ = true;
+ d->cancelled_ = true;
}
/**
@@ -339,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)
buffer->_d()->setRequest(nullptr);
if (buffer->metadata().status == FrameMetadata::FrameCancelled)
- cancelled_ = true;
+ _d()->cancelled_ = true;
return !hasPendingBuffers();
}
base-commit: 2f75a7e5b8c6258dc12e9e3128cb30133f66b4f9
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list