<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Laurent,<br>
</p>
<div class="moz-cite-prefix">On 9/8/21 7:03 PM, Laurent Pinchart
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:YTi7pnGPP%2FaZEeaa@pendragon.ideasonboard.com">
<pre class="moz-quote-pre" wrap="">On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On 07/09/2021 20:57, Umang Jain wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
s/subtaintial/substantial/
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
s/dequeud/dequeued/
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:umang.jain@ideasonboard.com"><umang.jain@ideasonboard.com></a>
---
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 @@ <a class="moz-txt-link-freetext" href="CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(">CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(</a>
/* 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 <a class="moz-txt-link-freetext" href="CameraDevice::processCaptureRequest(camera3_capture_request_t">CameraDevice::processCaptureRequest(camera3_capture_request_t</a> *camera3Reques
* once it has been processed.
*/
buffer = cameraStream->getBuffer();
+ descriptor.internalBuffer_ = buffer;
LOG(HAL, Debug) << ss.str() << " (internal)";
break;
}
@@ -1148,25 +1151,115 @@ void <a class="moz-txt-link-freetext" href="CameraDevice::requestComplete(Request">CameraDevice::requestComplete(Request</a> *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_ = <a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::NOT_FINISHED">Camera3RequestDescriptor::NOT_FINISHED</a>;
+ descriptor.resultMetadata_ = <a class="moz-txt-link-freetext" href="std::move(resultMetadata)">std::move(resultMetadata)</a>;
+ descriptor.captureResult_ = captureResult;
+
+ <a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><Camera3RequestDescriptor> reqDescriptor =
+ <a class="moz-txt-link-freetext" href="std::make_unique">std::make_unique</a><Camera3RequestDescriptor>();
+ *reqDescriptor = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
+ queuedDescriptor_.push_back(<a class="moz-txt-link-freetext" href="std::move(reqDescriptor))">std::move(reqDescriptor))</a>;
+ Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
+
+ CameraMetadata *metadata = currentDescriptor->resultMetadata_.get();
+ cameraStream->processComplete.connect(
+ this, [=](<a class="moz-txt-link-freetext" href="CameraStream::ProcessStatus">CameraStream::ProcessStatus</a> status) {
+ streamProcessingComplete(cameraStream, status,
+ metadata);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
</blockquote>
While this is true.. and a bug in my code<br>
<blockquote type="cite"
cite="mid:YTi7pnGPP%2FaZEeaa@pendragon.ideasonboard.com">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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, ...</pre>
</blockquote>
<p>Oh, I gotta see this in action by sprinkling some printfs...<br>
</p>
<p>But What is a alternative to this? While I tried to disconnect
signal handlers in the slot the last time, it came back with:</p>
<p>'''<br>
</p>
<pre>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 ?
'''
</pre>
<p>in
<a class="moz-txt-link-freetext" href="https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html">https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html</a><br>
</p>
<p>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. <br>
</p>
<p>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 <a class="moz-txt-link-freetext" href="CameraStream::configure()">CameraStream::configure()</a>. But how
can we pass a slot (which is probably private to class) to another
class's configure()? A function object ? <br>
</p>
<blockquote type="cite"
cite="mid:YTi7pnGPP%2FaZEeaa@pendragon.ideasonboard.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ });
+
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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
*1 duplication of completion callbacks (see below)
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ } 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_ = <a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::FINISHED_SUCCESS">Camera3RequestDescriptor::FINISHED_SUCCESS</a>;
+ descriptor.resultMetadata_ = <a class="moz-txt-link-freetext" href="std::move(resultMetadata)">std::move(resultMetadata)</a>;
+ descriptor.captureResult_ = captureResult;
+ <a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><Camera3RequestDescriptor> reqDescriptor =
+ <a class="moz-txt-link-freetext" href="std::make_unique">std::make_unique</a><Camera3RequestDescriptor>();
+ *reqDescriptor = <a class="moz-txt-link-freetext" href="std::move(descriptor)">std::move(descriptor)</a>;
+ queuedDescriptor_.push_back(<a class="moz-txt-link-freetext" href="std::move(reqDescriptor))">std::move(reqDescriptor))</a>;
+
+ sendQueuedCaptureResults();
+ }
+}
+
+void <a class="moz-txt-link-freetext" href="CameraDevice::streamProcessingComplete(CameraStream">CameraDevice::streamProcessingComplete(CameraStream</a> *cameraStream,
+ <a class="moz-txt-link-freetext" href="CameraStream::ProcessStatus">CameraStream::ProcessStatus</a> 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() == <a class="moz-txt-link-freetext" href="CameraStream::Type::Internal">CameraStream::Type::Internal</a>)
- 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 == <a class="moz-txt-link-freetext" href="CameraStream::ProcessStatus::Success">CameraStream::ProcessStatus::Success</a>) {
+ d->status_ = <a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::FINISHED_SUCCESS">Camera3RequestDescriptor::FINISHED_SUCCESS</a>;
+ } else {
+ d->status_ = <a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::FINISHED_FAILED">Camera3RequestDescriptor::FINISHED_FAILED</a>;
+ 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 <a class="moz-txt-link-freetext" href="CameraDevice::sendQueuedCaptureResults()">CameraDevice::sendQueuedCaptureResults()</a>
+{
+ while (!queuedDescriptor_.empty()) {
+ <a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><Camera3RequestDescriptor> &d = queuedDescriptor_.front();
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'd have this exit early if the front descriptor is not finished.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (d->status_ != <a class="moz-txt-link-freetext" href="Camera3RequestDescriptor::NOT_FINISHED">Camera3RequestDescriptor::NOT_FINISHED</a>) {
+ d->captureResult_.result = d->resultMetadata_->get();
+ callbacks_->process_capture_result(callbacks_,
+ &(d->captureResult_));
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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(<a class="moz-txt-link-freetext" href="CameraRequest::Success">CameraRequest::Success</a>);
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)·
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ queuedDescriptor_.pop_front();
+ } else {
+ break;
+ }
+ }
}
<a class="moz-txt-link-freetext" href="std::string">std::string</a> <a class="moz-txt-link-freetext" href="CameraDevice::logPrefix()">CameraDevice::logPrefix()</a> 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:
<a class="moz-txt-link-freetext" href="std::vector">std::vector</a><<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><a class="moz-txt-link-rfc2396E" href="libcamera::FrameBuffer"><libcamera::FrameBuffer></a>> frameBuffers_;
CameraMetadata settings_;
<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><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,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'd call this "Active" or "Pending" to reduce the negation...
"If (state == Active)"
rather than
"if (state != NOT_FINISHED)"
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ FINISHED_SUCCESS,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'd call this Success or Succeeded, or Successful ..
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ FINISHED_FAILED,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
And Failed
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ };
+ <a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><CameraMetadata> resultMetadata_;
+ camera3_capture_result_t captureResult_;
+ <a class="moz-txt-link-freetext" href="libcamera::FrameBuffer">libcamera::FrameBuffer</a> *internalBuffer_;
+ completionStatus status_;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
These are all used to track the post-processor context.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> };
enum class State {
@@ -105,6 +121,11 @@ private:
<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><CameraMetadata> getResultMetadata(
const Camera3RequestDescriptor &descriptor) const;
+ void sendQueuedCaptureResults();
+ void streamProcessingComplete(CameraStream *stream,
+ <a class="moz-txt-link-freetext" href="CameraStream::ProcessStatus">CameraStream::ProcessStatus</a> status,
+ CameraMetadata *resultMetadata);
+
unsigned int id_;
camera3_device_t camera3Device_;
@@ -125,6 +146,8 @@ private:
<a class="moz-txt-link-freetext" href="libcamera::Mutex">libcamera::Mutex</a> descriptorsMutex_; /* Protects descriptors_. */
<a class="moz-txt-link-freetext" href="std::map">std::map</a><uint64_t, Camera3RequestDescriptor> descriptors_;
+ <a class="moz-txt-link-freetext" href="std::deque">std::deque</a><<a class="moz-txt-link-freetext" href="std::unique_ptr">std::unique_ptr</a><Camera3RequestDescriptor>> queuedDescriptor_;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I'm not yet sure if this should be a unique_ptr ..
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
<a class="moz-txt-link-freetext" href="std::string">std::string</a> maker_;
<a class="moz-txt-link-freetext" href="std::string">std::string</a> model_;
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>