| Message ID | 20190801155420.24694-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Aug 01, 2019 at 05:54:16PM +0200, Jacopo Mondi wrote: > Make the Camera state accessible by providing an accessor operation and > moving the State enumeration definition in the public scope. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> I'm in two minds about this. The state is supposed to be internal, and exposing it will create an ABI that will make it more difficult to change it later. Looking at the camera HAL implementation, you use the state in two places only, once to check if a request is the first one, and once to decide if stop() should be called. The latter isn't needed, you can call stop() unconditionally. I'm tempted to ask you to drop this patch and keep the information you need for the first use inside the camera HAL implementation, at least until we find out if there are other reasons to expose the state. Would that be OK ? > --- > include/libcamera/camera.h | 16 +++++++++------- > src/libcamera/camera.cpp | 33 +++++++++++++++++++++++++++------ > 2 files changed, 36 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 21fac550f412..6e627943b3f1 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -66,6 +66,14 @@ protected: > std::vector<StreamConfiguration> config_; > }; > > +enum State { > + CameraAvailable, > + CameraAcquired, > + CameraConfigured, > + CameraPrepared, > + CameraRunning, > +}; > + > class Camera final : public std::enable_shared_from_this<Camera> > { > public: > @@ -77,6 +85,7 @@ public: > Camera &operator=(const Camera &) = delete; > > const std::string &name() const; > + State state() const { return state_; } > > Signal<Request *, Buffer *> bufferCompleted; > Signal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted; > @@ -101,13 +110,6 @@ public: > int stop(); > > private: > - enum State { > - CameraAvailable, > - CameraAcquired, > - CameraConfigured, > - CameraPrepared, > - CameraRunning, > - }; > > Camera(PipelineHandler *pipe, const std::string &name); > ~Camera(); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 76c737cb9381..e924c2085816 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -253,6 +253,21 @@ std::size_t CameraConfiguration::size() const > * \brief The vector of stream configurations > */ > > +/** > + * \enum State > + * \brief Describe the Camera state as defined in \ref camera_states > + * \var State::CameraAvailable > + * See \ref camera_available > + * \var State::CameraAcquired > + * See \ref camera_acquired > + * \var State::CameraConfigured > + * See \ref camera_configured > + * \var State::CameraPrepared > + * See \ref camera_prepared > + * \var State::CameraRunning > + * See \ref camera_running > + */ > + > /** > * \class Camera > * \brief Camera device > @@ -284,7 +299,7 @@ std::size_t CameraConfiguration::size() const > * not released. The camera may also be reconfigured provided that all > * resources allocated are freed prior to the reconfiguration. > * > - * \subsection Camera States > + * \subsection camera_states Camera States > * > * To help manage the sequence of operations needed to control the camera a set > * of states are defined. Each state describes which operations may be performed > @@ -318,27 +333,27 @@ std::size_t CameraConfiguration::size() const > * } > * \enddot > * > - * \subsubsection Available > + * \subsubsection camera_available Available > * The base state of a camera, an application can inspect the properties of the > * camera to determine if it wishes to use it. If an application wishes to use > * a camera it should acquire() it to proceed to the Acquired state. > * > - * \subsubsection Acquired > + * \subsubsection camera_acquired Acquired > * In the acquired state an application has exclusive access to the camera and > * may modify the camera's parameters to configure it and proceed to the > * Configured state. > * > - * \subsubsection Configured > + * \subsubsection camera_configured Configured > * The camera is configured and ready for the application to prepare it with > * resources. The camera may be reconfigured multiple times until resources > * are provided and the state progresses to Prepared. > * > - * \subsubsection Prepared > + * \subsubsection camera_prepared Prepared > * The camera has been configured and provided with resources and is ready to be > * started. The application may free the camera's resources to get back to the > * Configured state or start() it to progress to the Running state. > * > - * \subsubsection Running > + * \subsubsection camera_running Running > * The camera is running and ready to process requests queued by the > * application. The camera remains in this state until it is stopped and moved > * to the Prepared state. > @@ -380,6 +395,12 @@ const std::string &Camera::name() const > return name_; > } > > +/** > + * \fn Camera::state() > + * \brief Retrieve the current camera state as defined in \ref camera_states > + * \return The current camera state > + */ > + > /** > * \var Camera::bufferCompleted > * \brief Signal emitted when a buffer for a request queued to the camera has
Hi Laurent, On Mon, Aug 05, 2019 at 02:40:05PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Aug 01, 2019 at 05:54:16PM +0200, Jacopo Mondi wrote: > > Make the Camera state accessible by providing an accessor operation and > > moving the State enumeration definition in the public scope. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > I'm in two minds about this. The state is supposed to be internal, and > exposing it will create an ABI that will make it more difficult to > change it later. Looking at the camera HAL implementation, you use the > state in two places only, once to check if a request is the first one, > and once to decide if stop() should be called. The latter isn't needed, > you can call stop() unconditionally. I'm tempted to ask you to drop this > patch and keep the information you need for the first use inside the > camera HAL implementation, at least until we find out if there are other > reasons to expose the state. Would that be OK ? > I agree, I could keep the state in the HAL. I tried avoiding doing that as the internal state would actually replicate the camera state, and the less places where the same information is replicated the better, but I can work around it for sure. Thanks j > > --- > > include/libcamera/camera.h | 16 +++++++++------- > > src/libcamera/camera.cpp | 33 +++++++++++++++++++++++++++------ > > 2 files changed, 36 insertions(+), 13 deletions(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 21fac550f412..6e627943b3f1 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -66,6 +66,14 @@ protected: > > std::vector<StreamConfiguration> config_; > > }; > > > > +enum State { > > + CameraAvailable, > > + CameraAcquired, > > + CameraConfigured, > > + CameraPrepared, > > + CameraRunning, > > +}; > > + > > class Camera final : public std::enable_shared_from_this<Camera> > > { > > public: > > @@ -77,6 +85,7 @@ public: > > Camera &operator=(const Camera &) = delete; > > > > const std::string &name() const; > > + State state() const { return state_; } > > > > Signal<Request *, Buffer *> bufferCompleted; > > Signal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted; > > @@ -101,13 +110,6 @@ public: > > int stop(); > > > > private: > > - enum State { > > - CameraAvailable, > > - CameraAcquired, > > - CameraConfigured, > > - CameraPrepared, > > - CameraRunning, > > - }; > > > > Camera(PipelineHandler *pipe, const std::string &name); > > ~Camera(); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 76c737cb9381..e924c2085816 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -253,6 +253,21 @@ std::size_t CameraConfiguration::size() const > > * \brief The vector of stream configurations > > */ > > > > +/** > > + * \enum State > > + * \brief Describe the Camera state as defined in \ref camera_states > > + * \var State::CameraAvailable > > + * See \ref camera_available > > + * \var State::CameraAcquired > > + * See \ref camera_acquired > > + * \var State::CameraConfigured > > + * See \ref camera_configured > > + * \var State::CameraPrepared > > + * See \ref camera_prepared > > + * \var State::CameraRunning > > + * See \ref camera_running > > + */ > > + > > /** > > * \class Camera > > * \brief Camera device > > @@ -284,7 +299,7 @@ std::size_t CameraConfiguration::size() const > > * not released. The camera may also be reconfigured provided that all > > * resources allocated are freed prior to the reconfiguration. > > * > > - * \subsection Camera States > > + * \subsection camera_states Camera States > > * > > * To help manage the sequence of operations needed to control the camera a set > > * of states are defined. Each state describes which operations may be performed > > @@ -318,27 +333,27 @@ std::size_t CameraConfiguration::size() const > > * } > > * \enddot > > * > > - * \subsubsection Available > > + * \subsubsection camera_available Available > > * The base state of a camera, an application can inspect the properties of the > > * camera to determine if it wishes to use it. If an application wishes to use > > * a camera it should acquire() it to proceed to the Acquired state. > > * > > - * \subsubsection Acquired > > + * \subsubsection camera_acquired Acquired > > * In the acquired state an application has exclusive access to the camera and > > * may modify the camera's parameters to configure it and proceed to the > > * Configured state. > > * > > - * \subsubsection Configured > > + * \subsubsection camera_configured Configured > > * The camera is configured and ready for the application to prepare it with > > * resources. The camera may be reconfigured multiple times until resources > > * are provided and the state progresses to Prepared. > > * > > - * \subsubsection Prepared > > + * \subsubsection camera_prepared Prepared > > * The camera has been configured and provided with resources and is ready to be > > * started. The application may free the camera's resources to get back to the > > * Configured state or start() it to progress to the Running state. > > * > > - * \subsubsection Running > > + * \subsubsection camera_running Running > > * The camera is running and ready to process requests queued by the > > * application. The camera remains in this state until it is stopped and moved > > * to the Prepared state. > > @@ -380,6 +395,12 @@ const std::string &Camera::name() const > > return name_; > > } > > > > +/** > > + * \fn Camera::state() > > + * \brief Retrieve the current camera state as defined in \ref camera_states > > + * \return The current camera state > > + */ > > + > > /** > > * \var Camera::bufferCompleted > > * \brief Signal emitted when a buffer for a request queued to the camera has > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 21fac550f412..6e627943b3f1 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -66,6 +66,14 @@ protected: std::vector<StreamConfiguration> config_; }; +enum State { + CameraAvailable, + CameraAcquired, + CameraConfigured, + CameraPrepared, + CameraRunning, +}; + class Camera final : public std::enable_shared_from_this<Camera> { public: @@ -77,6 +85,7 @@ public: Camera &operator=(const Camera &) = delete; const std::string &name() const; + State state() const { return state_; } Signal<Request *, Buffer *> bufferCompleted; Signal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted; @@ -101,13 +110,6 @@ public: int stop(); private: - enum State { - CameraAvailable, - CameraAcquired, - CameraConfigured, - CameraPrepared, - CameraRunning, - }; Camera(PipelineHandler *pipe, const std::string &name); ~Camera(); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 76c737cb9381..e924c2085816 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -253,6 +253,21 @@ std::size_t CameraConfiguration::size() const * \brief The vector of stream configurations */ +/** + * \enum State + * \brief Describe the Camera state as defined in \ref camera_states + * \var State::CameraAvailable + * See \ref camera_available + * \var State::CameraAcquired + * See \ref camera_acquired + * \var State::CameraConfigured + * See \ref camera_configured + * \var State::CameraPrepared + * See \ref camera_prepared + * \var State::CameraRunning + * See \ref camera_running + */ + /** * \class Camera * \brief Camera device @@ -284,7 +299,7 @@ std::size_t CameraConfiguration::size() const * not released. The camera may also be reconfigured provided that all * resources allocated are freed prior to the reconfiguration. * - * \subsection Camera States + * \subsection camera_states Camera States * * To help manage the sequence of operations needed to control the camera a set * of states are defined. Each state describes which operations may be performed @@ -318,27 +333,27 @@ std::size_t CameraConfiguration::size() const * } * \enddot * - * \subsubsection Available + * \subsubsection camera_available Available * The base state of a camera, an application can inspect the properties of the * camera to determine if it wishes to use it. If an application wishes to use * a camera it should acquire() it to proceed to the Acquired state. * - * \subsubsection Acquired + * \subsubsection camera_acquired Acquired * In the acquired state an application has exclusive access to the camera and * may modify the camera's parameters to configure it and proceed to the * Configured state. * - * \subsubsection Configured + * \subsubsection camera_configured Configured * The camera is configured and ready for the application to prepare it with * resources. The camera may be reconfigured multiple times until resources * are provided and the state progresses to Prepared. * - * \subsubsection Prepared + * \subsubsection camera_prepared Prepared * The camera has been configured and provided with resources and is ready to be * started. The application may free the camera's resources to get back to the * Configured state or start() it to progress to the Running state. * - * \subsubsection Running + * \subsubsection camera_running Running * The camera is running and ready to process requests queued by the * application. The camera remains in this state until it is stopped and moved * to the Prepared state. @@ -380,6 +395,12 @@ const std::string &Camera::name() const return name_; } +/** + * \fn Camera::state() + * \brief Retrieve the current camera state as defined in \ref camera_states + * \return The current camera state + */ + /** * \var Camera::bufferCompleted * \brief Signal emitted when a buffer for a request queued to the camera has
Make the Camera state accessible by providing an accessor operation and moving the State enumeration definition in the public scope. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/camera.h | 16 +++++++++------- src/libcamera/camera.cpp | 33 +++++++++++++++++++++++++++------ 2 files changed, 36 insertions(+), 13 deletions(-)