| Message ID | 20210329091552.157208-4-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 29/03/2021 10:15, Hirokazu Honda wrote: > libcamera::Request contains an error value. Every > libcamera::Camera client should regards the error in a > request completion. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > Documentation/guides/application-developer.rst | 2 ++ > src/cam/capture.cpp | 5 +++++ > src/gstreamer/gstlibcamerasrc.cpp | 6 +++++- > src/qcam/main_window.cpp | 4 ++++ > src/v4l2/v4l2_camera.cpp | 5 +++++ > 5 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > index e3430f03..e8a1a7b4 100644 > --- a/Documentation/guides/application-developer.rst > +++ b/Documentation/guides/application-developer.rst > @@ -392,6 +392,8 @@ the `Request::Status`_ class enum documentation. > > if (request->status() == Request::RequestCancelled) > return; > + if (request->status() == Request::RequestError) > + // handle error. Even if it's just documentation, we should wrap that // handle error. in { } as it leaves the if () statement applying to whatever would happen next otherwise... > If the ``Request`` has completed successfully, applications can access the > completed buffers using the ``Request::buffers()`` function, which returns a map > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7b55fc67..3e9edf80 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -167,6 +167,11 @@ void Capture::requestComplete(Request *request) > { > if (request->status() == Request::RequestCancelled) > return; > + /* TODO: Handle an error correctly */ > + if (request->status() == Request::RequestError) { > + std::cout << "Failed to capture request: " << request->cookie(); > + return; I don't think we can let this return early here. It would cause us to drop errorred frames, and then run out of active requests. If a Request status is RequestError, then we need to decide if the frame is to be dropped or displayed regardless. (which is an application choice, rather than a libcamera choice). That's fine as we're here in cam.. An application which wants to display only 'good' frames would likely choose to not display this frame, but it would then need to make sure that a new request would be queued still. For cam, I think we *should* track errored frames, as they are important in testing. So this should certainly call into Capture::processRequest(), and likely print a notice saying that this frame was an error. And still save it accordingly... Although, I'm not sure what RequestError means yet. Does it mean there was an error and the Request wasn't processed completely, or that it wasn't processed at all. Perhaps the use case for a buffer not completely being processed is already handled by the buffer status... > + } > > /* > * Defer processing of the completed request to the event loop, to avoid > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 87246b40..1ecb9883 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -163,10 +163,14 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > g_return_if_fail(wrap->request_.get() == request); > > - if ((request->status() == Request::RequestCancelled)) { > + if (request->status() == Request::RequestCancelled) { > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > return; > } > + if (request->status() == Request::RequestError) { > + GST_ERROR_OBJECT(src_, "Request doesn't complete successfully"); s/doesn't/didn't/ I haven't looked into how gstlibcamerasrc uses it's requests yet, but if this leaks a request and causes the pipeline to stall that would be bad too. > + return; > + } > > GstBuffer *buffer; > for (GstPad *srcpad : srcpads_) { > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 39d034de..1288bcd5 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -694,6 +694,10 @@ void MainWindow::requestComplete(Request *request) > { > if (request->status() == Request::RequestCancelled) > return; > + if (request->status() == Request::RequestError) { > + qCritical() << "Request doesn't complete successfully"; s/doesn't/didn't/ And the same concerns of course. > + return; > + } > > /* > * We're running in the libcamera thread context, expensive operations > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 97825c71..d6b1d755 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -84,6 +84,11 @@ void V4L2Camera::requestComplete(Request *request) > { > if (request->status() == Request::RequestCancelled) > return; > + if (request->status() == Request::RequestError) { > + LOG(V4L2Compat, Error) > + << "Request doesn't complete successfully"; And same ;-) > + return; > + } > > /* We only have one stream at the moment. */ > bufferLock_.lock(); >
Hi Kieran and Hiro, On Mon, Mar 29, 2021 at 11:27:54AM +0100, Kieran Bingham wrote: > On 29/03/2021 10:15, Hirokazu Honda wrote: > > libcamera::Request contains an error value. Every > > libcamera::Camera client should regards the error in a > > request completion. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > Documentation/guides/application-developer.rst | 2 ++ > > src/cam/capture.cpp | 5 +++++ > > src/gstreamer/gstlibcamerasrc.cpp | 6 +++++- > > src/qcam/main_window.cpp | 4 ++++ > > src/v4l2/v4l2_camera.cpp | 5 +++++ > > 5 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > > index e3430f03..e8a1a7b4 100644 > > --- a/Documentation/guides/application-developer.rst > > +++ b/Documentation/guides/application-developer.rst > > @@ -392,6 +392,8 @@ the `Request::Status`_ class enum documentation. > > > > if (request->status() == Request::RequestCancelled) > > return; > > + if (request->status() == Request::RequestError) > > + // handle error. > > Even if it's just documentation, we should wrap that // handle error. in > { } as it leaves the if () statement applying to whatever would happen > next otherwise... > > > If the ``Request`` has completed successfully, applications can access the > > completed buffers using the ``Request::buffers()`` function, which returns a map > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > index 7b55fc67..3e9edf80 100644 > > --- a/src/cam/capture.cpp > > +++ b/src/cam/capture.cpp > > @@ -167,6 +167,11 @@ void Capture::requestComplete(Request *request) > > { > > if (request->status() == Request::RequestCancelled) > > return; > > + /* TODO: Handle an error correctly */ We use the doxygen syntax, /* \todo ... */ > > + if (request->status() == Request::RequestError) { > > + std::cout << "Failed to capture request: " << request->cookie(); > > + return; > > I don't think we can let this return early here. It would cause us to > drop errorred frames, and then run out of active requests. > > If a Request status is RequestError, then we need to decide if the frame > is to be dropped or displayed regardless. (which is an application > choice, rather than a libcamera choice). That's fine as we're here in cam.. I think we need to take a step back, and reconsider the design of error reporting. We currently have a request status, which can only report successful completion or full cancellation of the request, and we have a buffer status that can report erroneous buffers (without any additional information). The two don't really need to interact together. With the introduction of RequestError, this situation changes, and the interaction of buffer status and request status needs to be reconsidered. I'd like to see a design proposal for that (as part of patch 2/3), that would consider the combination of statuses that are allowed, and what each of the possible combinations mean. For instance (starting brainstorming here), we could consider that RequestError signals an error at the request level that means that no buffer have been written (all the buffer metadata would then be invalid), or we could consider that any buffer that is erroneous would result in the request being marked with RequestError (in which case applications would need to inspect individual buffers). This needs to be designed by considering use cases, including the error notification mechanism of the Android camera HAL, as well as ease of use for applications. We can also consider adding additional error information (possibly as part of this series, or later on top), to differentiate, for instance, between fatal errors that indicate that the camera has become unusable, and errors that are specific to a request but won't affect further requests (is this something that can ever happen ?). This can be done with additional information (an error code maybe ?), or additional request statuses. Finally, if we decide to add error codes to requests, we may consider turning RequestCancel into an error code. This is a bit of work, an RFC with a design proposal (or multiple options to choose from) and an explanation of the design rationale and associated use cases would help. > An application which wants to display only 'good' frames would likely > choose to not display this frame, but it would then need to make sure > that a new request would be queued still. > > For cam, I think we *should* track errored frames, as they are important > in testing. > > So this should certainly call into Capture::processRequest(), and likely > print a notice saying that this frame was an error. And still save it > accordingly... > > Although, I'm not sure what RequestError means yet. > Does it mean there was an error and the Request wasn't processed > completely, or that it wasn't processed at all. > > Perhaps the use case for a buffer not completely being processed is > already handled by the buffer status... > > > + } > > > > /* > > * Defer processing of the completed request to the event loop, to avoid > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 87246b40..1ecb9883 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -163,10 +163,14 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > > > g_return_if_fail(wrap->request_.get() == request); > > > > - if ((request->status() == Request::RequestCancelled)) { > > + if (request->status() == Request::RequestCancelled) { > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > return; > > } > > + if (request->status() == Request::RequestError) { > > + GST_ERROR_OBJECT(src_, "Request doesn't complete successfully"); > > s/doesn't/didn't/ > > I haven't looked into how gstlibcamerasrc uses it's requests yet, but if > this leaks a request and causes the pipeline to stall that would be bad too. > > > + return; > > + } > > > > GstBuffer *buffer; > > for (GstPad *srcpad : srcpads_) { > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 39d034de..1288bcd5 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -694,6 +694,10 @@ void MainWindow::requestComplete(Request *request) > > { > > if (request->status() == Request::RequestCancelled) > > return; > > + if (request->status() == Request::RequestError) { > > + qCritical() << "Request doesn't complete successfully"; > > s/doesn't/didn't/ > > And the same concerns of course. > > > + return; > > + } > > > > /* > > * We're running in the libcamera thread context, expensive operations > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > > index 97825c71..d6b1d755 100644 > > --- a/src/v4l2/v4l2_camera.cpp > > +++ b/src/v4l2/v4l2_camera.cpp > > @@ -84,6 +84,11 @@ void V4L2Camera::requestComplete(Request *request) > > { > > if (request->status() == Request::RequestCancelled) > > return; > > + if (request->status() == Request::RequestError) { > > + LOG(V4L2Compat, Error) > > + << "Request doesn't complete successfully"; > > And same ;-) > > > + return; > > + } > > > > /* We only have one stream at the moment. */ > > bufferLock_.lock();
diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst index e3430f03..e8a1a7b4 100644 --- a/Documentation/guides/application-developer.rst +++ b/Documentation/guides/application-developer.rst @@ -392,6 +392,8 @@ the `Request::Status`_ class enum documentation. if (request->status() == Request::RequestCancelled) return; + if (request->status() == Request::RequestError) + // handle error. If the ``Request`` has completed successfully, applications can access the completed buffers using the ``Request::buffers()`` function, which returns a map diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index 7b55fc67..3e9edf80 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -167,6 +167,11 @@ void Capture::requestComplete(Request *request) { if (request->status() == Request::RequestCancelled) return; + /* TODO: Handle an error correctly */ + if (request->status() == Request::RequestError) { + std::cout << "Failed to capture request: " << request->cookie(); + return; + } /* * Defer processing of the completed request to the event loop, to avoid diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 87246b40..1ecb9883 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -163,10 +163,14 @@ GstLibcameraSrcState::requestCompleted(Request *request) g_return_if_fail(wrap->request_.get() == request); - if ((request->status() == Request::RequestCancelled)) { + if (request->status() == Request::RequestCancelled) { GST_DEBUG_OBJECT(src_, "Request was cancelled"); return; } + if (request->status() == Request::RequestError) { + GST_ERROR_OBJECT(src_, "Request doesn't complete successfully"); + return; + } GstBuffer *buffer; for (GstPad *srcpad : srcpads_) { diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 39d034de..1288bcd5 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -694,6 +694,10 @@ void MainWindow::requestComplete(Request *request) { if (request->status() == Request::RequestCancelled) return; + if (request->status() == Request::RequestError) { + qCritical() << "Request doesn't complete successfully"; + return; + } /* * We're running in the libcamera thread context, expensive operations diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 97825c71..d6b1d755 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -84,6 +84,11 @@ void V4L2Camera::requestComplete(Request *request) { if (request->status() == Request::RequestCancelled) return; + if (request->status() == Request::RequestError) { + LOG(V4L2Compat, Error) + << "Request doesn't complete successfully"; + return; + } /* We only have one stream at the moment. */ bufferLock_.lock();
libcamera::Request contains an error value. Every libcamera::Camera client should regards the error in a request completion. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- Documentation/guides/application-developer.rst | 2 ++ src/cam/capture.cpp | 5 +++++ src/gstreamer/gstlibcamerasrc.cpp | 6 +++++- src/qcam/main_window.cpp | 4 ++++ src/v4l2/v4l2_camera.cpp | 5 +++++ 5 files changed, 21 insertions(+), 1 deletion(-)