[libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a queue for sending capture results
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 8 16:23:04 CEST 2021
Hi Laurent,
On 9/8/21 7:03 PM, Laurent Pinchart wrote:
> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:
>> On 07/09/2021 20:57, Umang Jain wrote:
>>> When a camera capture request completes, the next step is to send the
>>> capture results to the framework via process_capture_results(). All
>>> the capture associated result metadata is prepared and populated. If
>>> any post-processing is required, it will also take place in the same
>>> thread (which can be blocking for a subtaintial amount of time) before
>> s/subtaintial/substantial/
>>
>>> the results can be sent back to the framework.
>>>
>>> A follow up commit will move the post-processing to run in a separate
>>> thread. In order to do so, there is few bits of groundwork which this
>>> patch entails. Mainly, we need to preserve the order of sending
>>> the capture results back in which they were queued by the framework
>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue
>>> in which capture results can be queued with context.
>>>
>>> As per this patch, the post-processor still runs synchronously as
>>> before, but it will queue up the current capture results with context.
>>> Later down the line, the capture results are dequeud in order and
>> s/dequeud/dequeued/
>>
>>> sent back to the framework.
>>>
>>> The context is preserved using Camera3RequestDescriptor utility
>>> structure. It has been expanded accordingly to address the needs of
>>> the functionality.
>>>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>> ---
>>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---
>>> src/android/camera_device.h | 23 +++++++
>>> 2 files changed, 126 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index f2f36f32..9d4ec02e 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>>> /* Clone the controls associated with the camera3 request. */
>>> settings_ = CameraMetadata(camera3Request->settings);
>>>
>>> + internalBuffer_ = nullptr;
>>> +
>>> /*
>>> * Create the CaptureRequest, stored as a unique_ptr<> to tie its
>>> * lifetime to the descriptor.
>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>> * once it has been processed.
>>> */
>>> buffer = cameraStream->getBuffer();
>>> + descriptor.internalBuffer_ = buffer;
>>> LOG(HAL, Debug) << ss.str() << " (internal)";
>>> break;
>>> }
>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)
>>> continue;
>>> }
>>>
>>> + /*
>>> + * Save the current context of capture result and queue the
>>> + * descriptor before processing the camera stream.
>>> + *
>>> + * When the processing completes, the descriptor will be
>>> + * dequeued and capture results will be sent to the framework.
>>> + */
>>> + descriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;
>>> + descriptor.resultMetadata_ = std::move(resultMetadata);
>>> + descriptor.captureResult_ = captureResult;
>>> +
>>> + std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
>>> + std::make_unique<Camera3RequestDescriptor>();
>>> + *reqDescriptor = std::move(descriptor);
>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));
>>> + Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
>>> +
>>> + CameraMetadata *metadata = currentDescriptor->resultMetadata_.get();
>>> + cameraStream->processComplete.connect(
>>> + this, [=](CameraStream::ProcessStatus status) {
>>> + streamProcessingComplete(cameraStream, status,
>>> + metadata);
>> I believe this to be unsafe to manage multiple operations.
>>
>> cameraStream->processComplete() should not be connected with per-run
>> parameters. Those should be passed as the context of the signal emit(),
>> not by the connection.
>>
>> Otherwise, the lamba will be overwritten by the next requestComplete()
>> and incorrect parameters will be processed when the signal actually fires.
While this is true.. and a bug in my code
> It's worse than that, ever connect() will create a new connection, so
> the slot will be called once for the first request, twice for the
> second, ...
Oh, I gotta see this in action by sprinkling some printfs...
But What is a alternative to this? While I tried to disconnect signal
handlers in the slot the last time, it came back with:
'''
While signals can be connected and disconnected on demand, it often
indicates a design issue. Is there a reason why you can't connect the
signal when the cameraStream instance is created, and keep it connected
for the whole lifetime of the stream ?
'''
in
https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html
Not sure if it's possible to check on a object's signal to check for a
slot's existence. And doing so, feels weird as well.
So we need to connect to slot /only/ once, for all post-processing needs
to be done in future. This sounds like an operation that can sit well in
CameraStream::configure(). But how can we pass a slot (which is probably
private to class) to another class's configure()? A function object ?
>
>>> + });
>>> +
>>> int ret = cameraStream->process(src, *buffer.buffer,
>>> - descriptor.settings_,
>>> - resultMetadata.get());
>>> + currentDescriptor->settings_,
>>> + metadata);
>>> + return;
>>> + }
>>> +
>>> + if (queuedDescriptor_.empty()) {
>>> + captureResult.result = resultMetadata->get();
>>> + callbacks_->process_capture_result(callbacks_, &captureResult);
>> *1 duplication of completion callbacks (see below)
>>
>>> + } else {
>>> + /*
>>> + * Previous capture results waiting to be sent to framework
>>> + * hence, queue the current capture results as well. After that,
>>> + * check if any results are ready to be sent back to the
>>> + * framework.
>>> + */
>>> + descriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;
>>> + descriptor.resultMetadata_ = std::move(resultMetadata);
>>> + descriptor.captureResult_ = captureResult;
>>> + std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
>>> + std::make_unique<Camera3RequestDescriptor>();
>>> + *reqDescriptor = std::move(descriptor);
>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));
>>> +
>>> + sendQueuedCaptureResults();
>>> + }
>>> +}
>>> +
>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
>>> + CameraStream::ProcessStatus status,
>>> + CameraMetadata *resultMetadata)
>>> +{
>>> + for (auto &d : queuedDescriptor_) {
>>> + if (d->resultMetadata_.get() != resultMetadata)
>>> + continue;
>>> +
>>> /*
>>> * Return the FrameBuffer to the CameraStream now that we're
>>> * done processing it.
>>> */
>>> if (cameraStream->type() == CameraStream::Type::Internal)
>>> - cameraStream->putBuffer(src);
>>> -
>>> - if (ret) {
>>> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>> - notifyError(descriptor.frameNumber_, buffer.stream,
>>> - CAMERA3_MSG_ERROR_BUFFER);
>>> + cameraStream->putBuffer(d->internalBuffer_);
>>> +
>>> + if (status == CameraStream::ProcessStatus::Success) {
>>> + d->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;
>>> + } else {
>>> + d->status_ = Camera3RequestDescriptor::FINISHED_FAILED;
>>> + d->captureResult_.partial_result = 0;
>>> + for (camera3_stream_buffer_t &buffer : d->buffers_) {
>>> + CameraStream *cs = static_cast<CameraStream *>(buffer.stream->priv);
>>> +
>>> + if (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
>>> + continue;
>>> +
>>> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>> + notifyError(d->frameNumber_, buffer.stream,
>>> + CAMERA3_MSG_ERROR_BUFFER);
>>> + }
>>> }
>>> +
>>> + break;
>>> }
>>>
>>> - captureResult.result = resultMetadata->get();
>>> - callbacks_->process_capture_result(callbacks_, &captureResult);
>>> + /*
>>> + * Send back capture results to the framework by inspecting the queue.
>>> + * The framework can defer queueing further requests to the HAL (via
>>> + * process_capture_request) unless until it receives the capture results
>>> + * for already queued requests.
>>> + */
>>> + sendQueuedCaptureResults();
>>> +}
>>> +
>>> +void CameraDevice::sendQueuedCaptureResults()
>>> +{
>>> + while (!queuedDescriptor_.empty()) {
>>> + std::unique_ptr<Camera3RequestDescriptor> &d = queuedDescriptor_.front();
>> I'd have this exit early if the front descriptor is not finished.
>>
>>
>>> + if (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {
>>> + d->captureResult_.result = d->resultMetadata_->get();
>>> + callbacks_->process_capture_result(callbacks_,
>>> + &(d->captureResult_));
>> Not as part of this patch, but I feel like Camera3RequestDescriptor
>> could have a method called complete() to wrap the management of the
>> completion against a request descriptor.
>>
>> There's lots of occasions where the metadata is obtained or processed
>> into a result structure and called back with a flag.
>>
>> (such as *1 above, but I think there are more)
>>
>> It seems to me like that could be made into
>>
>> d->complete(CameraRequest::Success);
>>
>> or something.... But that's an overall design of breaking out the
>> Camera3RequestDescriptor to a full class.
>>
>> As this series is extending Camera3RequestDescriptor to maintain extra
>> state required for the asynchronous post-processing, that makes me think
>> it would be better to have a full class to manage that state. (and thus
>> methods to operate on it)·
>>
>>
>>
>>> + queuedDescriptor_.pop_front();
>>> + } else {
>>> + break;
>>> + }
>>> + }
>>> }
>>>
>>> std::string CameraDevice::logPrefix() const
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index a5576927..36425773 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -7,6 +7,7 @@
>>> #ifndef __ANDROID_CAMERA_DEVICE_H__
>>> #define __ANDROID_CAMERA_DEVICE_H__
>>>
>>> +#include <deque>
>>> #include <map>
>>> #include <memory>
>>> #include <mutex>
>>> @@ -83,6 +84,21 @@ private:
>>> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>>> CameraMetadata settings_;
>>> std::unique_ptr<CaptureRequest> request_;
>>> +
>>> + /*
>>> + * Below are utility placeholders used when a capture result
>>> + * needs to be queued before completion via
>>> + * process_capture_result() callback.
>>> + */
>>> + enum completionStatus {
>>> + NOT_FINISHED,
>> I'd call this "Active" or "Pending" to reduce the negation...
>>
>> "If (state == Active)"
>> rather than
>> "if (state != NOT_FINISHED)"
>>
>>
>>> + FINISHED_SUCCESS,
>> I'd call this Success or Succeeded, or Successful ..
>>
>>> + FINISHED_FAILED,
>> And Failed
>>
>>
>>
>>> + };
>>> + std::unique_ptr<CameraMetadata> resultMetadata_;
>>> + camera3_capture_result_t captureResult_;
>>> + libcamera::FrameBuffer *internalBuffer_;
>>> + completionStatus status_;
>> These are all used to track the post-processor context.
>>
>>
>>
>>
>>> };
>>>
>>> enum class State {
>>> @@ -105,6 +121,11 @@ private:
>>> std::unique_ptr<CameraMetadata> getResultMetadata(
>>> const Camera3RequestDescriptor &descriptor) const;
>>>
>>> + void sendQueuedCaptureResults();
>>> + void streamProcessingComplete(CameraStream *stream,
>>> + CameraStream::ProcessStatus status,
>>> + CameraMetadata *resultMetadata);
>>> +
>>> unsigned int id_;
>>> camera3_device_t camera3Device_;
>>>
>>> @@ -125,6 +146,8 @@ private:
>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
>>> std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>>>
>>> + std::deque<std::unique_ptr<Camera3RequestDescriptor>> queuedDescriptor_;
>> I'm not yet sure if this should be a unique_ptr ..
>>
>>
>>> +
>>> std::string maker_;
>>> std::string model_;
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210908/0038359b/attachment-0001.htm>
More information about the libcamera-devel
mailing list