From patchwork Mon Oct 18 11:54:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 14525 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by patchwork.libcamera.org (Postfix) with ESMTPS id 1FDA8BDB1C for ; Wed, 10 Nov 2021 14:01:27 +0000 (UTC) Received: from pendragon.ideasonboard.com (cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B1760501 for ; Wed, 10 Nov 2021 15:01:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1636552886; bh=IW45UBEXrWxqza+XU8+8MMsAX0+1SBOwgKbSev9XWK4=; h=From:To:Date:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:Resent-From:Resent-To:From; b=PpqS5stxRWbuWx2pb4XvfafjIYYlmTupsmHYiS975qQ3IURpNPtYElNJXmo3EgwN2 oJnlcj74x2bFzmcYnvYarvFwvemPSywXBDs7KAYiEJd8fSwec+RYDVKUg3vE9/QqA9 cZUKY8YnBqRAseQdtU3U7S2ekhQUaJ0l8PtTYazE= Delivered-To: kbingham@ideasonboard.com Received: from perceval.ideasonboard.com by perceval.ideasonboard.com with LMTP id WO00MJFgbWFUCQAA4E0KoQ (envelope-from ) for ; Mon, 18 Oct 2021 13:54:57 +0200 Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [IPv6:2001:4b98:dc0:43:216:3eff:fe33:f827]) by perceval.ideasonboard.com (Postfix) with ESMTPS id A49898C6; Mon, 18 Oct 2021 13:54:57 +0200 (CEST) Authentication-Results: perceval.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="wN7LEOc3"; dkim-atps=neutral Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id EAB5F68F56; Mon, 18 Oct 2021 13:54:56 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="wN7LEOc3"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id EBBF168F56 for ; Mon, 18 Oct 2021 13:54:54 +0200 (CEST) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 70E6A8C6 for ; Mon, 18 Oct 2021 13:54:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634558094; bh=IW45UBEXrWxqza+XU8+8MMsAX0+1SBOwgKbSev9XWK4=; h=From:To:Subject:Date:From; b=wN7LEOc3nbd5sicgyrP/BFPJUsIkmiry9t62QL449SkyvYkhNk0xZXAFPijQaVCxB pwwW0n7GL+vHVcDSQscoIv7tYsrauV7Xt/lkZSBLHLq762uzyk1BIsntv7iseDW2cT xYbfhuHlWoOA8fJlYIGJp+373z2u3S765eRqsMI8= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 18 Oct 2021 14:54:31 +0300 Message-Id: <20211018115431.17946-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.32.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH] libcamera: request: Make Request class Extensible X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" X-TUID: pRSGHd+qrqpx Resent-From: Kieran Bingham Resent-To: parsemail@patchwork.libcamera.org 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 --- 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 base-commit: 2f75a7e5b8c6258dc12e9e3128cb30133f66b4f9 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 + +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(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(); }