[libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use typed Flags<MapModes>
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 6 17:19:00 CEST 2021
Hi Kieran,
Thank you for the patch.
On Fri, Aug 06, 2021 at 02:53:15PM +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
> uint8_t as part of the Plane.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
> v2
> - Use fully scoped enum class
> - Swap MapMode and MapModes
> - s/mmap_flags/mmapFlags/
> - Remove and fix documentation regarding the modes
> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()
>
> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++--
> 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 | 3 +-
> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---
> test/mapped-buffer.cpp | 6 ++--
> 8 files changed, 52 insertions(+), 15 deletions(-)
>
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index 41e587364260..e8234ebf76ff 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>
I trust that you've looked through the code base to ensure no other
unnecessary inclusion of sys/mman.h is left.
> +#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,9 +45,20 @@ private:
> class MappedFrameBuffer : public MappedBuffer
> {
> public:
> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> + enum class MapMode {
> + Read = 0 << 1,
> + Write = 1 << 1,
> +
I'd drop this blank line.
> + ReadWrite = Read | Write,
> + };
> +
> + using MapModes = Flags<MapMode>;
> +
> + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags);
I'll be annoying again I'm afraid, but "MapModes flags" bothers me :-/
Looking at the two example from File, we could have
enum class MapModeFlag {
...
};
using MapMode = Flags<MapModeFlag>;
MappedFrameBuffer(const FrameBuffer *buffer, MapMode mode);
or
enum class MapFlag {
...
};
using MapFlags = Flags<MapFlag>;
MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
Other options are possible (and it may make sense to standardize naming
schemes once and for all), but naming the type "modes" and the argument
"flags" lacks consistency.
Sorry for being nitpicking, it's the first usage of Flags<> outside of
the File class, so it's affected by the 0, 1, N generalization theorem.
I'm also thinking that MappedBuffer may graduate as a public API, hence
the particular attention.
> };
>
> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode)
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 372018d2207f..34472e8da6a2 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::MapMode::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..66b4e696b107 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::MapMode::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..6d156c20646c 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::MapMode::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..c8fc26525707 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::MapMode::ReadWrite));
> }
> }
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 76f67dd4567a..54e22d91084b 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -411,7 +411,8 @@ 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::MapMode::ReadWrite));
> }
> }
>
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index 0e30fc542154..207c80b5aba2 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()
> * \brief Map a FrameBuffer using the MappedBuffer interface
> */
>
> +/**
> + * \enum MappedFrameBuffer::MapMode
> + * \brief Specify the mapping mode for the FrameBuffer
> + * \var MappedFrameBuffer::Read
> + * \brief Create a Read-only mapping
Maybe s/Read/read/ ? Same for write.
> + * \var MappedFrameBuffer::Write
> + * \brief Create a Write-only mapping
> + * \var MappedFrameBuffer::ReadWrite
> + * \brief Create a mapping with both read and write capabilities
Capabilities sound a bit weird to me in this context, I would have
written "Create a mapping that can be both read and written".
> + */
> +
> +/**
> + * \typedef MappedFrameBuffer::MapModes
> + * \brief A bitwise combination of MappedFrameBuffer::MapMode values
> + */
> +
> /**
> * \brief Map all planes of a FrameBuffer
> * \param[in] buffer FrameBuffer to be mapped
> * \param[in] flags Protection flags to apply to map
> *
> - * Construct an object to map a frame buffer for CPU access.
> - * The flags are passed directly to mmap and should be either PROT_READ,
> - * PROT_WRITE, or a bitwise-or combination of both.
> + * Construct an object to map a frame buffer for CPU access. The mapping can be
> + * made as Read only, Write only or support Read and Write operations by setting
> + * the MapMode flags accordingly.
> */
> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags)
> {
> maps_.reserve(buffer->planes().size());
>
> + int mmapFlags = 0;
> +
> + if (flags & MapMode::Read)
> + mmapFlags |= PROT_READ;
> +
> + if (flags & MapMode::Write)
> + mmapFlags |= PROT_WRITE;
> +
> for (const FrameBuffer::Plane &plane : buffer->planes()) {
> - void *address = mmap(nullptr, plane.length, flags,
> + void *address = mmap(nullptr, plane.length, mmapFlags,
> 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..63d20e7bc9c4 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::MapMode::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::MapMode::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::MapMode::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