libcamera: request: Make it extensible
diff mbox series

Message ID 20211019154915.2123366-1-kieran.bingham@ideasonboard.com
State Rejected
Headers show
Series
  • libcamera: request: Make it extensible
Related show

Commit Message

Kieran Bingham Oct. 19, 2021, 3:49 p.m. UTC
Provide an extensible private object for the Request class.
This allows us to make modifications to the private API and storage of
requests without affecting the public API or ABI.

Initially the pending_ structure and hasPendingBuffers are adapted to
support this move.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
v2:
 - Drop all the previous assertions that were added here.
 - Focus on making the Request use the d pointer design.
 - Move Request::Private to internal/request.h to make it available
   to other internal components such as PipelineHandlers

This directly competes with Laurent's implementation. Posted mostly as
reference, I don't mind which one goes forward.

If Laurent's goes in, I'll just have to pull out the remaining parts of
this and apply on top.

---
 include/libcamera/internal/meson.build |  1 +
 include/libcamera/internal/request.h   | 32 +++++++++++++++++++++
 include/libcamera/request.h            |  7 +++--
 src/libcamera/request.cpp              | 39 ++++++++++++++++++--------
 4 files changed, 64 insertions(+), 15 deletions(-)
 create mode 100644 include/libcamera/internal/request.h

Patch
diff mbox series

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..630ae60e8b6c
--- /dev/null
+++ b/include/libcamera/internal/request.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * request.h - Internal capture request handling
+ */
+#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
+#define __LIBCAMERA_INTERNAL_REQUEST_H__
+
+#include <unordered_set>
+
+#include <libcamera/base/class.h>
+
+#include <libcamera/request.h>
+
+namespace libcamera {
+
+class Camera;
+class CameraControlValidator;
+class FrameBuffer;
+
+class Request::Private : public Extensible::Private
+{
+	LIBCAMERA_DECLARE_PUBLIC(Request)
+
+private:
+	std::unordered_set<FrameBuffer *> pending_;
+};
+
+} /* namespace libcamera */
+
+#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index 2d361c9d97dc..71d088551d94 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -25,8 +25,10 @@  class CameraControlValidator;
 class FrameBuffer;
 class Stream;
 
-class Request
+class Request : public Extensible
 {
+	LIBCAMERA_DECLARE_PRIVATE()
+
 public:
 	enum Status {
 		RequestPending,
@@ -56,7 +58,7 @@  public:
 	uint64_t cookie() const { return cookie_; }
 	Status status() const { return status_; }
 
-	bool hasPendingBuffers() const { return !pending_.empty(); }
+	bool hasPendingBuffers() const;
 
 	std::string toString() const;
 
@@ -75,7 +77,6 @@  private:
 	ControlList *controls_;
 	ControlList *metadata_;
 	BufferMap bufferMap_;
-	std::unordered_set<FrameBuffer *> pending_;
 
 	uint32_t sequence_;
 	const uint64_t cookie_;
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index f95ce4db5eaa..d363d7a7b005 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -5,7 +5,7 @@ 
  * request.cpp - Capture request handling
  */
 
-#include <libcamera/request.h>
+#include "libcamera/internal/request.h"
 
 #include <map>
 #include <sstream>
@@ -22,14 +22,26 @@ 
 #include "libcamera/internal/tracepoints.h"
 
 /**
- * \file request.h
+ * \file libcamera/request.h
  * \brief Describes a frame capture request to be processed by a camera
+ *
+ * \file internal/request.h
+ * \brief Provides the internal implementation details for a Request
  */
 
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(Request)
 
+/**
+ * \class Request::Private
+ * \brief Base class for FrameBuffer 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 pipeline handlers.
+ */
+
 /**
  * \enum Request::Status
  * Request completion status
@@ -74,8 +86,9 @@  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_(camera),
+	  sequence_(0), cookie_(cookie), status_(RequestPending),
+	  cancelled_(false)
 {
 	/**
 	 * \todo Should the Camera expose a validator instance, to avoid
@@ -117,12 +130,11 @@  void Request::reuse(ReuseFlag flags)
 {
 	LIBCAMERA_TRACEPOINT(request_reuse, this);
 
-	pending_.clear();
+	_d()->pending_.clear();
 	if (flags & ReuseBuffers) {
 		for (auto pair : bufferMap_) {
 			FrameBuffer *buffer = pair.second;
 			buffer->_d()->setRequest(this);
-			pending_.insert(buffer);
 		}
 	} else {
 		bufferMap_.clear();
@@ -192,7 +204,7 @@  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
 	}
 
 	buffer->_d()->setRequest(this);
-	pending_.insert(buffer);
+	_d()->pending_.insert(buffer);
 	bufferMap_[stream] = buffer;
 
 	return 0;
@@ -267,12 +279,15 @@  FrameBuffer *Request::findBuffer(const Stream *stream) const
  */
 
 /**
- * \fn Request::hasPendingBuffers()
  * \brief Check if a request has buffers yet to be completed
  *
  * \return True if the request has buffers pending for completion, false
  * otherwise
  */
+bool Request::hasPendingBuffers() const
+{
+	return !_d()->pending_.empty();
+}
 
 /**
  * \brief Complete a queued request
@@ -307,12 +322,12 @@  void Request::cancel()
 
 	ASSERT(status_ == RequestPending);
 
-	for (FrameBuffer *buffer : pending_) {
+	for (FrameBuffer *buffer : _d()->pending_) {
 		buffer->cancel();
 		camera_->bufferCompleted.emit(this, buffer);
 	}
 
-	pending_.clear();
+	_d()->pending_.clear();
 	cancelled_ = true;
 }
 
@@ -333,7 +348,7 @@  bool Request::completeBuffer(FrameBuffer *buffer)
 {
 	LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
 
-	int ret = pending_.erase(buffer);
+	int ret = _d()->pending_.erase(buffer);
 	ASSERT(ret == 1);
 
 	buffer->_d()->setRequest(nullptr);
@@ -361,7 +376,7 @@  std::string Request::toString() const
 
 	/* Example Output: Request(55:P:1/2:6523524) */
 	ss << "Request(" << sequence_ << ":" << statuses[status_] << ":"
-	   << pending_.size() << "/" << bufferMap_.size() << ":"
+	   << _d()->pending_.size() << "/" << bufferMap_.size() << ":"
 	   << cookie_ << ")";
 
 	return ss.str();