[libcamera-devel,01/10] libcamera: request: Make Request class Extensible

14499 diff mbox series

From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 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

Quoting Jacopo Mondi (2021-10-28 12:15:11)
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Happy to get this patch merged earlier if it helps too, as it unblocks
other patches.


> ---
>  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();
>  }
> -- 
> 2.33.1
>