[{"id":2311,"web_url":"https://patchwork.libcamera.org/comment/2311/","msgid":"<20190805114005.GL29747@pendragon.ideasonboard.com>","date":"2019-08-05T11:40:05","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: camera: Make camera\n\tstate accessible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Aug 01, 2019 at 05:54:16PM +0200, Jacopo Mondi wrote:\n> Make the Camera state accessible by providing an accessor operation and\n> moving the State enumeration definition in the public scope.\n> \n> Signed-off-by: Jacopo Mondi \n\nI'm in two minds about this. The state is supposed to be internal, and\nexposing it will create an ABI that will make it more difficult to\nchange it later. Looking at the camera HAL implementation, you use the\nstate in two places only, once to check if a request is the first one,\nand once to decide if stop() should be called. The latter isn't needed,\nyou can call stop() unconditionally. I'm tempted to ask you to drop this\npatch and keep the information you need for the first use inside the\ncamera HAL implementation, at least until we find out if there are other\nreasons to expose the state. Would that be OK ?\n\n> ---\n> include/libcamera/camera.h | 16 +++++++++-------\n> src/libcamera/camera.cpp | 33 +++++++++++++++++++++++++++------\n> 2 files changed, 36 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 21fac550f412..6e627943b3f1 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -66,6 +66,14 @@ protected:\n> \tstd::vector config_;\n> };\n> \n> +enum State {\n> +\tCameraAvailable,\n> +\tCameraAcquired,\n> +\tCameraConfigured,\n> +\tCameraPrepared,\n> +\tCameraRunning,\n> +};\n> +\n> class Camera final : public std::enable_shared_from_this\n> {\n> public:\n> @@ -77,6 +85,7 @@ public:\n> \tCamera &operator=(const Camera &) = delete;\n> \n> \tconst std::string &name() const;\n> +\tState state() const { return state_; }\n> \n> \tSignal bufferCompleted;\n> \tSignal &> requestCompleted;\n> @@ -101,13 +110,6 @@ public:\n> \tint stop();\n> \n> private:\n> -\tenum State {\n> -\t\tCameraAvailable,\n> -\t\tCameraAcquired,\n> -\t\tCameraConfigured,\n> -\t\tCameraPrepared,\n> -\t\tCameraRunning,\n> -\t};\n> \n> \tCamera(PipelineHandler *pipe, const std::string &name);\n> \t~Camera();\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 76c737cb9381..e924c2085816 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -253,6 +253,21 @@ std::size_t CameraConfiguration::size() const\n> * \\brief The vector of stream configurations\n> */\n> \n> +/**\n> + * \\enum State\n> + * \\brief Describe the Camera state as defined in \\ref camera_states\n> + * \\var State::CameraAvailable\n> + * See \\ref camera_available\n> + * \\var State::CameraAcquired\n> + * See \\ref camera_acquired\n> + * \\var State::CameraConfigured\n> + * See \\ref camera_configured\n> + * \\var State::CameraPrepared\n> + * See \\ref camera_prepared\n> + * \\var State::CameraRunning\n> + * See \\ref camera_running\n> + */\n> +\n> /**\n> * \\class Camera\n> * \\brief Camera device\n> @@ -284,7 +299,7 @@ std::size_t CameraConfiguration::size() const\n> * not released. The camera may also be reconfigured provided that all\n> * resources allocated are freed prior to the reconfiguration.\n> *\n> - * \\subsection Camera States\n> + * \\subsection camera_states Camera States\n> *\n> * To help manage the sequence of operations needed to control the camera a set\n> * of states are defined. Each state describes which operations may be performed\n> @@ -318,27 +333,27 @@ std::size_t CameraConfiguration::size() const\n> * }\n> * \\enddot\n> *\n> - * \\subsubsection Available\n> + * \\subsubsection camera_available Available\n> * The base state of a camera, an application can inspect the properties of the\n> * camera to determine if it wishes to use it. If an application wishes to use\n> * a camera it should acquire() it to proceed to the Acquired state.\n> *\n> - * \\subsubsection Acquired\n> + * \\subsubsection camera_acquired Acquired\n> * In the acquired state an application has exclusive access to the camera and\n> * may modify the camera's parameters to configure it and proceed to the\n> * Configured state.\n> *\n> - * \\subsubsection Configured\n> + * \\subsubsection camera_configured Configured\n> * The camera is configured and ready for the application to prepare it with\n> * resources. The camera may be reconfigured multiple times until resources\n> * are provided and the state progresses to Prepared.\n> *\n> - * \\subsubsection Prepared\n> + * \\subsubsection camera_prepared Prepared\n> * The camera has been configured and provided with resources and is ready to be\n> * started. The application may free the camera's resources to get back to the\n> * Configured state or start() it to progress to the Running state.\n> *\n> - * \\subsubsection Running\n> + * \\subsubsection camera_running Running\n> * The camera is running and ready to process requests queued by the\n> * application. The camera remains in this state until it is stopped and moved\n> * to the Prepared state.\n> @@ -380,6 +395,12 @@ const std::string &Camera::name() const\n> \treturn name_;\n> }\n> \n> +/**\n> + * \\fn Camera::state()\n> + * \\brief Retrieve the current camera state as defined in \\ref camera_states\n> + * \\return The current camera state\n> + */\n> +\n> /**\n> * \\var Camera::bufferCompleted\n> * \\brief Signal emitted when a buffer for a request queued to the camera has","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81D4960E33\n\tfor ;\n\tMon, 5 Aug 2019 13:40:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AEE092F9;\n\tMon, 5 Aug 2019 13:40:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565005207;\n\tbh=OmjBzl0SFBZp9M8Lk4V3hV6vi/VYZzLCyZjnXEFzjGE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u1hcai5pIWnBQ1DLxrLy0ZLz0OPihs0TJF5ASIWtTtrzRamC5nwDBS6MpnjHy3mc+\n\tIN0Q9j3YfLUQ3/gtPSiYq+W0hxgwYqWJV3f23BaKurkibmFZvTShMMDhpNnC99InjU\n\tEnVjrDyCWsMHNN5o1OHOLY2ldq2kguPIsurLmHeo=","Date":"Mon, 5 Aug 2019 14:40:05 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190805114005.GL29747@pendragon.ideasonboard.com>","References":"<20190801155420.24694-1-jacopo@jmondi.org>\n\t<20190801155420.24694-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190801155420.24694-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: camera: Make camera\n\tstate accessible","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 05 Aug 2019 11:40:08 -0000"}},{"id":2322,"web_url":"https://patchwork.libcamera.org/comment/2322/","msgid":"<20190806093621.2hvgdbjsgenyvaye@uno.localdomain>","date":"2019-08-06T09:36:21","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: camera: Make camera\n\tstate accessible","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Aug 05, 2019 at 02:40:05PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 01, 2019 at 05:54:16PM +0200, Jacopo Mondi wrote:\n> > Make the Camera state accessible by providing an accessor operation and\n> > moving the State enumeration definition in the public scope.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> I'm in two minds about this. The state is supposed to be internal, and\n> exposing it will create an ABI that will make it more difficult to\n> change it later. Looking at the camera HAL implementation, you use the\n> state in two places only, once to check if a request is the first one,\n> and once to decide if stop() should be called. The latter isn't needed,\n> you can call stop() unconditionally. I'm tempted to ask you to drop this\n> patch and keep the information you need for the first use inside the\n> camera HAL implementation, at least until we find out if there are other\n> reasons to expose the state. Would that be OK ?\n>\n\nI agree, I could keep the state in the HAL. I tried avoiding doing\nthat as the internal state would actually replicate the camera state,\nand the less places where the same information is replicated the\nbetter, but I can work around it for sure.\n\nThanks\n j\n\n> > ---\n> > include/libcamera/camera.h | 16 +++++++++-------\n> > src/libcamera/camera.cpp | 33 +++++++++++++++++++++++++++------\n> > 2 files changed, 36 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 21fac550f412..6e627943b3f1 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -66,6 +66,14 @@ protected:\n> > \tstd::vector config_;\n> > };\n> >\n> > +enum State {\n> > +\tCameraAvailable,\n> > +\tCameraAcquired,\n> > +\tCameraConfigured,\n> > +\tCameraPrepared,\n> > +\tCameraRunning,\n> > +};\n> > +\n> > class Camera final : public std::enable_shared_from_this\n> > {\n> > public:\n> > @@ -77,6 +85,7 @@ public:\n> > \tCamera &operator=(const Camera &) = delete;\n> >\n> > \tconst std::string &name() const;\n> > +\tState state() const { return state_; }\n> >\n> > \tSignal bufferCompleted;\n> > \tSignal &> requestCompleted;\n> > @@ -101,13 +110,6 @@ public:\n> > \tint stop();\n> >\n> > private:\n> > -\tenum State {\n> > -\t\tCameraAvailable,\n> > -\t\tCameraAcquired,\n> > -\t\tCameraConfigured,\n> > -\t\tCameraPrepared,\n> > -\t\tCameraRunning,\n> > -\t};\n> >\n> > \tCamera(PipelineHandler *pipe, const std::string &name);\n> > \t~Camera();\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 76c737cb9381..e924c2085816 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -253,6 +253,21 @@ std::size_t CameraConfiguration::size() const\n> > * \\brief The vector of stream configurations\n> > */\n> >\n> > +/**\n> > + * \\enum State\n> > + * \\brief Describe the Camera state as defined in \\ref camera_states\n> > + * \\var State::CameraAvailable\n> > + * See \\ref camera_available\n> > + * \\var State::CameraAcquired\n> > + * See \\ref camera_acquired\n> > + * \\var State::CameraConfigured\n> > + * See \\ref camera_configured\n> > + * \\var State::CameraPrepared\n> > + * See \\ref camera_prepared\n> > + * \\var State::CameraRunning\n> > + * See \\ref camera_running\n> > + */\n> > +\n> > /**\n> > * \\class Camera\n> > * \\brief Camera device\n> > @@ -284,7 +299,7 @@ std::size_t CameraConfiguration::size() const\n> > * not released. The camera may also be reconfigured provided that all\n> > * resources allocated are freed prior to the reconfiguration.\n> > *\n> > - * \\subsection Camera States\n> > + * \\subsection camera_states Camera States\n> > *\n> > * To help manage the sequence of operations needed to control the camera a set\n> > * of states are defined. Each state describes which operations may be performed\n> > @@ -318,27 +333,27 @@ std::size_t CameraConfiguration::size() const\n> > * }\n> > * \\enddot\n> > *\n> > - * \\subsubsection Available\n> > + * \\subsubsection camera_available Available\n> > * The base state of a camera, an application can inspect the properties of the\n> > * camera to determine if it wishes to use it. If an application wishes to use\n> > * a camera it should acquire() it to proceed to the Acquired state.\n> > *\n> > - * \\subsubsection Acquired\n> > + * \\subsubsection camera_acquired Acquired\n> > * In the acquired state an application has exclusive access to the camera and\n> > * may modify the camera's parameters to configure it and proceed to the\n> > * Configured state.\n> > *\n> > - * \\subsubsection Configured\n> > + * \\subsubsection camera_configured Configured\n> > * The camera is configured and ready for the application to prepare it with\n> > * resources. The camera may be reconfigured multiple times until resources\n> > * are provided and the state progresses to Prepared.\n> > *\n> > - * \\subsubsection Prepared\n> > + * \\subsubsection camera_prepared Prepared\n> > * The camera has been configured and provided with resources and is ready to be\n> > * started. The application may free the camera's resources to get back to the\n> > * Configured state or start() it to progress to the Running state.\n> > *\n> > - * \\subsubsection Running\n> > + * \\subsubsection camera_running Running\n> > * The camera is running and ready to process requests queued by the\n> > * application. The camera remains in this state until it is stopped and moved\n> > * to the Prepared state.\n> > @@ -380,6 +395,12 @@ const std::string &Camera::name() const\n> > \treturn name_;\n> > }\n> >\n> > +/**\n> > + * \\fn Camera::state()\n> > + * \\brief Retrieve the current camera state as defined in \\ref camera_states\n> > + * \\return The current camera state\n> > + */\n> > +\n> > /**\n> > * \\var Camera::bufferCompleted\n> > * \\brief Signal emitted when a buffer for a request queued to the camera has\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0667160E31\n\tfor ;\n\tTue, 6 Aug 2019 11:34:58 +0200 (CEST)","from uno.localdomain\n\t(host150-24-dynamic.51-79-r.retail.telecomitalia.it [79.51.24.150])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 4D2A41C0006;\n\tTue, 6 Aug 2019 09:34:57 +0000 (UTC)"],"X-Originating-IP":"79.51.24.150","Date":"Tue, 6 Aug 2019 11:36:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190806093621.2hvgdbjsgenyvaye@uno.localdomain>","References":"<20190801155420.24694-1-jacopo@jmondi.org>\n\t<20190801155420.24694-2-jacopo@jmondi.org>\n\t<20190805114005.GL29747@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qxnfg5yc3po4omp2\"","Content-Disposition":"inline","In-Reply-To":"<20190805114005.GL29747@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: camera: Make camera\n\tstate accessible","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 06 Aug 2019 09:34:58 -0000"}}]