[{"id":890,"web_url":"https://patchwork.libcamera.org/comment/890/","msgid":"<20190226171343.vwk36elqyxposhtk@uno.localdomain>","date":"2019-02-26T17:13:43","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n thanks for this big work\n\nOn Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:\n> There is a need to better control the order of operations an application\n> perform on a camera for it to function correctly. Add a basic state\n> machine to ensure applications perform operations on the camera in good\n> order.\n>\n> Internal to the Camera four states are added; Disconnected, Free,\n> Acquired and Running. Each state represents a higher state of\n> configuration of the camera ultimately leading to the highest state\n> where the camera is capturing frames. Each state supports a subset of\n> operations the application can perform.\n>\n> * Disconnected\n> Is the lowest state a camera can be in. It indicates that the camera\n> have been disconnected from the system and the only operations an\n> application shall do at this point is clean up and release the camera so\n> it can be removed from libcamera as well.\n>\n> * Free\n> Is the base state of a camera, an application can inspect the properties\n> of the camera to determine if it wish to use it or not. If an\n> application wish to use a camera it should acquire it to proceed to the\n> next state.\n>\n> * Acquired\n> When an application have acquired a camera it have exclusive access to\n> it and can modify the cameras parameters to prepare it for capturing.\n> Once the application is done configure the camera it may be started to\n> progress to the running state. Once the camera is started it can not be\n> reconfigured until it's stopped.\n>\n> * Running\n> In this state the camera is running and able to process requests queued\n> to it by the application. Once an application finish capturing the\n> camera shall be stopped to change the state to acquired where it can be\n> reconfigured or released.\n>\n\nBefore going into the details of the patch, I wonder if the states you\nhave described here capture all the possible interactions with a\ncamera object, in particulare regarding confgiurations of streams.\n\nI would have expected to see a \"Configured\" state, which unlock access\nto the stream start/stop methods and to buffer allocation, and which\nmight be used to refuse other configurations, if there is an active\none.\n\nMy mental model of this is that once a camera's streams have been\nconfigured, the configuration should be deleted (by passing, say, an\nempty configuration vector) to make sure applications have an explicit\n\"unconfigure\" step to prevent overriding a configuration (and possibly\ncentralize buffer release, which should not be done explicitly).\n\nIt seems to me here a configuration could be replaced freely, and you\ncan call free and allocte buffers on a camera which has just been\nacquired (but not configured). Is this intentional?\n\nThanks\n j\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 14 ++++-\n> src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------\n> 2 files changed, 80 insertions(+), 44 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index bf70255a6a5ea364..8c8545b074e8ae13 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -55,20 +55,28 @@ public:\n> \tint stop();\n>\n> private:\n> +\tenum State {\n> +\t\tDisconnected,\n> +\t\tFree,\n> +\t\tAcquired,\n> +\t\tRunning,\n> +\t};\n> +\n> \tCamera(PipelineHandler *pipe, const std::string &name);\n> \t~Camera();\n>\n> +\tbool stateIs(State state) const;\n> +\tbool stateIsAtleast(State state) const;\n> +\n> \tfriend class PipelineHandler;\n> \tvoid disconnect();\n> -\tint exclusiveAccess();\n>\n> \tstd::shared_ptr pipe_;\n> \tstd::string name_;\n> \tstd::vector streams_;\n> \tstd::vector activeStreams_;\n>\n> -\tbool acquired_;\n> -\tbool disconnected_;\n> +\tState state_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index d4258fe3c7551af3..c50b14bbd904fc1c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -116,17 +116,47 @@ const std::string &Camera::name() const\n> */\n>\n> Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> -\t disconnected_(false)\n> +\t: pipe_(pipe->shared_from_this()), name_(name), state_(Free)\n> {\n> }\n>\n> Camera::~Camera()\n> {\n> -\tif (acquired_)\n> +\tif (state_ > Free)\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n>\n> +static const std::string stateNames[] = {\n> +\t\"Disconnected\",\n> +\t\"Free\",\n> +\t\"Acquired\",\n> +\t\"Running\",\n> +};\n> +\n> +bool Camera::stateIs(State state) const\n> +{\n> +\tif (state_ == state)\n> +\t\treturn true;\n> +\n> +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> +\t\t\t << \" state trying operation requiering \"\n> +\t\t\t << stateNames[state] << \" state\";\n> +\n> +\treturn false;\n> +}\n> +\n> +bool Camera::stateIsAtleast(State state) const\n> +{\n> +\tif (state_ >= state)\n> +\t\treturn true;\n> +\n> +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> +\t\t\t << \" state trying operation requiering at least \"\n> +\t\t\t << stateNames[state] << \" state\";\n> +\n> +\treturn false;\n> +}\n> +\n> /**\n> * \\brief Notify camera disconnection\n> *\n> @@ -140,7 +170,7 @@ void Camera::disconnect()\n> {\n> \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n>\n> -\tdisconnected_ = true;\n> +\tstate_ = Disconnected;\n> \tdisconnected.emit(this);\n> }\n>\n> @@ -162,10 +192,11 @@ void Camera::disconnect()\n> */\n> int Camera::acquire()\n> {\n> -\tif (acquired_)\n> +\tif (!stateIs(Free))\n> \t\treturn -EBUSY;\n>\n> -\tacquired_ = true;\n> +\tstate_ = Acquired;\n> +\n> \treturn 0;\n> }\n>\n> @@ -177,7 +208,10 @@ int Camera::acquire()\n> */\n> void Camera::release()\n> {\n> -\tacquired_ = false;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn;\n> +\n> +\tstate_ = Free;\n> }\n>\n> /**\n> @@ -191,6 +225,9 @@ void Camera::release()\n> */\n> const std::vector &Camera::streams() const\n> {\n> +\tif (!stateIsAtleast(Free))\n> +\t\tstd::vector{};\n> +\n> \treturn streams_;\n> }\n>\n> @@ -213,7 +250,7 @@ const std::vector &Camera::streams() const\n> std::map\n> Camera::streamConfiguration(std::vector &streams)\n> {\n> -\tif (disconnected_ || !streams.size())\n> +\tif (!stateIsAtleast(Free) || !streams.size())\n> \t\treturn std::map{};\n>\n> \treturn pipe_->streamConfiguration(this, streams);\n> @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map &config)\n> {\n> \tint ret;\n>\n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n>\n> \tif (!config.size()) {\n> \t\tLOG(Camera, Error)\n> @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map &config)\n> */\n> int Camera::allocateBuffers()\n> {\n> -\tint ret;\n> -\n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n>\n> \tif (activeStreams_.empty()) {\n> \t\tLOG(Camera, Error)\n> @@ -297,7 +330,7 @@ int Camera::allocateBuffers()\n> \t}\n>\n> \tfor (Stream *stream : activeStreams_) {\n> -\t\tret = pipe_->allocateBuffers(this, stream);\n> +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> \t\tif (ret) {\n> \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> \t\t\tfreeBuffers();\n> @@ -313,6 +346,9 @@ int Camera::allocateBuffers()\n> */\n> void Camera::freeBuffers()\n> {\n> +\tif (!stateIs(Acquired))\n> +\t\treturn;\n> +\n> \tfor (Stream *stream : activeStreams_) {\n> \t\tif (!stream->bufferPool().count())\n> \t\t\tcontinue;\n> @@ -336,7 +372,7 @@ void Camera::freeBuffers()\n> */\n> Request *Camera::createRequest()\n> {\n> -\tif (exclusiveAccess())\n> +\tif (!stateIsAtleast(Acquired))\n> \t\treturn nullptr;\n>\n> \treturn new Request(this);\n> @@ -358,13 +394,10 @@ Request *Camera::createRequest()\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> -\tint ret;\n> +\tif (!stateIs(Running))\n> +\t\treturn -EACCES;\n>\n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tret = request->prepare();\n> +\tint ret = request->prepare();\n> \tif (ret) {\n> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> \t\treturn ret;\n> @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)\n> */\n> int Camera::start()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n>\n> \tLOG(Camera, Debug) << \"Starting capture\";\n>\n> -\treturn pipe_->start(this);\n> +\tint ret = pipe_->start(this);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstate_ = Running;\n> +\n> +\treturn 0;\n> }\n>\n> /**\n> @@ -405,24 +443,14 @@ int Camera::start()\n> */\n> int Camera::stop()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Running))\n> +\t\treturn -EACCES;\n>\n> \tLOG(Camera, Debug) << \"Stopping capture\";\n>\n> \tpipe_->stop(this);\n>\n> -\treturn 0;\n> -}\n> -\n> -int Camera::exclusiveAccess()\n> -{\n> -\tif (disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!acquired_)\n> -\t\treturn -EACCES;\n> +\tstate_ = Acquired;\n>\n> \treturn 0;\n> }\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5194610B2\n\tfor ;\n\tTue, 26 Feb 2019 18:13:14 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 5E216FF805;\n\tTue, 26 Feb 2019 17:13:14 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 26 Feb 2019 18:13:43 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190226171343.vwk36elqyxposhtk@uno.localdomain>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"2el24st5ej4rc6eh\"","Content-Disposition":"inline","In-Reply-To":"<20190226021857.28255-6-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","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, 26 Feb 2019 17:13:15 -0000"}},{"id":895,"web_url":"https://patchwork.libcamera.org/comment/895/","msgid":"<20190226190019.GL899@bigcity.dyn.berto.se>","date":"2019-02-26T19:00:19","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2019-02-26 18:13:43 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> thanks for this big work\n> \n> On Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:\n> > There is a need to better control the order of operations an application\n> > perform on a camera for it to function correctly. Add a basic state\n> > machine to ensure applications perform operations on the camera in good\n> > order.\n> >\n> > Internal to the Camera four states are added; Disconnected, Free,\n> > Acquired and Running. Each state represents a higher state of\n> > configuration of the camera ultimately leading to the highest state\n> > where the camera is capturing frames. Each state supports a subset of\n> > operations the application can perform.\n> >\n> > * Disconnected\n> > Is the lowest state a camera can be in. It indicates that the camera\n> > have been disconnected from the system and the only operations an\n> > application shall do at this point is clean up and release the camera so\n> > it can be removed from libcamera as well.\n> >\n> > * Free\n> > Is the base state of a camera, an application can inspect the properties\n> > of the camera to determine if it wish to use it or not. If an\n> > application wish to use a camera it should acquire it to proceed to the\n> > next state.\n> >\n> > * Acquired\n> > When an application have acquired a camera it have exclusive access to\n> > it and can modify the cameras parameters to prepare it for capturing.\n> > Once the application is done configure the camera it may be started to\n> > progress to the running state. Once the camera is started it can not be\n> > reconfigured until it's stopped.\n> >\n> > * Running\n> > In this state the camera is running and able to process requests queued\n> > to it by the application. Once an application finish capturing the\n> > camera shall be stopped to change the state to acquired where it can be\n> > reconfigured or released.\n> >\n> \n> Before going into the details of the patch, I wonder if the states you\n> have described here capture all the possible interactions with a\n> camera object, in particulare regarding confgiurations of streams.\n> \n> I would have expected to see a \"Configured\" state, which unlock access\n> to the stream start/stop methods and to buffer allocation, and which\n> might be used to refuse other configurations, if there is an active\n> one.\n\nIn my mind it captures all current interactions with the camera and \ncovers our discussions in Brussels. I agree it might be extended in the \nfuture to also include a configured state. For now I feel this covers \nthe behavior discussed and agreed upon.\n\n> \n> My mental model of this is that once a camera's streams have been\n> configured, the configuration should be deleted (by passing, say, an\n> empty configuration vector) to make sure applications have an explicit\n> \"unconfigure\" step to prevent overriding a configuration (and possibly\n> centralize buffer release, which should not be done explicitly).\n\nThis seems messy and hard for applications to use. We might need it at \nsome point, but for now all this change is make the current behavior a \nbit more strict. If we need this I say we should do this on top of this \nchange.\n\n> \n> It seems to me here a configuration could be replaced freely, and you\n> can call free and allocte buffers on a camera which has just been\n> acquired (but not configured). Is this intentional?\n\nYes. However with all patches in this series applied more checks are \nadded then just simply converting the existing ones to the state machine \nlogic. But as you point out above even more checks might be needed and \nadded in the future.\n\n> \n> Thanks\n> j\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 14 ++++-\n> > src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------\n> > 2 files changed, 80 insertions(+), 44 deletions(-)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index bf70255a6a5ea364..8c8545b074e8ae13 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -55,20 +55,28 @@ public:\n> > \tint stop();\n> >\n> > private:\n> > +\tenum State {\n> > +\t\tDisconnected,\n> > +\t\tFree,\n> > +\t\tAcquired,\n> > +\t\tRunning,\n> > +\t};\n> > +\n> > \tCamera(PipelineHandler *pipe, const std::string &name);\n> > \t~Camera();\n> >\n> > +\tbool stateIs(State state) const;\n> > +\tbool stateIsAtleast(State state) const;\n> > +\n> > \tfriend class PipelineHandler;\n> > \tvoid disconnect();\n> > -\tint exclusiveAccess();\n> >\n> > \tstd::shared_ptr pipe_;\n> > \tstd::string name_;\n> > \tstd::vector streams_;\n> > \tstd::vector activeStreams_;\n> >\n> > -\tbool acquired_;\n> > -\tbool disconnected_;\n> > +\tState state_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index d4258fe3c7551af3..c50b14bbd904fc1c 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -116,17 +116,47 @@ const std::string &Camera::name() const\n> > */\n> >\n> > Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> > -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> > -\t disconnected_(false)\n> > +\t: pipe_(pipe->shared_from_this()), name_(name), state_(Free)\n> > {\n> > }\n> >\n> > Camera::~Camera()\n> > {\n> > -\tif (acquired_)\n> > +\tif (state_ > Free)\n> > \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > }\n> >\n> > +static const std::string stateNames[] = {\n> > +\t\"Disconnected\",\n> > +\t\"Free\",\n> > +\t\"Acquired\",\n> > +\t\"Running\",\n> > +};\n> > +\n> > +bool Camera::stateIs(State state) const\n> > +{\n> > +\tif (state_ == state)\n> > +\t\treturn true;\n> > +\n> > +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> > +\t\t\t << \" state trying operation requiering \"\n> > +\t\t\t << stateNames[state] << \" state\";\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +bool Camera::stateIsAtleast(State state) const\n> > +{\n> > +\tif (state_ >= state)\n> > +\t\treturn true;\n> > +\n> > +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> > +\t\t\t << \" state trying operation requiering at least \"\n> > +\t\t\t << stateNames[state] << \" state\";\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > /**\n> > * \\brief Notify camera disconnection\n> > *\n> > @@ -140,7 +170,7 @@ void Camera::disconnect()\n> > {\n> > \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> >\n> > -\tdisconnected_ = true;\n> > +\tstate_ = Disconnected;\n> > \tdisconnected.emit(this);\n> > }\n> >\n> > @@ -162,10 +192,11 @@ void Camera::disconnect()\n> > */\n> > int Camera::acquire()\n> > {\n> > -\tif (acquired_)\n> > +\tif (!stateIs(Free))\n> > \t\treturn -EBUSY;\n> >\n> > -\tacquired_ = true;\n> > +\tstate_ = Acquired;\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -177,7 +208,10 @@ int Camera::acquire()\n> > */\n> > void Camera::release()\n> > {\n> > -\tacquired_ = false;\n> > +\tif (!stateIs(Acquired))\n> > +\t\treturn;\n> > +\n> > +\tstate_ = Free;\n> > }\n> >\n> > /**\n> > @@ -191,6 +225,9 @@ void Camera::release()\n> > */\n> > const std::vector &Camera::streams() const\n> > {\n> > +\tif (!stateIsAtleast(Free))\n> > +\t\tstd::vector{};\n> > +\n> > \treturn streams_;\n> > }\n> >\n> > @@ -213,7 +250,7 @@ const std::vector &Camera::streams() const\n> > std::map\n> > Camera::streamConfiguration(std::vector &streams)\n> > {\n> > -\tif (disconnected_ || !streams.size())\n> > +\tif (!stateIsAtleast(Free) || !streams.size())\n> > \t\treturn std::map{};\n> >\n> > \treturn pipe_->streamConfiguration(this, streams);\n> > @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map &config)\n> > {\n> > \tint ret;\n> >\n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(Acquired))\n> > +\t\treturn -EACCES;\n> >\n> > \tif (!config.size()) {\n> > \t\tLOG(Camera, Error)\n> > @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map &config)\n> > */\n> > int Camera::allocateBuffers()\n> > {\n> > -\tint ret;\n> > -\n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(Acquired))\n> > +\t\treturn -EACCES;\n> >\n> > \tif (activeStreams_.empty()) {\n> > \t\tLOG(Camera, Error)\n> > @@ -297,7 +330,7 @@ int Camera::allocateBuffers()\n> > \t}\n> >\n> > \tfor (Stream *stream : activeStreams_) {\n> > -\t\tret = pipe_->allocateBuffers(this, stream);\n> > +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> > \t\tif (ret) {\n> > \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> > \t\t\tfreeBuffers();\n> > @@ -313,6 +346,9 @@ int Camera::allocateBuffers()\n> > */\n> > void Camera::freeBuffers()\n> > {\n> > +\tif (!stateIs(Acquired))\n> > +\t\treturn;\n> > +\n> > \tfor (Stream *stream : activeStreams_) {\n> > \t\tif (!stream->bufferPool().count())\n> > \t\t\tcontinue;\n> > @@ -336,7 +372,7 @@ void Camera::freeBuffers()\n> > */\n> > Request *Camera::createRequest()\n> > {\n> > -\tif (exclusiveAccess())\n> > +\tif (!stateIsAtleast(Acquired))\n> > \t\treturn nullptr;\n> >\n> > \treturn new Request(this);\n> > @@ -358,13 +394,10 @@ Request *Camera::createRequest()\n> > */\n> > int Camera::queueRequest(Request *request)\n> > {\n> > -\tint ret;\n> > +\tif (!stateIs(Running))\n> > +\t\treturn -EACCES;\n> >\n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tret = request->prepare();\n> > +\tint ret = request->prepare();\n> > \tif (ret) {\n> > \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> > \t\treturn ret;\n> > @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)\n> > */\n> > int Camera::start()\n> > {\n> > -\tint ret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(Acquired))\n> > +\t\treturn -EACCES;\n> >\n> > \tLOG(Camera, Debug) << \"Starting capture\";\n> >\n> > -\treturn pipe_->start(this);\n> > +\tint ret = pipe_->start(this);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tstate_ = Running;\n> > +\n> > +\treturn 0;\n> > }\n> >\n> > /**\n> > @@ -405,24 +443,14 @@ int Camera::start()\n> > */\n> > int Camera::stop()\n> > {\n> > -\tint ret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(Running))\n> > +\t\treturn -EACCES;\n> >\n> > \tLOG(Camera, Debug) << \"Stopping capture\";\n> >\n> > \tpipe_->stop(this);\n> >\n> > -\treturn 0;\n> > -}\n> > -\n> > -int Camera::exclusiveAccess()\n> > -{\n> > -\tif (disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!acquired_)\n> > -\t\treturn -EACCES;\n> > +\tstate_ = Acquired;\n> >\n> > \treturn 0;\n> > }\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F85D610B2\n\tfor ;\n\tTue, 26 Feb 2019 20:00:21 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id v10so11770417lji.3\n\tfor ;\n\tTue, 26 Feb 2019 11:00:21 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tu11sm3355746lfb.85.2019.02.26.11.00.19\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 26 Feb 2019 11:00:19 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=TVfahcegCHZf5ykbAm+SrKEa9oKo/YYnzspR4mZIoDM=;\n\tb=jWh9RG76ogNyBCDSAX3UmA9EMqudtZRvfLw6ECl9dZc/vEg1goAn/ww+DFDfWSLQHj\n\tnl8WwctQNhIinalHyTLupp9jFrCR+hik87UrNtdWElqkKDU8QLuwpxcx8dbCjC8CHLQi\n\tTP4h+asTOQsqMlWE6p044Jyxj8hlVBpw2GM4BTagFbZa/t/lDYK8sI1IjSQoA4REUZcQ\n\tnVBqsPq13hh6HeSUzy5j1QuZWAG0lblhLweWQz1cO7ZQZ/jXpdpAHjhm3v8znwv9kgTX\n\tJPQThvInT4hAlfGmHwBAlQRkh4y//p1kWhJt/yxTdhKIfirg2T8biMAPKFd6eyBN7jlg\n\tUCvg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=TVfahcegCHZf5ykbAm+SrKEa9oKo/YYnzspR4mZIoDM=;\n\tb=nXq8JXTQ+obqtWiN2Zw+B/XN3AXjeYSERV4L4ZQw3AeMpE2Fie/FmWF4uSvzw4+ekN\n\tD74pVzC+0zoc3uVGMG0BdIZnTr693JOkQGENyu7feA5QmWQR1lg36Uu9ZH71hKDyuD5/\n\tBFJ27f5dNnHGbikOKn+xdzVV6sexKn+Xg/qMP2KG23CkWgjPHtzZBLm0hO3U2Oh65ILa\n\t4fvwWhFmiSwDBwhLcT1kTAylZumNHY1NJBfphaWfwZa0CL2VYeBsJUU7OUTiIUa5eDq6\n\tGj11CjXIGSYdWdgEgkH6Xxxgd7LO/bi/w0dYeBVZtqfLn7YhqZeCOiBpP2LCEMa/QTds\n\tmaqQ==","X-Gm-Message-State":"AHQUAubpQiqg5keoSAzkI5fXiMqR6weVUwS0C0e0TVCmE2pOuEipG6Vh\n\t5Qj8CicuM20Xzzfut+U+ho5hUfIorVs=","X-Google-Smtp-Source":"AHgI3IYnD3WEpKPk1SP/Qj3pIwjcZAdhdEpfE7mB285nzxLjisVVGuvzoCUVQoc+ZrOoKCiVz6XuWg==","X-Received":"by 2002:a2e:880a:: with SMTP id\n\tx10mr13457467ljh.12.1551207620413; \n\tTue, 26 Feb 2019 11:00:20 -0800 (PST)","Date":"Tue, 26 Feb 2019 20:00:19 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190226190019.GL899@bigcity.dyn.berto.se>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-6-niklas.soderlund@ragnatech.se>\n\t<20190226171343.vwk36elqyxposhtk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190226171343.vwk36elqyxposhtk@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","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, 26 Feb 2019 19:00:21 -0000"}},{"id":923,"web_url":"https://patchwork.libcamera.org/comment/923/","msgid":"<20190227164302.GF4813@pendragon.ideasonboard.com>","date":"2019-02-27T16:43:02","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:\n> There is a need to better control the order of operations an application\n> perform on a camera for it to function correctly. Add a basic state\n\ns/perform/performs/\n\n> machine to ensure applications perform operations on the camera in good\n> order.\n> \n> Internal to the Camera four states are added; Disconnected, Free,\n> Acquired and Running. Each state represents a higher state of\n> configuration of the camera ultimately leading to the highest state\n> where the camera is capturing frames. Each state supports a subset of\n> operations the application can perform.\n> \n> * Disconnected\n> Is the lowest state a camera can be in. It indicates that the camera\n> have been disconnected from the system and the only operations an\n\ns/have/has/\n\n> application shall do at this point is clean up and release the camera so\n> it can be removed from libcamera as well.\n\nI think you'll have to keep the connection status separate from the\nstate, otherwise an application won't be able to free buffers for a\ndisconnected camera. Disconnection should be separate from the state\nmachine related to the operations performed by the application, but of\ncourse needs to be taken into account when checking whether a function\ncan be called or not.\n\nI know that the current implementation doesn't handle this correctly\neither, but I think we should fix it instead of moving the incorrect\nbehaviour to the new state machine implementation.\n\n> * Free\n> Is the base state of a camera, an application can inspect the properties\n> of the camera to determine if it wish to use it or not. If an\n\ns/wish/wishes/\n\n> application wish to use a camera it should acquire it to proceed to the\n\ns/wish/wishes/\n\n> next state.\n> \n> * Acquired\n> When an application have acquired a camera it have exclusive access to\n\ns/have/has/g\n\n> it and can modify the cameras parameters to prepare it for capturing.\n\ns/cameras/camera's/\n\n> Once the application is done configure the camera it may be started to\n\ns/configure/configuring/\n\n> progress to the running state. Once the camera is started it can not be\n> reconfigured until it's stopped.\n> \n> * Running\n> In this state the camera is running and able to process requests queued\n> to it by the application. Once an application finish capturing the\n\ns/finish/finishes/\n\n> camera shall be stopped to change the state to acquired where it can be\n> reconfigured or released.\n\nAll this should be captured in the documentation, with a state diagram\n(probably as part of the Camera \\class documentation). You can find an\nexample of how to embed dot diagrams in Doxygen at\nhttp://www.doxygen.nl/manual/commands.html#cmddot.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 14 ++++-\n> src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------\n> 2 files changed, 80 insertions(+), 44 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index bf70255a6a5ea364..8c8545b074e8ae13 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -55,20 +55,28 @@ public:\n> \tint stop();\n> \n> private:\n> +\tenum State {\n> +\t\tDisconnected,\n> +\t\tFree,\n> +\t\tAcquired,\n> +\t\tRunning,\n\nThat's pretty generic and could lead to namespace collision. How about\nprefixing these state names with Camera ?\n\n> +\t};\n> +\n> \tCamera(PipelineHandler *pipe, const std::string &name);\n> \t~Camera();\n> \n> +\tbool stateIs(State state) const;\n> +\tbool stateIsAtleast(State state) const;\n> +\n> \tfriend class PipelineHandler;\n> \tvoid disconnect();\n> -\tint exclusiveAccess();\n> \n> \tstd::shared_ptr pipe_;\n> \tstd::string name_;\n> \tstd::vector streams_;\n> \tstd::vector activeStreams_;\n> \n> -\tbool acquired_;\n> -\tbool disconnected_;\n> +\tState state_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index d4258fe3c7551af3..c50b14bbd904fc1c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -116,17 +116,47 @@ const std::string &Camera::name() const\n> */\n> \n> Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> -\t disconnected_(false)\n> +\t: pipe_(pipe->shared_from_this()), name_(name), state_(Free)\n> {\n> }\n> \n> Camera::~Camera()\n> {\n> -\tif (acquired_)\n> +\tif (state_ > Free)\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n> \n> +static const std::string stateNames[] = {\n> +\t\"Disconnected\",\n> +\t\"Free\",\n> +\t\"Acquired\",\n> +\t\"Running\",\n> +};\n> +\n> +bool Camera::stateIs(State state) const\n> +{\n> +\tif (state_ == state)\n> +\t\treturn true;\n> +\n> +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> +\t\t\t << \" state trying operation requiering \"\n\ns/requiering/requiring/\n\nIs this an error, or could it be a warning ? Or possibly even a debug\nmesage ? Are there cases when an application could try to call an\noperation to check whether access is possible during the course of\nnormal operation ? I think that would be the case of the acquire()\nfunction in particular. Same comment for the next function.\n\nIt would make sense to make the camera Loggable to print its name, but\nthat would require exposing the log.h header, which is likely not a good\nidea. I think we can postpone this until we implement a Camera facade\nobject.\n\n> +\t\t\t << stateNames[state] << \" state\";\n> +\n> +\treturn false;\n> +}\n> +\n> +bool Camera::stateIsAtleast(State state) const\n> +{\n> +\tif (state_ >= state)\n> +\t\treturn true;\n> +\n> +\tLOG(Camera, Error) << \"Camera in \" << stateNames[state_]\n> +\t\t\t << \" state trying operation requiering at least \"\n\ns/requiering/requiring/\n\n> +\t\t\t << stateNames[state] << \" state\";\n> +\n> +\treturn false;\n> +}\n> +\n> /**\n> * \\brief Notify camera disconnection\n> *\n> @@ -140,7 +170,7 @@ void Camera::disconnect()\n> {\n> \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> \n> -\tdisconnected_ = true;\n> +\tstate_ = Disconnected;\n> \tdisconnected.emit(this);\n> }\n> \n> @@ -162,10 +192,11 @@ void Camera::disconnect()\n> */\n> int Camera::acquire()\n> {\n> -\tif (acquired_)\n> +\tif (!stateIs(Free))\n> \t\treturn -EBUSY;\n> \n> -\tacquired_ = true;\n> +\tstate_ = Acquired;\n> +\n> \treturn 0;\n> }\n> \n> @@ -177,7 +208,10 @@ int Camera::acquire()\n> */\n> void Camera::release()\n> {\n> -\tacquired_ = false;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn;\n> +\n> +\tstate_ = Free;\n\nWould it make sense to allow release() to be called on a Free camera, to\nsimplify error handling in the application ?\n\n> }\n> \n> /**\n> @@ -191,6 +225,9 @@ void Camera::release()\n> */\n> const std::vector &Camera::streams() const\n> {\n> +\tif (!stateIsAtleast(Free))\n> +\t\tstd::vector{};\n\nMissing return ? This will lead you to another problem, which is that\nyou return a reference to a local object. There are multiple ways to\nsolve this, but I think you can drop the state check for this function,\nas streams have the same lifetime as the camera, there's no need to\nrestrict the stream() function.\n\n> +\n> \treturn streams_;\n> }\n> \n> @@ -213,7 +250,7 @@ const std::vector &Camera::streams() const\n> std::map\n> Camera::streamConfiguration(std::vector &streams)\n> {\n> -\tif (disconnected_ || !streams.size())\n> +\tif (!stateIsAtleast(Free) || !streams.size())\n> \t\treturn std::map{};\n> \n> \treturn pipe_->streamConfiguration(this, streams);\n> @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map &config)\n> {\n> \tint ret;\n> \n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n> \n> \tif (!config.size()) {\n> \t\tLOG(Camera, Error)\n> @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map &config)\n> */\n> int Camera::allocateBuffers()\n> {\n> -\tint ret;\n> -\n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n\nI agree with Jacopo that a Configured state is likely needed.\n\n> \n> \tif (activeStreams_.empty()) {\n> \t\tLOG(Camera, Error)\n> @@ -297,7 +330,7 @@ int Camera::allocateBuffers()\n> \t}\n> \n> \tfor (Stream *stream : activeStreams_) {\n> -\t\tret = pipe_->allocateBuffers(this, stream);\n> +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> \t\tif (ret) {\n> \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> \t\t\tfreeBuffers();\n> @@ -313,6 +346,9 @@ int Camera::allocateBuffers()\n> */\n> void Camera::freeBuffers()\n> {\n> +\tif (!stateIs(Acquired))\n> +\t\treturn;\n> +\n\nThis allows calling freeBuffers() even when no buffers have been\nallocated. I think it's a good idea, but it calls for allowing release()\non a released camera, as mentioned above.\n\n> \tfor (Stream *stream : activeStreams_) {\n> \t\tif (!stream->bufferPool().count())\n> \t\t\tcontinue;\n> @@ -336,7 +372,7 @@ void Camera::freeBuffers()\n> */\n> Request *Camera::createRequest()\n> {\n> -\tif (exclusiveAccess())\n> +\tif (!stateIsAtleast(Acquired))\n> \t\treturn nullptr;\n> \n> \treturn new Request(this);\n> @@ -358,13 +394,10 @@ Request *Camera::createRequest()\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> -\tint ret;\n> +\tif (!stateIs(Running))\n> +\t\treturn -EACCES;\n> \n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tret = request->prepare();\n> +\tint ret = request->prepare();\n> \tif (ret) {\n> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> \t\treturn ret;\n> @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)\n> */\n> int Camera::start()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Acquired))\n> +\t\treturn -EACCES;\n> \n> \tLOG(Camera, Debug) << \"Starting capture\";\n> \n> -\treturn pipe_->start(this);\n> +\tint ret = pipe_->start(this);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstate_ = Running;\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -405,24 +443,14 @@ int Camera::start()\n> */\n> int Camera::stop()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(Running))\n> +\t\treturn -EACCES;\n> \n> \tLOG(Camera, Debug) << \"Stopping capture\";\n> \n> \tpipe_->stop(this);\n> \n> -\treturn 0;\n> -}\n> -\n> -int Camera::exclusiveAccess()\n> -{\n> -\tif (disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!acquired_)\n> -\t\treturn -EACCES;\n> +\tstate_ = Acquired;\n> \n> \treturn 0;\n> }","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 D08E6610B3\n\tfor ;\n\tWed, 27 Feb 2019 17:43:14 +0100 (CET)","from pendragon.ideasonboard.com (unknown [83.145.195.18])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C876449;\n\tWed, 27 Feb 2019 17:43:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551285794;\n\tbh=jrOriOSs7K+p3svAVjXDBFtZmbUNuJTB+S/pQblZSiI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p96KpFV8PtVC9N7mPAiyx9VVl++8Fe3MUF5/9mKWtNdthx2i7/cT3ebcAOd5lTOJP\n\teRp0xsImddJhMUsJzcvy1qtJ4MD+M3XYlF4vnU34K03w9vBqtGMPxDnB5Mu05S5qgv\n\t2T5a8btKDnBeZ/JE3bdhG1+vW7e8ZaYhsWCSn5rg=","Date":"Wed, 27 Feb 2019 18:43:02 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227164302.GF4813@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190226021857.28255-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: camera: add state\n\tmachine to control access from applications","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":"Wed, 27 Feb 2019 16:43:15 -0000"}}]