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

[Laurent Pinchart]

Hi Hiro,

On Mon, Apr 26, 2021 at 11:51:40AM +0900, Hirokazu Honda wrote:
> On Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart wrote:
> > 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.

I actually makes review easier if you factor out the code in a separate
function in a patch of its own, before this patch.

> > > +     }
> > >       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