| Message ID | 20210329002715.74403-4-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Mar 29, 2021 at 09:27:15AM +0900, 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 | 4 ++-- > src/android/camera_device.cpp | 6 ++++-- > src/cam/capture.cpp | 4 +++- > src/gstreamer/gstlibcamerasrc.cpp | 4 ++++ > src/qcam/main_window.cpp | 4 +++- > src/v4l2/v4l2_camera.cpp | 4 +++- > test/camera/buffer_import.cpp | 4 +++- > test/camera/capture.cpp | 4 +++- > 8 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > index e3430f03..d7232e7c 100644 > --- a/Documentation/guides/application-developer.rst > +++ b/Documentation/guides/application-developer.rst > @@ -384,13 +384,13 @@ Request completion events can be emitted for requests which have been canceled, > for example, by unexpected application shutdown. To avoid an application > processing invalid image data, it’s worth checking that the request has > completed successfully. The list of request completion statuses is available in > -the `Request::Status`_ class enum documentation. > +the `Request::Status`_ class enum documentation and `Request::result`_. > > .. _Request::Status: https://www.libcamera.org/api-html/classlibcamera_1_1Request.html#a2209ba8d51af8167b25f6e3e94d5c45b > > .. code:: cpp > > - if (request->status() == Request::RequestCancelled) > + if (request->status() == Request::RequestCancelled || request->result() != 0) > return; > > If the ``Request`` has completed successfully, applications can access the > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index ae693664..d02c1776 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1908,9 +1908,11 @@ void CameraDevice::requestComplete(Request *request) > Camera3RequestDescriptor *descriptor = > reinterpret_cast<Camera3RequestDescriptor *>(request->cookie()); > > - if (request->status() != Request::RequestComplete) { > + if (request->status() != Request::RequestComplete || > + request->result() != 0) { > LOG(HAL, Error) << "Request not successfully completed: " > - << request->status(); > + << "status=" << request->status() > + << ", result=" << request->result(); > status = CAMERA3_BUFFER_STATUS_ERROR; Do we also need to return the error through the notify() callback ? > } > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7b55fc67..24d0be32 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -165,8 +165,10 @@ int Capture::queueRequest(Request *request) > > void Capture::requestComplete(Request *request) > { > - if (request->status() == Request::RequestCancelled) > + if (request->status() == Request::RequestCancelled || > + request->result() != 0) { I'm not sure this is right. Cancelled requests are ignored as they indicate we're stopping the camera, but for requests that complete with an error, shouldn't they be requeued ? It depends whether the error is fatal or not, and we don't have that information yet. All errors that we currently report would be caused by the pipeline handler failing to accept the request, which is fatal, but that's a bug in the pipeline handler :-) I'm thus not sure what the best option is in application code. > return; > + } No need for braces. > > /* > * 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..b36ac33c 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -167,6 +167,10 @@ GstLibcameraSrcState::requestCompleted(Request *request) > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > return; > } > + if (request->result() != 0) { > + GST_DEBUG_OBJECT(src_, "Request was not completed successfully"); > + return; > + } > > GstBuffer *buffer; > for (GstPad *srcpad : srcpads_) { > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 39d034de..f90310f9 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -692,8 +692,10 @@ void MainWindow::processRaw(FrameBuffer *buffer, > > void MainWindow::requestComplete(Request *request) > { > - if (request->status() == Request::RequestCancelled) > + if (request->status() == Request::RequestCancelled || > + request->result() != 0) { > 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..87153cd4 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -82,8 +82,10 @@ std::vector<V4L2Camera::Buffer> V4L2Camera::completedBuffers() > > void V4L2Camera::requestComplete(Request *request) > { > - if (request->status() == Request::RequestCancelled) > + if (request->status() == Request::RequestCancelled || > + request->result() != 0) { > return; Here I'm pretty sure that ignoring requests with an error isn't right. The error needs to instead be reported to upper layers, all the way to the application. The same may apply to the gstreamer element above. > + } > > /* We only have one stream at the moment. */ > bufferLock_.lock(); > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > index 61f4eb92..8f4efa5e 100644 > --- a/test/camera/buffer_import.cpp > +++ b/test/camera/buffer_import.cpp > @@ -47,8 +47,10 @@ protected: > > void requestComplete(Request *request) > { > - if (request->status() != Request::RequestComplete) > + if (request->status() != Request::RequestComplete || > + request->result() != 0) { > return; > + } > > const Request::BufferMap &buffers = request->buffers(); > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > index c4bc2110..6cff9bb4 100644 > --- a/test/camera/capture.cpp > +++ b/test/camera/capture.cpp > @@ -43,8 +43,10 @@ protected: > > void requestComplete(Request *request) > { > - if (request->status() != Request::RequestComplete) > + if (request->status() != Request::RequestComplete || > + request->result() != 0) { > return; > + } > > const Request::BufferMap &buffers = request->buffers(); >
diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst index e3430f03..d7232e7c 100644 --- a/Documentation/guides/application-developer.rst +++ b/Documentation/guides/application-developer.rst @@ -384,13 +384,13 @@ Request completion events can be emitted for requests which have been canceled, for example, by unexpected application shutdown. To avoid an application processing invalid image data, it’s worth checking that the request has completed successfully. The list of request completion statuses is available in -the `Request::Status`_ class enum documentation. +the `Request::Status`_ class enum documentation and `Request::result`_. .. _Request::Status: https://www.libcamera.org/api-html/classlibcamera_1_1Request.html#a2209ba8d51af8167b25f6e3e94d5c45b .. code:: cpp - if (request->status() == Request::RequestCancelled) + if (request->status() == Request::RequestCancelled || request->result() != 0) return; If the ``Request`` has completed successfully, applications can access the diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index ae693664..d02c1776 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1908,9 +1908,11 @@ void CameraDevice::requestComplete(Request *request) Camera3RequestDescriptor *descriptor = reinterpret_cast<Camera3RequestDescriptor *>(request->cookie()); - if (request->status() != Request::RequestComplete) { + if (request->status() != Request::RequestComplete || + request->result() != 0) { LOG(HAL, Error) << "Request not successfully completed: " - << request->status(); + << "status=" << request->status() + << ", result=" << request->result(); status = CAMERA3_BUFFER_STATUS_ERROR; } diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index 7b55fc67..24d0be32 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -165,8 +165,10 @@ int Capture::queueRequest(Request *request) void Capture::requestComplete(Request *request) { - if (request->status() == Request::RequestCancelled) + if (request->status() == Request::RequestCancelled || + request->result() != 0) { 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..b36ac33c 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -167,6 +167,10 @@ GstLibcameraSrcState::requestCompleted(Request *request) GST_DEBUG_OBJECT(src_, "Request was cancelled"); return; } + if (request->result() != 0) { + GST_DEBUG_OBJECT(src_, "Request was not completed successfully"); + return; + } GstBuffer *buffer; for (GstPad *srcpad : srcpads_) { diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 39d034de..f90310f9 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -692,8 +692,10 @@ void MainWindow::processRaw(FrameBuffer *buffer, void MainWindow::requestComplete(Request *request) { - if (request->status() == Request::RequestCancelled) + if (request->status() == Request::RequestCancelled || + request->result() != 0) { 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..87153cd4 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -82,8 +82,10 @@ std::vector<V4L2Camera::Buffer> V4L2Camera::completedBuffers() void V4L2Camera::requestComplete(Request *request) { - if (request->status() == Request::RequestCancelled) + if (request->status() == Request::RequestCancelled || + request->result() != 0) { return; + } /* We only have one stream at the moment. */ bufferLock_.lock(); diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp index 61f4eb92..8f4efa5e 100644 --- a/test/camera/buffer_import.cpp +++ b/test/camera/buffer_import.cpp @@ -47,8 +47,10 @@ protected: void requestComplete(Request *request) { - if (request->status() != Request::RequestComplete) + if (request->status() != Request::RequestComplete || + request->result() != 0) { return; + } const Request::BufferMap &buffers = request->buffers(); diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp index c4bc2110..6cff9bb4 100644 --- a/test/camera/capture.cpp +++ b/test/camera/capture.cpp @@ -43,8 +43,10 @@ protected: void requestComplete(Request *request) { - if (request->status() != Request::RequestComplete) + if (request->status() != Request::RequestComplete || + request->result() != 0) { return; + } const Request::BufferMap &buffers = request->buffers();
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 | 4 ++-- src/android/camera_device.cpp | 6 ++++-- src/cam/capture.cpp | 4 +++- src/gstreamer/gstlibcamerasrc.cpp | 4 ++++ src/qcam/main_window.cpp | 4 +++- src/v4l2/v4l2_camera.cpp | 4 +++- test/camera/buffer_import.cpp | 4 +++- test/camera/capture.cpp | 4 +++- 8 files changed, 25 insertions(+), 9 deletions(-)