[libcamera-devel] [PATCH] android: CameraDevice: factorize generating a capture result

[Hirokazu Honda]

Hi Laurent,

On Mon, Apr 26, 2021 at 2:28 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:
> > This factorizes a code of generating camera3_capture_result. It
> > will be useful to report capture result in other places than
> > CameraDevice::reqeustComplete().
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > ---
> > I was going to implement
> > reportShutter() - createCaptureResult() + notifyShutter(), and
> > reportError() - createCaptureResult() + notifyError().
> >
> > But reportError() can be called after reportShutter(). It is a
> > bit less efficient to create capture result twice in the case.
> > So I only factorize a code of generating a capture result.
>
> Can't we have a single function to do all that ? Right now we have
>
>         /* Prepare to call back the Android camera stack. */
>         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 = status;
>         }
>         captureResult.output_buffers = descriptor.buffers_.data();
>
>         if (status == CAMERA3_BUFFER_STATUS_OK) {
>                 notifyShutter(descriptor.frameNumber_, timestamp);
>
>                 captureResult.partial_result = 1;
>                 captureResult.result = resultMetadata->get();
>         }
>
>         if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
>                 /* \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
>                  * is here signalled. Make sure the error path plays well with
>                  * the camera stack state machine.
>                  */
>                 notifyError(descriptor.frameNumber_,
>                             descriptor.buffers_[0].stream);
>         }
>
>         callbacks_->process_capture_result(callbacks_, &captureResult);
>
> Could we move all that to a function that takes descriptor, status,
> timestamp and resultMedadata as arguments ? In stop(), to implement
> flush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,
> so notifyShutter() would be skipped.
>
> If you think that would cause issues, this patch is already an
> improvement, so we could proceed with it.
>

I did so some times ago when I worked with Jacopo for Flush().
Jacopo suggested to not not contain a code for shutter and error in
the same function, as either of them are not executed.
Jacopo, how do you think?

-Hiro

> > ---
> >  src/android/camera_device.cpp | 29 ++++++++++++++++++++---------
> >  src/android/camera_device.h   |  3 +++
> >  2 files changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a71aee2f..ced8efcc 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)
> >       }
> >
> >       /* Prepare to call back the Android camera stack. */
> > -     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 = status;
> > -     }
> > -     captureResult.output_buffers = descriptor.buffers_.data();
> > +     camera3_capture_result_t captureResult =
> > +             createCaptureResult(descriptor, status);
> >
> >       if (status == CAMERA3_BUFFER_STATUS_OK) {
> >               notifyShutter(descriptor.frameNumber_, timestamp);
> > @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const
> >       return "'" + camera_->id() + "'";
> >  }
> >
> > +
> > +camera3_capture_result_t CameraDevice::createCaptureResult(
> > +     Camera3RequestDescriptor &descriptor,
> > +     camera3_buffer_status status) const
> > +{
> > +     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 = status;
> > +     }
> > +     captureResult.output_buffers = descriptor.buffers_.data();
> > +
> > +     return captureResult;
> > +}
> > +
> >  void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> >  {
> >       camera3_notify_msg_t notify = {};
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index c63e8e21..a1abcead 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -100,6 +100,9 @@ private:
> >
> >       std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >       libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > +     camera3_capture_result_t createCaptureResult(
> > +             Camera3RequestDescriptor &descriptor,
> > +             camera3_buffer_status status) const;
> >       void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >       void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >       std::unique_ptr<CameraMetadata> requestTemplatePreview();
>
> --
> Regards,
>
> Laurent Pinchart