[libcamera-devel] [PATCH v6 5/7] android: Track and notify post processing of streams
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 25 15:25:09 CEST 2021
Hi Umang,
On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:
> On 10/25/21 11:33 AM, Laurent Pinchart wrote:
> > On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
> >> Notify that the post processing for a request has been completed,
> >> via a signal. The signal emit with a context pointer along with status
> > s/emit/is emitted/ ?
> >
> >> of the buffer. The function CameraDevice::streamProcessingComplete() will
> >> finally set the status on the request descriptor and complete the
> >> descriptor if all the streams requiring post processing are completed.
> >> If buffer status obtained is in error state, notify the status to the
> >> framework and set the overall error status on the descriptor via
> >> setBufferStatus().
> >>
> >> We need to track the number of streams requiring post-processing
> >> per Camera3RequestDescriptor (i.e. per capture request). Introduce
> >> a std::map<> to track the post-processing of streams. The nodes
> >> are dropped from the map when a particular stream post processing
> >> is completed (or on error paths). A std::map is selected for tracking
> >> post-processing requests, since we will move post-processing to be
> >> asynchronous in subsequent commits. A vector or queue will not be
> >> suitable then as the sequential order of post-processing completion
> >> of various requests won't be guaranteed then.
> >>
> >> A streamProcessMutex_ has been introduced here as well, which will be
> >
> > s/streamProcessMutex_/streamsProcessMutex_/
> >
> >> applicable to guard access to descriptor's pendingStreamsToProcess_ when
> >> post-processing is moved to be asynchronous in subsequent commits.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/android/camera_device.cpp | 95 ++++++++++++++++++------
> >> src/android/camera_device.h | 4 +
> >> src/android/camera_request.h | 6 ++
> >> src/android/camera_stream.cpp | 15 ++++
> >> src/android/jpeg/post_processor_jpeg.cpp | 2 +
> >> src/android/post_processor.h | 9 +++
> >> src/android/yuv/post_processor_yuv.cpp | 8 +-
> >> 7 files changed, 114 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 2a98a2e6..3114def0 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >> * Request.
> >> */
> >> LOG(HAL, Debug) << ss.str() << " (mapped)";
> > Blank line here, or no blank line below.
> >
> >> + descriptor->pendingStreamsToProcess_.insert(
> >> + { cameraStream, &buffer });
> >> continue;
> >>
> >> case CameraStream::Type::Direct:
> >> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >> frameBuffer = cameraStream->getBuffer();
> >> buffer.internalBuffer = frameBuffer;
> >> LOG(HAL, Debug) << ss.str() << " (internal)";
> >> +
> >> + descriptor->pendingStreamsToProcess_.insert(
> >> + { cameraStream, &buffer });
> >> break;
> >> }
> >>
> >> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
> >> }
> >>
> >> /* Handle post-processing. */
> >> - bool hasPostProcessingErrors = false;
> >> - for (auto &buffer : descriptor->buffers_) {
> >> - CameraStream *stream = buffer.stream;
> >> -
> >> - if (stream->type() == CameraStream::Type::Direct)
> >> - continue;
> >> + bool needPostProcessing = false;
> >> + /*
> >> + * \todo Protect the loop below with streamProcessMutex_ when post
> >> + * processor runs asynchronously.
> >> + */
> >> + auto iter = descriptor->pendingStreamsToProcess_.begin();
> >> + while (descriptor->pendingStreamsToProcess_.size() > 0) {
> > while (!descriptor->pendingStreamsToProcess_.empty()) {
> >
> > But it's not correct. The fix is in 7/7:
> >
> > while (iter != descriptor->pendingStreamsToProcess_.end()) {
>
> Yes, it was a tricky to handle the loop for async and sync variations
>
> For the async one, iter == end() was the only choice I believe (Without
> introducing a counter)
>
> >> + CameraStream *stream = iter->first;
> >> + Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
> >> + needPostProcessing = true;
> >>
> >> FrameBuffer *src = request->findBuffer(stream->stream());
> >> if (!src) {
> >> LOG(HAL, Error) << "Failed to find a source stream buffer";
> >> - buffer.status = Camera3RequestDescriptor::Status::Error;
> >> - notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >> - CAMERA3_MSG_ERROR_BUFFER);
> >> - hasPostProcessingErrors = true;
> >> + setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >> + iter = descriptor->pendingStreamsToProcess_.erase(iter);
> >> continue;
> >> }
> >>
> >> - int ret = stream->process(*src, buffer);
> >> + ++iter;
> >> + int ret = stream->process(*src, *buffer);
> >> + if (ret) {
> >> + setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >> + descriptor->pendingStreamsToProcess_.erase(stream);
> >> + }
> >> + }
> >>
> >> + if (needPostProcessing) {
> >> /*
> >> - * If the framebuffer is internal to CameraStream return it back
> >> - * now that we're done processing it.
> >> + * \todo We will require to check if we failed to queue
> >> + * post-processing requests when we migrate to post-processor
> >> + * running asynchronously.
> >> + *
> >> + * if (descriptor->pendingStreamsToProcess_.size() == 0)
> >> + * completeDescriptor(descriptor);
> >
> > Can't we do this already here ? I think you can actually drop the
> > needPostProcessing variable and just write
> >
> > if (descriptor->pendingStreamsToProcess_.empty())
> > completeDescriptor(descriptor);
> >
> > as needPostProcessing can only be false if pendingStreamsToProcess_ was
> > empty before the while loop above, and it will thus be empty after the
> > loop as well in that case.
>
> True, but we need needPostProcessing variable for async.
Why is that ? Won't
if (descriptor->pendingStreamsToProcess_.empty())
completeDescriptor(descriptor);
work in async mode too ?
> I pre-empted
> its introduction deliberately to set the design beforehand andthen, I
> can introduce async bits with minimal diff for
> CamreraDevice::requestComplete()
>
> >> */
> >> - if (buffer.internalBuffer)
> >> - stream->putBuffer(buffer.internalBuffer);
> >>
> >> - if (ret) {
> >> - buffer.status = Camera3RequestDescriptor::Status::Error;
> >> - hasPostProcessingErrors = true;
> >> - notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >> - CAMERA3_MSG_ERROR_BUFFER);
> >> - }
> >> + return;
> >> }
> >>
> >> - if (hasPostProcessingErrors)
> >> - descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >> -
> >> completeDescriptor(descriptor);
> >> }
> >>
> >> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
> >> }
> >> }
> >>
> >> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> >> + Camera3RequestDescriptor::Status status)
> >> +{
> >> + /*
> >> + * If the framebuffer is internal to CameraStream return it back now
> >> + * that we're done processing it.
> >> + */
> >> + if (streamBuffer.internalBuffer)
> >> + streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
> > I'd move this to the caller, as it's not about the buffer status.
> >
> >> +
> >> + streamBuffer.status = status;
> >> + if (status != Camera3RequestDescriptor::Status::Success) {
> >> + notifyError(streamBuffer.request->frameNumber_,
> >> + streamBuffer.stream->camera3Stream(),
> >> + CAMERA3_MSG_ERROR_BUFFER);
> >> +
> >> + /* Also set error status on entire request descriptor. */
> >> + streamBuffer.request->status_ =
> >> + Camera3RequestDescriptor::Status::Error;
> >> + }
> >> +}
> >> +
> >> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >> + Camera3RequestDescriptor::Status status)
> >> +{
> >> + Camera3RequestDescriptor *request = streamBuffer->request;
> >> + MutexLocker locker(request->streamsProcessMutex_);
> >> +
> >> + setBufferStatus(*streamBuffer, status);
> > Do we need to protect the setBufferStatus() call with
> > streamsProcessMutex_ ? I thought it only protects
> > pendingStreamsToProcess_.
>
>
> Ah, I don't see why we need to lock it. This is a good catch.
>
> For the async version, the main thread isn't expected to call
> setBufferStatus() on the same descriptor (because we are already
> handling synchronous errors beforehand), so I don't expect to race. So I
> should remove setBufferStatus() from the lock section.
>
> >
> >> + request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> >> +
> >> + if (request->pendingStreamsToProcess_.size() > 0)
> > if (!request->pendingStreamsToProcess_.empty())
> >
> >> + return;
> >> +
> >> + locker.unlock();
> >> +
> >> + completeDescriptor(streamBuffer->request);
> >> +}
> >> +
> >> std::string CameraDevice::logPrefix() const
> >> {
> >> return "'" + camera_->id() + "'";
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index e544f2bd..2a414020 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -66,6 +66,8 @@ public:
> >> int configureStreams(camera3_stream_configuration_t *stream_list);
> >> int processCaptureRequest(camera3_capture_request_t *request);
> >> void requestComplete(libcamera::Request *request);
> >> + void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
> >> + Camera3RequestDescriptor::Status status);
> >>
> >> protected:
> >> std::string logPrefix() const override;
> >> @@ -95,6 +97,8 @@ private:
> >> int processControls(Camera3RequestDescriptor *descriptor);
> >> void completeDescriptor(Camera3RequestDescriptor *descriptor);
> >> void sendCaptureResults();
> >> + void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
> >> + Camera3RequestDescriptor::Status status);
> >> std::unique_ptr<CameraMetadata> getResultMetadata(
> >> const Camera3RequestDescriptor &descriptor) const;
> >>
> >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> >> index c4bc5d6e..cc2b7035 100644
> >> --- a/src/android/camera_request.h
> >> +++ b/src/android/camera_request.h
> >> @@ -7,7 +7,9 @@
> >> #ifndef __ANDROID_CAMERA_REQUEST_H__
> >> #define __ANDROID_CAMERA_REQUEST_H__
> >>
> >> +#include <map>
> >> #include <memory>
> >> +#include <mutex>
> >> #include <vector>
> >>
> >> #include <libcamera/base/class.h>
> >> @@ -43,6 +45,10 @@ public:
> >> Camera3RequestDescriptor *request;
> >> };
> >>
> >> + /* Keeps track of streams requiring post-processing. */
> >> + std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> >> + std::mutex streamsProcessMutex_;
> >> +
> >> Camera3RequestDescriptor(libcamera::Camera *camera,
> >> const camera3_capture_request_t *camera3Request);
> >> ~Camera3RequestDescriptor();
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 0e268cdf..4e275cde 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -22,6 +22,7 @@
> >> #include "camera_capabilities.h"
> >> #include "camera_device.h"
> >> #include "camera_metadata.h"
> >> +#include "post_processor.h"
> >>
> >> using namespace libcamera;
> >>
> >> @@ -97,6 +98,20 @@ int CameraStream::configure()
> >> int ret = postProcessor_->configure(configuration(), output);
> >> if (ret)
> >> return ret;
> >> +
> >> + postProcessor_->processComplete.connect(
> >> + this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >> + PostProcessor::Status status) {
> >> + Camera3RequestDescriptor::Status bufferStatus;
> >> +
> >> + if (status == PostProcessor::Status::Success)
> >> + bufferStatus = Camera3RequestDescriptor::Status::Success;
> >> + else
> >> + bufferStatus = Camera3RequestDescriptor::Status::Error;
> >> +
> >> + cameraDevice_->streamProcessingComplete(streamBuffer,
> >> + bufferStatus);
> >> + });
> >> }
> >>
> >> if (type_ == Type::Internal) {
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index da71f113..cbbe7128 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >> exif.data(), quality);
> >> if (jpeg_size < 0) {
> >> LOG(JPEG, Error) << "Failed to encode stream image";
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >> return jpeg_size;
> >> }
> >>
> >> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >>
> >> /* Update the JPEG result Metadata. */
> >> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >>
> >> return 0;
> >> }
> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> >> index 128161c8..4ac74fcf 100644
> >> --- a/src/android/post_processor.h
> >> +++ b/src/android/post_processor.h
> >> @@ -7,6 +7,8 @@
> >> #ifndef __ANDROID_POST_PROCESSOR_H__
> >> #define __ANDROID_POST_PROCESSOR_H__
> >>
> >> +#include <libcamera/base/signal.h>
> >> +
> >> #include <libcamera/framebuffer.h>
> >> #include <libcamera/stream.h>
> >>
> >> @@ -16,11 +18,18 @@
> >> class PostProcessor
> >> {
> >> public:
> >> + enum class Status {
> >> + Error,
> >> + Success
> >> + };
> >> +
> >> virtual ~PostProcessor() = default;
> >>
> >> virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >> const libcamera::StreamConfiguration &outCfg) = 0;
> >> virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> >> +
> >> + libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
> >> };
> >>
> >> #endif /* __ANDROID_POST_PROCESSOR_H__ */
> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> >> index eeb8f1f4..8e77bf57 100644
> >> --- a/src/android/yuv/post_processor_yuv.cpp
> >> +++ b/src/android/yuv/post_processor_yuv.cpp
> >> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >> const FrameBuffer &source = *streamBuffer->srcBuffer;
> >> CameraBuffer *destination = streamBuffer->destBuffer.get();
> >>
> >> - if (!isValidBuffers(source, *destination))
> >> + if (!isValidBuffers(source, *destination)) {
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >> return -EINVAL;
> >> + }
> >>
> >> const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
> >> if (!sourceMapped.isValid()) {
> >> LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >> return -EINVAL;
> >> }
> >>
> >> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >> libyuv::FilterMode::kFilterBilinear);
> >> if (ret) {
> >> LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >> return -EINVAL;
> >> }
> >>
> >> + processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >> +
> >> return 0;
> >> }
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list