[{"id":22598,"web_url":"https://patchwork.libcamera.org/comment/22598/","msgid":"","date":"2022-04-06T01:15:51","subject":"Re: [libcamera-devel] [PATCH 2/9] libcamera: Add options() and\n\tsetOptions() operations to Camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-Lin,\n\nThank you for the patch.\n\nOn Wed, Feb 09, 2022 at 03:19:10PM +0800, Han-Lin Chen wrote:\n> Add options() and setOptions() operations to Camera. Both operations\n> takes a camera instance and reads or writes the options to it. Because\n> an option might change the supported characteristics of a camera module,\n> it could only be set before configuring.\n\nThe problem of configuring pipeline handlers is something I've\nconsidered before, but I haven't gone far enough to propose a good\nsolution. You're right that configuration can't change after\nconfiguration, but I think we need an even stricter constraint. The\nconfiguration file may affect the features and capabilities exposed by\nthe pipeline handler through the camera (for instance it could affect\ncontrol limits, due to different IPA configurations, but also possibly\nthe list of controls themselves, and of course the capabilities of the\nstreams, in particular the list of supported resolutions), and I think\nit thus needs to be set before even querying the camera for any of the\nsupported features. There's a bit of a chicken and egg issue, we can't\nset options before the Camera instance is constructed if the API to set\noptions it exposed by the Camera class.\n\nOne option would be to instead expose the API through the CameraManager\nclass, and require options to be set before the CameraManager is\nstarted. The API would need to support per-camera options, which could\nbe set based on the camera ID. We may also decide to only support a\nsingle option, the name of a top-level configuration file, which would\ncontain per-camera options and possibly point to other files (in\nparticular I think IPA tuning files should be separate, but we may want\nto split other configuration data to separate files too - I'm not sure\nwhat data and why though).\n\nWe may also rework the libcamera initialization API further, including\nhow and when Camera objects are constructed, and hower applications get\nhold of them. Feel free to think out of the box here, we're really in\nbrainstorming mode.\n\nYou're tackling a lot of hard problems in a single patch series, so I'm\nraising lots of potential issues in return. I'm sorry about that, please\nrest assured I'm grateful for your work on this topic. I'll try to help\nas much as possible.\n\n> Signed-off-by: Han-Lin Chen \n> ---\n> include/libcamera/camera.h | 3 +\n> include/libcamera/internal/camera.h | 2 +\n> include/libcamera/internal/pipeline_handler.h | 2 +\n> src/libcamera/camera.cpp | 59 +++++++++++++++++++\n> src/libcamera/pipeline_handler.cpp | 26 ++++++++\n> 5 files changed, 92 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 5bb06584..1bb80a6d 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -98,9 +98,12 @@ public:\n> \tSignal requestCompleted;\n> \tSignal<> disconnected;\n> \n> +\tint setOptions(const ControlList *options = nullptr);\n> +\n> \tint acquire();\n> \tint release();\n> \n> +\tconst ControlInfoMap &options() const;\n> \tconst ControlInfoMap &controls() const;\n> \tconst ControlList &properties() const;\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 597426a6..c3cb1865 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -34,6 +34,8 @@ public:\n> \tPipelineHandler *pipe() { return pipe_.get(); }\n> \n> \tstd::list queuedRequests_;\n> +\n> +\tControlInfoMap optionInfo_;\n> \tControlInfoMap controlInfo_;\n> \tControlList properties_;\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c3e4c258..184e4b9a 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -45,6 +45,8 @@ public:\n> \tMediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n> \t\t\t\t\tconst DeviceMatch &dm);\n> \n> +\tvirtual int setOptions(Camera *camera, const ControlList *options);\n> +\n> \tbool lock();\n> \tvoid unlock();\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index bb856d60..9e95e6cc 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -480,6 +480,16 @@ Camera::Private::~Private()\n> * well, when implementing official support for control info updates.\n> */\n> \n> +/**\n> + * \\var Camera::Private::optionInfo_\n> + * \\brief The set of options supported by the camera\n> + *\n> + * The optionInfo_ information shall be initialised by the pipeline handler when\n> + * creating the camera.\n> + *\n> + * This member was meant to stay constant after the camera is created.\n> + */\n> +\n> /**\n> * \\var Camera::Private::properties_\n> * \\brief The list of properties supported by the camera\n> @@ -802,6 +812,40 @@ int Camera::exportFrameBuffers(Stream *stream,\n> \t\t\t\t buffers);\n> }\n> \n> +/**\n> + * \\brief Set Options for a camera.\n> + * \\param[in] options Options to be applied before configuring a camera\n> + *\n> + * Prior to configuring a camera, it's optional to set options to a camera to\n> + * change its characteristics or internal behaviors. The supported options are\n> + * depended on the platform. The caller should iterate supported Options by\n> + * options() before setting them.\n> + *\n> + * Options might change the internal behavior of configure() and capturing\n> + * images, and thus the function can only be called before configure().\n> + * It's encouraged to re-check the results of controls(), properties() and\n> + * generateConfiguration() after setting options to a camera.\n> + *\n> + * \\context This function shall be synchronized by the caller with other\n> + * functions that affect the camera state.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EACCES The camera is not in a state where it can set options\n> + * \\retval -EINVAL The options is not valid\n> + */\n> +int Camera::setOptions(const ControlList *options)\n> +{\n> +\tPrivate *const d = _d();\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t Private::CameraAcquired);\n> +\tif (ret < 0)\n> +\t\treturn -EACCES;\n> +\n> +\treturn d->pipe_->invokeMethod(&PipelineHandler::setOptions,\n> +\t\t\t\t ConnectionTypeBlocking, this, options);\n> +}\n> +\n> /**\n> * \\brief Acquire the camera device for exclusive access\n> *\n> @@ -894,6 +938,21 @@ const ControlInfoMap &Camera::controls() const\n> \treturn _d()->controlInfo_;\n> }\n> \n> +/**\n> + * \\brief Retrieve the list of options supported by the camera\n> + *\n> + * The list of options supported by the camera and their associated\n> + * constraints remain constant through the lifetime of the Camera object.\n> + *\n> + * \\context This function is \\threadsafe.\n> + *\n> + * \\return A ControlInfoMap listing the options supported by the camera\n> + */\n> +const ControlInfoMap &Camera::options() const\n> +{\n> +\treturn _d()->optionInfo_;\n> +}\n> +\n> /**\n> * \\brief Retrieve the list of properties of the camera\n> *\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 7ebd76ad..1f2a6d7e 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -142,6 +142,32 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> \treturn media.get();\n> }\n> \n> +/**\n> + * \\fn PipelineHandler::setOptions()\n> + * \\brief Set options for camera\n> + * \\param[in] camera The camera to set options\n> + * \\param[in] options Options to be applied to the Camera\n> + *\n> + * Set options to the camera. The supported options are different for each\n> + * pipeline handler. The intended caller of the this function could iterate\n> + * options supported by the camera with Camera::options().\n> + *\n> + * The intended caller of this function is the Camera class which will in turn\n> + * be called from the application to indicate that a set of options should be\n> + * applied. The user should check supported options with Camera::options()\n> + * before setting them.\n> + *\n> + * \\context This function is called from the CameraManager thread.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int PipelineHandler::setOptions([[maybe_unused]] Camera *camera,\n> +\t\t\t\t[[maybe_unused]] const ControlList *options)\n> +{\n> +\t/* The default implementation which supported no options. */\n> +\treturn -EINVAL;\n> +}\n> +\n> /**\n> * \\brief Lock all media devices acquired by the pipeline\n> *","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 80F0BC0F1B\n\tfor ;\n\tWed, 6 Apr 2022 01:15:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC8A65644;\n\tWed, 6 Apr 2022 03:15:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07305604B6\n\tfor ;\n\tWed, 6 Apr 2022 03:15:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F1B5482;\n\tWed, 6 Apr 2022 03:15:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649207755;\n\tbh=4rQKAgP0axF/BoidZraxYCxfgvNVt8V1KEeh9Qc7sT0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1UP9E8KYMT52Fy33rQ63Ya5ZylReEfBqKXhCUgm8XiyDUoLagFYdx/gCZ8XeX0+2F\n\tz+2+g3+6UIRKdOxd9BEb/eXkqn9CUh9DO5hbFpjZ7HFtoRIf7eG2hy9lElk5C0yW5K\n\tFgQ2kbiTophpXjqKO3Xaaz9WK0Jt5jyCWFBPRiAgQsn3OaiN/lgBtYmpsjO+GROn6l\n\tzCEef8PWrWEeICMZzMyOA/o5dlKOZ24dLqlOAhIhU6AUfoGUbmp//QY0ZWpU4n5AVI\n\tAG8tizFw0MTXwda36qCCrlH/lQySp8PcBOxCpm9VjVGd3QcB4qGr+QCNVNAM/wvndf\n\tTZa79VZLv6+rA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649207754;\n\tbh=4rQKAgP0axF/BoidZraxYCxfgvNVt8V1KEeh9Qc7sT0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KbkBMoxX2tSI6OczZegtebvn7itBuqy/fSaVJJX1u+7Wyz9z428Ke3N7BFiZU1IiG\n\t+gzDv2RhBbGDhDk3Gavh5lBnsvxc76GTDRxKG+5maSg4RAiVcAPQ6Q0s4tRAKZfeb7\n\tXdcSYOrL05GDo2KHVJm3G3PGPdIarTUaRvJQRrBc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KbkBMoxX\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 04:15:51 +0300","To":"Han-Lin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-3-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220209071917.559993-3-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 2/9] libcamera: Add options() and\n\tsetOptions() operations to Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]