[libcamera-devel] [PATCH 1/5] libcamera: camera: Make camera state accessible

[Jacopo Mondi]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190806/e0a386f9/attachment.sig>