| Message ID | 20210503193307.108607-2-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote: > Make Camera::stop() idempotent so that it can be called in any state and > consecutive times. When called in any state other than CameraRunning, it > is a no-op. This simplifies the cleanup path for applications. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Changes in v2: > - Suggested by Kieran: > - Add new isRunning() function instead of relying on isAccessAllowed() > - Suggested by Laurent: > - Make stop()'s description clearer > > src/libcamera/camera.cpp | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 1340c266cc5f..1c6c76c7c01e 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -347,6 +347,7 @@ public: > const std::set<Stream *> &streams); > ~Private(); > > + bool isRunning() const; > int isAccessAllowed(State state, bool allowDisconnected = false, > const char *from = __builtin_FUNCTION()) const; > int isAccessAllowed(State low, State high, > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { > "Running", > }; > > +bool Camera::Private::isRunning() const > +{ > + State currentState = state_.load(std::memory_order_acquire); > + if (currentState == CameraRunning) > + return true; > + > + return false; > +} > + > int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > const char *from) const > { > @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) > * This method stops capturing and processing requests immediately. All pending > * requests are cancelled and complete synchronously in an error state. > * > - * \context This function may only be called when the camera is in the Running > - * state as defined in \ref camera_operation, and shall be synchronized by the > - * caller with other functions that affect the camera state. > + * \context This function may be called in any camera state as defined in \ref > + * camera_operation, and shall be synchronized by the caller with other > + * functions that affect the camera state. If called when the camera isn't > + * running, it is a no-op. > * > * \return 0 on success or a negative error code otherwise > * \retval -ENODEV The camera has been disconnected from the system > @@ -1073,6 +1084,9 @@ int Camera::stop() > { > Private *const d = LIBCAMERA_D_PTR(); > > + if (!d->isRunning()) > + return 0; > + This kind of defeats the purpose of the isAccessAllowed() here below. Please note that isAccessAllowed(Private::CameraRunning) will return true only if the camera is running, which is the same thing your new function checks for. Let me guess, you wanted to get rid of the error message ? Personally, I would be in favour of calling stop() only when it's appropriate instead of shortcutting the camera state machine. What is the use case that makes it hard to call stop() only when actually required ? Thanks j > int ret = d->isAccessAllowed(Private::CameraRunning); > if (ret < 0) > return ret; > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Em 2021-05-04 04:41, Jacopo Mondi escreveu: > Hi Nicolas, > > On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote: > > Make Camera::stop() idempotent so that it can be called in any state and > > consecutive times. When called in any state other than CameraRunning, it > > is a no-op. This simplifies the cleanup path for applications. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v2: > > - Suggested by Kieran: > > - Add new isRunning() function instead of relying on isAccessAllowed() > > - Suggested by Laurent: > > - Make stop()'s description clearer > > > > src/libcamera/camera.cpp | 20 +++++++++++++++++--- > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 1340c266cc5f..1c6c76c7c01e 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -347,6 +347,7 @@ public: > > const std::set<Stream *> &streams); > > ~Private(); > > > > + bool isRunning() const; > > int isAccessAllowed(State state, bool allowDisconnected = false, > > const char *from = __builtin_FUNCTION()) const; > > int isAccessAllowed(State low, State high, > > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { > > "Running", > > }; > > > > +bool Camera::Private::isRunning() const > > +{ > > + State currentState = state_.load(std::memory_order_acquire); > > + if (currentState == CameraRunning) > > + return true; > > + > > + return false; > > +} > > + > > int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > > const char *from) const > > { > > @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) > > * This method stops capturing and processing requests immediately. All pending > > * requests are cancelled and complete synchronously in an error state. > > * > > - * \context This function may only be called when the camera is in the Running > > - * state as defined in \ref camera_operation, and shall be synchronized by the > > - * caller with other functions that affect the camera state. > > + * \context This function may be called in any camera state as defined in \ref > > + * camera_operation, and shall be synchronized by the caller with other > > + * functions that affect the camera state. If called when the camera isn't > > + * running, it is a no-op. > > * > > * \return 0 on success or a negative error code otherwise > > * \retval -ENODEV The camera has been disconnected from the system > > @@ -1073,6 +1084,9 @@ int Camera::stop() > > { > > Private *const d = LIBCAMERA_D_PTR(); > > > > + if (!d->isRunning()) > > + return 0; > > + > > This kind of defeats the purpose of the isAccessAllowed() here below. > > Please note that isAccessAllowed(Private::CameraRunning) will return > true only if the camera is running, which is the same thing your new > function checks for. Let me guess, you wanted to get rid of the error > message ? Indeed, I missed that, sorry. In v1 [1] I added other parameters to isAccessAllowed() just to be able to exit silently, but Kieran pointed out that it would be cleaner with an isRunning() function. So I did that, but forgot to remove the isAccessAllowed() below as well. [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html > > Personally, I would be in favour of calling stop() only when it's > appropriate instead of shortcutting the camera state machine. What is > the use case that makes it hard to call stop() only when actually > required ? The use case is always calling stop() in the destructor of the capture in lc-compliance, which makes the cleanup path simpler as we can safely fail an assert and still have the camera stopped. That said, it wouldn't be hard to call stop() only when required. We could track whether the camera was started internally in lc-compliance. But it seemed like this would be a common pattern and handling it by making Camera::stop() itself be idempotent would be beneficial. I'll wait for others to comment on this as well. Thanks, Nícolas > > Thanks > j > > int ret = d->isAccessAllowed(Private::CameraRunning); > > if (ret < 0) > > return ret; > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Nicolas, Jacopo, On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > Hi Jacopo, > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > >> Hi Nicolas, >> >> On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote: >>> Make Camera::stop() idempotent so that it can be called in any state and >>> consecutive times. When called in any state other than CameraRunning, it >>> is a no-op. This simplifies the cleanup path for applications. >>> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> >>> --- >>> Changes in v2: >>> - Suggested by Kieran: >>> - Add new isRunning() function instead of relying on isAccessAllowed() >>> - Suggested by Laurent: >>> - Make stop()'s description clearer >>> >>> src/libcamera/camera.cpp | 20 +++++++++++++++++--- >>> 1 file changed, 17 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>> index 1340c266cc5f..1c6c76c7c01e 100644 >>> --- a/src/libcamera/camera.cpp >>> +++ b/src/libcamera/camera.cpp >>> @@ -347,6 +347,7 @@ public: >>> const std::set<Stream *> &streams); >>> ~Private(); >>> >>> + bool isRunning() const; >>> int isAccessAllowed(State state, bool allowDisconnected = false, >>> const char *from = __builtin_FUNCTION()) const; >>> int isAccessAllowed(State low, State high, >>> @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { >>> "Running", >>> }; >>> >>> +bool Camera::Private::isRunning() const >>> +{ >>> + State currentState = state_.load(std::memory_order_acquire); >>> + if (currentState == CameraRunning) >>> + return true; >>> + >>> + return false; >>> +} >>> + >>> int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, >>> const char *from) const >>> { >>> @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) >>> * This method stops capturing and processing requests immediately. All pending >>> * requests are cancelled and complete synchronously in an error state. >>> * >>> - * \context This function may only be called when the camera is in the Running >>> - * state as defined in \ref camera_operation, and shall be synchronized by the >>> - * caller with other functions that affect the camera state. >>> + * \context This function may be called in any camera state as defined in \ref >>> + * camera_operation, and shall be synchronized by the caller with other >>> + * functions that affect the camera state. If called when the camera isn't >>> + * running, it is a no-op. >>> * >>> * \return 0 on success or a negative error code otherwise >>> * \retval -ENODEV The camera has been disconnected from the system >>> @@ -1073,6 +1084,9 @@ int Camera::stop() >>> { >>> Private *const d = LIBCAMERA_D_PTR(); >>> >>> + if (!d->isRunning()) >>> + return 0; >>> + >> >> This kind of defeats the purpose of the isAccessAllowed() here below. >> >> Please note that isAccessAllowed(Private::CameraRunning) will return >> true only if the camera is running, which is the same thing your new >> function checks for. Let me guess, you wanted to get rid of the error >> message ? > > Indeed, I missed that, sorry. In v1 [1] I added other parameters to > isAccessAllowed() just to be able to exit silently, but Kieran pointed out that > it would be cleaner with an isRunning() function. So I did that, but forgot to > remove the isAccessAllowed() below as well. Yes, the extra checks should be removed. > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html > >> >> Personally, I would be in favour of calling stop() only when it's >> appropriate instead of shortcutting the camera state machine. What is >> the use case that makes it hard to call stop() only when actually >> required ? > > The use case is always calling stop() in the destructor of the capture in > lc-compliance, which makes the cleanup path simpler as we can safely fail an > assert and still have the camera stopped. > > That said, it wouldn't be hard to call stop() only when required. We could track > whether the camera was started internally in lc-compliance. But it seemed like > this would be a common pattern and handling it by making Camera::stop() itself > be idempotent would be beneficial. > > I'll wait for others to comment on this as well. I think being able to clean up easily on shutdowns is beneficial. I would consider this use case to be that of being able to call p = NULL; ... kfree(p); rather than if (p) kfree(p); It's a convenience feature, so that extra state doesn't have to be repeatedly maintained elsewhere. -- Kieran > > Thanks, > Nícolas > >> >> Thanks >> j >>> int ret = d->isAccessAllowed(Private::CameraRunning); >>> if (ret < 0) >>> return ret; >>> -- >>> 2.31.1 >>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hello, On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: > Hi Nicolas, Jacopo, > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > > Hi Jacopo, > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > > [snip] > >> Personally, I would be in favour of calling stop() only when it's > >> appropriate instead of shortcutting the camera state machine. What is > >> the use case that makes it hard to call stop() only when actually > >> required ? > > > > The use case is always calling stop() in the destructor of the capture in > > lc-compliance, which makes the cleanup path simpler as we can safely fail an > > assert and still have the camera stopped. > > > > That said, it wouldn't be hard to call stop() only when required. We could track > > whether the camera was started internally in lc-compliance. But it seemed like > > this would be a common pattern and handling it by making Camera::stop() itself > > be idempotent would be beneficial. > > > > I'll wait for others to comment on this as well. > > I think being able to clean up easily on shutdowns is beneficial. > > I would consider this use case to be that of being able to call > > p = NULL; > ... > kfree(p); > > rather than > if (p) > kfree(p); > > It's a convenience feature, so that extra state doesn't have to be > repeatedly maintained elsewhere. > Might be a personal opinion, but I would prefer, as a general idea, a stricter API. I concur there's not much value in the error message when stop() is called from the wrong state, but sometime it helped me finding a wrong patter when working on the camera HAL. I guess it might be beneficial for other use cases too... Anyway, I won't push, whatever's best for the majority is fine with me. Thanks j > -- > Kieran > > > > > > Thanks, > > Nícolas > > > >> > >> Thanks > >> j > >>> int ret = d->isAccessAllowed(Private::CameraRunning); > >>> if (ret < 0) > >>> return ret; > >>> -- > >>> 2.31.1 > >>> > >>> _______________________________________________ > >>> libcamera-devel mailing list > >>> libcamera-devel@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
Hi, Em 2021-05-04 12:29, Jacopo Mondi escreveu: > Hello, > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: > > Hi Nicolas, Jacopo, > > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > > > Hi Jacopo, > > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > > > > > [snip] > > > >> Personally, I would be in favour of calling stop() only when it's > > >> appropriate instead of shortcutting the camera state machine. What is > > >> the use case that makes it hard to call stop() only when actually > > >> required ? > > > > > > The use case is always calling stop() in the destructor of the capture in > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an > > > assert and still have the camera stopped. > > > > > > That said, it wouldn't be hard to call stop() only when required. We could track > > > whether the camera was started internally in lc-compliance. But it seemed like > > > this would be a common pattern and handling it by making Camera::stop() itself > > > be idempotent would be beneficial. > > > > > > I'll wait for others to comment on this as well. > > > > I think being able to clean up easily on shutdowns is beneficial. > > > > I would consider this use case to be that of being able to call > > > > p = NULL; > > ... > > kfree(p); > > > > rather than > > if (p) > > kfree(p); > > > > It's a convenience feature, so that extra state doesn't have to be > > repeatedly maintained elsewhere. > > > > Might be a personal opinion, but I would prefer, as a general idea, > a stricter API. I concur there's not much value in the error message > when stop() is called from the wrong state, but sometime it helped me > finding a wrong patter when working on the camera HAL. I guess it > might be beneficial for other use cases too... I think adding a log at level DEBUG or INFO stating that "stop() called for non-running camera" would help debug those cases while still allowing the convenience of calling it in any state. Thanks, Nícolas > > Anyway, I won't push, whatever's best for the majority is fine with > me. > > Thanks > j > > > -- > > Kieran > > > > > > > > > > Thanks, > > > Nícolas > > > > > >> > > >> Thanks > > >> j > > >>> int ret = d->isAccessAllowed(Private::CameraRunning); > > >>> if (ret < 0) > > >>> return ret; > > >>> -- > > >>> 2.31.1 > > >>> > > >>> _______________________________________________ > > >>> libcamera-devel mailing list > > >>> libcamera-devel@lists.libcamera.org > > >>> https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > -- > > Regards > > -- > > Kieran > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
Hello, On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote: > Em 2021-05-04 12:29, Jacopo Mondi escreveu: > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > > > > Hi Jacopo, > > > > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > > > > > > > > [snip] > > > > > >> Personally, I would be in favour of calling stop() only when it's > > > >> appropriate instead of shortcutting the camera state machine. What is > > > >> the use case that makes it hard to call stop() only when actually > > > >> required ? > > > > > > > > The use case is always calling stop() in the destructor of the capture in > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an > > > > assert and still have the camera stopped. > > > > > > > > That said, it wouldn't be hard to call stop() only when required. We could track > > > > whether the camera was started internally in lc-compliance. But it seemed like > > > > this would be a common pattern and handling it by making Camera::stop() itself > > > > be idempotent would be beneficial. > > > > > > > > I'll wait for others to comment on this as well. > > > > > > I think being able to clean up easily on shutdowns is beneficial. > > > > > > I would consider this use case to be that of being able to call > > > > > > p = NULL; > > > ... > > > kfree(p); > > > > > > rather than > > > if (p) > > > kfree(p); > > > > > > It's a convenience feature, so that extra state doesn't have to be > > > repeatedly maintained elsewhere. > > > > Might be a personal opinion, but I would prefer, as a general idea, > > a stricter API. I concur there's not much value in the error message > > when stop() is called from the wrong state, but sometime it helped me > > finding a wrong patter when working on the camera HAL. I guess it > > might be beneficial for other use cases too... > > I think adding a log at level DEBUG or INFO stating that "stop() called for > non-running camera" would help debug those cases while still allowing the > convenience of calling it in any state. Sorry about the conflicting directions :-S There are two options. Calling Camera::stop() when stopped is either valid, in which case the function shouldn't log any message with a level higher than debug, or it's invalid, in which case we should log an error. In the latter case, the caller needs to keep track of the camera state, which is a bit of a hassle. Taking Niklas' example, it would be similar to requiring the caller to have a bool is_p_null variable, as the Camera class doesn't expose its state, so a caller can do if (cam->state() == CameraRunning) cam->stop(); We could expose the camera state, but that increases the surface of the public API, for little gain I believe. Jacopo, what do you think ? > > Anyway, I won't push, whatever's best for the majority is fine with > > me. > > > > > >>> int ret = d->isAccessAllowed(Private::CameraRunning); > > > >>> if (ret < 0) > > > >>> return ret;
Hi Laurent, On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote: > Hello, > > On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote: > > Em 2021-05-04 12:29, Jacopo Mondi escreveu: > > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: > > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > > > > > Hi Jacopo, > > > > > > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > > > > > > > > > > > [snip] > > > > > > > >> Personally, I would be in favour of calling stop() only when it's > > > > >> appropriate instead of shortcutting the camera state machine. What is > > > > >> the use case that makes it hard to call stop() only when actually > > > > >> required ? > > > > > > > > > > The use case is always calling stop() in the destructor of the capture in > > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an > > > > > assert and still have the camera stopped. > > > > > > > > > > That said, it wouldn't be hard to call stop() only when required. We could track > > > > > whether the camera was started internally in lc-compliance. But it seemed like > > > > > this would be a common pattern and handling it by making Camera::stop() itself > > > > > be idempotent would be beneficial. > > > > > > > > > > I'll wait for others to comment on this as well. > > > > > > > > I think being able to clean up easily on shutdowns is beneficial. > > > > > > > > I would consider this use case to be that of being able to call > > > > > > > > p = NULL; > > > > ... > > > > kfree(p); > > > > > > > > rather than > > > > if (p) > > > > kfree(p); > > > > > > > > It's a convenience feature, so that extra state doesn't have to be > > > > repeatedly maintained elsewhere. > > > > > > Might be a personal opinion, but I would prefer, as a general idea, > > > a stricter API. I concur there's not much value in the error message > > > when stop() is called from the wrong state, but sometime it helped me > > > finding a wrong patter when working on the camera HAL. I guess it > > > might be beneficial for other use cases too... > > > > I think adding a log at level DEBUG or INFO stating that "stop() called for > > non-running camera" would help debug those cases while still allowing the > > convenience of calling it in any state. > > Sorry about the conflicting directions :-S > > There are two options. Calling Camera::stop() when stopped is either > valid, in which case the function shouldn't log any message with a level > higher than debug, or it's invalid, in which case we should log an > error. In the latter case, the caller needs to keep track of the camera > state, which is a bit of a hassle. Taking Niklas' example, it would be > similar to requiring the caller to have a bool is_p_null variable, as > the Camera class doesn't expose its state, so a caller can do > > if (cam->state() == CameraRunning) > cam->stop(); > > We could expose the camera state, but that increases the surface of the > public API, for little gain I believe. > > Jacopo, what do you think ? > Either we change the camera state machine and allow Stop->Stop by adding a state Stopping -> Stopping [label = "stop()"]; and change isAccessAllowed(Private::CameraStopped, Private::CameraRunning); not to throw any error Or we mandate applications to track the state themeselves. Exposing the state has a little gain, as application will unconditionally repeat the pattern if (cam->state() == CameraRunning) cam->stop(); hence they could similarly be allowed to just call stop() without bothering about the state check. I would like application to be in charge of tracking the state and know what they're doing precisely, but I'm afraid they will just call stop() and ignore the error, and I'm not 100% sure it is fair to mandate the state tracking burden to apps. So yes, allowing stop->stop seems fair, but it should be made part of the Camera state machine and not worked around by not checking if the access is actually allowed or not imho. Thanks j > > > Anyway, I won't push, whatever's best for the majority is fine with > > > me. > > > > > > > >>> int ret = d->isAccessAllowed(Private::CameraRunning); > > > > >>> if (ret < 0) > > > > >>> return ret; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On 08/05/2021 08:24, Jacopo Mondi wrote: > Hi Laurent, > > On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote: >> Hello, >> >> On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote: >>> Em 2021-05-04 12:29, Jacopo Mondi escreveu: >>>> On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: >>>>> On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: >>>>>> Hi Jacopo, >>>>>> >>>>>> Em 2021-05-04 04:41, Jacopo Mondi escreveu: >>>>>> >>>> >>>> [snip] >>>> >>>>>>> Personally, I would be in favour of calling stop() only when it's >>>>>>> appropriate instead of shortcutting the camera state machine. What is >>>>>>> the use case that makes it hard to call stop() only when actually >>>>>>> required ? >>>>>> >>>>>> The use case is always calling stop() in the destructor of the capture in >>>>>> lc-compliance, which makes the cleanup path simpler as we can safely fail an >>>>>> assert and still have the camera stopped. >>>>>> >>>>>> That said, it wouldn't be hard to call stop() only when required. We could track >>>>>> whether the camera was started internally in lc-compliance. But it seemed like >>>>>> this would be a common pattern and handling it by making Camera::stop() itself >>>>>> be idempotent would be beneficial. >>>>>> >>>>>> I'll wait for others to comment on this as well. >>>>> >>>>> I think being able to clean up easily on shutdowns is beneficial. >>>>> >>>>> I would consider this use case to be that of being able to call >>>>> >>>>> p = NULL; >>>>> ... >>>>> kfree(p); >>>>> >>>>> rather than >>>>> if (p) >>>>> kfree(p); >>>>> >>>>> It's a convenience feature, so that extra state doesn't have to be >>>>> repeatedly maintained elsewhere. >>>> >>>> Might be a personal opinion, but I would prefer, as a general idea, >>>> a stricter API. I concur there's not much value in the error message >>>> when stop() is called from the wrong state, but sometime it helped me >>>> finding a wrong patter when working on the camera HAL. I guess it >>>> might be beneficial for other use cases too... >>> >>> I think adding a log at level DEBUG or INFO stating that "stop() called for >>> non-running camera" would help debug those cases while still allowing the >>> convenience of calling it in any state. >> >> Sorry about the conflicting directions :-S >> >> There are two options. Calling Camera::stop() when stopped is either >> valid, in which case the function shouldn't log any message with a level >> higher than debug, or it's invalid, in which case we should log an >> error. In the latter case, the caller needs to keep track of the camera >> state, which is a bit of a hassle. Taking Niklas' example, it would be >> similar to requiring the caller to have a bool is_p_null variable, as >> the Camera class doesn't expose its state, so a caller can do >> >> if (cam->state() == CameraRunning) >> cam->stop(); >> >> We could expose the camera state, but that increases the surface of the >> public API, for little gain I believe. >> >> Jacopo, what do you think ? >> > > Either we change the camera state machine and allow Stop->Stop by > adding a state > Stopping -> Stopping [label = "stop()"]; Hrm - Are we missing some updates to our state diagram already? Shouldn't this be 'Stopped'? (I bet that's my fault) > and change > isAccessAllowed(Private::CameraStopped, > Private::CameraRunning); > > not to throw any error > > Or we mandate applications to track the state themeselves. > > Exposing the state has a little gain, as application will > unconditionally repeat the pattern > > if (cam->state() == CameraRunning) > cam->stop(); > > hence they could similarly be allowed to just call stop() without > bothering about the state check. > > I would like application to be in charge of tracking the state and > know what they're doing precisely, but I'm afraid they will just > call stop() and ignore the error, and I'm not 100% sure it is fair to > mandate the state tracking burden to apps. > > So yes, allowing stop->stop seems fair, but it should be made part of > the Camera state machine and not worked around by not checking if the > access is actually allowed or not imho. I think making it clear in the state machine diagrams is a very good point. But doesn't it then mean allowing stop() from any of the other states? What happens if we call stop in {Available, Acquired, Configured} for example. In fact, indeed we shouldn't allow calling stop() on a camera which has not been acquired, as that would imply that the caller could stop() a camera which was in a running state, belonging to someone else. We may still need to have the instrumentation in the stop function for isRunning() so that it doesn't try to stop, when already stopped though, but I agree the isAccessAllowed() could in fact be a bit more restrictive. > > Thanks > j > >>>> Anyway, I won't push, whatever's best for the majority is fine with >>>> me. >>>> >>>>>>>> int ret = d->isAccessAllowed(Private::CameraRunning); >>>>>>>> if (ret < 0) >>>>>>>> return ret; >> >> -- >> Regards, >> >> Laurent Pinchart
Hi Jacopo, Em 2021-05-08 04:24, Jacopo Mondi escreveu: > Hi Laurent, > > On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote: > > Hello, > > > > On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote: > > > Em 2021-05-04 12:29, Jacopo Mondi escreveu: > > > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote: > > > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote: > > > > > > Hi Jacopo, > > > > > > > > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu: > > > > > > > > > > > > > > [snip] > > > > > > > > > >> Personally, I would be in favour of calling stop() only when it's > > > > > >> appropriate instead of shortcutting the camera state machine. What is > > > > > >> the use case that makes it hard to call stop() only when actually > > > > > >> required ? > > > > > > > > > > > > The use case is always calling stop() in the destructor of the capture in > > > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an > > > > > > assert and still have the camera stopped. > > > > > > > > > > > > That said, it wouldn't be hard to call stop() only when required. We could track > > > > > > whether the camera was started internally in lc-compliance. But it seemed like > > > > > > this would be a common pattern and handling it by making Camera::stop() itself > > > > > > be idempotent would be beneficial. > > > > > > > > > > > > I'll wait for others to comment on this as well. > > > > > > > > > > I think being able to clean up easily on shutdowns is beneficial. > > > > > > > > > > I would consider this use case to be that of being able to call > > > > > > > > > > p = NULL; > > > > > ... > > > > > kfree(p); > > > > > > > > > > rather than > > > > > if (p) > > > > > kfree(p); > > > > > > > > > > It's a convenience feature, so that extra state doesn't have to be > > > > > repeatedly maintained elsewhere. > > > > > > > > Might be a personal opinion, but I would prefer, as a general idea, > > > > a stricter API. I concur there's not much value in the error message > > > > when stop() is called from the wrong state, but sometime it helped me > > > > finding a wrong patter when working on the camera HAL. I guess it > > > > might be beneficial for other use cases too... > > > > > > I think adding a log at level DEBUG or INFO stating that "stop() called for > > > non-running camera" would help debug those cases while still allowing the > > > convenience of calling it in any state. > > > > Sorry about the conflicting directions :-S > > > > There are two options. Calling Camera::stop() when stopped is either > > valid, in which case the function shouldn't log any message with a level > > higher than debug, or it's invalid, in which case we should log an > > error. In the latter case, the caller needs to keep track of the camera > > state, which is a bit of a hassle. Taking Niklas' example, it would be > > similar to requiring the caller to have a bool is_p_null variable, as > > the Camera class doesn't expose its state, so a caller can do > > > > if (cam->state() == CameraRunning) > > cam->stop(); > > > > We could expose the camera state, but that increases the surface of the > > public API, for little gain I believe. > > > > Jacopo, what do you think ? > > > > Either we change the camera state machine and allow Stop->Stop by > adding a state > Stopping -> Stopping [label = "stop()"]; > > and change > isAccessAllowed(Private::CameraStopped, > Private::CameraRunning); > > not to throw any error But this wouldn't really solve the problem. The Stopping state only happens while requests are being finished, and the intention here is to make it possible to call stop() in any state to make cleanup easier. If we were to add it as part of the state machine, it would mean making each state (apart from Running) point to itself with label "stop()", as it wouldn't make sense to transition Acquired to Stopping, for example. But if we don't want to transition to the Stopping state from a state like Acquired, it means that we want to return early on stop() for the cases where the camera isn't Running. This implies not making use of isAccessAllowed(). Because if we did use it, there would have to be two isAccessAllowed() calls in stop(), like I did in v1 [1]. But as Kieran noted there, this isn't a case of having access, since all states are accounted for. Regarding the state diagram, maybe it is okay to omit the fact that "stop()" can be called from states other than Running (ie keep it the way it is), given that those are no-ops and aren't there to change the operation but to make it more convenient? (It will be documented in the stop() function, just not on the state diagram) Finally, having an error message in isAccessAllowed() sounds important to warn about invalid state transitions. It doesn't seem right to me to remove that message just because stop() is a special case. These are my thoughts :). Thanks, Nícolas [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html > > Or we mandate applications to track the state themeselves. > > Exposing the state has a little gain, as application will > unconditionally repeat the pattern > > if (cam->state() == CameraRunning) > cam->stop(); > > hence they could similarly be allowed to just call stop() without > bothering about the state check. > > I would like application to be in charge of tracking the state and > know what they're doing precisely, but I'm afraid they will just > call stop() and ignore the error, and I'm not 100% sure it is fair to > mandate the state tracking burden to apps. > > So yes, allowing stop->stop seems fair, but it should be made part of > the Camera state machine and not worked around by not checking if the > access is actually allowed or not imho. > > Thanks > j > > > > > Anyway, I won't push, whatever's best for the majority is fine with > > > > me. > > > > > > > > > >>> int ret = d->isAccessAllowed(Private::CameraRunning); > > > > > >>> if (ret < 0) > > > > > >>> return ret; > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 1340c266cc5f..1c6c76c7c01e 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -347,6 +347,7 @@ public: const std::set<Stream *> &streams); ~Private(); + bool isRunning() const; int isAccessAllowed(State state, bool allowDisconnected = false, const char *from = __builtin_FUNCTION()) const; int isAccessAllowed(State low, State high, @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { "Running", }; +bool Camera::Private::isRunning() const +{ + State currentState = state_.load(std::memory_order_acquire); + if (currentState == CameraRunning) + return true; + + return false; +} + int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, const char *from) const { @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete synchronously in an error state. * - * \context This function may only be called when the camera is in the Running - * state as defined in \ref camera_operation, and shall be synchronized by the - * caller with other functions that affect the camera state. + * \context This function may be called in any camera state as defined in \ref + * camera_operation, and shall be synchronized by the caller with other + * functions that affect the camera state. If called when the camera isn't + * running, it is a no-op. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -1073,6 +1084,9 @@ int Camera::stop() { Private *const d = LIBCAMERA_D_PTR(); + if (!d->isRunning()) + return 0; + int ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0) return ret;
Make Camera::stop() idempotent so that it can be called in any state and consecutive times. When called in any state other than CameraRunning, it is a no-op. This simplifies the cleanup path for applications. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v2: - Suggested by Kieran: - Add new isRunning() function instead of relying on isAccessAllowed() - Suggested by Laurent: - Make stop()'s description clearer src/libcamera/camera.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)