[{"id":20163,"web_url":"https://patchwork.libcamera.org/comment/20163/","msgid":"","date":"2021-10-13T03:40:57","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] android: Send alternative\n\tstream configuration if no buffer to be sent exists","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Sep 27, 2021 at 07:48:21PM +0900, Hirokazu Honda wrote:\n> A request given in processCaptureRequest() can contain only streams\n> that are resolved CameraStream::Type::Mapped. In the case, no buffer\n> is sent to a native camera. This fixes the issue by looking for the\n> stream to be requested and alternatively sending a buffer for the\n> stream to a camera.\n> \n> However, the substitute stream is Direct and a buffer needs to be\n> provided by a HAL client. So we have to allocate a buffer in\n> Android HAL adaptation layer using PlatformFrameBufferAllocator.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------\n> src/android/camera_device.h | 4 +++\n> src/android/camera_stream.cpp | 37 ++++++++++++++++++++++----\n> src/android/camera_stream.h | 10 +++++--\n> 4 files changed, 86 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a693dcbe..5887e85e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig->addConfiguration(streamConfig.config);\n> \n> +\t\tCameraStream *mappedStream = nullptr;\n\nmappedStream isn't a very good name I think, as it is non-null when\nconstructing a Mapped CameraStream and points to a Direct stream. How\nabout sourceStream instead ?\n\n> \t\tfor (auto &stream : streamConfig.streams) {\n> \t\t\tstreams_.emplace_back(this, config.get(), stream.type,\n> -\t\t\t\t\t stream.stream, config->size() - 1);\n> +\t\t\t\t\t stream.stream, mappedStream,\n> +\t\t\t\t\t config->size() - 1);\n> \t\t\tstream.stream->priv = static_cast(&streams_.back());\n> +\t\t\tif (stream.type == CameraStream::Type::Direct)\n> +\t\t\t\tmappedStream = &streams_.back();\n\nIs there a guarantee that a mapped stream uses the direct stream that\nhas been created just before it ? This seems fragile.\n\n> \t\t}\n> \t}\n> \n> @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> \tLOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> \t\t\t<< \" with \" << descriptor.buffers_.size() << \" streams\";\n> +\n> +\tstd::set addedStreams;\n> \tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> \t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * once it has been processed.\n> \t\t\t */\n> \t\t\tbuffer = cameraStream->getBuffer();\n> +\t\t\tdescriptor.internalBuffers_[cameraStream] = buffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> \t\t\tbreak;\n> \t\t}\n> @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> \t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> \t\t\t\t\t\tcamera3Buffer.acquire_fence);\n> +\t\taddedStreams.insert(cameraStream->stream());\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> +\t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> +\t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n> +\t\t\tcontinue;\n> +\t\tif (addedStreams.find(cameraStream->stream()) != addedStreams.end())\n> +\t\t\tcontinue;\n\nAll this deserves comments, it's far from trivial. The code is hard to\nfollow in the context of this patch already, knowing what you're trying\nto achieve, it will be much harder in a few months from now when reading\nthe implementation. The whole idea behind this needs to be captured in\ncomments.\n\n> +\n> +\t\tCameraStream *mappedStream = cameraStream->mappedStream();\n> +\t\tif (!mappedStream) {\n> +\t\t\tLOG(HAL, Error)\n> +\t\t\t\t<< \"Could not find mappedStream for (\"\n> +\t\t\t\t<< camera3Stream->width << \"x\"\n> +\t\t\t\t<< camera3Stream->height << \")\"\n> +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"]\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tASSERT(mappedStream->type() == CameraStream::Type::Direct);\n> +\t\tFrameBuffer *mappedBuffer = mappedStream->getBuffer();\n> +\t\tif (!mappedBuffer) {\n> +\t\t\tLOG(HAL, Error) << \"Failed getting a buffer\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tdescriptor.internalBuffers_[mappedStream] = mappedBuffer;\n> +\t\tdescriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1);\n> \t}\n> \n> \t/*\n> @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> -\t\t/*\n> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n> -\t\t * done processing it.\n> -\t\t */\n> -\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> -\t\t\tcameraStream->putBuffer(src);\n> -\n> \t\tif (ret) {\n> \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t}\n> \t}\n> \n> +\tfor (auto &[stream, buffer] : descriptor.internalBuffers_)\n> +\t\tstream->putBuffer(buffer);\n> +\n> \tcaptureResult.result = resultMetadata->get();\n> \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> }\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 296c2f18..74d8150b 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -21,6 +21,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -84,6 +85,7 @@ private:\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> \t\tstd::unique_ptr request_;\n> +\t\tstd::map internalBuffers_;\n> \t};\n\nCamera3RequestDescriptor will need to be documented in the near future,\nor it will become unmaintainable :-(\n\n> \n> \tenum class State {\n> @@ -118,6 +120,8 @@ private:\n> \tstd::unique_ptr config_;\n> \tCameraCapabilities capabilities_;\n> \n> +\tstd::unique_ptr frame_buffer_allocator_;\n\nThis isn't used.\n\n> +\n> \tstd::map> requestTemplates_;\n> \tconst camera3_callback_ops_t *callbacks_;\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index e30c7ee4..fc2e82cd 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -18,6 +18,7 @@\n> #include \"camera_capabilities.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> +#include \"frame_buffer.h\"\n> \n> using namespace libcamera;\n> \n> @@ -46,10 +47,14 @@ LOG_DECLARE_CATEGORY(HAL)\n> \n> CameraStream::CameraStream(CameraDevice *const cameraDevice,\n> \t\t\t CameraConfiguration *config, Type type,\n> -\t\t\t camera3_stream_t *camera3Stream, unsigned int index)\n> +\t\t\t camera3_stream_t *camera3Stream,\n> +\t\t\t CameraStream *const mappedStream,\n> +\t\t\t unsigned int index)\n> \t: cameraDevice_(cameraDevice), config_(config), type_(type),\n> -\t camera3Stream_(camera3Stream), index_(index)\n> +\t camera3Stream_(camera3Stream), mappedStream_(mappedStream),\n> +\t index_(index)\n> {\n> +\tASSERT(type_ != Type::Mapped || !!mappedStream);\n> }\n> \n> const StreamConfiguration &CameraStream::configuration() const\n> @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source,\n> \n> FrameBuffer *CameraStream::getBuffer()\n> {\n> -\tif (!allocator_)\n> -\t\treturn nullptr;\n> +\tif (!mutex_) {\n\nUsing the mutex for this seems to be a hack, it makes it hard to follow\nthe code flow and ensure correctness. There's quite a bit of code here\nthat looks like work in progress, so I assume this will be refactored in\nthe next version.\n\n> +\t\tASSERT(type_ == Type::Direct);\n> +\t\tASSERT(!allocator_);\n> +\t\tmutex_ = std::make_unique();\n> +\t\tplatform_allocator_ = std::make_unique(cameraDevice_);\n> +\t\tconst StreamConfiguration &config = configuration();\n> +\t\tsize_t numBuffers = config.bufferCount;\n> +\t\tconst int halPixelFormat = camera3Stream_->format;\n> +\t\tconst uint32_t usage = camera3Stream_->usage;\n> +\t\tLOG(HAL, Error) << \"getBuffer@@@@@ \" << numBuffers;\n> +\t\t// numBuffers is 4 (=config.bufferCount) is not enough sadly...\n> +\t\t// Should we dynamically allocate?\n\nI think we should, 20 is a lot (beside being random).\n\nWhat is the impact of allocating buffers at runtime ? It can be a costly\noperation, could it cause significant delays to the processing of\ncapture requests ?\n\nIt would be nice if the code could be designed in such a way that the\ndecision to preallocate at configure time or allocate dynamically at\nruntime could be made in a single place, likely in the CameraStream\nclass, and not visible outside. That way we could also experiment with\npre-allocation if needed, changing CameraStream only without touching\nCameraDevice. Maybe it already is architectured that way, I'm not\nentirely sure :-)\n\n> +\t\tnumBuffers = 20;\n> +\t\tconst auto &buffers = platform_allocator_->allocate(\n> +\t\t\thalPixelFormat, config.size, usage, numBuffers);\n> +\t\tif (buffers.empty() || buffers.size() != numBuffers) {\n> +\t\t\tLOG(HAL, Error) << \"Failed allocating FrameBuffers\";\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\tbuffers_.reserve(numBuffers);\n> +\t\tfor (const auto &frameBuffer : buffers)\n> +\t\t\tbuffers_.push_back(frameBuffer.get());\n> +\t}\n> \n> \tstd::lock_guard locker(*mutex_);\n> \n> @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer()\n> \t\tLOG(HAL, Error) << \"Buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> -\n> +\tLOG(HAL, Error) << buffers_.size();\n> \tFrameBuffer *buffer = buffers_.back();\n> \tbuffers_.pop_back();\n> \n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 2dab6c3a..c8eee908 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -15,10 +15,11 @@\n> \n> #include \n> #include \n> -#include \n> #include \n> #include \n> \n> +#include \"frame_buffer.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> class PostProcessor;\n> @@ -110,12 +111,14 @@ public:\n> \t};\n> \tCameraStream(CameraDevice *const cameraDevice,\n> \t\t libcamera::CameraConfiguration *config, Type type,\n> -\t\t camera3_stream_t *camera3Stream, unsigned int index);\n> +\t\t camera3_stream_t *camera3Stream,\n> +\t\t CameraStream *const mappedStream, unsigned int index);\n> \n> \tType type() const { return type_; }\n> \tconst camera3_stream_t &camera3Stream() const { return *camera3Stream_; }\n> \tconst libcamera::StreamConfiguration &configuration() const;\n> \tlibcamera::Stream *stream() const;\n> +\tCameraStream *mappedStream() const { return mappedStream_; }\n> \n> \tint configure();\n> \tint process(const libcamera::FrameBuffer &source,\n> @@ -130,10 +133,13 @@ private:\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;\n> \tcamera3_stream_t *camera3Stream_;\n> +\tCameraStream *const mappedStream_;\n> \tconst unsigned int index_;\n> \n> \tstd::unique_ptr allocator_;\n\nNow that we have a platform-specific allocator, shouldn't we drop this\none (not in this patch of course) ? It will require changes to the\nPlatformFrameBufferAllocator API to allocate buffers individually, but I\nthink that would result in a cleaner API, with the caller being\nresponsible for handling the allocated buffer life time (through a\nunique pointer). That part of the API change is something that could\nmake sense in this series.\n\n> +\tstd::unique_ptr platform_allocator_;\n\nplatformAllocator_\n\nConstructing the allocator is cheap, you don't have to use a pointer, up\nto you.\n\nOverall, I think the direction is right, but this needs some more work.\nNot a surprise, given that the patch is an RFC :-)\n\n> \tstd::vector buffers_;\n> +\tstd::vector> frameBuffers_;\n> \t/*\n> \t * The class has to be MoveConstructible as instances are stored in\n> \t * an std::vector in CameraDevice.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D32EBBDC71\n\tfor ;\n\tWed, 13 Oct 2021 03:41:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FAC268F4F;\n\tWed, 13 Oct 2021 05:41:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1AA27604FF\n\tfor ;\n\tWed, 13 Oct 2021 05:41:12 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88DA5291;\n\tWed, 13 Oct 2021 05:41:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UiLRx77r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634096471;\n\tbh=X1IY8NkYTASRZrC/td+ovlHxLuM6UFag66UfamhtFbQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UiLRx77rpG3eZzTkwJXEubzLchzNVEblCdeTeBaOWStm+2by4Y5lilh/Nx7fy9GwK\n\tbTM5c3u4NvyPWs1aQ6E6nYxIVagopBjs7ERxoWYe6qqxF4oxsm2EoFliexJ3g8Ktcr\n\tRTYc+YhkqLwKbCL7/HbQvEGmjukHENS7B2PPac04=","Date":"Wed, 13 Oct 2021 06:40:57 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927104821.2526508-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] android: Send alternative\n\tstream configuration if no buffer to be sent exists","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20181,"web_url":"https://patchwork.libcamera.org/comment/20181/","msgid":"<20211013114141.4avi6j73obvtxnaq@uno.localdomain>","date":"2021-10-13T11:41:41","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] android: Send alternative\n\tstream configuration if no buffer to be sent exists","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\n I'll try not to repeat what Laurent already asked\n\nOn Mon, Sep 27, 2021 at 07:48:21PM +0900, Hirokazu Honda wrote:\n> A request given in processCaptureRequest() can contain only streams\n> that are resolved CameraStream::Type::Mapped. In the case, no buffer\n> is sent to a native camera. This fixes the issue by looking for the\n> stream to be requested and alternatively sending a buffer for the\n> stream to a camera.\n>\n> However, the substitute stream is Direct and a buffer needs to be\n> provided by a HAL client. So we have to allocate a buffer in\n> Android HAL adaptation layer using PlatformFrameBufferAllocator.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------\n> src/android/camera_device.h | 4 +++\n> src/android/camera_stream.cpp | 37 ++++++++++++++++++++++----\n> src/android/camera_stream.h | 10 +++++--\n> 4 files changed, 86 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a693dcbe..5887e85e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig->addConfiguration(streamConfig.config);\n>\n> +\t\tCameraStream *mappedStream = nullptr;\n> \t\tfor (auto &stream : streamConfig.streams) {\n> \t\t\tstreams_.emplace_back(this, config.get(), stream.type,\n> -\t\t\t\t\t stream.stream, config->size() - 1);\n> +\t\t\t\t\t stream.stream, mappedStream,\n> +\t\t\t\t\t config->size() - 1);\n> \t\t\tstream.stream->priv = static_cast(&streams_.back());\n> +\t\t\tif (stream.type == CameraStream::Type::Direct)\n> +\t\t\t\tmappedStream = &streams_.back();\n> \t\t}\n> \t}\n>\n> @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> \tLOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> \t\t\t<< \" with \" << descriptor.buffers_.size() << \" streams\";\n> +\n> +\tstd::set addedStreams;\n> \tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> \t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * once it has been processed.\n> \t\t\t */\n> \t\t\tbuffer = cameraStream->getBuffer();\n> +\t\t\tdescriptor.internalBuffers_[cameraStream] = buffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> \t\t\tbreak;\n> \t\t}\n> @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> \t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> \t\t\t\t\t\tcamera3Buffer.acquire_fence);\n> +\t\taddedStreams.insert(cameraStream->stream());\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> +\t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> +\t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n> +\t\t\tcontinue;\n> +\t\tif (addedStreams.find(cameraStream->stream()) != addedStreams.end())\n> +\t\t\tcontinue;\n\nCan't you just iterate on addedStreams then ?\n\n> +\n> +\t\tCameraStream *mappedStream = cameraStream->mappedStream();\n> +\t\tif (!mappedStream) {\n> +\t\t\tLOG(HAL, Error)\n> +\t\t\t\t<< \"Could not find mappedStream for (\"\n> +\t\t\t\t<< camera3Stream->width << \"x\"\n> +\t\t\t\t<< camera3Stream->height << \")\"\n> +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"]\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tASSERT(mappedStream->type() == CameraStream::Type::Direct);\n> +\t\tFrameBuffer *mappedBuffer = mappedStream->getBuffer();\n> +\t\tif (!mappedBuffer) {\n> +\t\t\tLOG(HAL, Error) << \"Failed getting a buffer\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tdescriptor.internalBuffers_[mappedStream] = mappedBuffer;\n> +\t\tdescriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1);\n\nI don't get this. What if the 'mapped' stream was already part of the\nrequest ? Does it get added twice ?\n\n> \t}\n>\n> \t/*\n> @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> -\t\t/*\n> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n> -\t\t * done processing it.\n> -\t\t */\n> -\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> -\t\t\tcameraStream->putBuffer(src);\n> -\n> \t\tif (ret) {\n> \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t}\n> \t}\n>\n> +\tfor (auto &[stream, buffer] : descriptor.internalBuffers_)\n> +\t\tstream->putBuffer(buffer);\n> +\n> \tcaptureResult.result = resultMetadata->get();\n> \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> }\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 296c2f18..74d8150b 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -21,6 +21,7 @@\n>\n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> @@ -84,6 +85,7 @@ private:\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> \t\tstd::unique_ptr request_;\n> +\t\tstd::map internalBuffers_;\n> \t};\n>\n> \tenum class State {\n> @@ -118,6 +120,8 @@ private:\n> \tstd::unique_ptr config_;\n> \tCameraCapabilities capabilities_;\n>\n> +\tstd::unique_ptr frame_buffer_allocator_;\n> +\n> \tstd::map> requestTemplates_;\n> \tconst camera3_callback_ops_t *callbacks_;\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index e30c7ee4..fc2e82cd 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -18,6 +18,7 @@\n> #include \"camera_capabilities.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> +#include \"frame_buffer.h\"\n>\n> using namespace libcamera;\n>\n> @@ -46,10 +47,14 @@ LOG_DECLARE_CATEGORY(HAL)\n>\n> CameraStream::CameraStream(CameraDevice *const cameraDevice,\n> \t\t\t CameraConfiguration *config, Type type,\n> -\t\t\t camera3_stream_t *camera3Stream, unsigned int index)\n> +\t\t\t camera3_stream_t *camera3Stream,\n> +\t\t\t CameraStream *const mappedStream,\n> +\t\t\t unsigned int index)\n> \t: cameraDevice_(cameraDevice), config_(config), type_(type),\n> -\t camera3Stream_(camera3Stream), index_(index)\n> +\t camera3Stream_(camera3Stream), mappedStream_(mappedStream),\n> +\t index_(index)\n> {\n> +\tASSERT(type_ != Type::Mapped || !!mappedStream);\n> }\n>\n> const StreamConfiguration &CameraStream::configuration() const\n> @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source,\n>\n> FrameBuffer *CameraStream::getBuffer()\n> {\n> -\tif (!allocator_)\n> -\t\treturn nullptr;\n\nI assume the style is due the series being an RFC...\n\nAnyway ...\n\n> +\tif (!mutex_) {\n\nRelying on !mutex to verify that !Internal and \"first time this gets\ncalled\" is fragile. Why not handle Direct in configure() and prepare a\nbuffer cache there.\n\nAnyway, I've not really gone into detail in the platform buffer\nallocator but why can't this use the Camera's frame buffer allocator ?\n\n> +\t\tASSERT(type_ == Type::Direct);\n> +\t\tASSERT(!allocator_);\n> +\t\tmutex_ = std::make_unique();\n> +\t\tplatform_allocator_ = std::make_unique(cameraDevice_);\n> +\t\tconst StreamConfiguration &config = configuration();\n> +\t\tsize_t numBuffers = config.bufferCount;\n> +\t\tconst int halPixelFormat = camera3Stream_->format;\n> +\t\tconst uint32_t usage = camera3Stream_->usage;\n> +\t\tLOG(HAL, Error) << \"getBuffer@@@@@ \" << numBuffers;\n> +\t\t// numBuffers is 4 (=config.bufferCount) is not enough sadly...\n> +\t\t// Should we dynamically allocate?\n> +\t\tnumBuffers = 20;\n> +\t\tconst auto &buffers = platform_allocator_->allocate(\n> +\t\t\thalPixelFormat, config.size, usage, numBuffers);\n> +\t\tif (buffers.empty() || buffers.size() != numBuffers) {\n> +\t\t\tLOG(HAL, Error) << \"Failed allocating FrameBuffers\";\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\tbuffers_.reserve(numBuffers);\n> +\t\tfor (const auto &frameBuffer : buffers)\n> +\t\t\tbuffers_.push_back(frameBuffer.get());\n> +\t}\n>\n> \tstd::lock_guard locker(*mutex_);\n>\n> @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer()\n> \t\tLOG(HAL, Error) << \"Buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> -\n> +\tLOG(HAL, Error) << buffers_.size();\n> \tFrameBuffer *buffer = buffers_.back();\n> \tbuffers_.pop_back();\n>\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 2dab6c3a..c8eee908 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -15,10 +15,11 @@\n>\n> #include \n> #include \n> -#include \n> #include \n> #include \n>\n> +#include \"frame_buffer.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> class PostProcessor;\n> @@ -110,12 +111,14 @@ public:\n> \t};\n> \tCameraStream(CameraDevice *const cameraDevice,\n> \t\t libcamera::CameraConfiguration *config, Type type,\n> -\t\t camera3_stream_t *camera3Stream, unsigned int index);\n> +\t\t camera3_stream_t *camera3Stream,\n> +\t\t CameraStream *const mappedStream, unsigned int index);\n>\n> \tType type() const { return type_; }\n> \tconst camera3_stream_t &camera3Stream() const { return *camera3Stream_; }\n> \tconst libcamera::StreamConfiguration &configuration() const;\n> \tlibcamera::Stream *stream() const;\n> +\tCameraStream *mappedStream() const { return mappedStream_; }\n>\n> \tint configure();\n> \tint process(const libcamera::FrameBuffer &source,\n> @@ -130,10 +133,13 @@ private:\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;\n> \tcamera3_stream_t *camera3Stream_;\n> +\tCameraStream *const mappedStream_;\n> \tconst unsigned int index_;\n>\n> \tstd::unique_ptr allocator_;\n> +\tstd::unique_ptr platform_allocator_;\n> \tstd::vector buffers_;\n> +\tstd::vector> frameBuffers_;\n> \t/*\n> \t * The class has to be MoveConstructible as instances are stored in\n> \t * an std::vector in CameraDevice.\n> --\n> 2.33.0.685.g46640cef36-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6C7B2BDC71\n\tfor ;\n\tWed, 13 Oct 2021 11:40:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B711368F51;\n\tWed, 13 Oct 2021 13:40:56 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46FFE60501\n\tfor ;\n\tWed, 13 Oct 2021 13:40:55 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id DA765240009;\n\tWed, 13 Oct 2021 11:40:53 +0000 (UTC)"],"Date":"Wed, 13 Oct 2021 13:41:41 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211013114141.4avi6j73obvtxnaq@uno.localdomain>","References":"<20210927104821.2526508-1-hiroh@chromium.org>\n\t<20210927104821.2526508-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927104821.2526508-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] android: Send alternative\n\tstream configuration if no buffer to be sent exists","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]