[libcamera-devel] [PATCH v6 5/7] android: Track and notify post processing of streams
Hirokazu Honda
hiroh at chromium.org
Mon Oct 25 13:49:11 CEST 2021
Hi Umang, thank you for the patch.
On Mon, Oct 25, 2021 at 3:03 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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;
nit: can just be
const bool needPostProcessing = !descriptor->pendingStreamToProcess.empty()?
> > + /*
> > + * \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()) {
>
> > + 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);
Do we need to proceed processing in this case?
> > 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.
>
> > */
> > - 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_.
>
I think if this function is called by any thread, we need to protect status too.
If it's true, I would rename the mutex name.
> > + 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,
nit: streamBuffer may be better name?
> > + 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) {
I would not like capturing a reference of all visible variables.
cameraDevice looks only needed.
I would do [cameraDevice=cameraDevice_](...).
> > + 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