[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:46:56 CEST 2021
Hi Laurent,
On 10/25/21 6:55 PM, Laurent Pinchart wrote:
> 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 ?
Ok yes, it should work (sorry I confused myself a bit, thinking it's
will be a double completeDescriptor(descriptor) for the same descriptor,
in case post-processing completes early when main thread reaches this
line. But I forgot that we are already holding a lock restricted slots
to run)
We would end up requestComplete() like: https://paste.debian.net/1216820/
>
>> 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