[libcamera-devel] [PATCH 01/10] libcamera: request: Make Request class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 01:37:43 CET 2021
On Thu, Nov 11, 2021 at 09:30:08AM +0100, Jacopo Mondi wrote:
> On Tue, Nov 09, 2021 at 12:08:50PM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2021-10-28 12:15:11)
> > > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > 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>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Happy to get this patch merged earlier if it helps too, as it unblocks
> > other patches.
>
> FTR I get this warning from checkstyle on this patch.
> Warning that I'm afraid I don't understand
>
> --- src/libcamera/request.cpp
> +++ src/libcamera/request.cpp
> @@ -15,12 +15,12 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/framebuffer.h>
> +#include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/camera.h"
> #include "libcamera/internal/camera_controls.h"
> #include "libcamera/internal/framebuffer.h"
> -#include "libcamera/internal/request.h"
> #include "libcamera/internal/tracepoints.h"
>
> /**
That's because clang-format moved the libcamera/internal/request.h
header to the top (to follow our self-containted compilation check
through top inclusion rule), and moved the licamera/request.h header
that was at the top back in the list of libcamera public headers. Given
that libcamera/internal/request.h includes libcamera/request.h, I think
this is better. Feel free to include this diff in the next version.
> > > ---
> > > include/libcamera/internal/meson.build | 1 +
> > > include/libcamera/internal/request.h | 34 ++++++++++++++++++
> > > include/libcamera/request.h | 6 ++--
> > > src/libcamera/pipeline_handler.cpp | 7 ++--
> > > src/libcamera/request.cpp | 50 +++++++++++++++++++++-----
> > > 5 files changed, 84 insertions(+), 14 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..df0cc014067e
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/request.h
> > > @@ -0,0 +1,34 @@
> > > +/* 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 FrameBuffer;
> > > +
> > > +class Request::Private : public Extensible::Private
> > > +{
> > > + LIBCAMERA_DECLARE_PUBLIC(Request)
> > > +
> > > +public:
> > > + Private(Camera *camera);
> > > + ~Private();
> > > +
> > > + Camera *camera() const { return camera_; }
> > > +
> > > +private:
> > > + Camera *camera_;
> > > + bool cancelled_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index d16904e6b679..d6bd0ba2ac17 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,
> > > @@ -70,7 +72,6 @@ private:
> > >
> > > bool completeBuffer(FrameBuffer *buffer);
> > >
> > > - Camera *camera_;
> > > ControlList *controls_;
> > > ControlList *metadata_;
> > > BufferMap bufferMap_;
> > > @@ -79,7 +80,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 17fefab7ad0e..33fee1ac05ba 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -20,10 +20,11 @@
> > > #include "libcamera/internal/camera.h"
> > > #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
> > > */
> > >
> > > @@ -31,6 +32,37 @@ 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).
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a Request::Private
> > > + * \param camera The Camera that creates the request
> > > + */
> > > +Request::Private::Private(Camera *camera)
> > > + : camera_(camera), cancelled_(false)
> > > +{
> > > +}
> > > +
> > > +Request::Private::~Private()
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \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
> > > @@ -75,8 +107,8 @@ 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)
> > > {
> > > controls_ = new ControlList(controls::controls,
> > > camera->_d()->validator());
> > > @@ -126,7 +158,7 @@ void Request::reuse(ReuseFlag flags)
> > >
> > > sequence_ = 0;
> > > status_ = RequestPending;
> > > - cancelled_ = false;
> > > + _d()->cancelled_ = false;
> > >
> > > controls_->clear();
> > > metadata_->clear();
> > > @@ -282,7 +314,7 @@ void Request::complete()
> > > ASSERT(status_ == RequestPending);
> > > ASSERT(!hasPendingBuffers());
> > >
> > > - status_ = cancelled_ ? RequestCancelled : RequestComplete;
> > > + status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;
> > >
> > > LOG(Request, Debug) << toString();
> > >
> > > @@ -299,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;
> > > }
> > >
> > > /**
> > > @@ -335,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();
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list