| Message ID | 20211023103302.152769-5-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. 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@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 ? > - 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. > + 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 ? Reviewed-by: Laurent Pinchart <laurent.pinchart@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,
Hi Umang, thank you for the patch. On Mon, Oct 25, 2021 at 2:23 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Umang, > > Thank you for the patch. > > 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@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 ? > > > - 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. > > > + const libcamera::FrameBuffer *srcBuffer; > > + Camera3RequestDescriptor *request; > > }; > > Could you add a document about these in .cpp? It becomes more and more difficult to track > > 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); > dest seems to be no longer a proper name. As Laurent said, if we set streamBuffer.srcBuffer to source in a caller side, perhaps shall this renamed to streamBuffer? Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > You're passing a reference here, and a pointer in > PostProcessor::process(). Could you pick one of the two and use it > consistently ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Hi Laurent, On 10/25/21 10:53 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > 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@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? > >> - 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@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,
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@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@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,
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; } - 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 }); } /* 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" #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; + 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); 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,
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@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(-)