[libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use typed Flags<MapModes>
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 9 16:45:39 CEST 2021
Hi Kieran,
On Mon, Aug 09, 2021 at 01:47:42PM +0100, Kieran Bingham wrote:
> On 06/08/2021 16:19, Laurent Pinchart wrote:
> > 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.
>
> No ;-)
>
> But now I have at least removed them all and added back only the ones
> that break compilation:
>
> Which leaves:
>
> modified: src/android/camera_device.cpp
> modified: src/android/jpeg/encoder_libjpeg.cpp
> modified: src/ipa/ipu3/ipu3.cpp
> modified: src/libcamera/framebuffer.cpp
> modified: src/libcamera/ipa_module.cpp
> modified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> modified: src/libcamera/v4l2_videodevice.cpp
> modified: src/v4l2/v4l2_camera_proxy.h
> modified: src/v4l2/v4l2_compat.cpp
> modified: src/v4l2/v4l2_compat_manager.h
>
> that can potentially have mman.h removed, but those are not all
> necessarily caused by this series, nor no longer required if they simply
> pass compilation due to other components including the requirements for
> them ...
>
> Especially given that I can remove all those references to mman.h before
> my MappedFrameBuffer patch - and have a successful build just the same.
>
>
> It might be better to sweep through things like that with a tool like
> include-what-you-use as a more general task, or inclusion in checkstyle.py.
>
>
> >> +#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.
>
> Dropped.
>
> >> + 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.
>
> Yes, I think I introduced 'Modes' because Read/Write/ReadWrite are more
> modes to me than flags.
>
> But as they are still MapFlags, I'm going to pick your second option.
>
> I sort of dislike the enum being singular - because that's where the
> flag(*s*) are stored.
It bothers me slightly too, but if I then think about how enums are used
in the general case, I conclude that using the plural would be worse.
enum Status {
Beautiful,
Ugly,
};
Status analyzeCode(...);
vs.
enum Statuses {
Beautiful,
Ugly,
};
Statuses analyzeCode(...);
I try to remind myself that an enum is a type that stores a single
value, so the singular makes sense.
> > 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".
>
> All updated/taken.
>
> >> + */
> >> +
> >> +/**
> >> + * \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