[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:53:45 CEST 2021
Hi Umang,
On Mon, Oct 25, 2021 at 07:16:56PM +0530, Umang Jain wrote:
> On 10/25/21 6:55 PM, Laurent Pinchart wrote:
> > 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/
With
descriptor->pendingStreamsToProcess_.size() == 0
replaced with
descriptor->pendingStreamsToProcess_.empty()
it seems good to me.
> >> 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