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

[Hirokazu Honda]

Hi Jacopo,

On Mon, Apr 26, 2021 at 7:38 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Apr 26, 2021 at 06:08:10PM +0900, Hirokazu Honda wrote:
> > 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?
> >
>
> It's terribily hard to have a sense of how a patch is used without
> context with all the pending work we have in flux.
>
> If I recall correctly, in the context of the flush() implementation we
> discussed, was that having all in one function was not that beneficial
> as notifications will have to be sent to the framework asynchronously
> in the long term, and part of the function was dead code.
>
> Again, memory can fail me, there's really a lot of things moving and
> keep the full picture in mind is hard.
>

I uploaded the next patch series. Let's discuss there.

> Thanks
>    j
>
> > -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