[libcamera-devel] [PATCH 1/1] android: camera_device: Configure one stream for identical stream requests
Jacopo Mondi
jacopo at jmondi.org
Thu Jan 6 13:36:03 CET 2022
Hi Hiro,
On Thu, Jan 06, 2022 at 06:43:23PM +0900, Hirokazu Honda wrote:
> An Android HAL client may request multiple identical streams. It is
> redundant that a native camera device produces a separate stream for
> each of the identical requests. Configure the camera with a single
> stream in that case. The other identical HAL streams will be produced by
> the YUV post-processor.
>
> The Android HAL client can provide capture requests that are resolved as
> they are produced by YUV post-processor. Then a buffer to be filled a
> camera is not given. So the HAL adaptation layer looks up buffer
> information to be passed to a camera and allocate the buffer by using
> PlatformBufferAllocator.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/camera_device.cpp | 84 ++++++++++++++++++++++++++++++++++-
> src/android/camera_request.h | 2 +
> src/android/camera_stream.cpp | 12 ++++-
> src/android/camera_stream.h | 6 ++-
> 4 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 83825736..53533064 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -9,6 +9,7 @@
>
> #include <algorithm>
> #include <fstream>
> +#include <set>
> #include <sys/mman.h>
> #include <unistd.h>
> #include <vector>
> @@ -604,6 +605,35 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> continue;
> }
>
> + /*
> + * While gralloc usage flags are supposed to report usage
> + * patterns to select a suitable buffer allocation strategy, in
> + * practice they're also used to make other decisions, such as
> + * selecting the actual format for the IMPLEMENTATION_DEFINED
> + * HAL pixel format. To avoid issues, we thus have to set the
> + * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for
> + * streams that will be produced in software.
> + */
> + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
You can now remove
/* This stream will be produced by hardware. */
stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
at the end of the for loop
> +
> + /*
> + * If a CameraStream with the same size and format of the
> + * current stream has already been requested, associate the two.
> + */
> + auto iter = std::find_if(
> + streamConfigs.begin(), streamConfigs.end(),
> + [size, format](const Camera3StreamConfig &streamConfig) {
Should [size, format] be captured by reference ?
> + return streamConfig.config.size == size &&
> + streamConfig.config.pixelFormat == format;
> + });
> + if (iter != streamConfigs.end()) {
> + /* Add usage to copy the buffer in streams[0] to stream. */
> + iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> + stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> + iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> + continue;
> + }
> +
> Camera3StreamConfig streamConfig;
> streamConfig.streams = { { stream, CameraStream::Type::Direct } };
> streamConfig.config.size = size;
> @@ -681,10 +711,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> for (const auto &streamConfig : streamConfigs) {
> config->addConfiguration(streamConfig.config);
>
> + CameraStream *directStream = nullptr;
> for (auto &stream : streamConfig.streams) {
> + CameraStream *sourceStream = nullptr;
> +
> + /*
> + * Sets the source stream for Mapped type streams to the
> + * same Direct type stream. The Direct type stream is
> + * put earlier than Mapped type streams in the current
> + * implementation. This might be broken in the future.
> + *
> + * \todo Remove this order assumption.
> + */
> + if (stream.type == CameraStream::Type::Mapped) {
> + ASSERT(directStream);
> + sourceStream = directStream;
> + }
> +
> streams_.emplace_back(this, config.get(), stream.type,
> - stream.stream, config->size() - 1);
> + stream.stream,
> + sourceStream,
> + config->size() - 1);
> stream.stream->priv = static_cast<void *>(&streams_.back());
> + if (!directStream &&
> + stream.type == CameraStream::Type::Direct) {
> + directStream = &streams_.back();
> + }
Am I mistaken or streams of type Mapped will always have a Direct
stream in streams[0] ? You seem to have the same assumption about
ordering. Can this be simplified as:
CameraStream *sourceStream = nullptr;
for (auto &stream : streamConfig.streams) {
streams_.emplace_back(this, config.get(), stream.type,
stream.stream,
sourceStream,
config->size() - 1);
stream.stream->priv = static_cast<void *>(&streams_.back());
if (stream.type == CameraStream::Type::Direct)
sourceStream = &streams_.back();
}
Streams of type Mapped will always follow a Direct.
Internal streams have no Mapped associated.
This should give you sourceStream == nullptr for Internal and Direct,
which I think it's what you want.
> }
> }
>
> @@ -917,6 +969,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> << " with " << descriptor->buffers_.size() << " streams";
>
> + std::set<CameraStream *> requiredStreams;
> + std::set<CameraStream *> providedStreams;
I understand the logic, it took me a while but now I like it. I wonder
if we could do better and exploit the fact std::set<> stores objects
uniquely. Also, the introduction of putBuffers_ seems like a temporary
solution, we should aim to re-use the same mechanism as for Internal
buffers to avoid the additional complexity at
streamProcessingComplete(). We have been adding ad-hoc solutions for
each new issue we faced, and the complexity of keeping it all together
has increased enough already.
I would rather iterate a few more times on the list of requested
streams, which is of limited lenght, in order to
1) Collect all the (unique) CameraStream that have to be queued to the
request
2) Handle Direct and Internal which have a CameraStream associated and
no sourceStream
3) Handle Mapped and add sourceStream to the Request if needed
The hunk I get looks like the following
@@ -917,9 +954,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
<< " with " << descriptor->buffers_.size() << " streams";
+ /*
+ * Collect the CameraStream associated to each requested capture stream.
+ * Being requestedStreams an std::set<> no duplications can happen.
+ */
+ std::set<CameraStream *> requestedStreams;
+ for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
+ CameraStream *cameraStream = buffer.stream;
+
+ switch (cameraStream->type()) {
+ case CameraStream::Type::Mapped:
+ requestedStreams.insert(cameraStream->sourceStream());
+ break;
+
+ case CameraStream::Type::Direct:
+ case CameraStream::Type::Internal:
+ requestedStreams.insert(cameraStream);
+ break;
+ }
+ }
+
+ /*
+ * Process all the Direct and Internal streams, for which the CameraStream
+ * they refer to is the one that points to the right libcamera::Stream.
+ *
+ * Streams of type Mapped will be handled later.
+ */
for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
CameraStream *cameraStream = buffer.stream;
camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
+ FrameBuffer *frameBuffer = nullptr;
+ UniqueFD acquireFence;
std::stringstream ss;
ss << i << " - (" << camera3Stream->width << "x"
@@ -928,25 +993,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
<< "(" << cameraStream->configuration().size.toString() << ")["
<< cameraStream->configuration().pixelFormat.toString() << "]";
- /*
- * Inspect the camera stream type, create buffers opportunely
- * and add them to the Request if required.
- */
- FrameBuffer *frameBuffer = nullptr;
- UniqueFD acquireFence;
-
- MutexLocker lock(descriptor->streamsProcessMutex_);
-
switch (cameraStream->type()) {
case CameraStream::Type::Mapped:
- /*
- * Mapped streams don't need buffers added to the
- * Request.
- */
- LOG(HAL, Debug) << ss.str() << " (mapped)";
-
- descriptor->pendingStreamsToProcess_.insert(
- { cameraStream, &buffer });
continue;
case CameraStream::Type::Direct:
@@ -987,9 +1035,52 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
return -ENOMEM;
}
+ MutexLocker lock(descriptor->streamsProcessMutex_);
auto fence = std::make_unique<Fence>(std::move(acquireFence));
descriptor->request_->addBuffer(cameraStream->stream(),
frameBuffer, std::move(fence));
+
+ requestedStreams.erase(cameraStream);
+ }
+
+ /*
+ * Now handle the mapped streams. If no buffer has been addded for them
+ * as their corresponding direct source stream has not been requested,
+ * add it here.
+ */
+ for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
+ CameraStream *cameraStream = buffer.stream;
+ camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
+ CameraStream *sourceStream = cameraStream->sourceStream();
+ FrameBuffer *frameBuffer = nullptr;
+
+ if (cameraStream->type() != CameraStream::Type::Mapped)
+ continue;
+
+ std::stringstream ss;
+ ss << i << " - (" << camera3Stream->width << "x"
+ << camera3Stream->height << ")"
+ << "[" << utils::hex(camera3Stream->format) << "] -> "
+ << "(" << cameraStream->configuration().size.toString() << ")["
+ << cameraStream->configuration().pixelFormat.toString() << "]";
+ LOG(HAL, Debug) << ss.str() << " (mapped)";
+
+ descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });
+
+ /*
+ * Make sure the CameraStream this stream is mapped on has been
+ * added to the request.
+ */
+ if (requestedStreams.find(sourceStream) == requestedStreams.end())
+ continue;
+
+ /* If that's not the case, we need to do so here. */
+ frameBuffer = sourceStream->getBuffer();
+ buffer.internalBuffer = frameBuffer;
+
+ MutexLocker lock(descriptor->streamsProcessMutex_);
+ descriptor->request_->addBuffer(sourceStream->stream(),
+ frameBuffer, nullptr);
}
I have only compiled this, but if you know a CTS test which can be run
in isolation and exercize this use case I would be happy to test it.
This way you might drop putBuffers_ if I'm not mistaken.
Thanks
j
> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> CameraStream *cameraStream = buffer.stream;
> camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> @@ -947,6 +1001,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>
> descriptor->pendingStreamsToProcess_.insert(
> { cameraStream, &buffer });
> + ASSERT(cameraStream->sourceStream());
> + requiredStreams.insert(cameraStream->sourceStream());
> continue;
>
> case CameraStream::Type::Direct:
> @@ -990,8 +1046,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> auto fence = std::make_unique<Fence>(std::move(acquireFence));
> descriptor->request_->addBuffer(cameraStream->stream(),
> frameBuffer, std::move(fence));
> + providedStreams.insert(cameraStream);
> }
>
> + for (CameraStream *requiredStream : requiredStreams) {
> + if (providedStreams.find(requiredStream) != providedStreams.end())
> + continue;
> +
> + /* \todo Can we Map stream to Internal type stream? */
> + ASSERT(requiredStream->type() == CameraStream::Type::Direct);
> +
> + FrameBuffer *frameBuffer = requiredStream->getBuffer();
> + if (!frameBuffer) {
> + LOG(HAL, Error) << "Failed to create frame buffer";
> + return -ENOMEM;
> + }
> +
> + /*
> + * addBuffer() lets a buffer for requiredStream be output by
> + * camera. Records frameBuffer with requiredStream to call
> + * putBuffer() after post-processing is complete.
> + */
> + descriptor->request_->addBuffer(requiredStream->stream(),
> + frameBuffer, nullptr);
> + MutexLocker lock(descriptor->streamsProcessMutex_);
> + descriptor->putBuffers_.push_back({ requiredStream, frameBuffer });
> + }
> /*
> * Translate controls from Android to libcamera and queue the request
> * to the camera.
> @@ -1251,6 +1331,8 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff
> request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> if (!request->pendingStreamsToProcess_.empty())
> return;
> + for (auto [cameraStream, buffer] : request->putBuffers_)
> + cameraStream->putBuffer(buffer);
> }
>
> completeDescriptor(streamBuffer->request);
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 37b6ae32..7a41c6d9 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -59,6 +59,8 @@ public:
> /* Keeps track of streams requiring post-processing. */
> std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_
> LIBCAMERA_TSA_GUARDED_BY(streamsProcessMutex_);
> + std::vector<std::pair<CameraStream *, libcamera::FrameBuffer *>> putBuffers_
> + LIBCAMERA_TSA_GUARDED_BY(streamsProcessMutex_);
> libcamera::Mutex streamsProcessMutex_;
>
> Camera3RequestDescriptor(libcamera::Camera *camera,
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index c2157450..634cf319 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -52,9 +52,11 @@ LOG_DECLARE_CATEGORY(HAL)
>
> CameraStream::CameraStream(CameraDevice *const cameraDevice,
> CameraConfiguration *config, Type type,
> - camera3_stream_t *camera3Stream, unsigned int index)
> + camera3_stream_t *camera3Stream,
> + CameraStream *const sourceStream, unsigned int index)
> : cameraDevice_(cameraDevice), config_(config), type_(type),
> - camera3Stream_(camera3Stream), index_(index)
> + camera3Stream_(camera3Stream), sourceStream_(sourceStream),
> + index_(index)
> {
> }
>
> @@ -206,6 +208,12 @@ void CameraStream::flush()
>
> FrameBuffer *CameraStream::getBuffer()
> {
> + if (type_ == Type::Direct && !allocator_) {
> + allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_);
> + ASSERT(!mutex_);
> + mutex_ = std::make_unique<Mutex>();
> + }
> +
> if (!allocator_)
> return nullptr;
>
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index f9504462..4c5078b2 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -114,7 +114,9 @@ public:
> };
> CameraStream(CameraDevice *const cameraDevice,
> libcamera::CameraConfiguration *config, Type type,
> - camera3_stream_t *camera3Stream, unsigned int index);
> + camera3_stream_t *camera3Stream,
> + CameraStream *const sourceStream,
> + unsigned int index);
> CameraStream(CameraStream &&other);
> ~CameraStream();
>
> @@ -122,6 +124,7 @@ public:
> camera3_stream_t *camera3Stream() const { return camera3Stream_; }
> const libcamera::StreamConfiguration &configuration() const;
> libcamera::Stream *stream() const;
> + CameraStream *sourceStream() const { return sourceStream_; }
>
> int configure();
> int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer);
> @@ -167,6 +170,7 @@ private:
> const libcamera::CameraConfiguration *config_;
> const Type type_;
> camera3_stream_t *camera3Stream_;
> + CameraStream *const sourceStream_;
> const unsigned int index_;
>
> std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
> --
> 2.34.1.448.ga2b2bfdf31-goog
More information about the libcamera-devel
mailing list