[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:31:20 CEST 2021
Hi again,
On 9/8/21 7:53 PM, Umang Jain wrote:
> 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 ?
Ah no, that would be bad as well. Probably when streams are constructed
in CameraDevice::configureStreams(). We can connect the
post-processing-complete-signal right there and not on every
post-process's run.
>
>>
>>>> + });
>>>> +
>>>> 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_;
>>>>
>
More information about the libcamera-devel
mailing list