| Message ID | 20210927104821.2526508-3-hiroh@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Sep 27, 2021 at 07:48:21PM +0900, Hirokazu Honda wrote: > A request given in processCaptureRequest() can contain only streams > that are resolved CameraStream::Type::Mapped. In the case, no buffer > is sent to a native camera. This fixes the issue by looking for the > stream to be requested and alternatively sending a buffer for the > stream to a camera. > > However, the substitute stream is Direct and a buffer needs to be > provided by a HAL client. So we have to allocate a buffer in > Android HAL adaptation layer using PlatformFrameBufferAllocator. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------ > src/android/camera_device.h | 4 +++ > src/android/camera_stream.cpp | 37 ++++++++++++++++++++++---- > src/android/camera_stream.h | 10 +++++-- > 4 files changed, 86 insertions(+), 15 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a693dcbe..5887e85e 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > for (const auto &streamConfig : streamConfigs) { > config->addConfiguration(streamConfig.config); > > + CameraStream *mappedStream = nullptr; mappedStream isn't a very good name I think, as it is non-null when constructing a Mapped CameraStream and points to a Direct stream. How about sourceStream instead ? > for (auto &stream : streamConfig.streams) { > streams_.emplace_back(this, config.get(), stream.type, > - stream.stream, config->size() - 1); > + stream.stream, mappedStream, > + config->size() - 1); > stream.stream->priv = static_cast<void *>(&streams_.back()); > + if (stream.type == CameraStream::Type::Direct) > + mappedStream = &streams_.back(); Is there a guarantee that a mapped stream uses the direct stream that has been created just before it ? This seems fragile. > } > } > > @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() > << " with " << descriptor.buffers_.size() << " streams"; > + > + std::set<Stream *> addedStreams; > for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > camera3_stream *camera3Stream = camera3Buffer.stream; > @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * once it has been processed. > */ > buffer = cameraStream->getBuffer(); > + descriptor.internalBuffers_[cameraStream] = buffer; > LOG(HAL, Debug) << ss.str() << " (internal)"; > break; > } > @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > descriptor.request_->addBuffer(cameraStream->stream(), buffer, > camera3Buffer.acquire_fence); > + addedStreams.insert(cameraStream->stream()); > + } > + > + for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > + const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > + camera3_stream *camera3Stream = camera3Buffer.stream; > + CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > + if (cameraStream->type() != CameraStream::Type::Mapped) > + continue; > + if (addedStreams.find(cameraStream->stream()) != addedStreams.end()) > + continue; All this deserves comments, it's far from trivial. The code is hard to follow in the context of this patch already, knowing what you're trying to achieve, it will be much harder in a few months from now when reading the implementation. The whole idea behind this needs to be captured in comments. > + > + CameraStream *mappedStream = cameraStream->mappedStream(); > + if (!mappedStream) { > + LOG(HAL, Error) > + << "Could not find mappedStream for (" > + << camera3Stream->width << "x" > + << camera3Stream->height << ")" > + << "[" << utils::hex(camera3Stream->format) << "]"; > + return -EINVAL; > + } > + > + ASSERT(mappedStream->type() == CameraStream::Type::Direct); > + FrameBuffer *mappedBuffer = mappedStream->getBuffer(); > + if (!mappedBuffer) { > + LOG(HAL, Error) << "Failed getting a buffer"; > + return -EINVAL; > + } > + > + descriptor.internalBuffers_[mappedStream] = mappedBuffer; > + descriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1); > } > > /* > @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request) > int ret = cameraStream->process(*src, *buffer.buffer, > descriptor.settings_, > resultMetadata.get()); > - /* > - * 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, > @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request) > } > } > > + for (auto &[stream, buffer] : descriptor.internalBuffers_) > + stream->putBuffer(buffer); > + > captureResult.result = resultMetadata->get(); > callbacks_->process_capture_result(callbacks_, &captureResult); > } > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 296c2f18..74d8150b 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -21,6 +21,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > +#include <libcamera/framebuffer_allocator.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > #include <libcamera/request.h> > @@ -84,6 +85,7 @@ private: > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > }; Camera3RequestDescriptor will need to be documented in the near future, or it will become unmaintainable :-( > > enum class State { > @@ -118,6 +120,8 @@ private: > std::unique_ptr<libcamera::CameraConfiguration> config_; > CameraCapabilities capabilities_; > > + std::unique_ptr<libcamera::FrameBufferAllocator> frame_buffer_allocator_; This isn't used. > + > std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_; > const camera3_callback_ops_t *callbacks_; > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index e30c7ee4..fc2e82cd 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -18,6 +18,7 @@ > #include "camera_capabilities.h" > #include "camera_device.h" > #include "camera_metadata.h" > +#include "frame_buffer.h" > > using namespace libcamera; > > @@ -46,10 +47,14 @@ 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 mappedStream, > + unsigned int index) > : cameraDevice_(cameraDevice), config_(config), type_(type), > - camera3Stream_(camera3Stream), index_(index) > + camera3Stream_(camera3Stream), mappedStream_(mappedStream), > + index_(index) > { > + ASSERT(type_ != Type::Mapped || !!mappedStream); > } > > const StreamConfiguration &CameraStream::configuration() const > @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source, > > FrameBuffer *CameraStream::getBuffer() > { > - if (!allocator_) > - return nullptr; > + if (!mutex_) { Using the mutex for this seems to be a hack, it makes it hard to follow the code flow and ensure correctness. There's quite a bit of code here that looks like work in progress, so I assume this will be refactored in the next version. > + ASSERT(type_ == Type::Direct); > + ASSERT(!allocator_); > + mutex_ = std::make_unique<std::mutex>(); > + platform_allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_); > + const StreamConfiguration &config = configuration(); > + size_t numBuffers = config.bufferCount; > + const int halPixelFormat = camera3Stream_->format; > + const uint32_t usage = camera3Stream_->usage; > + LOG(HAL, Error) << "getBuffer@@@@@ " << numBuffers; > + // numBuffers is 4 (=config.bufferCount) is not enough sadly... > + // Should we dynamically allocate? I think we should, 20 is a lot (beside being random). What is the impact of allocating buffers at runtime ? It can be a costly operation, could it cause significant delays to the processing of capture requests ? It would be nice if the code could be designed in such a way that the decision to preallocate at configure time or allocate dynamically at runtime could be made in a single place, likely in the CameraStream class, and not visible outside. That way we could also experiment with pre-allocation if needed, changing CameraStream only without touching CameraDevice. Maybe it already is architectured that way, I'm not entirely sure :-) > + numBuffers = 20; > + const auto &buffers = platform_allocator_->allocate( > + halPixelFormat, config.size, usage, numBuffers); > + if (buffers.empty() || buffers.size() != numBuffers) { > + LOG(HAL, Error) << "Failed allocating FrameBuffers"; > + return nullptr; > + } > + > + buffers_.reserve(numBuffers); > + for (const auto &frameBuffer : buffers) > + buffers_.push_back(frameBuffer.get()); > + } > > std::lock_guard<std::mutex> locker(*mutex_); > > @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer() > LOG(HAL, Error) << "Buffer underrun"; > return nullptr; > } > - > + LOG(HAL, Error) << buffers_.size(); > FrameBuffer *buffer = buffers_.back(); > buffers_.pop_back(); > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 2dab6c3a..c8eee908 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -15,10 +15,11 @@ > > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > -#include <libcamera/framebuffer_allocator.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > +#include "frame_buffer.h" > + > class CameraDevice; > class CameraMetadata; > class PostProcessor; > @@ -110,12 +111,14 @@ public: > }; > CameraStream(CameraDevice *const cameraDevice, > libcamera::CameraConfiguration *config, Type type, > - camera3_stream_t *camera3Stream, unsigned int index); > + camera3_stream_t *camera3Stream, > + CameraStream *const mappedStream, unsigned int index); > > Type type() const { return type_; } > const camera3_stream_t &camera3Stream() const { return *camera3Stream_; } > const libcamera::StreamConfiguration &configuration() const; > libcamera::Stream *stream() const; > + CameraStream *mappedStream() const { return mappedStream_; } > > int configure(); > int process(const libcamera::FrameBuffer &source, > @@ -130,10 +133,13 @@ private: > const libcamera::CameraConfiguration *config_; > const Type type_; > camera3_stream_t *camera3Stream_; > + CameraStream *const mappedStream_; > const unsigned int index_; > > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; Now that we have a platform-specific allocator, shouldn't we drop this one (not in this patch of course) ? It will require changes to the PlatformFrameBufferAllocator API to allocate buffers individually, but I think that would result in a cleaner API, with the caller being responsible for handling the allocated buffer life time (through a unique pointer). That part of the API change is something that could make sense in this series. > + std::unique_ptr<PlatformFrameBufferAllocator> platform_allocator_; platformAllocator_ Constructing the allocator is cheap, you don't have to use a pointer, up to you. Overall, I think the direction is right, but this needs some more work. Not a surprise, given that the patch is an RFC :-) > std::vector<libcamera::FrameBuffer *> buffers_; > + std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > /* > * The class has to be MoveConstructible as instances are stored in > * an std::vector in CameraDevice.
Hi Hiro, I'll try not to repeat what Laurent already asked On Mon, Sep 27, 2021 at 07:48:21PM +0900, Hirokazu Honda wrote: > A request given in processCaptureRequest() can contain only streams > that are resolved CameraStream::Type::Mapped. In the case, no buffer > is sent to a native camera. This fixes the issue by looking for the > stream to be requested and alternatively sending a buffer for the > stream to a camera. > > However, the substitute stream is Direct and a buffer needs to be > provided by a HAL client. So we have to allocate a buffer in > Android HAL adaptation layer using PlatformFrameBufferAllocator. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------ > src/android/camera_device.h | 4 +++ > src/android/camera_stream.cpp | 37 ++++++++++++++++++++++---- > src/android/camera_stream.h | 10 +++++-- > 4 files changed, 86 insertions(+), 15 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a693dcbe..5887e85e 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > for (const auto &streamConfig : streamConfigs) { > config->addConfiguration(streamConfig.config); > > + CameraStream *mappedStream = nullptr; > for (auto &stream : streamConfig.streams) { > streams_.emplace_back(this, config.get(), stream.type, > - stream.stream, config->size() - 1); > + stream.stream, mappedStream, > + config->size() - 1); > stream.stream->priv = static_cast<void *>(&streams_.back()); > + if (stream.type == CameraStream::Type::Direct) > + mappedStream = &streams_.back(); > } > } > > @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() > << " with " << descriptor.buffers_.size() << " streams"; > + > + std::set<Stream *> addedStreams; > for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > camera3_stream *camera3Stream = camera3Buffer.stream; > @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * once it has been processed. > */ > buffer = cameraStream->getBuffer(); > + descriptor.internalBuffers_[cameraStream] = buffer; > LOG(HAL, Debug) << ss.str() << " (internal)"; > break; > } > @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > descriptor.request_->addBuffer(cameraStream->stream(), buffer, > camera3Buffer.acquire_fence); > + addedStreams.insert(cameraStream->stream()); > + } > + > + for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > + const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > + camera3_stream *camera3Stream = camera3Buffer.stream; > + CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > + if (cameraStream->type() != CameraStream::Type::Mapped) > + continue; > + if (addedStreams.find(cameraStream->stream()) != addedStreams.end()) > + continue; Can't you just iterate on addedStreams then ? > + > + CameraStream *mappedStream = cameraStream->mappedStream(); > + if (!mappedStream) { > + LOG(HAL, Error) > + << "Could not find mappedStream for (" > + << camera3Stream->width << "x" > + << camera3Stream->height << ")" > + << "[" << utils::hex(camera3Stream->format) << "]"; > + return -EINVAL; > + } > + > + ASSERT(mappedStream->type() == CameraStream::Type::Direct); > + FrameBuffer *mappedBuffer = mappedStream->getBuffer(); > + if (!mappedBuffer) { > + LOG(HAL, Error) << "Failed getting a buffer"; > + return -EINVAL; > + } > + > + descriptor.internalBuffers_[mappedStream] = mappedBuffer; > + descriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1); I don't get this. What if the 'mapped' stream was already part of the request ? Does it get added twice ? > } > > /* > @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request) > int ret = cameraStream->process(*src, *buffer.buffer, > descriptor.settings_, > resultMetadata.get()); > - /* > - * 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, > @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request) > } > } > > + for (auto &[stream, buffer] : descriptor.internalBuffers_) > + stream->putBuffer(buffer); > + > captureResult.result = resultMetadata->get(); > callbacks_->process_capture_result(callbacks_, &captureResult); > } > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 296c2f18..74d8150b 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -21,6 +21,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > #include <libcamera/request.h> > @@ -84,6 +85,7 @@ private: > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > }; > > enum class State { > @@ -118,6 +120,8 @@ private: > std::unique_ptr<libcamera::CameraConfiguration> config_; > CameraCapabilities capabilities_; > > + std::unique_ptr<libcamera::FrameBufferAllocator> frame_buffer_allocator_; > + > std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_; > const camera3_callback_ops_t *callbacks_; > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index e30c7ee4..fc2e82cd 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -18,6 +18,7 @@ > #include "camera_capabilities.h" > #include "camera_device.h" > #include "camera_metadata.h" > +#include "frame_buffer.h" > > using namespace libcamera; > > @@ -46,10 +47,14 @@ 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 mappedStream, > + unsigned int index) > : cameraDevice_(cameraDevice), config_(config), type_(type), > - camera3Stream_(camera3Stream), index_(index) > + camera3Stream_(camera3Stream), mappedStream_(mappedStream), > + index_(index) > { > + ASSERT(type_ != Type::Mapped || !!mappedStream); > } > > const StreamConfiguration &CameraStream::configuration() const > @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source, > > FrameBuffer *CameraStream::getBuffer() > { > - if (!allocator_) > - return nullptr; I assume the style is due the series being an RFC... Anyway ... > + if (!mutex_) { Relying on !mutex to verify that !Internal and "first time this gets called" is fragile. Why not handle Direct in configure() and prepare a buffer cache there. Anyway, I've not really gone into detail in the platform buffer allocator but why can't this use the Camera's frame buffer allocator ? > + ASSERT(type_ == Type::Direct); > + ASSERT(!allocator_); > + mutex_ = std::make_unique<std::mutex>(); > + platform_allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_); > + const StreamConfiguration &config = configuration(); > + size_t numBuffers = config.bufferCount; > + const int halPixelFormat = camera3Stream_->format; > + const uint32_t usage = camera3Stream_->usage; > + LOG(HAL, Error) << "getBuffer@@@@@ " << numBuffers; > + // numBuffers is 4 (=config.bufferCount) is not enough sadly... > + // Should we dynamically allocate? > + numBuffers = 20; > + const auto &buffers = platform_allocator_->allocate( > + halPixelFormat, config.size, usage, numBuffers); > + if (buffers.empty() || buffers.size() != numBuffers) { > + LOG(HAL, Error) << "Failed allocating FrameBuffers"; > + return nullptr; > + } > + > + buffers_.reserve(numBuffers); > + for (const auto &frameBuffer : buffers) > + buffers_.push_back(frameBuffer.get()); > + } > > std::lock_guard<std::mutex> locker(*mutex_); > > @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer() > LOG(HAL, Error) << "Buffer underrun"; > return nullptr; > } > - > + LOG(HAL, Error) << buffers_.size(); > FrameBuffer *buffer = buffers_.back(); > buffers_.pop_back(); > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 2dab6c3a..c8eee908 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -15,10 +15,11 @@ > > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > -#include <libcamera/framebuffer_allocator.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > +#include "frame_buffer.h" > + > class CameraDevice; > class CameraMetadata; > class PostProcessor; > @@ -110,12 +111,14 @@ public: > }; > CameraStream(CameraDevice *const cameraDevice, > libcamera::CameraConfiguration *config, Type type, > - camera3_stream_t *camera3Stream, unsigned int index); > + camera3_stream_t *camera3Stream, > + CameraStream *const mappedStream, unsigned int index); > > Type type() const { return type_; } > const camera3_stream_t &camera3Stream() const { return *camera3Stream_; } > const libcamera::StreamConfiguration &configuration() const; > libcamera::Stream *stream() const; > + CameraStream *mappedStream() const { return mappedStream_; } > > int configure(); > int process(const libcamera::FrameBuffer &source, > @@ -130,10 +133,13 @@ private: > const libcamera::CameraConfiguration *config_; > const Type type_; > camera3_stream_t *camera3Stream_; > + CameraStream *const mappedStream_; > const unsigned int index_; > > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > + std::unique_ptr<PlatformFrameBufferAllocator> platform_allocator_; > std::vector<libcamera::FrameBuffer *> buffers_; > + std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > /* > * The class has to be MoveConstructible as instances are stored in > * an std::vector in CameraDevice. > -- > 2.33.0.685.g46640cef36-goog >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a693dcbe..5887e85e 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) for (const auto &streamConfig : streamConfigs) { config->addConfiguration(streamConfig.config); + CameraStream *mappedStream = nullptr; for (auto &stream : streamConfig.streams) { streams_.emplace_back(this, config.get(), stream.type, - stream.stream, config->size() - 1); + stream.stream, mappedStream, + config->size() - 1); stream.stream->priv = static_cast<void *>(&streams_.back()); + if (stream.type == CameraStream::Type::Direct) + mappedStream = &streams_.back(); } } @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() << " with " << descriptor.buffers_.size() << " streams"; + + std::set<Stream *> addedStreams; for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; camera3_stream *camera3Stream = camera3Buffer.stream; @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * once it has been processed. */ buffer = cameraStream->getBuffer(); + descriptor.internalBuffers_[cameraStream] = buffer; LOG(HAL, Debug) << ss.str() << " (internal)"; break; } @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques descriptor.request_->addBuffer(cameraStream->stream(), buffer, camera3Buffer.acquire_fence); + addedStreams.insert(cameraStream->stream()); + } + + for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { + const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; + camera3_stream *camera3Stream = camera3Buffer.stream; + CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); + if (cameraStream->type() != CameraStream::Type::Mapped) + continue; + if (addedStreams.find(cameraStream->stream()) != addedStreams.end()) + continue; + + CameraStream *mappedStream = cameraStream->mappedStream(); + if (!mappedStream) { + LOG(HAL, Error) + << "Could not find mappedStream for (" + << camera3Stream->width << "x" + << camera3Stream->height << ")" + << "[" << utils::hex(camera3Stream->format) << "]"; + return -EINVAL; + } + + ASSERT(mappedStream->type() == CameraStream::Type::Direct); + FrameBuffer *mappedBuffer = mappedStream->getBuffer(); + if (!mappedBuffer) { + LOG(HAL, Error) << "Failed getting a buffer"; + return -EINVAL; + } + + descriptor.internalBuffers_[mappedStream] = mappedBuffer; + descriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1); } /* @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request) int ret = cameraStream->process(*src, *buffer.buffer, descriptor.settings_, resultMetadata.get()); - /* - * 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, @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request) } } + for (auto &[stream, buffer] : descriptor.internalBuffers_) + stream->putBuffer(buffer); + captureResult.result = resultMetadata->get(); callbacks_->process_capture_result(callbacks_, &captureResult); } diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 296c2f18..74d8150b 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -21,6 +21,7 @@ #include <libcamera/camera.h> #include <libcamera/framebuffer.h> +#include <libcamera/framebuffer_allocator.h> #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> #include <libcamera/request.h> @@ -84,6 +85,7 @@ private: std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; CameraMetadata settings_; std::unique_ptr<CaptureRequest> request_; + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; }; enum class State { @@ -118,6 +120,8 @@ private: std::unique_ptr<libcamera::CameraConfiguration> config_; CameraCapabilities capabilities_; + std::unique_ptr<libcamera::FrameBufferAllocator> frame_buffer_allocator_; + std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_; const camera3_callback_ops_t *callbacks_; diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index e30c7ee4..fc2e82cd 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -18,6 +18,7 @@ #include "camera_capabilities.h" #include "camera_device.h" #include "camera_metadata.h" +#include "frame_buffer.h" using namespace libcamera; @@ -46,10 +47,14 @@ 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 mappedStream, + unsigned int index) : cameraDevice_(cameraDevice), config_(config), type_(type), - camera3Stream_(camera3Stream), index_(index) + camera3Stream_(camera3Stream), mappedStream_(mappedStream), + index_(index) { + ASSERT(type_ != Type::Mapped || !!mappedStream); } const StreamConfiguration &CameraStream::configuration() const @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source, FrameBuffer *CameraStream::getBuffer() { - if (!allocator_) - return nullptr; + if (!mutex_) { + ASSERT(type_ == Type::Direct); + ASSERT(!allocator_); + mutex_ = std::make_unique<std::mutex>(); + platform_allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_); + const StreamConfiguration &config = configuration(); + size_t numBuffers = config.bufferCount; + const int halPixelFormat = camera3Stream_->format; + const uint32_t usage = camera3Stream_->usage; + LOG(HAL, Error) << "getBuffer@@@@@ " << numBuffers; + // numBuffers is 4 (=config.bufferCount) is not enough sadly... + // Should we dynamically allocate? + numBuffers = 20; + const auto &buffers = platform_allocator_->allocate( + halPixelFormat, config.size, usage, numBuffers); + if (buffers.empty() || buffers.size() != numBuffers) { + LOG(HAL, Error) << "Failed allocating FrameBuffers"; + return nullptr; + } + + buffers_.reserve(numBuffers); + for (const auto &frameBuffer : buffers) + buffers_.push_back(frameBuffer.get()); + } std::lock_guard<std::mutex> locker(*mutex_); @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer() LOG(HAL, Error) << "Buffer underrun"; return nullptr; } - + LOG(HAL, Error) << buffers_.size(); FrameBuffer *buffer = buffers_.back(); buffers_.pop_back(); diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index 2dab6c3a..c8eee908 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -15,10 +15,11 @@ #include <libcamera/camera.h> #include <libcamera/framebuffer.h> -#include <libcamera/framebuffer_allocator.h> #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> +#include "frame_buffer.h" + class CameraDevice; class CameraMetadata; class PostProcessor; @@ -110,12 +111,14 @@ public: }; CameraStream(CameraDevice *const cameraDevice, libcamera::CameraConfiguration *config, Type type, - camera3_stream_t *camera3Stream, unsigned int index); + camera3_stream_t *camera3Stream, + CameraStream *const mappedStream, unsigned int index); Type type() const { return type_; } const camera3_stream_t &camera3Stream() const { return *camera3Stream_; } const libcamera::StreamConfiguration &configuration() const; libcamera::Stream *stream() const; + CameraStream *mappedStream() const { return mappedStream_; } int configure(); int process(const libcamera::FrameBuffer &source, @@ -130,10 +133,13 @@ private: const libcamera::CameraConfiguration *config_; const Type type_; camera3_stream_t *camera3Stream_; + CameraStream *const mappedStream_; const unsigned int index_; std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; + std::unique_ptr<PlatformFrameBufferAllocator> platform_allocator_; std::vector<libcamera::FrameBuffer *> buffers_; + std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; /* * The class has to be MoveConstructible as instances are stored in * an std::vector in CameraDevice.
A request given in processCaptureRequest() can contain only streams that are resolved CameraStream::Type::Mapped. In the case, no buffer is sent to a native camera. This fixes the issue by looking for the stream to be requested and alternatively sending a buffer for the stream to a camera. However, the substitute stream is Direct and a buffer needs to be provided by a HAL client. So we have to allocate a buffer in Android HAL adaptation layer using PlatformFrameBufferAllocator. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------ src/android/camera_device.h | 4 +++ src/android/camera_stream.cpp | 37 ++++++++++++++++++++++---- src/android/camera_stream.h | 10 +++++-- 4 files changed, 86 insertions(+), 15 deletions(-)