[libcamera-devel] [PATCH] libcamera: request: Make Request class Extensible
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Oct 19 17:20:08 CEST 2021
Hi Laurent,
Quoting Laurent Pinchart (2021-10-18 12:54:31)
> 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.
Ahh, I have a very similar patch in my branch in the section 'ready to
post' cleaned up from [0] but I've been distracted too much lately with
CTS to send it...
At least I have something to compare as I go down through this ;-)
[0] https://patchwork.libcamera.org/patch/12016/
> 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.
2021 ...
> + *
> + * 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_;
Aha, I think this is what prompted me to write [1]:
libcamera: camera: Create a CameraControlValidator
libcamera: request: Use external CameraControlValidator
[1] https://patchwork.libcamera.org/project/libcamera/list/?series=2335
I guess I should repost those too.
> + 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_;
My patch took 'pending_' over to the private, as it was really minimal
changes. But we can move other parts across on top too.
> };
>
> } /* 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
I have the following here :
+ *
+ * \file internal/request.h
+ * \brief Provides the internal implementation details for a Request
Should it be added? or is it not needed...
With that if needed:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
But further follow up below regarding CameraControlValidator
> */
>
> @@ -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?
Yes, patches are already posted. But it was when you had a lot of churn
going, so I couldn't integrate them at the time. I can't remember the
full details here. Perhaps the context is on the list.
If I recall correctly, I was trying to get that done before making
Request use private which is why my version of this patch didn't get
posted.
Ho hum - I guess it will get done somehow.
> + */
> + 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