[libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a queue for sending capture results
Umang Jain
umang.jain at ideasonboard.com
Thu Sep 9 07:05:05 CEST 2021
Hi Laurent,
Thanks for the inputs.
On 9/9/21 3:22 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Wed, Sep 08, 2021 at 07:53:04PM +0530, Umang Jain wrote:
>> 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().
> Correct. You've removed the disconnection based on my comments for the
> previous version, but the connection also needs to be addressed :-)
> Connecting in
>
>> But how can we pass a slot (which is probably
>> private to class) to another class's configure()? A function object ?
> You can create a slot in CameraStream, and from there call
>
> cameraDevice_->streamProcessingComplete(this, request, status);
>
> request should be a pointer to Camera3RequestDescriptor (which we could
> rename to Camera3Request), it's the object that models the context of
I would leave re-naming stuff for now. Camera3RequestDescriptor seems to
be getting bigger and we (Kieran and I) have discussed re-working it to
reduce code duplication and efficient use of the structure. I am, in the
favour of, not doing that as part of this series, but another series
which is solely dedicated to the refactor of this.
> the processing. You need to pass it to cameraStream->process() in the
> first place of course.
This would require exposing Camera3RequestDescriptor struct To
CameraStream class and PostProcessor. Probably it worth to break it out
from CameraDevice private: and set the definition sit outside the scope
of CameraDevice
I quickly compile-tested this to check: https://paste.debian.net/1211030/
>
> It's tempting to merge Camera3RequestDescriptor with CaptureRequest, but
> let's not do that, as fence handling needs to move to the libcamera
> core, so the CameraWorker will disappear then, and so will
> CaptureRequest.
>
>>>>> + });
>>>>> +
>>>>> 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;
> This is a hack that you'll be able to drop once you get the pointer to
> the Camera3RequestDescriptor passed as a function argument.
Yep, I was thinking to pass the frameNumber alternatively ... and
compare it here to find the descriptor..
>
>>>>> +
>>>>> /*
>>>>> * 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