[libcamera-devel] [PATCH v3 09/10] android: camera_device: Send capture results inspecting the descriptor
Hirokazu Honda
hiroh at chromium.org
Mon Sep 27 09:02:14 CEST 2021
Hi Umang, thank you for the patch.
On Wed, Sep 22, 2021 at 8:27 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote:
> > Hi Jacopo,
> >
> > (Post in call discussion summary on this if anyone here is interested)
> >
> > On 9/21/21 5:16 PM, Jacopo Mondi wrote:
> > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:
> > >> A Camera3RequestDescriptors is constructed and queued to descriptors_
>
> s/Camera3RequestDescriptors/Camera3RequestDescriptor/
>
> > >> queue as soon as an (incoming) capture request is received on the
> > >> libcamera HAL. The capture request is picked up by HAL, in order to run
> > >> it to completion. At completion, CameraDevice::requestComplete() gets
> > >> invoked and capture results are populated and ready to be sent back
> > >> to the framework.
> > >>
> > >> All the data and framebuffers associated with the request are alive
> > >> and encapsulated inside this Camera3RequestDescriptor descriptor.
> > >> By inspecting the ProcessStatus on the descriptor, we can now send
> > >> capture results via the process_capture_result() callback.
> > >>
> > >> Hence, introduce a new private member function sendCaptureResults()
> > >> which will be responsible to send capture results back to the
> > >> framework by inspecting the descriptor on the queue. In subsequent
> > >> commit, when the post processsor shall run async, sendCaptureResults()
> > >> can be called from the post-processor's thread for e.g.
> > >> streamProcessComplete() hence, introduce the mutex lock to avoid the
> > >> races.
> > >>
> > >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > >> ---
> > >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------
> > >> src/android/camera_device.h | 4 +++
> > >> 2 files changed, 38 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > >> index 4658e881..16ecdfc5 100644
> > >> --- a/src/android/camera_device.cpp
> > >> +++ b/src/android/camera_device.cpp
> > >> @@ -1078,7 +1078,7 @@ 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 = {};
> > >> + 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_) {
> > >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)
> > >> continue;
> > >> }
> > >>
> > >> + if (cameraStream->type() == CameraStream::Type::Internal)
> > >> + descriptor->internalBuffer_ = src;
> > >> +
> > >> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;
> > >>
> > >> cameraStream->process(src, *buffer.buffer, descriptor);
> > >> -
> > >> - /*
> > >> - * Return the FrameBuffer to the CameraStream now that we're
> > >> - * done processing it.
> > >> - */
> > >> - if (cameraStream->type() == CameraStream::Type::Internal)
> > >> - cameraStream->putBuffer(src);
> > >> + return;
I read the patches 07-09/10.
The new code looks to assume the number of buffers that require post
processing is only one.
I consider so from
1.) return added here
2.) no protection to the Status
3.) no check to wait for the number of pot-processing in
streamProcessingComplete(), so that a capture request complete is sent
multiple times.
I also think descriptors_ pop logic is wrong.
I think we ideally deny a request given in requestComplete if it is
not a top of descriptors_.
The top of descriptors is kept to the currently processing capture request.
I think we should have two queues here like following.
descriptors_ - for request processed by camera
descriptors_on_processing - for request processed by post processors
Then we pop descriptors_ in completeRequest and descriptors_on_processing.
Even if a capture request doesn't require post processing, we should
add it to descriptors_on_processing to send it after processing for
all the preceding request is complete
These may be resolved in the next patch. But this happens at least at
this patch if I understand correctly.
-Hiro
> > >> }
> > >>
> > >> - captureResult.result = descriptor->resultMetadata_->get();
> > >> - callbacks_->process_capture_result(callbacks_, &captureResult);
> > >> -
> > >> - descriptors_.pop_front();
> > >> + /*
> > >> + * Mark the status on the descriptor as success as no processing
> > >> + * is neeeded.
> > >> + */
> > >> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;
> > >> + sendCaptureResults();
> > >> }
> > >>
> > >>
> > >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,
> > >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
> > >> Camera3RequestDescriptor *request)
> > >> {
> > >> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {
> > >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer
> > >> CAMERA3_MSG_ERROR_BUFFER);
> > >> }
> > >> }
> > >> +
> > >> + /*
> > >> + * Return the FrameBuffer to the CameraStream now that we're
> > >> + * done processing it.
> > >> + */
> > >> + if (cameraStream->type() == CameraStream::Type::Internal)
> > >> + cameraStream->putBuffer(request->internalBuffer_);
> > >> +
> > >> + sendCaptureResults();
> > >> +}
> > >> +
> > >> +void CameraDevice::sendCaptureResults()
> > >> +{
> > >> + Camera3RequestDescriptor *d = descriptors_.front().get();
>
> The variable is named descriptor everywhere else, let's keep a common
> naming scheme (I'm tempted to rename Camera3RequestDescriptor to
> Camera3Request, and thus also rename all variables accordingly, but
> that's for later, on top of this series).
>
> > >
> > > If I'm not mistaken, in case a request that doesn't need
> > > post-processing will complete while the post-processed one is still
> > > being worked on, this could lead to a completion order inversion
> > >
> > > processCaptureRequest(n)
> > > descriptors.push(n)
> > >
> > > processCaptureRequest(n+1)
> > > descriptors.push(n+1)
> > >
> > > equestComplete(n)
> > > postProcess(n) -----------> process(n)
> > > |
> > > .... ...
> > > |
> > > requestComplete(n+1) |
> > > sendCaptureResult(n+1) |
> > > d = descriptors.front() |
> > > process_capture_res(n) |
> > > |
> > > sendCaptureResult(n) <----------------
> > > d = descritpros.front()
> > > process_capture_res(n + 1)
> > >
> >
> > Nice flow diagram btw, I need to get better at that ;-)
> >
> > So, first of all sendCaptureResults() doesn't take an argument, but I
> > get your point that you are trying to match for which 'n' request,
> > sendCapture result is called...
> >
> > sendCaptureResults() works on the queue's head. It doesn't care from
> > where it's called; it can be called from requestComplete()[same thread]
> > or streamProcessingComplete()[different thread]. All it does or suppose
> > to do, is basically looks at the queue's head and see if the HEAD
> > descriptor says - "Send me back to the framework".
> > - If yes, it will send the results via process_capture_result of the
> > HEAD and drop the HEAD from the queue
> > - If no, simply return;
> >
> > That's it. Since, it looks at the head, it will only send results in
> > order (since it's a queue) and drop the HEAD.
> >
> > However, the discussion has un-covered another point that
> > sendCaptureResults() can lag behind w.r.t libcamera:Requests being
> > completed, since there is no loop around to process other descriptors in
> > the queue which are ready to send (these descriptors can potentially be
> > the ones not requiring any post-processing). As discussed in the call,
> > we both agree that we should probably iterate over the queue and send
> > the results back to the framework as soon as possible, for ready
> > descriptor. So, sendCaptureResults() will be wrapped in a loop in
> > subsequent version.
>
> I'd include the loop inside sendCaptureResults(). Something along the
> lines of
>
> MutexLocker lock(descriptorsMutex_);
>
> while (!descriptors_.empty() &&
> descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) {
> std::unique_ptr<Camera3RequestDescriptor> descriptor =
> std::move(descriptors_.front());
> descriptors_.pop();
>
> lock.unlock();
>
> /* Signal completion to the camera service. */
>
> lock.lock();
> }
>
> (see my review of 07/10 for the Pending status)
>
> Note that you'll need to handle both the Success and Error statuses
> here, as requests that complete with an error need to be completed in
> order too.
>
> One a side note, maybe adding the following helper to
> Camera3RequestDescriptor would be nice, to make the loop more readable:
>
> bool isPending() const { return status_ == Status::Pending; }
>
> You would then get
>
> while (!descriptors_.empty() && descriptors_.front()->isPending()) {
> ...
> }
>
> > >> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||
> > >> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None)
> > >> + return;
> > >> +
> > >> + MutexLocker lock(descriptorsMutex_);
> > >> + d->captureResult_.result = d->resultMetadata_->get();
> > >> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_));
> > >> + descriptors_.pop_front();
> > >> }
> > >>
> > >> std::string CameraDevice::logPrefix() const
> > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > >> index 60c134dc..0bd570a1 100644
> > >> --- a/src/android/camera_device.h
> > >> +++ b/src/android/camera_device.h
> > >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {
> > >> std::unique_ptr<CaptureRequest> request_;
> > >> std::unique_ptr<CameraMetadata> resultMetadata_;
> > >>
> > >> + camera3_capture_result_t captureResult_ = {};
> > >> + libcamera::FrameBuffer *internalBuffer_;
> > >> +
> > >> ProcessStatus status_;
> > >> };
> > >>
> > >> @@ -118,6 +121,7 @@ private:
> > >> int processControls(Camera3RequestDescriptor *descriptor);
> > >> std::unique_ptr<CameraMetadata> getResultMetadata(
> > >> const Camera3RequestDescriptor *descriptor) const;
> > >> + void sendCaptureResults();
> > >>
> > >> unsigned int id_;
> > >> camera3_device_t camera3Device_;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list