[libcamera-devel] [PATCH v2 2/2] android: CameraDevice: Implement HAL3 API flush

[Hirokazu Honda]

Hi Laurent,

On Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote:
> > This implements flush(). It is mostly the same as close()
> > though the canceled events are reported with error messages.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------
> >  src/android/camera_device.h   |  1 +
> >  src/android/camera_ops.cpp    |  6 ++++-
> >  3 files changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index c74dc0e7..22a2e13c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -753,6 +753,15 @@ void CameraDevice::close()
> >       stop(/*releaseCamera=*/true);
> >  }
> >
> > +int CameraDevice::flush()
> > +{
> > +     std::scoped_lock<std::mutex> lock(mutex_);
> > +
> > +     stop();
> > +
> > +     return 0;
> > +}
> > +
> >  void CameraDevice::stop(bool releaseCamera)
> >  {
> >       streams_.clear();
> > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera)
> >       worker_.stop();
> >       camera_->stop();
> >
> > +     for (auto &[cookie, descriptor] : descriptors_) {
> > +             LOG(HAL, Debug) << "request is canceled: " << cookie;
>
>                 LOG(HAL, Debug) << "request " << cookie << " is canceled";
>
> > +
> > +             camera3_capture_result_t captureResult = {};
> > +             captureResult.frame_number = descriptor.frameNumber_;
> > +             captureResult.num_output_buffers = descriptor.buffers_.size();
> > +             for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > +                     buffer.acquire_fence = -1;
> > +                     buffer.release_fence = -1;
> > +                     buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +             }
> > +             captureResult.output_buffers = descriptor.buffers_.data();
> > +
> > +             notifyError(descriptor.frameNumber_,
> > +                         descriptor.buffers_[0].stream);
> > +             callbacks_->process_capture_result(callbacks_, &captureResult);
>
> This looks very similar to the code in CameraDevice::requestComplete(),
> would it make sense to move it to a separate function (including the
> call to notifyError) ?
>

Sure, I will factorize in the next patch.
My first thought is to reduce the number of patches in the series and
factorize them after the patches are merged to not complicate this
patch series any more.

> > +     }
> >       descriptors_.clear();
> >
> >       running_ = false;
> > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request)
> >       }
> >
> >       if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> > +             /*
> > +              * \todo Report and identify the stream number or configuration
> > +              * to clarify the stream that failed.
> > +              */
> > +             LOG(HAL, Error)
> > +                     << "Error occurred on frame " << descriptor.frameNumber_ << " ("
> > +                     << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")";
> > +
> >               /* \todo Improve error handling. In case we notify an error
> >                * because the metadata generation fails, a shutter event has
> >                * already been notified for this frame number before the error
> > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> >  {
> >       camera3_notify_msg_t notify = {};
> >
> > -     /*
> > -      * \todo Report and identify the stream number or configuration to
> > -      * clarify the stream that failed.
> > -      */
> > -     LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> > -                     << toPixelFormat(stream->format).toString() << ")";
> > -
> >       notify.type = CAMERA3_MSG_ERROR;
> >       notify.message.error.error_stream = stream;
> >       notify.message.error.frame_number = frameNumber;
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 08553584..7004c89a 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -41,6 +41,7 @@ public:
> >
> >       int open(const hw_module_t *hardwareModule);
> >       void close();
> > +     int flush();
> >
> >       unsigned int id() const { return id_; }
> >       camera3_device_t *camera3Device() { return &camera3Device_; }
> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> > index 696e8043..981a3d30 100644
> > --- a/src/android/camera_ops.cpp
> > +++ b/src/android/camera_ops.cpp
> > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
> >
> >  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
>
> You can drop [[maybe_unused]].
>
> It would be nice to have an [[unused]] that causes a warning when the
> variable is used.
>
> >  {
> > -     return 0;
> > +     if (!dev)
> > +             return -EINVAL;
> > +
> > +     CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> > +     return camera->flush();
> >  }
> >
> >  int hal_dev_close(hw_device_t *hw_device)
>
> --
> Regards,
>
> Laurent Pinchart