[libcamera-devel] [PATCH v6 4/7] android: post_processor: Consolidate contextual information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 25 15:26:21 CEST 2021
Hi Umang,
On Mon, Oct 25, 2021 at 06:30:41PM +0530, Umang Jain wrote:
> On 10/25/21 10:53 AM, Laurent Pinchart wrote:
> > On Sat, Oct 23, 2021 at 04:02:59PM +0530, Umang Jain wrote:
> >> Save and provide the context for post-processor of a camera stream
> >> via Camera3RequestDescriptor::StreamBuffer. We extend the structure
> >> to include source and destination buffers for the post processor, along
> >> with CameraStream::Type::Internal buffer pointer (if any). In addition
> >> to that, a back pointer to Camera3RequestDescriptor is convienient to
> >> get access to overall descriptor (status, metadata settings etc.)
> >>
> >> Also, migrate PostProcessor::process() signature to use
> >> Camera3RequestDescriptor::StreamBuffer only. This will be helpful
> >> when we move to async post-processing in subsequent commits.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/android/camera_device.cpp | 11 ++++++-----
> >> src/android/camera_request.cpp | 3 ++-
> >> src/android/camera_request.h | 5 +++++
> >> src/android/camera_stream.cpp | 14 ++++++++------
> >> src/android/camera_stream.h | 3 +--
> >> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++++-----
> >> src/android/jpeg/post_processor_jpeg.h | 4 +---
> >> src/android/post_processor.h | 7 ++-----
> >> src/android/yuv/post_processor_yuv.cpp | 7 ++++---
> >> src/android/yuv/post_processor_yuv.h | 4 +---
> >> 10 files changed, 37 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index f4d4fb09..2a98a2e6 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -953,6 +953,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >> * once it has been processed.
> >> */
> >> frameBuffer = cameraStream->getBuffer();
> >> + buffer.internalBuffer = frameBuffer;
> >> LOG(HAL, Debug) << ss.str() << " (internal)";
> >> break;
> >> }
> >> @@ -1134,14 +1135,14 @@ void CameraDevice::requestComplete(Request *request)
> >> continue;
> >> }
> >>
> >
> > Should we store src in buffer here instead of in
> > CameraStream::process(), to make the signature of all process()
> > functions the same ?
>
> That's a good point.
>
> I'll try to address it in this series, but see if it's easy enough
> changeset for a fixup! patch? If I see major conflicts, would it be OK
> on top?
If it's too difficult I think it would be ok on top.
> >> - int ret = stream->process(*src, buffer, descriptor);
> >> + int ret = stream->process(*src, buffer);
> >>
> >> /*
> >> - * Return the FrameBuffer to the CameraStream now that we're
> >> - * done processing it.
> >> + * If the framebuffer is internal to CameraStream return it back
> >> + * now that we're done processing it.
> >> */
> >> - if (stream->type() == CameraStream::Type::Internal)
> >> - stream->putBuffer(src);
> >> + if (buffer.internalBuffer)
> >> + stream->putBuffer(buffer.internalBuffer);
> >>
> >> if (ret) {
> >> buffer.status = Camera3RequestDescriptor::Status::Error;
> >> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> >> index 16cf266f..fd927167 100644
> >> --- a/src/android/camera_request.cpp
> >> +++ b/src/android/camera_request.cpp
> >> @@ -36,7 +36,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >> static_cast<CameraStream *>(buffer.stream->priv);
> >>
> >> buffers_.push_back({ stream, buffer.buffer, nullptr,
> >> - buffer.acquire_fence, Status::Success });
> >> + buffer.acquire_fence, Status::Success,
> >> + nullptr, nullptr, nullptr, this });
> >
> > A constructor will be nice at some point :-)
> >
> >> }
> >>
> >> /* Clone the controls associated with the camera3 request. */
> >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> >> index 904886da..c4bc5d6e 100644
> >> --- a/src/android/camera_request.h
> >> +++ b/src/android/camera_request.h
> >> @@ -17,6 +17,7 @@
> >>
> >> #include <hardware/camera3.h>
> >>
> >> +#include "camera_buffer.h"
> >
> > Can you forward-declare CameraBuffer instead of including the header
> > here ?
> >
> >> #include "camera_metadata.h"
> >> #include "camera_worker.h"
> >>
> >> @@ -36,6 +37,10 @@ public:
> >> std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> >> int fence;
> >> Status status;
> >> + libcamera::FrameBuffer *internalBuffer;
> >> + std::unique_ptr<CameraBuffer> destBuffer;
> >
> > Maybe dstBuffer to match srcBuffer ? I'd also place srcBuffer first to
> > match the usual source -> destination logical order, but that's a
> > detail.
>
> Yes, renaming that should be fine.
>
> >> + const libcamera::FrameBuffer *srcBuffer;
> >> + Camera3RequestDescriptor *request;
> >> };
> >>
> >> Camera3RequestDescriptor(libcamera::Camera *camera,
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index ca25c4db..0e268cdf 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -147,8 +147,7 @@ int CameraStream::waitFence(int fence)
> >> }
> >>
> >> int CameraStream::process(const FrameBuffer &source,
> >> - Camera3RequestDescriptor::StreamBuffer &dest,
> >> - Camera3RequestDescriptor *request)
> >> + Camera3RequestDescriptor::StreamBuffer &dest)
> >> {
> >> /* Handle waiting on fences on the destination buffer. */
> >> if (dest.fence != -1) {
> >> @@ -170,14 +169,17 @@ int CameraStream::process(const FrameBuffer &source,
> >> * separate thread.
> >> */
> >> const StreamConfiguration &output = configuration();
> >> - CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,
> >> - output.size, PROT_READ | PROT_WRITE);
> >> - if (!destBuffer.isValid()) {
> >> + dest.destBuffer = std::make_unique<CameraBuffer>(
> >> + *dest.camera3Buffer, output.pixelFormat, output.size,
> >> + PROT_READ | PROT_WRITE);
> >> + if (!dest.destBuffer->isValid()) {
> >> LOG(HAL, Error) << "Failed to create destination buffer";
> >> return -EINVAL;
> >> }
> >>
> >> - return postProcessor_->process(source, &destBuffer, request);
> >> + dest.srcBuffer = &source;
> >> +
> >> + return postProcessor_->process(&dest);
> >> }
> >>
> >> FrameBuffer *CameraStream::getBuffer()
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index f242336e..e9c320b1 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -122,8 +122,7 @@ public:
> >>
> >> int configure();
> >> int process(const libcamera::FrameBuffer &source,
> >> - Camera3RequestDescriptor::StreamBuffer &dest,
> >> - Camera3RequestDescriptor *request);
> >> + Camera3RequestDescriptor::StreamBuffer &dest);
> >
> > You're passing a reference here, and a pointer in
> > PostProcessor::process(). Could you pick one of the two and use it
> > consistently ?
>
> Ah right, I see that now. I will probably make use of pointers.
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >> libcamera::FrameBuffer *getBuffer();
> >> void putBuffer(libcamera::FrameBuffer *buffer);
> >>
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index 49483836..da71f113 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -98,15 +98,17 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >> }
> >> }
> >>
> >> -int PostProcessorJpeg::process(const FrameBuffer &source,
> >> - CameraBuffer *destination,
> >> - Camera3RequestDescriptor *request)
> >> +int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> >> {
> >> ASSERT(encoder_);
> >> +
> >> + const FrameBuffer &source = *streamBuffer->srcBuffer;
> >> + CameraBuffer *destination = streamBuffer->destBuffer.get();
> >> +
> >> ASSERT(destination->numPlanes() == 1);
> >>
> >> - const CameraMetadata &requestMetadata = request->settings_;
> >> - CameraMetadata *resultMetadata = request->resultMetadata_.get();
> >> + const CameraMetadata &requestMetadata = streamBuffer->request->settings_;
> >> + CameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();
> >> camera_metadata_ro_entry_t entry;
> >> int ret;
> >>
> >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> >> index 0184d77e..92385548 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.h
> >> +++ b/src/android/jpeg/post_processor_jpeg.h
> >> @@ -22,9 +22,7 @@ public:
> >>
> >> int configure(const libcamera::StreamConfiguration &incfg,
> >> const libcamera::StreamConfiguration &outcfg) override;
> >> - int process(const libcamera::FrameBuffer &source,
> >> - CameraBuffer *destination,
> >> - Camera3RequestDescriptor *request) override;
> >> + int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
> >>
> >> private:
> >> void generateThumbnail(const libcamera::FrameBuffer &source,
> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> >> index 27eaef88..128161c8 100644
> >> --- a/src/android/post_processor.h
> >> +++ b/src/android/post_processor.h
> >> @@ -11,8 +11,7 @@
> >> #include <libcamera/stream.h>
> >>
> >> #include "camera_buffer.h"
> >> -
> >> -class Camera3RequestDescriptor;
> >> +#include "camera_request.h"
> >>
> >> class PostProcessor
> >> {
> >> @@ -21,9 +20,7 @@ public:
> >>
> >> virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >> const libcamera::StreamConfiguration &outCfg) = 0;
> >> - virtual int process(const libcamera::FrameBuffer &source,
> >> - CameraBuffer *destination,
> >> - Camera3RequestDescriptor *request) = 0;
> >> + virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> >> };
> >>
> >> #endif /* __ANDROID_POST_PROCESSOR_H__ */
> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> >> index 8110a1f1..eeb8f1f4 100644
> >> --- a/src/android/yuv/post_processor_yuv.cpp
> >> +++ b/src/android/yuv/post_processor_yuv.cpp
> >> @@ -49,10 +49,11 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
> >> return 0;
> >> }
> >>
> >> -int PostProcessorYuv::process(const FrameBuffer &source,
> >> - CameraBuffer *destination,
> >> - [[maybe_unused]] Camera3RequestDescriptor *request)
> >> +int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> >> {
> >> + const FrameBuffer &source = *streamBuffer->srcBuffer;
> >> + CameraBuffer *destination = streamBuffer->destBuffer.get();
> >> +
> >> if (!isValidBuffers(source, *destination))
> >> return -EINVAL;
> >>
> >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
> >> index a4e0ff5d..5954e11b 100644
> >> --- a/src/android/yuv/post_processor_yuv.h
> >> +++ b/src/android/yuv/post_processor_yuv.h
> >> @@ -18,9 +18,7 @@ public:
> >>
> >> int configure(const libcamera::StreamConfiguration &incfg,
> >> const libcamera::StreamConfiguration &outcfg) override;
> >> - int process(const libcamera::FrameBuffer &source,
> >> - CameraBuffer *destination,
> >> - Camera3RequestDescriptor *request) override;
> >> + int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;
> >>
> >> private:
> >> bool isValidBuffers(const libcamera::FrameBuffer &source,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list