[libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a queue for sending capture results
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 10 06:25:50 CEST 2021
Hi Umang,
On Thu, Sep 09, 2021 at 10:35:05AM +0530, Umang Jain wrote:
> On 9/9/21 3:22 AM, Laurent Pinchart wrote:
> > 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.
OK.
> 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.
That's fine with me, but I think that refactoring series should then
come before this one.
> > 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/
Soudns good to me.
> > 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_;
> >>>>>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list