[libcamera-devel] [PATCH 1/1] android: camera_device: Configure one stream for identical stream requests
Hirokazu Honda
hiroh at chromium.org
Fri Jan 7 06:46:00 CET 2022
Hi Jacopo, thank you for reviewing.
On Thu, Jan 6, 2022 at 9:35 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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. What a nice suggestion!
putBuffer() is invoked against buffer.stream.
buffer.stream is Mapped stream, though FrameBuffer to be returned
(i.e. putBuffer) is Direct Stream.
So I have to introduce putBuffers to pair DirectStream and FrameBuffer.
This problem is not resolved in your implementation if I understand
your code correctly.
-Hiro
>
> 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