[libcamera-devel] [PATCH v3 09/10] android: camera_device: Send capture results inspecting the descriptor
Umang Jain
umang.jain at ideasonboard.com
Mon Sep 27 13:26:28 CEST 2021
Hi Hiro,
On 9/27/21 12:32 PM, Hirokazu Honda wrote:
> 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.
The v3 *now* is a mere reference for future development on this front.
We have split up design of HAL into 3 parts and have been internally
discussing to even clean up more the implementation that we have today.
It's still work-in-progress but I will posting the relevant modules of
the design that will eventually lead up to setting up infrastructure
required for async post-processors.
More or less broadly:
- We expect to have only queue only - to track each incoming request,
irrespective of whether it requires post-processing or not
- status variables will be set on Camera3RequestDescriptors-wide and
then Post-Processing-wide separately
- Async Post-processing somehow needs to be "cancellable" on demand - to
satisfy flush use cases
- Single point of sending back capture results ONLY a.k.a single
process_capture_result
These are the hard requirements we are tracking to clean up the
implementation we have currently. It's substantial work plus keeping in
mind not to regress CTS (which has happened recently, even hampering
development on this front). I may / may not address reviews on this
particular v3, since I will be more inclined to develop in line to our
design goals, while treating this series and it's reviews just as a
point of reference at this point.
Thanks!
>
> -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