[libcamera-devel] [PATCH v6 5/7] android: Track and notify post processing of streams
Umang Jain
umang.jain at ideasonboard.com
Mon Oct 25 15:16:39 CEST 2021
Hi Laurent,
On 10/25/21 11:33 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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. 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;
>> }
>>
More information about the libcamera-devel
mailing list