| Message ID | 20210423142412.460460-1-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, On 23/04/2021 15:24, 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> > --- > This will be used to simplify the cleanup path in lc-compliance without having > error messages [1]. > > Also, I'm not sure if I should add the silent parameter to the other > isAccessAllowed variant as well. It would make sense for consistency's sake, but > it's not needed currently. > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html > > src/libcamera/camera.cpp | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 763f3b9926fd..baffdafc8146 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -350,7 +350,7 @@ public: > int isAccessAllowed(State state, bool allowDisconnected = false, > const char *from = __builtin_FUNCTION()) const; > int isAccessAllowed(State low, State high, > - bool allowDisconnected = false, > + bool allowDisconnected = false, bool silent = false, > const char *from = __builtin_FUNCTION()) const; > > void disconnect(); > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > } > > int Camera::Private::isAccessAllowed(State low, State high, > - bool allowDisconnected, > + bool allowDisconnected, bool silent, > const char *from) const > { > if (!allowDisconnected && disconnected_) > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high, > ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && > static_cast<unsigned int>(high) < std::size(camera_state_names)); > > - LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > - << " state trying " << from > - << "() requiring state between " > - << camera_state_names[low] << " and " > - << camera_state_names[high]; > + if (!silent) > + LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > + << " state trying " << from > + << "() requiring state between " > + << camera_state_names[low] << " and " > + << camera_state_names[high]; > > return -EACCES; > } > @@ -1061,9 +1062,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 even if the camera isn't in the Running > + * state as defined in \ref camera_operation, in which cases it is a no-op. It > + * shall be synchronized by the caller with other functions that affect the > + * camera state. > * > * \return 0 on success or a negative error code otherwise > * \retval -ENODEV The camera has been disconnected from the system > @@ -1073,7 +1075,12 @@ int Camera::stop() > { > Private *const d = LIBCAMERA_D_PTR(); > > - int ret = d->isAccessAllowed(Private::CameraRunning); > + int ret = d->isAccessAllowed(Private::CameraAvailable, > + Private::CameraStopping, false, true); > + if (!ret) > + return 0; This seems like a lot of complexity to add to be able to return out of this function silently if the state is not running. Effectively we're saying that this function 'Is allowed' in any state, but it simply returns early if state < CameraRunning. That makes me think using a function called 'isAccessAllowed()' isn't quite right - It's always allowed. Would it be better to provide an accessor on the private state to better represent that? Like isRunning()? Then this simply becomes if (!d->isRunning()) return 0; > + > + ret = d->isAccessAllowed(Private::CameraRunning); > if (ret < 0) > return ret; > >
Em 2021-04-23 12:10, Kieran Bingham escreveu: > Hi Nicolas, > > On 23/04/2021 15:24, 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> > > --- > > This will be used to simplify the cleanup path in lc-compliance without having > > error messages [1]. > > > > Also, I'm not sure if I should add the silent parameter to the other > > isAccessAllowed variant as well. It would make sense for consistency's sake, but > > it's not needed currently. > > > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html > > > > src/libcamera/camera.cpp | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 763f3b9926fd..baffdafc8146 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -350,7 +350,7 @@ public: > > int isAccessAllowed(State state, bool allowDisconnected = false, > > const char *from = __builtin_FUNCTION()) const; > > int isAccessAllowed(State low, State high, > > - bool allowDisconnected = false, > > + bool allowDisconnected = false, bool silent = false, > > const char *from = __builtin_FUNCTION()) const; > > > > void disconnect(); > > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > > } > > > > int Camera::Private::isAccessAllowed(State low, State high, > > - bool allowDisconnected, > > + bool allowDisconnected, bool silent, > > const char *from) const > > { > > if (!allowDisconnected && disconnected_) > > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high, > > ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && > > static_cast<unsigned int>(high) < std::size(camera_state_names)); > > > > - LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > > - << " state trying " << from > > - << "() requiring state between " > > - << camera_state_names[low] << " and " > > - << camera_state_names[high]; > > + if (!silent) > > + LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > > + << " state trying " << from > > + << "() requiring state between " > > + << camera_state_names[low] << " and " > > + << camera_state_names[high]; > > > > return -EACCES; > > } > > @@ -1061,9 +1062,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 even if the camera isn't in the Running > > + * state as defined in \ref camera_operation, in which cases it is a no-op. It > > + * shall be synchronized by the caller with other functions that affect the > > + * camera state. > > * > > * \return 0 on success or a negative error code otherwise > > * \retval -ENODEV The camera has been disconnected from the system > > @@ -1073,7 +1075,12 @@ int Camera::stop() > > { > > Private *const d = LIBCAMERA_D_PTR(); > > > > - int ret = d->isAccessAllowed(Private::CameraRunning); > > + int ret = d->isAccessAllowed(Private::CameraAvailable, > > + Private::CameraStopping, false, true); > > + if (!ret) > > + return 0; > > This seems like a lot of complexity to add to be able to return out of > this function silently if the state is not running. > > Effectively we're saying that this function 'Is allowed' in any state, > but it simply returns early if state < CameraRunning. That makes me > think using a function called 'isAccessAllowed()' isn't quite right - > It's always allowed. > > Would it be better to provide an accessor on the private state to better > represent that? > > Like isRunning()? > > Then this simply becomes > > if (!d->isRunning()) > return 0; Yep, that sounds a lot better. I'll do that instead for v2. Thanks, Nícolas
Hi Nícolas, On Fri, Apr 23, 2021 at 01:29:47PM -0300, Nícolas F. R. A. Prado wrote: > Em 2021-04-23 12:10, Kieran Bingham escreveu: > > On 23/04/2021 15:24, 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> > > > --- > > > This will be used to simplify the cleanup path in lc-compliance without having > > > error messages [1]. > > > > > > Also, I'm not sure if I should add the silent parameter to the other > > > isAccessAllowed variant as well. It would make sense for consistency's sake, but > > > it's not needed currently. > > > > > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html > > > > > > src/libcamera/camera.cpp | 29 ++++++++++++++++++----------- > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 763f3b9926fd..baffdafc8146 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -350,7 +350,7 @@ public: > > > int isAccessAllowed(State state, bool allowDisconnected = false, > > > const char *from = __builtin_FUNCTION()) const; > > > int isAccessAllowed(State low, State high, > > > - bool allowDisconnected = false, > > > + bool allowDisconnected = false, bool silent = false, I know this will change in v2, but on a general note, bool parameters are not great (this applies to the existing allowDisconnected parameter too). When reading the caller int ret = d->isAccessAllowed(Private::CameraAvailable, Private::CameraStopping, false, true); it's hard to tell what false and true refer to. Instead, the following would be better: int ret = d->isAccessAllowed(Private::CameraAvailable, Private::CameraStopping, Private::AccessCheck::NoLogging); with a new enum AccessCbeck { AllowDisconnected = 0x1, NoLogging = 0x2, }; in the Private class. An enum class would be even better, but that wouldn't allow us to | multiple flags. https://patchwork.libcamera.org/cover/8991/ could be a solution. > > > const char *from = __builtin_FUNCTION()) const; > > > > > > void disconnect(); > > > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > > > } > > > > > > int Camera::Private::isAccessAllowed(State low, State high, > > > - bool allowDisconnected, > > > + bool allowDisconnected, bool silent, > > > const char *from) const > > > { > > > if (!allowDisconnected && disconnected_) > > > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high, > > > ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && > > > static_cast<unsigned int>(high) < std::size(camera_state_names)); > > > > > > - LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > > > - << " state trying " << from > > > - << "() requiring state between " > > > - << camera_state_names[low] << " and " > > > - << camera_state_names[high]; > > > + if (!silent) > > > + LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] > > > + << " state trying " << from > > > + << "() requiring state between " > > > + << camera_state_names[low] << " and " > > > + << camera_state_names[high]; > > > > > > return -EACCES; > > > } > > > @@ -1061,9 +1062,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 even if the camera isn't in the Running > > > + * state as defined in \ref camera_operation, in which cases it is a no-op. It > > > + * shall be synchronized by the caller with other functions that affect the > > > + * camera state. How about stating that the function may be called in any camera state ? > > > * > > > * \return 0 on success or a negative error code otherwise > > > * \retval -ENODEV The camera has been disconnected from the system > > > @@ -1073,7 +1075,12 @@ int Camera::stop() > > > { > > > Private *const d = LIBCAMERA_D_PTR(); > > > > > > - int ret = d->isAccessAllowed(Private::CameraRunning); > > > + int ret = d->isAccessAllowed(Private::CameraAvailable, > > > + Private::CameraStopping, false, true); > > > + if (!ret) > > > + return 0; > > > > This seems like a lot of complexity to add to be able to return out of > > this function silently if the state is not running. > > > > Effectively we're saying that this function 'Is allowed' in any state, > > but it simply returns early if state < CameraRunning. That makes me > > think using a function called 'isAccessAllowed()' isn't quite right - > > It's always allowed. > > > > Would it be better to provide an accessor on the private state to better > > represent that? > > > > Like isRunning()? > > > > Then this simply becomes > > > > if (!d->isRunning()) > > return 0; > > Yep, that sounds a lot better. I'll do that instead for v2.
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 763f3b9926fd..baffdafc8146 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -350,7 +350,7 @@ public: int isAccessAllowed(State state, bool allowDisconnected = false, const char *from = __builtin_FUNCTION()) const; int isAccessAllowed(State low, State high, - bool allowDisconnected = false, + bool allowDisconnected = false, bool silent = false, const char *from = __builtin_FUNCTION()) const; void disconnect(); @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, } int Camera::Private::isAccessAllowed(State low, State high, - bool allowDisconnected, + bool allowDisconnected, bool silent, const char *from) const { if (!allowDisconnected && disconnected_) @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high, ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) && static_cast<unsigned int>(high) < std::size(camera_state_names)); - LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] - << " state trying " << from - << "() requiring state between " - << camera_state_names[low] << " and " - << camera_state_names[high]; + if (!silent) + LOG(Camera, Error) << "Camera in " << camera_state_names[currentState] + << " state trying " << from + << "() requiring state between " + << camera_state_names[low] << " and " + << camera_state_names[high]; return -EACCES; } @@ -1061,9 +1062,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 even if the camera isn't in the Running + * state as defined in \ref camera_operation, in which cases it is a no-op. It + * shall be synchronized by the caller with other functions that affect the + * camera state. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -1073,7 +1075,12 @@ int Camera::stop() { Private *const d = LIBCAMERA_D_PTR(); - int ret = d->isAccessAllowed(Private::CameraRunning); + int ret = d->isAccessAllowed(Private::CameraAvailable, + Private::CameraStopping, false, true); + if (!ret) + return 0; + + 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> --- This will be used to simplify the cleanup path in lc-compliance without having error messages [1]. Also, I'm not sure if I should add the silent parameter to the other isAccessAllowed variant as well. It would make sense for consistency's sake, but it's not needed currently. [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html src/libcamera/camera.cpp | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)