| Message ID | 20210312054727.852622-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 2021-03-12 05:47:23 +0000, Kieran Bingham wrote: > All requests must have completed before the Camera has fully stopped. > > Requests completing when the camera is not running represent an internal > pipeline handler bug. > > Trap this event with a fatal error. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I like this change too, but sure this must break some of our pipelines? Even so they should then be fixed, for this change, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 84edbb8f494d..19fb9eb415b0 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1062,11 +1062,11 @@ int Camera::stop() > > LOG(Camera, Debug) << "Stopping capture"; > > - d->setState(Private::CameraConfigured); > - > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > this); > > + d->setState(Private::CameraConfigured); > + > return 0; > } > > @@ -1079,6 +1079,12 @@ int Camera::stop() > */ > void Camera::requestComplete(Request *request) > { > + Private *const d = LIBCAMERA_D_PTR(); > + > + int ret = d->isAccessAllowed(Private::CameraRunning); > + if (ret < 0) > + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > + > requestCompleted.emit(request); > } > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote: > All requests must have completed before the Camera has fully stopped. > > Requests completing when the camera is not running represent an internal > pipeline handler bug. > > Trap this event with a fatal error. > Am I wrong or this is actually the other way around ? Marking the Camera as Configured -after- the pipeline's stop() has been invoked makes sure the requests that complete with error due to stop() are refused. IOW stop() is invoked on the pipeline thread, which might complete requests with error asynchronously from the camera state being moved to 'CameraConfigured'. After the Camera state has changed, all the requests completed with error will trigger a Fatal. Am I missing something obvious ? > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 84edbb8f494d..19fb9eb415b0 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1062,11 +1062,11 @@ int Camera::stop() > > LOG(Camera, Debug) << "Stopping capture"; > > - d->setState(Private::CameraConfigured); > - > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > this); > > + d->setState(Private::CameraConfigured); > + > return 0; > } > > @@ -1079,6 +1079,12 @@ int Camera::stop() > */ > void Camera::requestComplete(Request *request) > { > + Private *const d = LIBCAMERA_D_PTR(); > + > + int ret = d->isAccessAllowed(Private::CameraRunning); > + if (ret < 0) > + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > + > requestCompleted.emit(request); > } > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote: > On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote: > > All requests must have completed before the Camera has fully stopped. > > > > Requests completing when the camera is not running represent an internal > > pipeline handler bug. > > > > Trap this event with a fatal error. > > Am I wrong or this is actually the other way around ? Marking the > Camera as Configured -after- the pipeline's stop() has been invoked > makes sure the requests that complete with error due to stop() are > refused. > > IOW stop() is invoked on the pipeline thread, which might complete > requests with error asynchronously from the camera state being > moved to 'CameraConfigured'. After the Camera state has changed, > all the requests completed with error will trigger a Fatal. > Am I missing something obvious ? The call to PipelineHandler::stop() is blocking, which means that the state will only be set back to CameraConfigured once the pipeline handler's stop() function returns. Pipeline handlers are required to complete all pending requests before returning from stop(), so the assertion shouldn't be triggered. Am I missing something ? :-) > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/camera.cpp | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 84edbb8f494d..19fb9eb415b0 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -1062,11 +1062,11 @@ int Camera::stop() > > > > LOG(Camera, Debug) << "Stopping capture"; > > > > - d->setState(Private::CameraConfigured); > > - > > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > > this); > > > > + d->setState(Private::CameraConfigured); > > + > > return 0; > > } > > > > @@ -1079,6 +1079,12 @@ int Camera::stop() > > */ > > void Camera::requestComplete(Request *request) > > { > > + Private *const d = LIBCAMERA_D_PTR(); > > + > > + int ret = d->isAccessAllowed(Private::CameraRunning); > > + if (ret < 0) > > + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; Nitpicking, as there's no Stopped state, I'd write "stopped". And you could also write if (d->isAccessAllowed(Private::CameraRunning) < 0) LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; I also wonder what then happens when a camera is unplugged. Could you try unplugging a UVC camera while streaming ? > > + > > requestCompleted.emit(request); > > } > >
Hi Laurent, On Sun, Mar 14, 2021 at 01:00:04AM +0200, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote: > > On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote: > > > All requests must have completed before the Camera has fully stopped. > > > > > > Requests completing when the camera is not running represent an internal > > > pipeline handler bug. > > > > > > Trap this event with a fatal error. > > > > Am I wrong or this is actually the other way around ? Marking the > > Camera as Configured -after- the pipeline's stop() has been invoked > > makes sure the requests that complete with error due to stop() are > > refused. > > > > IOW stop() is invoked on the pipeline thread, which might complete > > requests with error asynchronously from the camera state being > > moved to 'CameraConfigured'. After the Camera state has changed, > > all the requests completed with error will trigger a Fatal. > > Am I missing something obvious ? > > The call to PipelineHandler::stop() is blocking, which means that the > state will only be set back to CameraConfigured once the pipeline > handler's stop() function returns. Pipeline handlers are required to > complete all pending requests before returning from stop(), so the > assertion shouldn't be triggered. Am I missing something ? :-) > I was missing the (obvious) fact that stop() is blocking it seems :) Thanks for clearing this out! > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/libcamera/camera.cpp | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 84edbb8f494d..19fb9eb415b0 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -1062,11 +1062,11 @@ int Camera::stop() > > > > > > LOG(Camera, Debug) << "Stopping capture"; > > > > > > - d->setState(Private::CameraConfigured); > > > - > > > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > > > this); > > > > > > + d->setState(Private::CameraConfigured); > > > + > > > return 0; > > > } > > > > > > @@ -1079,6 +1079,12 @@ int Camera::stop() > > > */ > > > void Camera::requestComplete(Request *request) > > > { > > > + Private *const d = LIBCAMERA_D_PTR(); > > > + > > > + int ret = d->isAccessAllowed(Private::CameraRunning); > > > + if (ret < 0) > > > + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > > Nitpicking, as there's no Stopped state, I'd write "stopped". > > And you could also write > > if (d->isAccessAllowed(Private::CameraRunning) < 0) > LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > > I also wonder what then happens when a camera is unplugged. Could you > try unplugging a UVC camera while streaming ? > > > > + > > > requestCompleted.emit(request); > > > } > > > > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 13/03/2021 23:00, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote: >> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote: >>> All requests must have completed before the Camera has fully stopped. >>> >>> Requests completing when the camera is not running represent an internal >>> pipeline handler bug. >>> >>> Trap this event with a fatal error. >> >> Am I wrong or this is actually the other way around ? Marking the >> Camera as Configured -after- the pipeline's stop() has been invoked >> makes sure the requests that complete with error due to stop() are >> refused. >> >> IOW stop() is invoked on the pipeline thread, which might complete >> requests with error asynchronously from the camera state being >> moved to 'CameraConfigured'. After the Camera state has changed, >> all the requests completed with error will trigger a Fatal. >> Am I missing something obvious ? > > The call to PipelineHandler::stop() is blocking, which means that the > state will only be set back to CameraConfigured once the pipeline > handler's stop() function returns. Pipeline handlers are required to > complete all pending requests before returning from stop(), so the > assertion shouldn't be triggered. Am I missing something ? :-) > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/libcamera/camera.cpp | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>> index 84edbb8f494d..19fb9eb415b0 100644 >>> --- a/src/libcamera/camera.cpp >>> +++ b/src/libcamera/camera.cpp >>> @@ -1062,11 +1062,11 @@ int Camera::stop() >>> >>> LOG(Camera, Debug) << "Stopping capture"; >>> >>> - d->setState(Private::CameraConfigured); >>> - >>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, >>> this); >>> >>> + d->setState(Private::CameraConfigured); >>> + >>> return 0; >>> } >>> >>> @@ -1079,6 +1079,12 @@ int Camera::stop() >>> */ >>> void Camera::requestComplete(Request *request) >>> { >>> + Private *const d = LIBCAMERA_D_PTR(); >>> + >>> + int ret = d->isAccessAllowed(Private::CameraRunning); >>> + if (ret < 0) >>> + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > > Nitpicking, as there's no Stopped state, I'd write "stopped". > > And you could also write > > if (d->isAccessAllowed(Private::CameraRunning) < 0) > LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; As we don't otherwise need this ret value I agree, until ... > I also wonder what then happens when a camera is unplugged. Could you > try unplugging a UVC camera while streaming ? Hrm ... indeed this is a race probably. When a camera is disconnected, it should set disconnected_, which will then causes isAccessAllowed() to return -ENODEV, which would be entirely permittable here to still complete the request and pass it back to the application. How about: /* Disconnected cameras are still able to complete requests. */ ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0 && ret != -ENODEV) LOG(Camera, Fatal) << "Trying to complete a request when stopped"; >>> + >>> requestCompleted.emit(request); >>> } >>> >
Hi Kieran, On Wed, Mar 24, 2021 at 12:49:38PM +0000, Kieran Bingham wrote: > On 13/03/2021 23:00, Laurent Pinchart wrote: > > On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote: > >> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote: > >>> All requests must have completed before the Camera has fully stopped. > >>> > >>> Requests completing when the camera is not running represent an internal > >>> pipeline handler bug. > >>> > >>> Trap this event with a fatal error. > >> > >> Am I wrong or this is actually the other way around ? Marking the > >> Camera as Configured -after- the pipeline's stop() has been invoked > >> makes sure the requests that complete with error due to stop() are > >> refused. > >> > >> IOW stop() is invoked on the pipeline thread, which might complete > >> requests with error asynchronously from the camera state being > >> moved to 'CameraConfigured'. After the Camera state has changed, > >> all the requests completed with error will trigger a Fatal. > >> Am I missing something obvious ? > > > > The call to PipelineHandler::stop() is blocking, which means that the > > state will only be set back to CameraConfigured once the pipeline > > handler's stop() function returns. Pipeline handlers are required to > > complete all pending requests before returning from stop(), so the > > assertion shouldn't be triggered. Am I missing something ? :-) > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> src/libcamera/camera.cpp | 10 ++++++++-- > >>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >>> index 84edbb8f494d..19fb9eb415b0 100644 > >>> --- a/src/libcamera/camera.cpp > >>> +++ b/src/libcamera/camera.cpp > >>> @@ -1062,11 +1062,11 @@ int Camera::stop() > >>> > >>> LOG(Camera, Debug) << "Stopping capture"; > >>> > >>> - d->setState(Private::CameraConfigured); > >>> - > >>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > >>> this); > >>> > >>> + d->setState(Private::CameraConfigured); > >>> + > >>> return 0; > >>> } > >>> > >>> @@ -1079,6 +1079,12 @@ int Camera::stop() > >>> */ > >>> void Camera::requestComplete(Request *request) > >>> { > >>> + Private *const d = LIBCAMERA_D_PTR(); > >>> + > >>> + int ret = d->isAccessAllowed(Private::CameraRunning); > >>> + if (ret < 0) > >>> + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > > > > Nitpicking, as there's no Stopped state, I'd write "stopped". > > > > And you could also write > > > > if (d->isAccessAllowed(Private::CameraRunning) < 0) > > LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; > > As we don't otherwise need this ret value I agree, until ... > > > I also wonder what then happens when a camera is unplugged. Could you > > try unplugging a UVC camera while streaming ? > > Hrm ... indeed this is a race probably. > > When a camera is disconnected, it should set disconnected_, which will > then causes isAccessAllowed() to return -ENODEV, which would be entirely > permittable here to still complete the request and pass it back to the > application. > > How about: > > /* Disconnected cameras are still able to complete requests. */ > ret = d->isAccessAllowed(Private::CameraRunning); > if (ret < 0 && ret != -ENODEV) > LOG(Camera, Fatal) << "Trying to complete a request when stopped"; Seems better to me. Another option would be to pas true as the second argument of isAccessAllowed(). > >>> + > >>> requestCompleted.emit(request); > >>> } > >>>
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 84edbb8f494d..19fb9eb415b0 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1062,11 +1062,11 @@ int Camera::stop() LOG(Camera, Debug) << "Stopping capture"; - d->setState(Private::CameraConfigured); - d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, this); + d->setState(Private::CameraConfigured); + return 0; } @@ -1079,6 +1079,12 @@ int Camera::stop() */ void Camera::requestComplete(Request *request) { + Private *const d = LIBCAMERA_D_PTR(); + + int ret = d->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + LOG(Camera, Fatal) << "Trying to complete a request when Stopped"; + requestCompleted.emit(request); }
All requests must have completed before the Camera has fully stopped. Requests completing when the camera is not running represent an internal pipeline handler bug. Trap this event with a fatal error. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)