| Message ID | 20210406091334.3823748-1-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote: > This implements camera3_device_ops::flush(). > Is this enough ? reading the flush() operation documentation in camera3.h it seems it can be called concurrently to process_capture_request() and requests yet to be queued shall be returned immediately. Should that be handled as well ? > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++---- > src/android/camera_device.h | 3 +++ > src/android/camera_ops.cpp | 3 ++- > 3 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 4a3f6f8e..5a56fe4b 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request) > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK; > std::unique_ptr<CameraMetadata> resultMetadata; > > - decltype(descriptors_)::node_type node; > + Camera3RequestDescriptor descriptor; > { > std::scoped_lock<std::mutex> lock(mutex_); > auto it = descriptors_.find(request->cookie()); > if (it == descriptors_.end()) { > LOG(HAL, Fatal) > << "Unknown request: " << request->cookie(); > - status = CAMERA3_BUFFER_STATUS_ERROR; > return; > } > > - node = descriptors_.extract(it); > + descriptor = std::move(it->second); > + /* Restore frameNumber_ because it is used in the case of flush > + * timeout case. > + */ > + it->second.frameNumber_ = descriptor.frameNumber_; Is accessing it->second after having moved it valid ? also, aren't the lhs and rhs referring to the same variable ? > } > - Camera3RequestDescriptor &descriptor = node.mapped(); > > if (request->status() != Request::RequestComplete) { > LOG(HAL, Error) << "Request not successfully completed: " > @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request) > descriptor.buffers_[0].stream); > } > > - callbacks_->process_capture_result(callbacks_, &captureResult); > + { > + std::scoped_lock<std::mutex> lock(mutex_); > + if (descriptors_.erase(request->cookie()) == 0) { > + // flush() cleans up the descriptor due to time out > + // during a post processing. No C++ comments please > + return; > + } > + callbacks_->process_capture_result(callbacks_, &captureResult); > + conditionVariable_.notify_one(); aren't this function and flush() being run in the same thread ? Won't this contend the mutex ? To be honest I would have expected flush to stop the libcamera::Camera to have all requests in-flight be immediately be completed with errors. As said flush also prevents calls to process_capture_request() to be queued if flush is called concurrently (which makes me think my assumption on the flush calls happening on the same thread to be wrong). Another question is how to interrupt post-processing to happen once it will be moved to a separate thread. If flush() times out and delete the descriptor, won't post-processor then tries to access a non-valid memory address ? Thanks j > + } > +} > + > +int CameraDevice::flush() > +{ > + std::unique_lock<std::mutex> lock(mutex_); > + /* flush() should return in less than one second. Sets the timeout to 900ms. */ > + auto flushTimeout = > + std::chrono::system_clock::now() + std::chrono::milliseconds(900); > + bool completeAllRequests = conditionVariable_.wait_until( > + lock, flushTimeout, [this]() { return descriptors_.empty(); }); > + > + if (!completeAllRequests) { > + for (auto &node : descriptors_) { > + auto &descriptor = node.second; > + camera3_capture_result_t captureResult{}; > + captureResult.frame_number = descriptor.frameNumber_; > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > + buffer.acquire_fence = -1; > + buffer.release_fence = -1; > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > + } > + callbacks_->process_capture_result(callbacks_, &captureResult); > + } > + descriptors_.clear(); > + } > + > + return 0; > } > > std::string CameraDevice::logPrefix() const > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index c63e8e21..dd5336ee 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -7,6 +7,7 @@ > #ifndef __ANDROID_CAMERA_DEVICE_H__ > #define __ANDROID_CAMERA_DEVICE_H__ > > +#include <condition_variable> > #include <map> > #include <memory> > #include <mutex> > @@ -62,6 +63,7 @@ public: > int configureStreams(camera3_stream_configuration_t *stream_list); > int processCaptureRequest(camera3_capture_request_t *request); > void requestComplete(libcamera::Request *request); > + int flush(); > > protected: > std::string logPrefix() const override; > @@ -128,6 +130,7 @@ private: > std::vector<CameraStream> streams_; > > std::mutex mutex_; /* Protect descriptors_ */ > + std::condition_variable conditionVariable_; > std::map<uint64_t, Camera3RequestDescriptor> descriptors_; > > std::string maker_; > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > index 696e8043..1b73af13 100644 > --- a/src/android/camera_ops.cpp > +++ b/src/android/camera_ops.cpp > @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) > { > - return 0; > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > + return camera->flush(); > } > > int hal_dev_close(hw_device_t *hw_device) > -- > 2.31.0.208.g409f899ff0-goog > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Thanks Jacopo for comments, You're right. I missed the description. I will fix it. On Tue, Apr 6, 2021 at 10:23 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote: > > This implements camera3_device_ops::flush(). > > > > Is this enough ? reading the flush() operation documentation in > camera3.h it seems it can be called concurrently to > process_capture_request() and requests yet to be queued shall be > returned immediately. Should that be handled as well ? > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++---- > > src/android/camera_device.h | 3 +++ > > src/android/camera_ops.cpp | 3 ++- > > 3 files changed, 48 insertions(+), 6 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 4a3f6f8e..5a56fe4b 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request) > > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK; > > std::unique_ptr<CameraMetadata> resultMetadata; > > > > - decltype(descriptors_)::node_type node; > > + Camera3RequestDescriptor descriptor; > > { > > std::scoped_lock<std::mutex> lock(mutex_); > > auto it = descriptors_.find(request->cookie()); > > if (it == descriptors_.end()) { > > LOG(HAL, Fatal) > > << "Unknown request: " << request->cookie(); > > - status = CAMERA3_BUFFER_STATUS_ERROR; > > return; > > } > > > > - node = descriptors_.extract(it); > > + descriptor = std::move(it->second); > > + /* Restore frameNumber_ because it is used in the case of flush > > + * timeout case. > > + */ > > + it->second.frameNumber_ = descriptor.frameNumber_; > > Is accessing it->second after having moved it valid ? also, aren't > the lhs and rhs referring to the same variable ? > > > } > > - Camera3RequestDescriptor &descriptor = node.mapped(); > > > > if (request->status() != Request::RequestComplete) { > > LOG(HAL, Error) << "Request not successfully completed: " > > @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request) > > descriptor.buffers_[0].stream); > > } > > > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > + { > > + std::scoped_lock<std::mutex> lock(mutex_); > > + if (descriptors_.erase(request->cookie()) == 0) { > > + // flush() cleans up the descriptor due to time out > > + // during a post processing. > > No C++ comments please > > > + return; > > + } > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > + conditionVariable_.notify_one(); > > aren't this function and flush() being run in the same thread ? Won't > this contend the mutex ? > > To be honest I would have expected flush to stop the libcamera::Camera > to have all requests in-flight be immediately be completed with > errors. As said flush also prevents calls to > process_capture_request() to be queued if flush is called concurrently > (which makes me think my assumption on the flush calls happening on the same > thread to be wrong). > > Another question is how to interrupt post-processing to happen once it > will be moved to a separate thread. If flush() times out and delete > the descriptor, won't post-processor then tries to access a non-valid > memory address ? > > Thanks > j > > > + } > > +} > > + > > +int CameraDevice::flush() > > +{ > > + std::unique_lock<std::mutex> lock(mutex_); > > + /* flush() should return in less than one second. Sets the timeout to 900ms. */ > > + auto flushTimeout = > > + std::chrono::system_clock::now() + std::chrono::milliseconds(900); > > + bool completeAllRequests = conditionVariable_.wait_until( > > + lock, flushTimeout, [this]() { return descriptors_.empty(); }); > > + > > + if (!completeAllRequests) { > > + for (auto &node : descriptors_) { > > + auto &descriptor = node.second; > > + camera3_capture_result_t captureResult{}; > > + captureResult.frame_number = descriptor.frameNumber_; > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > + buffer.acquire_fence = -1; > > + buffer.release_fence = -1; > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > + } > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > + } > > + descriptors_.clear(); > > + } > > + > > + return 0; > > } > > > > std::string CameraDevice::logPrefix() const > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index c63e8e21..dd5336ee 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -7,6 +7,7 @@ > > #ifndef __ANDROID_CAMERA_DEVICE_H__ > > #define __ANDROID_CAMERA_DEVICE_H__ > > > > +#include <condition_variable> > > #include <map> > > #include <memory> > > #include <mutex> > > @@ -62,6 +63,7 @@ public: > > int configureStreams(camera3_stream_configuration_t *stream_list); > > int processCaptureRequest(camera3_capture_request_t *request); > > void requestComplete(libcamera::Request *request); > > + int flush(); > > > > protected: > > std::string logPrefix() const override; > > @@ -128,6 +130,7 @@ private: > > std::vector<CameraStream> streams_; > > > > std::mutex mutex_; /* Protect descriptors_ */ > > + std::condition_variable conditionVariable_; > > std::map<uint64_t, Camera3RequestDescriptor> descriptors_; > > > > std::string maker_; > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > > index 696e8043..1b73af13 100644 > > --- a/src/android/camera_ops.cpp > > +++ b/src/android/camera_ops.cpp > > @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) > > { > > - return 0; > > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > > + return camera->flush(); > > } > > > > int hal_dev_close(hw_device_t *hw_device) > > -- > > 2.31.0.208.g409f899ff0-goog > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Thu, Apr 08, 2021 at 06:17:00PM +0900, Hirokazu Honda wrote: > Thanks Jacopo for comments, > You're right. I missed the description. I will fix it. > > On Tue, Apr 6, 2021 at 10:23 PM Jacopo Mondi wrote: > > On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote: > > > This implements camera3_device_ops::flush(). > > > > Is this enough ? reading the flush() operation documentation in > > camera3.h it seems it can be called concurrently to > > process_capture_request() and requests yet to be queued shall be > > returned immediately. Should that be handled as well ? The documentation states * flush() may be called concurrently to processCaptureRequest(), with the * expectation that processCaptureRequest returns quickly and the * request submitted in that processCaptureRequest call is treated like * all other in-flight requests. Due to concurrency issues, it is possible * that from the HAL's point of view, a processCaptureRequest() call may * be started after flush has been invoked but has not returned yet. If such * a call happens before flush() returns, the HAL must treat the new * capture request like other in-flight pending requests (see #4 below). This seems to be a fairly ill-defined behaviour, as in theory, the late processCaptureRequest() call that races flush() could arrive after flush() returns, and be interpreted as an instruction to restart the camera. This should ideally be clarified with Android developers. > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++---- > > > src/android/camera_device.h | 3 +++ > > > src/android/camera_ops.cpp | 3 ++- > > > 3 files changed, 48 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 4a3f6f8e..5a56fe4b 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request) > > > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK; > > > std::unique_ptr<CameraMetadata> resultMetadata; > > > > > > - decltype(descriptors_)::node_type node; > > > + Camera3RequestDescriptor descriptor; > > > { > > > std::scoped_lock<std::mutex> lock(mutex_); > > > auto it = descriptors_.find(request->cookie()); > > > if (it == descriptors_.end()) { > > > LOG(HAL, Fatal) > > > << "Unknown request: " << request->cookie(); > > > - status = CAMERA3_BUFFER_STATUS_ERROR; > > > return; > > > } > > > > > > - node = descriptors_.extract(it); > > > + descriptor = std::move(it->second); > > > + /* Restore frameNumber_ because it is used in the case of flush > > > + * timeout case. > > > + */ > > > + it->second.frameNumber_ = descriptor.frameNumber_; > > > > Is accessing it->second after having moved it valid ? also, aren't > > the lhs and rhs referring to the same variable ? > > > > > } > > > - Camera3RequestDescriptor &descriptor = node.mapped(); > > > > > > if (request->status() != Request::RequestComplete) { > > > LOG(HAL, Error) << "Request not successfully completed: " > > > @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request) > > > descriptor.buffers_[0].stream); > > > } > > > > > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > > + { > > > + std::scoped_lock<std::mutex> lock(mutex_); > > > + if (descriptors_.erase(request->cookie()) == 0) { > > > + // flush() cleans up the descriptor due to time out > > > + // during a post processing. > > > > No C++ comments please > > > > > + return; > > > + } > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > + conditionVariable_.notify_one(); The condition variable should be signalled without he lock held. > > > > aren't this function and flush() being run in the same thread ? Won't > > this contend the mutex ? > > > > To be honest I would have expected flush to stop the libcamera::Camera > > to have all requests in-flight be immediately be completed with > > errors. As said flush also prevents calls to > > process_capture_request() to be queued if flush is called concurrently > > (which makes me think my assumption on the flush calls happening on the same > > thread to be wrong). Agreed, I would also expect flush() to call Camera::stop(). The hardware will effectively have to stop, as there will be no buffers anymore. > > Another question is how to interrupt post-processing to happen once it > > will be moved to a separate thread. If flush() times out and delete > > the descriptor, won't post-processor then tries to access a non-valid > > memory address ? We'll have to wait on the post-processing tasks, possibly with cancellation points in the implementation (it would be interesting to add cancellation points to libyuv). > > > + } > > > +} > > > + > > > +int CameraDevice::flush() > > > +{ > > > + std::unique_lock<std::mutex> lock(mutex_); > > > + /* flush() should return in less than one second. Sets the timeout to 900ms. */ As Jacopo pointed out, this is dangerous. I'd rather print a warning if we take too long than risk a crash. > > > + auto flushTimeout = > > > + std::chrono::system_clock::now() + std::chrono::milliseconds(900); > > > + bool completeAllRequests = conditionVariable_.wait_until( > > > + lock, flushTimeout, [this]() { return descriptors_.empty(); }); > > > + > > > + if (!completeAllRequests) { > > > + for (auto &node : descriptors_) { > > > + auto &descriptor = node.second; > > > + camera3_capture_result_t captureResult{}; > > > + captureResult.frame_number = descriptor.frameNumber_; > > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > > + buffer.acquire_fence = -1; > > > + buffer.release_fence = -1; > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > + } > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > + } > > > + descriptors_.clear(); > > > + } > > > + > > > + return 0; > > > } > > > > > > std::string CameraDevice::logPrefix() const > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index c63e8e21..dd5336ee 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -7,6 +7,7 @@ > > > #ifndef __ANDROID_CAMERA_DEVICE_H__ > > > #define __ANDROID_CAMERA_DEVICE_H__ > > > > > > +#include <condition_variable> > > > #include <map> > > > #include <memory> > > > #include <mutex> > > > @@ -62,6 +63,7 @@ public: > > > int configureStreams(camera3_stream_configuration_t *stream_list); > > > int processCaptureRequest(camera3_capture_request_t *request); > > > void requestComplete(libcamera::Request *request); > > > + int flush(); > > > > > > protected: > > > std::string logPrefix() const override; > > > @@ -128,6 +130,7 @@ private: > > > std::vector<CameraStream> streams_; > > > > > > std::mutex mutex_; /* Protect descriptors_ */ > > > + std::condition_variable conditionVariable_; > > > std::map<uint64_t, Camera3RequestDescriptor> descriptors_; > > > > > > std::string maker_; > > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp > > > index 696e8043..1b73af13 100644 > > > --- a/src/android/camera_ops.cpp > > > +++ b/src/android/camera_ops.cpp > > > @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, > > > > > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) > > > { > > > - return 0; > > > + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); > > > + return camera->flush(); > > > } > > > > > > int hal_dev_close(hw_device_t *hw_device)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 4a3f6f8e..5a56fe4b 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request) camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK; std::unique_ptr<CameraMetadata> resultMetadata; - decltype(descriptors_)::node_type node; + Camera3RequestDescriptor descriptor; { std::scoped_lock<std::mutex> lock(mutex_); auto it = descriptors_.find(request->cookie()); if (it == descriptors_.end()) { LOG(HAL, Fatal) << "Unknown request: " << request->cookie(); - status = CAMERA3_BUFFER_STATUS_ERROR; return; } - node = descriptors_.extract(it); + descriptor = std::move(it->second); + /* Restore frameNumber_ because it is used in the case of flush + * timeout case. + */ + it->second.frameNumber_ = descriptor.frameNumber_; } - Camera3RequestDescriptor &descriptor = node.mapped(); if (request->status() != Request::RequestComplete) { LOG(HAL, Error) << "Request not successfully completed: " @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request) descriptor.buffers_[0].stream); } - callbacks_->process_capture_result(callbacks_, &captureResult); + { + std::scoped_lock<std::mutex> lock(mutex_); + if (descriptors_.erase(request->cookie()) == 0) { + // flush() cleans up the descriptor due to time out + // during a post processing. + return; + } + callbacks_->process_capture_result(callbacks_, &captureResult); + conditionVariable_.notify_one(); + } +} + +int CameraDevice::flush() +{ + std::unique_lock<std::mutex> lock(mutex_); + /* flush() should return in less than one second. Sets the timeout to 900ms. */ + auto flushTimeout = + std::chrono::system_clock::now() + std::chrono::milliseconds(900); + bool completeAllRequests = conditionVariable_.wait_until( + lock, flushTimeout, [this]() { return descriptors_.empty(); }); + + if (!completeAllRequests) { + for (auto &node : descriptors_) { + auto &descriptor = node.second; + camera3_capture_result_t captureResult{}; + captureResult.frame_number = descriptor.frameNumber_; + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + buffer.acquire_fence = -1; + buffer.release_fence = -1; + buffer.status = CAMERA3_BUFFER_STATUS_ERROR; + } + callbacks_->process_capture_result(callbacks_, &captureResult); + } + descriptors_.clear(); + } + + return 0; } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index c63e8e21..dd5336ee 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -7,6 +7,7 @@ #ifndef __ANDROID_CAMERA_DEVICE_H__ #define __ANDROID_CAMERA_DEVICE_H__ +#include <condition_variable> #include <map> #include <memory> #include <mutex> @@ -62,6 +63,7 @@ public: int configureStreams(camera3_stream_configuration_t *stream_list); int processCaptureRequest(camera3_capture_request_t *request); void requestComplete(libcamera::Request *request); + int flush(); protected: std::string logPrefix() const override; @@ -128,6 +130,7 @@ private: std::vector<CameraStream> streams_; std::mutex mutex_; /* Protect descriptors_ */ + std::condition_variable conditionVariable_; std::map<uint64_t, Camera3RequestDescriptor> descriptors_; std::string maker_; diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp index 696e8043..1b73af13 100644 --- a/src/android/camera_ops.cpp +++ b/src/android/camera_ops.cpp @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev, static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev) { - return 0; + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv); + return camera->flush(); } int hal_dev_close(hw_device_t *hw_device)
This implements camera3_device_ops::flush(). Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++---- src/android/camera_device.h | 3 +++ src/android/camera_ops.cpp | 3 ++- 3 files changed, 48 insertions(+), 6 deletions(-)