[libcamera-devel] [PATCH 03/11] android: camera_device: Build capture_result dynamically
Hirokazu Honda
hiroh at chromium.org
Tue Oct 19 06:41:04 CEST 2021
Hi Umang and Laurent, thank you for the patch.
On Tue, Oct 19, 2021 at 2:39 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Mon, Oct 18, 2021 at 06:42:51PM +0200, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote:
> > > Hi Jacopo
> > >
> > > On 10/18/21 9:18 PM, Jacopo Mondi wrote:
> > > > Hi Umang,
> > > >
> > > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:
> > > > > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > >
> > > > > The camera3_capture_result_t is only needed to convey capture results to
> > > > > the camera service through the process_capture_result() callback.
> > > > > There's no need to store it in the Camera3RequestDescriptor. Build it
> > > > > dynamically in CameraDevice::sendCaptureResults() instead.
> > > > >
> > > > > This requires storing the result metadata created in
> > > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side
> > > > > effect of this change is that the request metadata lifetime will match
> > > > > the Camera3RequestDescriptor instead of being destroyed at the end of
> > > > > requestComplete(). This will be needed to support asynchronous
> > > > > post-processing, where the request completion will be signaled to the
> > > > > camera service asynchronously from requestComplete().
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > ---
> > > > > src/android/camera_device.cpp | 43 ++++++++++++++++-------------------
> > > > > src/android/camera_request.h | 2 +-
> > > > > 2 files changed, 21 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > index b4ab5da1..c6ae8930 100644
> > > > > --- a/src/android/camera_device.cpp
> > > > > +++ b/src/android/camera_device.cpp
> > > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> > > > > {
> > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> > > > >
> > > > > - camera3_capture_result_t &result = descriptor->captureResult_;
> > > > > - result.num_output_buffers = descriptor->buffers_.size();
> > > > > - result.frame_number = descriptor->frameNumber_;
> > > > > - result.partial_result = 0;
> > > > > -
> > > > > - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> > > > > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> > > > > - buffer = descriptor->buffers_[i];
> > > > > - buffer.release_fence = descriptor->buffers_[i].acquire_fence;
> > > > > + for (auto &buffer : descriptor->buffers_) {
> > > > > + buffer.release_fence = buffer.acquire_fence;
> > > > > buffer.acquire_fence = -1;
> > > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > > > }
> > > > > - result.output_buffers = resultBuffers.data();
> > > > >
> > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > > > > }
> > > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)
> > > > > * The buffer status is set to OK and later changed to ERROR if
> > > > > * post-processing/compression fails.
> > > > > */
> > > > > - camera3_capture_result_t &captureResult = descriptor->captureResult_;
> > > > > - captureResult.frame_number = descriptor->frameNumber_;
> > > > > - captureResult.num_output_buffers = descriptor->buffers_.size();
> > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > > > > CameraStream *cameraStream =
> > > > > static_cast<CameraStream *>(buffer.stream->priv);
> > > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)
> > > > > buffer.release_fence = -1;
> > > > > buffer.status = CAMERA3_BUFFER_STATUS_OK;
> > > > > }
> > > > > - captureResult.output_buffers = descriptor->buffers_.data();
> > > > > - captureResult.partial_result = 1;
> > > > >
> > > > > /*
> > > > > * If the Request has failed, abort the request by notifying the error
> > > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)
> > > > > notifyError(descriptor->frameNumber_, nullptr,
> > > > > CAMERA3_MSG_ERROR_REQUEST);
> > > > >
> > > > > - captureResult.partial_result = 0;
> > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > > > > /*
> > > > > * Signal to the framework it has to handle fences that
> > > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)
> > > > > * Notify if the metadata generation has failed, but continue processing
> > > > > * buffers and return an empty metadata pack.
> > > > > */
> > > > > - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);
> > > > > - if (!resultMetadata) {
> > > > > + descriptor->resultMetadata_ = getResultMetadata(*descriptor);
> > > > > + if (!descriptor->resultMetadata_) {
> > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > > > >
> > > > > /* The camera framework expects an empty metadata pack on error. */
> > > > > - resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> > > > > + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0);
> > > >
> > > > Since we don't sendCaptureResult() and return here we get to
> > > > post-processing with a (0, 0) metadata pack. I wonder if the
> > > > post-processing code is robust enough to cope with that situation
> > > > (even if I suspect I already know the answer :)
> > > >
> > > > It is my understanding the situation is like this already, so it's
> > > > fine, but maybe we should add a todo.
> > >
> > > \todo stating what? The situation or the solution. I am not sure of a
> > > potential solution of this yet
> >
> > I wish I had a solution. Just recording the potential issue, if you
> > think it's opportune.
>
> * \todo Check that the post-processor code handles this situation
> * correctly.
>
> > > > > }
> > > > >
> > > > > /* Handle post-processing. */
> > > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)
> > > > >
> > > > > int ret = cameraStream->process(*src, buffer,
> > > > > descriptor->settings_,
> > > > > - resultMetadata.get());
> > > > > + descriptor->resultMetadata_.get());
> > > > > /*
> > > > > * Return the FrameBuffer to the CameraStream now that we're
> > > > > * done processing it.
> > > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)
> > > > > }
> > > > > }
> > > > >
> > > > > - captureResult.result = resultMetadata->get();
> > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> > > > > sendCaptureResults();
> > > > > }
> > > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()
> > > > > * impact on performance which should be measured.
> > > > > */
> > > > > lock.unlock();
> > > > > - callbacks_->process_capture_result(callbacks_,
> > > > > - &descriptor->captureResult_);
> > > > > +
> > > > > + camera3_capture_result_t captureResult = {};
> > > > > +
> > > > > + captureResult.frame_number = descriptor->frameNumber_;
> > > > > + if (descriptor->resultMetadata_)
> > > >
> > > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is
> > > > constructed owning nothing, and calling get() on it simnply return
> > > > nullptr, which I think it's what we want.
> > >
> > > Yes we need this. There are two get() here
> > >
> > > a) one of the unique_ptr one, .get()
> > >
> > > b) another is the descriptor->resultMetadata_->get()) one
> > >
> > > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it,
> > > will segfault
> > >
> > > So I found best to guard it with an if () block for now. Does it make sense?
> >
> > Sorry, I confused unique_ptr<>.get() with CameraMetadata::get().
>
> Renaming CameraMetadata::get() to getMetadata() may be a good idea to
> avoid future confusion.
>
> > > > > + captureResult.result = descriptor->resultMetadata_->get();
> > > > > + captureResult.num_output_buffers = descriptor->buffers_.size();
> > > > > + captureResult.output_buffers = descriptor->buffers_.data();
> > > > > +
> > > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)
> > > > > + captureResult.partial_result = 1;
> > > > > +
> > > > > + callbacks_->process_capture_result(callbacks_, &captureResult);
> > > > > +
> > > > > lock.lock();
> > > > > }
> > > > > }
> > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > > > > index 79dfdb58..db13f624 100644
> > > > > --- a/src/android/camera_request.h
> > > > > +++ b/src/android/camera_request.h
> > > > > @@ -40,8 +40,8 @@ public:
> > > > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > > > > CameraMetadata settings_;
> > > > > std::unique_ptr<CaptureRequest> request_;
> > > > > + std::unique_ptr<CameraMetadata> resultMetadata_;
> > > > >
> > > > > - camera3_capture_result_t captureResult_ = {};
> > > > > Status status_ = Status::Pending;
> > > > >
> > > > > private:
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list