[libcamera-devel] [PATCH] libcamera: MappedFrameBuffer: Use typed Flags<MapModes>
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 6 15:24:17 CEST 2021
Hi Kieran,
Thank you for the patch.
On Fri, Aug 06, 2021 at 02:15:11PM +0100, Kieran Bingham wrote:
> Remove the need for callers to reference PROT_READ/PROT_WRITE directly
> from <sys/mman.h> by instead exposing the Read/Write mapping options as
> flags from the MappedFrameBuffer class itself.
>
> While here, introduce the <stdint.h> header which is required for the
> int8_t as part of the Plane.
s/int8_t/uint8_t/
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> .../libcamera/internal/mapped_framebuffer.h | 14 ++++++++--
> src/android/jpeg/encoder_libjpeg.cpp | 2 +-
> src/android/jpeg/thumbnailer.cpp | 2 +-
> src/android/yuv/post_processor_yuv.cpp | 2 +-
> src/ipa/ipu3/ipu3.cpp | 2 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> src/libcamera/mapped_framebuffer.cpp | 28 +++++++++++++++++--
> test/mapped-buffer.cpp | 6 ++--
> 8 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index 41e587364260..24bd2c50cd27 100644
> --- a/include/libcamera/internal/mapped_framebuffer.h
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -7,10 +7,11 @@
> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>
> -#include <sys/mman.h>
> +#include <stdint.h>
> #include <vector>
>
> #include <libcamera/base/class.h>
> +#include <libcamera/base/flags.h>
> #include <libcamera/base/span.h>
>
> #include <libcamera/framebuffer.h>
> @@ -44,7 +45,16 @@ private:
> class MappedFrameBuffer : public MappedBuffer
> {
> public:
> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> + enum MapModes {
To get the full benefits from Flags<>, this should be a scoped enum
(enum class). Don't forget LIBCAMERA_FLAGS_ENABLE_OPERATORS(). Yes, it
will be a bit more verbose in the caller, but I think it's worth it to
ensure type safety.
> + Read = 0 << 1,
> + Write = 1 << 1,
> +
> + ReadWrite = Read | Write,
> + };
> +
> + using MapMode = Flags<MapModes>;
The File class uses MapFlags = Flags<MapFlag> and OpenMode =
Flags<OpenModeFlag>. Now we have yet another naming scheme :-) I think
it would be nice to try and be consistent.
> +
> + MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags);
> };
>
> } /* namespace libcamera */
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 372018d2207f..c37611f6b6cd 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -182,7 +182,7 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
> int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
> Span<const uint8_t> exifData, unsigned int quality)
> {
> - MappedFrameBuffer frame(&source, PROT_READ);
> + MappedFrameBuffer frame(&source, MappedFrameBuffer::Read);
> if (!frame.isValid()) {
> LOG(JPEG, Error) << "Failed to map FrameBuffer : "
> << strerror(frame.error());
> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> index 535e2cece914..17196b2dd257 100644
> --- a/src/android/jpeg/thumbnailer.cpp
> +++ b/src/android/jpeg/thumbnailer.cpp
> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,
> const Size &targetSize,
> std::vector<unsigned char> *destination)
> {
> - MappedFrameBuffer frame(&source, PROT_READ);
> + MappedFrameBuffer frame(&source, MappedFrameBuffer::Read);
> if (!frame.isValid()) {
> LOG(Thumbnailer, Error)
> << "Failed to map FrameBuffer : "
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index 509d4244d614..6f599a82a3a0 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,
> if (!isValidBuffers(source, *destination))
> return -EINVAL;
>
> - const MappedFrameBuffer sourceMapped(&source, PROT_READ);
> + const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::Read);
> if (!sourceMapped.isValid()) {
> LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> return -EINVAL;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2647bf2f3b96..6028638c314c 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -211,7 +211,7 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> for (const IPABuffer &buffer : buffers) {
> const FrameBuffer fb(buffer.planes);
> buffers_.emplace(buffer.id,
> - MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
> + MappedFrameBuffer(&fb, MappedFrameBuffer::ReadWrite));
> }
> }
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 76f67dd4567a..b905a9ca1dbf 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -411,7 +411,7 @@ void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> {
> for (const IPABuffer &buffer : buffers) {
> const FrameBuffer fb(buffer.planes);
> - buffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
> + buffers_.emplace(buffer.id, MappedFrameBuffer(&fb, MappedFrameBuffer::ReadWrite));
> }
> }
>
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index 0e30fc542154..65bdf9249133 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -142,6 +142,22 @@ MappedBuffer::~MappedBuffer()
> * \brief Map a FrameBuffer using the MappedBuffer interface
> */
>
> +/**
> + * \enum MappedFrameBuffer::MapModes
> + * \brief Specify the mapping mode for the FrameBuffer
> + * \var MappedFrameBuffer::Read
> + * \brief Create a Read-only mapping
> + * \var MappedFrameBuffer::Write
> + * \brief Create a Write-only mapping
> + * \var MappedFrameBuffer::ReadWrite
> + * \brief Create a mapping with both read and write capabilities
> + */
> +
> +/**
> + * \typedef MappedFrameBuffer::MapMode
> + * \brief A bitwise combination of MappedFrameBuffer::MapModes values
> + */
> +
> /**
> * \brief Map all planes of a FrameBuffer
> * \param[in] buffer FrameBuffer to be mapped
> @@ -151,12 +167,20 @@ MappedBuffer::~MappedBuffer()
> * The flags are passed directly to mmap and should be either PROT_READ,
> * PROT_WRITE, or a bitwise-or combination of both.
The documentation should be updated.
> */
> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags)
> {
> maps_.reserve(buffer->planes().size());
>
> + int mmap_flags = 0;
s/mmap_flags/mmapFlags/
> +
> + if (flags & Read)
> + mmap_flags |= PROT_READ;
> +
> + if (flags & Write)
> + mmap_flags |= PROT_WRITE;
> +
> for (const FrameBuffer::Plane &plane : buffer->planes()) {
> - void *address = mmap(nullptr, plane.length, flags,
> + void *address = mmap(nullptr, plane.length, mmap_flags,
> MAP_SHARED, plane.fd.fd(), 0);
> if (address == MAP_FAILED) {
> error_ = -errno;
> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> index a3d1511b74ce..ce75d9a9dbbd 100644
> --- a/test/mapped-buffer.cpp
> +++ b/test/mapped-buffer.cpp
> @@ -71,7 +71,7 @@ protected:
> const std::unique_ptr<FrameBuffer> &buffer = allocator_->buffers(stream_).front();
> std::vector<MappedBuffer> maps;
>
> - MappedFrameBuffer map(buffer.get(), PROT_READ);
> + MappedFrameBuffer map(buffer.get(), MappedFrameBuffer::Read);
> if (!map.isValid()) {
> cout << "Failed to successfully map buffer" << endl;
> return TestFail;
> @@ -90,13 +90,13 @@ protected:
> }
>
> /* Test for multiple successful maps on the same buffer. */
> - MappedFrameBuffer write_map(buffer.get(), PROT_WRITE);
> + MappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::Write);
> if (!write_map.isValid()) {
> cout << "Failed to map write buffer" << endl;
> return TestFail;
> }
>
> - MappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);
> + MappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::ReadWrite);
> if (!rw_map.isValid()) {
> cout << "Failed to map RW buffer" << endl;
> return TestFail;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list