| Message ID | 20210301150111.61791-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Mar 01, 2021 at 04:01:08PM +0100, Jacopo Mondi wrote: > Use the newly introduced CameraBuffer class as the type for the > destination buffer in the PostProcessor class hierarchy in place of the > libcamera::MappedFrameBuffer one and use its API to retrieve the length > and the location of the CameraBuffer plane allocated for JPEG > post-processing. > > Remove all the assumption on the underlying memory storage and only go > through the CameraBuffer API when dealing with memory buffers. To do so > rework the Encoder interface to use a raw pointer and an explicit size > to remove access to the Span<uint8_t> maps that serve as memory storage > for the current implementation but might not be ideal for other memory > backend. > > Now that the whole PostProcessor hierarchy has been converted to use > the CameraBuffer API remove libcamera::MappedBuffer as base class > of the CameraBuffer interface and only reply on its interface. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_buffer.h | 4 +--- > src/android/jpeg/post_processor_jpeg.cpp | 25 ++++++++---------------- > src/android/jpeg/post_processor_jpeg.h | 2 +- > src/android/mm/generic_camera_buffer.cpp | 1 + > src/android/post_processor.h | 4 +++- > src/android/yuv/post_processor_yuv.cpp | 20 +++++++++---------- > src/android/yuv/post_processor_yuv.h | 6 ++++-- > 7 files changed, 28 insertions(+), 34 deletions(-) > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > index ca4f4527c690..2311cdaf96b2 100644 > --- a/src/android/camera_buffer.h > +++ b/src/android/camera_buffer.h > @@ -10,11 +10,9 @@ > #include <hardware/camera3.h> > > #include <libcamera/class.h> > -#include <libcamera/internal/buffer.h> > #include <libcamera/span.h> > > -class CameraBuffer final : public libcamera::Extensible, > - public libcamera::MappedBuffer > +class CameraBuffer final : public libcamera::Extensible > { > LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index ab5b63844067..83244ce6769e 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -83,13 +83,15 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > } > > int PostProcessorJpeg::process(const FrameBuffer &source, > - libcamera::MappedBuffer *destination, > + CameraBuffer *destination, > const CameraMetadata &requestMetadata, > CameraMetadata *resultMetadata) > { > if (!encoder_) > return 0; > > + ASSERT(destination->numPlanes() == 1); > + > camera_metadata_ro_entry_t entry; > int ret; > > @@ -172,28 +174,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source, > const uint8_t quality = ret ? *entry.data.u8 : 95; > resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1); > > - int jpeg_size = encoder_->encode(source, destination->maps()[0], > + int jpeg_size = encoder_->encode(source, destination->plane(0), > exif.data(), quality); > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > return jpeg_size; > } > > - /* > - * Fill in the JPEG blob header. > - * > - * The mapped size of the buffer is being returned as > - * substantially larger than the requested JPEG_MAX_SIZE > - * (which is referenced from maxJpegBufferSize_). Utilise > - * this static size to ensure the correct offset of the blob is > - * determined. > - * > - * \todo Investigate if the buffer size mismatch is an issue or > - * expected behaviour. > - */ > - uint8_t *resultPtr = destination->maps()[0].data() + > - cameraDevice_->maxJpegBufferSize() - > - sizeof(struct camera3_jpeg_blob); > + /* Fill in the JPEG blob header. */ > + uint8_t *resultPtr = destination->plane(0).data() > + + destination->plane(0).size() > + - sizeof(struct camera3_jpeg_blob); > auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); > blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; > blob->jpeg_size = jpeg_size; > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index 7689de73c664..5d2d4ab224b1 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -25,7 +25,7 @@ public: > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > int process(const libcamera::FrameBuffer &source, > - libcamera::MappedBuffer *destination, > + CameraBuffer *destination, > const CameraMetadata &requestMetadata, > CameraMetadata *resultMetadata) override; > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > index eedf16b76542..ea85be805260 100644 > --- a/src/android/mm/generic_camera_buffer.cpp > +++ b/src/android/mm/generic_camera_buffer.cpp > @@ -7,6 +7,7 @@ > > #include "../camera_buffer.h" > > +#include <libcamera/internal/buffer.h> > #include "libcamera/internal/log.h" > > using namespace libcamera; > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index ac40d3414892..4944078b490c 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -12,6 +12,8 @@ > > #include <libcamera/internal/buffer.h> > > +#include "camera_buffer.h" > + > class CameraMetadata; > > class PostProcessor > @@ -22,7 +24,7 @@ public: > virtual int configure(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg) = 0; > virtual int process(const libcamera::FrameBuffer &source, > - libcamera::MappedBuffer *destination, > + CameraBuffer *destination, > const CameraMetadata &requestMetadata, > CameraMetadata *resultMetadata) = 0; > }; > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > index 1318349ad66b..b67364c8f147 100644 > --- a/src/android/yuv/post_processor_yuv.cpp > +++ b/src/android/yuv/post_processor_yuv.cpp > @@ -48,7 +48,7 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > } > > int PostProcessorYuv::process(const FrameBuffer &source, > - libcamera::MappedBuffer *destination, > + CameraBuffer *destination, > [[maybe_unused]] const CameraMetadata &requestMetadata, > [[maybe_unused]] CameraMetadata *metadata) > { > @@ -66,9 +66,9 @@ int PostProcessorYuv::process(const FrameBuffer &source, > sourceMapped.maps()[1].data(), > sourceStride_[1], > sourceSize_.width, sourceSize_.height, > - destination->maps()[0].data(), > + destination->plane(0).data(), > destinationStride_[0], > - destination->maps()[1].data(), > + destination->plane(1).data(), > destinationStride_[1], > destinationSize_.width, > destinationSize_.height, > @@ -82,16 +82,16 @@ int PostProcessorYuv::process(const FrameBuffer &source, > } > > bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > - const libcamera::MappedBuffer &destination) const > + const CameraBuffer &destination) const > { > if (source.planes().size() != 2) { > LOG(YUV, Error) << "Invalid number of source planes: " > << source.planes().size(); > return false; > } > - if (destination.maps().size() != 2) { > + if (destination.numPlanes() != 2) { > LOG(YUV, Error) << "Invalid number of destination planes: " > - << destination.maps().size(); > + << destination.numPlanes(); > return false; > } > > @@ -106,12 +106,12 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > << sourceLength_[1] << "}"; > return false; > } > - if (destination.maps()[0].size() < destinationLength_[0] || > - destination.maps()[1].size() < destinationLength_[1]) { > + if (destination.plane(0).size() < destinationLength_[0] || > + destination.plane(1).size() < destinationLength_[1]) { > LOG(YUV, Error) > << "The destination planes lengths are too small, actual size: {" > - << destination.maps()[0].size() << ", " > - << destination.maps()[1].size() > + << destination.plane(0).size() << ", " > + << destination.plane(1).size() > << "}, expected size: {" > << sourceLength_[0] << ", " > << sourceLength_[1] << "}"; > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h > index c58b4cf790fc..94620dd0a870 100644 > --- a/src/android/yuv/post_processor_yuv.h > +++ b/src/android/yuv/post_processor_yuv.h > @@ -9,6 +9,8 @@ > > #include "../post_processor.h" > > +#include "../camera_buffer.h" As post_processor.h defines the process() function and thus pulls the CameraBuffer type definition, you don't need to include camera_buffer.h here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > #include <libcamera/geometry.h> > > class CameraDevice; > @@ -21,13 +23,13 @@ public: > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > int process(const libcamera::FrameBuffer &source, > - libcamera::MappedBuffer *destination, > + CameraBuffer *destination, > const CameraMetadata &requestMetadata, > CameraMetadata *metadata) override; > > private: > bool isValidBuffers(const libcamera::FrameBuffer &source, > - const libcamera::MappedBuffer &destination) const; > + const CameraBuffer &destination) const; > void calculateLengths(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg); >
diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h index ca4f4527c690..2311cdaf96b2 100644 --- a/src/android/camera_buffer.h +++ b/src/android/camera_buffer.h @@ -10,11 +10,9 @@ #include <hardware/camera3.h> #include <libcamera/class.h> -#include <libcamera/internal/buffer.h> #include <libcamera/span.h> -class CameraBuffer final : public libcamera::Extensible, - public libcamera::MappedBuffer +class CameraBuffer final : public libcamera::Extensible { LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index ab5b63844067..83244ce6769e 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -83,13 +83,15 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, } int PostProcessorJpeg::process(const FrameBuffer &source, - libcamera::MappedBuffer *destination, + CameraBuffer *destination, const CameraMetadata &requestMetadata, CameraMetadata *resultMetadata) { if (!encoder_) return 0; + ASSERT(destination->numPlanes() == 1); + camera_metadata_ro_entry_t entry; int ret; @@ -172,28 +174,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source, const uint8_t quality = ret ? *entry.data.u8 : 95; resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1); - int jpeg_size = encoder_->encode(source, destination->maps()[0], + int jpeg_size = encoder_->encode(source, destination->plane(0), exif.data(), quality); if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; return jpeg_size; } - /* - * Fill in the JPEG blob header. - * - * The mapped size of the buffer is being returned as - * substantially larger than the requested JPEG_MAX_SIZE - * (which is referenced from maxJpegBufferSize_). Utilise - * this static size to ensure the correct offset of the blob is - * determined. - * - * \todo Investigate if the buffer size mismatch is an issue or - * expected behaviour. - */ - uint8_t *resultPtr = destination->maps()[0].data() + - cameraDevice_->maxJpegBufferSize() - - sizeof(struct camera3_jpeg_blob); + /* Fill in the JPEG blob header. */ + uint8_t *resultPtr = destination->plane(0).data() + + destination->plane(0).size() + - sizeof(struct camera3_jpeg_blob); auto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr); blob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID; blob->jpeg_size = jpeg_size; diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index 7689de73c664..5d2d4ab224b1 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -25,7 +25,7 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; int process(const libcamera::FrameBuffer &source, - libcamera::MappedBuffer *destination, + CameraBuffer *destination, const CameraMetadata &requestMetadata, CameraMetadata *resultMetadata) override; diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp index eedf16b76542..ea85be805260 100644 --- a/src/android/mm/generic_camera_buffer.cpp +++ b/src/android/mm/generic_camera_buffer.cpp @@ -7,6 +7,7 @@ #include "../camera_buffer.h" +#include <libcamera/internal/buffer.h> #include "libcamera/internal/log.h" using namespace libcamera; diff --git a/src/android/post_processor.h b/src/android/post_processor.h index ac40d3414892..4944078b490c 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -12,6 +12,8 @@ #include <libcamera/internal/buffer.h> +#include "camera_buffer.h" + class CameraMetadata; class PostProcessor @@ -22,7 +24,7 @@ public: virtual int configure(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg) = 0; virtual int process(const libcamera::FrameBuffer &source, - libcamera::MappedBuffer *destination, + CameraBuffer *destination, const CameraMetadata &requestMetadata, CameraMetadata *resultMetadata) = 0; }; diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index 1318349ad66b..b67364c8f147 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -48,7 +48,7 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, } int PostProcessorYuv::process(const FrameBuffer &source, - libcamera::MappedBuffer *destination, + CameraBuffer *destination, [[maybe_unused]] const CameraMetadata &requestMetadata, [[maybe_unused]] CameraMetadata *metadata) { @@ -66,9 +66,9 @@ int PostProcessorYuv::process(const FrameBuffer &source, sourceMapped.maps()[1].data(), sourceStride_[1], sourceSize_.width, sourceSize_.height, - destination->maps()[0].data(), + destination->plane(0).data(), destinationStride_[0], - destination->maps()[1].data(), + destination->plane(1).data(), destinationStride_[1], destinationSize_.width, destinationSize_.height, @@ -82,16 +82,16 @@ int PostProcessorYuv::process(const FrameBuffer &source, } bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, - const libcamera::MappedBuffer &destination) const + const CameraBuffer &destination) const { if (source.planes().size() != 2) { LOG(YUV, Error) << "Invalid number of source planes: " << source.planes().size(); return false; } - if (destination.maps().size() != 2) { + if (destination.numPlanes() != 2) { LOG(YUV, Error) << "Invalid number of destination planes: " - << destination.maps().size(); + << destination.numPlanes(); return false; } @@ -106,12 +106,12 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, << sourceLength_[1] << "}"; return false; } - if (destination.maps()[0].size() < destinationLength_[0] || - destination.maps()[1].size() < destinationLength_[1]) { + if (destination.plane(0).size() < destinationLength_[0] || + destination.plane(1).size() < destinationLength_[1]) { LOG(YUV, Error) << "The destination planes lengths are too small, actual size: {" - << destination.maps()[0].size() << ", " - << destination.maps()[1].size() + << destination.plane(0).size() << ", " + << destination.plane(1).size() << "}, expected size: {" << sourceLength_[0] << ", " << sourceLength_[1] << "}"; diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h index c58b4cf790fc..94620dd0a870 100644 --- a/src/android/yuv/post_processor_yuv.h +++ b/src/android/yuv/post_processor_yuv.h @@ -9,6 +9,8 @@ #include "../post_processor.h" +#include "../camera_buffer.h" + #include <libcamera/geometry.h> class CameraDevice; @@ -21,13 +23,13 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; int process(const libcamera::FrameBuffer &source, - libcamera::MappedBuffer *destination, + CameraBuffer *destination, const CameraMetadata &requestMetadata, CameraMetadata *metadata) override; private: bool isValidBuffers(const libcamera::FrameBuffer &source, - const libcamera::MappedBuffer &destination) const; + const CameraBuffer &destination) const; void calculateLengths(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg);
Use the newly introduced CameraBuffer class as the type for the destination buffer in the PostProcessor class hierarchy in place of the libcamera::MappedFrameBuffer one and use its API to retrieve the length and the location of the CameraBuffer plane allocated for JPEG post-processing. Remove all the assumption on the underlying memory storage and only go through the CameraBuffer API when dealing with memory buffers. To do so rework the Encoder interface to use a raw pointer and an explicit size to remove access to the Span<uint8_t> maps that serve as memory storage for the current implementation but might not be ideal for other memory backend. Now that the whole PostProcessor hierarchy has been converted to use the CameraBuffer API remove libcamera::MappedBuffer as base class of the CameraBuffer interface and only reply on its interface. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_buffer.h | 4 +--- src/android/jpeg/post_processor_jpeg.cpp | 25 ++++++++---------------- src/android/jpeg/post_processor_jpeg.h | 2 +- src/android/mm/generic_camera_buffer.cpp | 1 + src/android/post_processor.h | 4 +++- src/android/yuv/post_processor_yuv.cpp | 20 +++++++++---------- src/android/yuv/post_processor_yuv.h | 6 ++++-- 7 files changed, 28 insertions(+), 34 deletions(-)