[libcamera-devel] [PATCH] android: Implement flush()

[Hirokazu Honda]

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 at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel